Re: [PATCH v3 01/11] vpci/header: Move emulating cap list logic into new function

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

Hi,

On 2025/4/21 14:18, Jiqian Chen wrote:
> No functional changes.
> Follow-on changes will benifit from this.
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> ---
> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> ---
> v2->v3 changes:
> new patch.
Are there any other changes that need to be made to this series of patches?
If have, I can modify them in my upcoming new version.

> 
> Best regards,
> Jiqian Chen.
> ---
>  xen/drivers/vpci/header.c | 138 ++++++++++++++++++++------------------
>  1 file changed, 73 insertions(+), 65 deletions(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index ef6c965c081c..3e9b44454b43 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -745,6 +745,75 @@ static int bar_add_rangeset(const struct pci_dev *pdev, 
> struct vpci_bar *bar,
>      return !bar->mem ? -ENOMEM : 0;
>  }
>  
> +static int vpci_init_capability_list(struct pci_dev *pdev)
> +{
> +    int rc;
> +    bool mask_cap_list = false;
> +
> +    if ( !is_hardware_domain(pdev->domain) &&
> +         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;
> +        static const unsigned int supported_caps[] = {
> +            PCI_CAP_ID_MSI,
> +            PCI_CAP_ID_MSIX,
> +        };
> +
> +        next = pci_find_next_cap_ttl(pdev->sbdf, PCI_CAPABILITY_LIST,
> +                                     supported_caps,
> +                                     ARRAY_SIZE(supported_caps), &ttl);
> +
> +        rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
> +                               PCI_CAPABILITY_LIST, 1,
> +                               (void *)(uintptr_t)next);
> +        if ( rc )
> +            return rc;
> +
> +        next &= ~3;
> +
> +        if ( !next )
> +            /*
> +             * If we don't have any supported capabilities to expose to the
> +             * guest, mask the PCI_STATUS_CAP_LIST bit in the status
> +             * register.
> +             */
> +            mask_cap_list = true;
> +
> +        while ( next && ttl )
> +        {
> +            unsigned int pos = next;
> +
> +            next = pci_find_next_cap_ttl(pdev->sbdf,
> +                                         pos + PCI_CAP_LIST_NEXT,
> +                                         supported_caps,
> +                                         ARRAY_SIZE(supported_caps), &ttl);
> +
> +            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,
> +                                   pos + PCI_CAP_LIST_NEXT, 1,
> +                                   (void *)(uintptr_t)next);
> +            if ( rc )
> +                return rc;
> +
> +            next &= ~3;
> +        }
> +    }
> +
> +    /* 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);
> +}
> +
>  static int cf_check init_header(struct pci_dev *pdev)
>  {
>      uint16_t cmd;
> @@ -753,7 +822,6 @@ static int cf_check init_header(struct pci_dev *pdev)
>      struct vpci_header *header = &pdev->vpci->header;
>      struct vpci_bar *bars = header->bars;
>      int rc;
> -    bool mask_cap_list = false;
>      bool is_hwdom = is_hardware_domain(pdev->domain);
>  
>      ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
> @@ -794,61 +862,12 @@ static int cf_check init_header(struct pci_dev *pdev)
>      if ( rc )
>          return rc;
>  
> +    rc = vpci_init_capability_list(pdev);
> +    if ( rc )
> +        return rc;
> +
>      if ( !is_hwdom )
>      {
> -        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;
> -            static const unsigned int supported_caps[] = {
> -                PCI_CAP_ID_MSI,
> -                PCI_CAP_ID_MSIX,
> -            };
> -
> -            next = pci_find_next_cap_ttl(pdev->sbdf, PCI_CAPABILITY_LIST,
> -                                         supported_caps,
> -                                         ARRAY_SIZE(supported_caps), &ttl);
> -
> -            rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
> -                                   PCI_CAPABILITY_LIST, 1,
> -                                   (void *)(uintptr_t)next);
> -            if ( rc )
> -                return rc;
> -
> -            next &= ~3;
> -
> -            if ( !next )
> -                /*
> -                 * If we don't have any supported capabilities to expose to 
> the
> -                 * guest, mask the PCI_STATUS_CAP_LIST bit in the status
> -                 * register.
> -                 */
> -                mask_cap_list = true;
> -
> -            while ( next && ttl )
> -            {
> -                unsigned int pos = next;
> -
> -                next = pci_find_next_cap_ttl(pdev->sbdf,
> -                                             pos + PCI_CAP_LIST_NEXT,
> -                                             supported_caps,
> -                                             ARRAY_SIZE(supported_caps), 
> &ttl);
> -
> -                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,
> -                                       pos + PCI_CAP_LIST_NEXT, 1,
> -                                       (void *)(uintptr_t)next);
> -                if ( rc )
> -                    return rc;
> -
> -                next &= ~3;
> -            }
> -        }
> -
>          /* Extended capabilities read as zero, write ignore */
>          rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL, 0x100, 4,
>                                 (void *)0);
> @@ -856,17 +875,6 @@ static int cf_check init_header(struct pci_dev *pdev)
>              return rc;
>      }
>  
> -    /* Utilize rsvdp_mask to hide PCI_STATUS_CAP_LIST from the guest. */
> -    rc = 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);
> -    if ( rc )
> -        return rc;
> -
>      if ( pdev->ignore_bars )
>          return 0;
>  

-- 
Best regards,
Jiqian Chen.