Re: [PATCH v6 1/8] vpci/header: Emulate extended capability list for dom0

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

On 2025/6/18 21:52, Jan Beulich wrote:
> On 12.06.2025 11:29, Jiqian Chen wrote:
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -836,6 +836,42 @@ static int vpci_init_capability_list(struct pci_dev 
>> *pdev)
>>                                    PCI_STATUS_RSVDZ_MASK);
>>  }
>>  
>> +static int vpci_init_ext_capability_list(struct pci_dev *pdev)
>> +{
>> +    unsigned int pos = PCI_CFG_SPACE_SIZE;
>> +
>> +    if ( !is_hardware_domain(pdev->domain) )
>> +        /* Extended capabilities read as zero, write ignore for guest */
> 
> s/guest/DomU/ ?
Will do.

> 
>> +        return vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>> +                                 pos, 4, (void *)0);
>> +
>> +    while ( pos >= PCI_CFG_SPACE_SIZE )
>> +    {
>> +        uint32_t header = pci_conf_read32(pdev->sbdf, pos);
>> +        int rc;
>> +
>> +        if ( !header )
>> +            return 0;
> 
> Is this a valid check to make for anything other than the first read? And even
> if valid for the first one, shouldn't that also go through ...
> 
>> +        rc = vpci_add_register(pdev->vpci, vpci_read_val, vpci_hw_write32,
>> +                               pos, 4, (void *)(uintptr_t)header);
> 
> ... here?
If header of first is zero. There is no need to add a register I think, since 
the dom0 can read/write directly.

> 
>> +        if ( rc == -EEXIST )
>> +        {
>> +            printk(XENLOG_WARNING
>> +                   "%pd %pp: overlap in extended cap list, offset %#x\n",
>> +                   pdev->domain, &pdev->sbdf, pos);
>> +            return 0;
>> +        }
>> +
>> +        if ( rc )
>> +            return rc;
>> +
>> +        pos = PCI_EXT_CAP_NEXT(header);
>> +    }
> 
> As a more general remark - this is imo the kind of situation where using
> do ... while() would be better.
> 
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -267,6 +267,12 @@ void cf_check vpci_hw_write16(
>>      pci_conf_write16(pdev->sbdf, reg, val);
>>  }
>>  
>> +void cf_check vpci_hw_write32(
>> +    const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data)
>> +{
>> +    pci_conf_write32(pdev->sbdf, reg, val);
>> +}
> 
> Iirc we've been there before, yet I continue to wonder whether we're doing
> ourselves any good in allowing writes to something that certainly better
> wouldn't change. Even if we limit this to Dom0.
I remember this was suggested by Roger in V2, since the Dom0 has no limitations 
to write the extended register.

> 
> Jan

-- 
Best regards,
Jiqian Chen.