Re: [PATCH v4 02/10] vpci/header: Emulate legacy capability list for dom0
On 2025/5/16 00:29, Roger Pau Monné wrote:
> On Fri, May 09, 2025 at 05:05:34PM +0800, Jiqian Chen wrote:
>> @@ -786,15 +792,18 @@ static int vpci_init_capability_list(struct pci_dev
>> *pdev)
>>
>> next = pci_find_next_cap_ttl(pdev->sbdf,
>> pos + PCI_CAP_LIST_NEXT,
>> - supported_caps,
>> - ARRAY_SIZE(supported_caps), &ttl);
>> + supported_caps, n, &ttl);
>>
>> -rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
>> - pos + PCI_CAP_LIST_ID, 1, NULL);
>> -if ( rc )
>> -return rc;
>> +if ( !is_hwdom )
>> +{
>> +rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
>> + pos + PCI_CAP_LIST_ID, 1, NULL);
>> +if ( rc )
>> +return rc;
>> +}
>>
>> -rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>> +rc = vpci_add_register(pdev->vpci, vpci_read_val,
>> + is_hwdom ? vpci_hw_write8 : NULL,
>> pos + PCI_CAP_LIST_NEXT, 1,
>> (void *)(uintptr_t)next);
>> if ( rc )
>> @@ -805,13 +814,17 @@ static int vpci_init_capability_list(struct pci_dev
>> *pdev)
>> }
>>
>> /* Utilize rsvdp_mask to hide PCI_STATUS_CAP_LIST from the guest. */
>> -return vpci_add_register_mask(pdev->vpci, vpci_hw_read16,
>> vpci_hw_write16,
>> - PCI_STATUS, 2, NULL,
>> - PCI_STATUS_RO_MASK &
>> -~(mask_cap_list ? PCI_STATUS_CAP_LIST :
>> 0),
>> - PCI_STATUS_RW1C_MASK,
>> - mask_cap_list ? PCI_STATUS_CAP_LIST : 0,
>> - PCI_STATUS_RSVDZ_MASK);
>> +return is_hwdom ? 0 : vpci_add_register_mask(pdev->vpci, vpci_hw_read16,
>> + vpci_hw_write16,
>> PCI_STATUS,
>> + 2, NULL,
>> + PCI_STATUS_RO_MASK &
>> +~(mask_cap_list ?
>> +PCI_STATUS_CAP_LIST
>> :
>> +0),
>> + PCI_STATUS_RW1C_MASK,
>> + mask_cap_list ?
>> +PCI_STATUS_CAP_LIST : 0,
>> + PCI_STATUS_RSVDZ_MASK);
>
> Wow, that's a bit too much indentation for my taste. Do you think you
> could do:
>
> /* Return early for the hw domain, no masking of PCI_STATUS. */
> if ( is_hwdom )
> return 0;
> ...
>
> So that you don't have to touch the current return chunk?
It seems better.
Will do in next version.
>
> Thanks, Roger.
--
Best regards,
Jiqian Chen.
Re: [PATCH v4 02/10] vpci/header: Emulate legacy capability list for dom0
On Fri, May 09, 2025 at 05:05:34PM +0800, Jiqian Chen wrote:
> Current logic of emulating legacy capability list is only for domU.
> So, expand it to emulate for dom0 too. Then it will be easy to hide
> a capability whose initialization fails in a function.
>
> And restrict adding PCI_STATUS register only for domU since dom0
> has no limitation to access that register.
>
> Signed-off-by: Jiqian Chen
> ---
> cc: "Roger Pau Monné"
> ---
> v3->v4 changes:
> * Also pass supported_caps to pci_find_next_cap_ttl() for dom0 since the n is
> zero when dom0,
> and add a comment to explain it.
> * Restrict adding PCI_STATUS register only for domU since dom0 has no
> limitation to access that register.
> * For dom0 register handler, set vpci_hw_write8 to it instead of NULL.
>
> v2->v3 changes:
> * Not to add handler of PCI_CAP_LIST_ID when domain is dom0.
>
> v1->v2 changes:
> new patch.
>
> Best regards,
> Jiqian Chen.
> ---
> xen/drivers/vpci/header.c | 53 ---
> xen/drivers/vpci/vpci.c | 6 +
> xen/include/xen/vpci.h| 2 ++
> 3 files changed, 41 insertions(+), 20 deletions(-)
>
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 3e9b44454b43..a06c518c506c 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -749,9 +749,9 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
> {
> int rc;
> bool mask_cap_list = false;
> +bool is_hwdom = is_hardware_domain(pdev->domain);
>
> -if ( !is_hardware_domain(pdev->domain) &&
> - pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST )
> +if ( pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST )
> {
> /* Only expose capabilities to the guest that vPCI can handle. */
> unsigned int next, ttl = 48;
> @@ -759,12 +759,18 @@ static int vpci_init_capability_list(struct pci_dev
> *pdev)
> PCI_CAP_ID_MSI,
> PCI_CAP_ID_MSIX,
> };
> +/*
> + * For dom0, we should expose all capabilities instead of a fixed
> + * capabilities array, so setting n to 0 here is to get the next
> + * capability position directly in pci_find_next_cap_ttl.
> + */
> +const unsigned int n = is_hwdom ? 0 : ARRAY_SIZE(supported_caps);
>
> next = pci_find_next_cap_ttl(pdev->sbdf, PCI_CAPABILITY_LIST,
> - supported_caps,
> - ARRAY_SIZE(supported_caps), &ttl);
> + supported_caps, n, &ttl);
>
> -rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
> +rc = vpci_add_register(pdev->vpci, vpci_read_val,
> + is_hwdom ? vpci_hw_write8 : NULL,
> PCI_CAPABILITY_LIST, 1,
> (void *)(uintptr_t)next);
> if ( rc )
> @@ -772,7 +778,7 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
>
> next &= ~3;
>
> -if ( !next )
> +if ( !next && !is_hwdom )
> /*
> * If we don't have any supported capabilities to expose to the
> * guest, mask the PCI_STATUS_CAP_LIST bit in the status
> @@ -786,15 +792,18 @@ static int vpci_init_capability_list(struct pci_dev
> *pdev)
>
> next = pci_find_next_cap_ttl(pdev->sbdf,
> pos + PCI_CAP_LIST_NEXT,
> - supported_caps,
> - ARRAY_SIZE(supported_caps), &ttl);
> + supported_caps, n, &ttl);
>
> -rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
> - pos + PCI_CAP_LIST_ID, 1, NULL);
> -if ( rc )
> -return rc;
> +if ( !is_hwdom )
> +{
> +rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
> + pos + PCI_CAP_LIST_ID, 1, NULL);
> +if ( rc )
> +return rc;
> +}
>
> -rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
> +rc = vpci_add_register(pdev->vpci, vpci_read_val,
> + is_hwdom ? vpci_hw_write8 : NULL,
> pos + PCI_CAP_LIST_NEXT, 1,
> (void *)(uintptr_t)next);
> if ( rc )
> @@ -805,13 +814,17 @@ static int vpci_init_capability_list(struct pci_dev
> *pdev)
> }
>
> /* Utilize rsvdp_mask to hide PCI_STATUS_CAP_LIST from the guest. */
> -return vpci_add_register_mask(pdev->vpci, vpci_hw_read16,
> vpci_hw_write16,
> - PCI_STATUS, 2, NULL,
> - PCI_STATUS_RO_MASK &
> -~(mask_cap_list
[PATCH v4 02/10] vpci/header: Emulate legacy capability list for dom0
Current logic of emulating legacy capability list is only for domU.
So, expand it to emulate for dom0 too. Then it will be easy to hide
a capability whose initialization fails in a function.
And restrict adding PCI_STATUS register only for domU since dom0
has no limitation to access that register.
Signed-off-by: Jiqian Chen
---
cc: "Roger Pau Monné"
---
v3->v4 changes:
* Also pass supported_caps to pci_find_next_cap_ttl() for dom0 since the n is
zero when dom0,
and add a comment to explain it.
* Restrict adding PCI_STATUS register only for domU since dom0 has no
limitation to access that register.
* For dom0 register handler, set vpci_hw_write8 to it instead of NULL.
v2->v3 changes:
* Not to add handler of PCI_CAP_LIST_ID when domain is dom0.
v1->v2 changes:
new patch.
Best regards,
Jiqian Chen.
---
xen/drivers/vpci/header.c | 53 ---
xen/drivers/vpci/vpci.c | 6 +
xen/include/xen/vpci.h| 2 ++
3 files changed, 41 insertions(+), 20 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 3e9b44454b43..a06c518c506c 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -749,9 +749,9 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
{
int rc;
bool mask_cap_list = false;
+bool is_hwdom = is_hardware_domain(pdev->domain);
-if ( !is_hardware_domain(pdev->domain) &&
- pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST )
+if ( pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST )
{
/* Only expose capabilities to the guest that vPCI can handle. */
unsigned int next, ttl = 48;
@@ -759,12 +759,18 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
PCI_CAP_ID_MSI,
PCI_CAP_ID_MSIX,
};
+/*
+ * For dom0, we should expose all capabilities instead of a fixed
+ * capabilities array, so setting n to 0 here is to get the next
+ * capability position directly in pci_find_next_cap_ttl.
+ */
+const unsigned int n = is_hwdom ? 0 : ARRAY_SIZE(supported_caps);
next = pci_find_next_cap_ttl(pdev->sbdf, PCI_CAPABILITY_LIST,
- supported_caps,
- ARRAY_SIZE(supported_caps), &ttl);
+ supported_caps, n, &ttl);
-rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
+rc = vpci_add_register(pdev->vpci, vpci_read_val,
+ is_hwdom ? vpci_hw_write8 : NULL,
PCI_CAPABILITY_LIST, 1,
(void *)(uintptr_t)next);
if ( rc )
@@ -772,7 +778,7 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
next &= ~3;
-if ( !next )
+if ( !next && !is_hwdom )
/*
* If we don't have any supported capabilities to expose to the
* guest, mask the PCI_STATUS_CAP_LIST bit in the status
@@ -786,15 +792,18 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
next = pci_find_next_cap_ttl(pdev->sbdf,
pos + PCI_CAP_LIST_NEXT,
- supported_caps,
- ARRAY_SIZE(supported_caps), &ttl);
+ supported_caps, n, &ttl);
-rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
- pos + PCI_CAP_LIST_ID, 1, NULL);
-if ( rc )
-return rc;
+if ( !is_hwdom )
+{
+rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
+ pos + PCI_CAP_LIST_ID, 1, NULL);
+if ( rc )
+return rc;
+}
-rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
+rc = vpci_add_register(pdev->vpci, vpci_read_val,
+ is_hwdom ? vpci_hw_write8 : NULL,
pos + PCI_CAP_LIST_NEXT, 1,
(void *)(uintptr_t)next);
if ( rc )
@@ -805,13 +814,17 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
}
/* Utilize rsvdp_mask to hide PCI_STATUS_CAP_LIST from the guest. */
-return vpci_add_register_mask(pdev->vpci, vpci_hw_read16, vpci_hw_write16,
- PCI_STATUS, 2, NULL,
- PCI_STATUS_RO_MASK &
-~(mask_cap_list ? PCI_STATUS_CAP_LIST : 0),
- PCI_STATUS_RW1C_MASK,
- mask_cap_list ? PCI_STATUS_CAP_LIST : 0,
- PCI_STATUS_RSVDZ_MASK);
+return is_hwdom ? 0 : vpci_add_register_mask(pdev->vpci, vpci_hw_read16,
+
