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.
Thanks, Roger.
