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