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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> Or you prefer to pass NULL here?

Yes, that's what I've been trying to argue for.

Jan