Re: [PATCH v2 2/8] vpci/header: Emulate legacy capability list for host

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

On 2025/4/15 17:25, Roger Pau Monné wrote:
> On Wed, Apr 09, 2025 at 02:45:22PM +0800, Jiqian Chen wrote:
>> Current logic of init_header() only emulates legacy capability list
>> for guest, expand it to emulate for host too. So that it will be
>> easy to hide a capability whose initialization fails and no need
>> to distinguish host or guest.
> 
> It might be best if the initial code movement of the logic in
> init_header() into it's own separate function was done as a
> non-functional change, and a later patch added support for dom0.
> 
> It's easier to then spot the differences that you are adding to
> support dom0.
Got it, I will re-arrange my patch in next version.

> 
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> ---
>> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
>> ---
>> v1->v2 changes:
>> new patch
>>
>> Best regards,
>> Jiqian Chen.
>> ---
>>  xen/drivers/vpci/header.c | 139 ++++++++++++++++++++------------------
>>  1 file changed, 74 insertions(+), 65 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index ef6c965c081c..0910eb940e23 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -745,6 +745,76 @@ static int bar_add_rangeset(const struct pci_dev *pdev, 
>> struct vpci_bar *bar,
>>      return !bar->mem ? -ENOMEM : 0;
>>  }
>>  
>> +/* These capabilities can be exposed to the guest, that vPCI can handle. */
>> +static const unsigned int guest_supported_caps[] = {
>> +    PCI_CAP_ID_MSI,
>> +    PCI_CAP_ID_MSIX,
>> +};
> 
> Is there a reason this needs to be defined outside of the function
> scope?  So far it's only used by vpci_init_capability_list().
Because, for dom0 I don't need to pass this array, so I need to set below 
parameter "caps" to be NULL or guest_supported_caps according to the type of 
domain.
If inside the function, I can't to do that since "caps" is const, I think.
Do you have any suggestions?

> 
>> +
>> +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);
>> +    const unsigned int *caps = is_hwdom ? NULL : guest_supported_caps;
>> +    const unsigned int n = is_hwdom ? 0 : ARRAY_SIZE(guest_supported_caps);
>> +
>> +    if ( pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST )
>> +    {
>> +        unsigned int next, ttl = 48;
>> +
>> +        next = pci_find_next_cap_ttl(pdev->sbdf, PCI_CAPABILITY_LIST,
>> +                                     caps, n, &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 && !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 
>> 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,
>> +                                         caps, n, &ttl);
>> +
>> +            rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
>> +                                   pos + PCI_CAP_LIST_ID, 1, NULL);
> 
> There's no need to add this handler for the hardware domain, that's
> already the default behavior in that case.
But if not, I have no handler to remove from capability list in next patch's 
hiding function vpci_capability_mask(),
then I can't success to hide it.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.