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

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

On 2025/7/21 22:16, Roger Pau Monné wrote:
> On Wed, Jul 09, 2025 at 05:34:28AM +0000, Chen, Jiqian wrote:
>> On 2025/7/9 13:32, Jan Beulich wrote:
>>> On 09.07.2025 07:29, Chen, Jiqian wrote:
>>>> On 2025/7/8 22:10, Jan Beulich wrote:
>>>>> On 04.07.2025 09:07, Jiqian Chen wrote:
>>>>>> --- a/xen/drivers/vpci/header.c
>>>>>> +++ b/xen/drivers/vpci/header.c
>>>>>> @@ -836,6 +836,39 @@ static int vpci_init_capability_list(struct pci_dev 
>>>>>> *pdev)
>>>>>>                                    PCI_STATUS_RSVDZ_MASK);
>>>>>>  }
>>>>>>  
>>>>>> +static int vpci_init_ext_capability_list(const 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 DomU */
>>>>>> +        return vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>>>>>> +                                 pos, 4, (void *)0);
>>>>>> +
>>>>>> +    do
>>>>>> +    {
>>>>>> +        uint32_t header = pci_conf_read32(pdev->sbdf, pos);
>>>>>> +        int rc;
>>>>>> +
>>>>>> +        rc = vpci_add_register(pdev->vpci, vpci_read_val, 
>>>>>> vpci_hw_write32,
>>>>>> +                               pos, 4, (void *)(uintptr_t)header);
>>>>>
>>>>> If it wasn't for this use of vpci_hw_write32(), I'd be happy to provide my
>>>>> R-b. But this continues to look bogus to me: What use is it to allow 
>>>>> writes
>>>>> when Dom0 then can't read back any possible effect of such a write (in the
>>>>> unexpected event that some of the bits were indeed writable)?
>>>> Oh, I got your concern.
>>>> What do you think about updating the header value after writing to 
>>>> hardware in write function?
>>
>>> That would imo be a layering violation. Once again that's something that you
>>> primarily would need Roger's input on.
>> OK, wait for Roger's input.
> 
> Hm, I see the asymmetry of allowing writes but not direct writes, my
> thought was to give the hw domain as less interference as possibly,
> hence my recommendation to use vpci_hw_write32().
> 
> I practice I think it's very unlikely that devices re-use reserved
> bits in the capability register, so I'm fine with using NULL (thus
> discarding the write).  We can always add more complex handling here
> if we ever came across a device that requires it.
OK, I will delete vpci_hw_write32() in next version.
Thanks.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.