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

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

On 2025/6/20 14:29, Jan Beulich wrote:
> 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.
If want to cover these special values.
Should I need to change the check from "!header" to "! PCI_EXT_CAP_ID(header)" ?

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

-- 
Best regards,
Jiqian Chen.