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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.06.2025 04:29, Chen, Jiqian wrote:
> 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.

Well, my remark of course did go along with that further down. Plus I wonder
why the entire field being zero is special, but the field holding, say,
0x00010000 isn't. Yes, the spec calls out zeroes in all fields specially,
yet at the same time it does say nothing about certain other special values.

Jan

>>> --- 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
>