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

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

On 2025/4/15 17:42, Roger Pau Monné wrote:
> On Wed, Apr 09, 2025 at 02:45:23PM +0800, Jiqian Chen wrote:
>> Add a new function to emulate extended capability list for host,
>> and call it in init_header(). So that, it will be easy to hide
>> a capability whose initialization fails.
>>
>> As for the extended capability list of guest, keep hiding it.
>>
>> 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 | 44 ++++++++++++++++++++++++++++++++-------
>>  1 file changed, 36 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index 0910eb940e23..6833d456566b 100644
>> --- 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;
> 
> uint32_t would be preferred.
> 
>> +    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;
> 
> I think you want to unconditionally return here, otherwise you will
> most likely add a duplicated handler over "pos" when going inside the
> loop below?
Oh, it's my bad.
I should return here for any rc.

> 
> Also for domU we don't want to expose any extended capabilities yet.
> 
>> +    }
>> +
>> +    while ( pos && ttl-- )
>> +    {
>> +        header = pci_conf_read32(pdev->sbdf, pos);
>> +
>> +        rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
> 
> You don't want to pass NULL here, as that would prevent dom0 from
> writing to the register, you instead want to pass vpci_hw_write32 I
> think.
Will change in next version.

> 
>> +                               pos, 4, (void *)(uintptr_t)header);
>> +        if ( rc )
>> +            return rc;
>> +
>> +        if ( (header == 0) || (header == -1) )
>> +            return 0;
>> +
>> +        pos = PCI_EXT_CAP_NEXT(header);
> 
> Don't you need to check that pos >= 0x100?  Possibly done in the while
> loop condition: while ( pos >= 0x100 && ... )
Yes, will change in next version.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.