Re: [PATCH v2 3/8] vpci/header: Emulate extended capability list for host

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.04.2025 12:18, Chen, Jiqian wrote:
> On 2025/4/15 17:49, Jan Beulich wrote:
>> On 09.04.2025 08:45, Jiqian Chen wrote:
>>> --- a/xen/drivers/vpci/header.c
>>> +++ b/xen/drivers/vpci/header.c
>>> @@ -815,6 +815,39 @@ static int vpci_init_capability_list(struct pci_dev 
>>> *pdev)
>>>      return rc;
>>>  }
>>>  
>>> +static int vpci_init_ext_capability_list(struct pci_dev *pdev)
>>> +{
>>> +    int rc;
>>> +    u32 header;
>>> +    unsigned int pos = 0x100U, ttl = 480;
>>> +
>>> +    if ( !is_hardware_domain(pdev->domain) )
>>> +    {
>>> +        /* Extended capabilities read as zero, write ignore */
>>> +        rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>>> +                               pos, 4, (void *)0);
>>> +        if ( rc )
>>> +            return rc;
>>> +    }
>>> +
>>> +    while ( pos && ttl-- )
>>> +    {
>>> +        header = pci_conf_read32(pdev->sbdf, pos);
>>> +
>>> +        rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>>> +                               pos, 4, (void *)(uintptr_t)header);
>>> +        if ( rc )
>>> +            return rc;
>>> +
>>> +        if ( (header == 0) || (header == -1) )
>>> +            return 0;
>>
>> I realize pci_find_next_ext_capability() also has such a check, but even
>> there it's not really clear to me why compare not only against 0, but also
>> again -1 (aka ~0).
> Thank you for raising this question.
> When I coded this part, I also had this confuse since 
> pci_find_next_ext_capability() has this check,
> so I chose to keep the same check.
> Do you think I need to remove this -1 check?

Unless you can explain why it's needed (perhaps by figuring out why the other
one is there), I'd say - yes.

Jan