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

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

Hi Roger,

On 2025/6/24 15:36, Jan Beulich wrote:
> On 24.06.2025 09:01, Chen, Jiqian wrote:
>> 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)" ?
> 
> As indicated - my take is that the check may best be dropped. Roger?
May I get your feedback? Since some discussions need your input.
Also discussion in patch 2 and patch 4 as I remembered.

> 
> Jan

-- 
Best regards,
Jiqian Chen.