Re: [PATCH v4 02/10] vpci/header: Emulate legacy capability list for dom0

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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 <Jiqian.Chen@xxxxxxx>
> ---
> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> ---
> 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,
> +                                                 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?

Thanks, Roger.