Re: [PATCH] PCI: hv: fix reading of PCI_INTERRUPT_LINE and PCI_INTERRUPT_PIN

[Wei Liu <wei.liu@kernel.org>]

On Fri, Jun 21, 2024 at 06:41:04PM +0000, Dexuan Cui wrote:
> From: Jake Oshins <jakeo@microsoft.com> 
> Sent: Friday, June 21, 2024 9:51 AM
> > [...]
> >On Fri, Jun 21, 2024 at 06:19:05AM +0000, Wei Liu wrote:
> > On Fri, Jun 21, 2024 at 03:15:19AM +0000, Michael Kelley wrote:
> > > From: Wei Liu <mailto:wei.liu@kernel.org> Sent: Thursday, June 20, 2024 6:48 PM
> > > >
> > > > The intent of the code snippet is to always return 0 for both fields.
> > > > The check is wrong though. Fix that.
> > > >
> > > > This is discovered by this call in VFIO:
> > > >
> > > >     pci_read_config_byte(vdev->pdev, PCI_INTERRUPT_PIN, &pin);
> > > >
> > > > The old code does not set *val to 0 because the second half of the check is
> > > > incorrect.
> 
> Hi Wei, so you got a non-zero 'pin' value returned by the host when the guest reads
> from the MMIO config page. What's the consequence? Will VFIO try to use the legacy INTx 
> rather than MSI/MSI-X? I'm curious how you noticed the bug. I'm also curious why the
> host doesn't return 0 for the 'PIN' register when the guest reads it from the config page.

It is not the guest reading the register. The VM has not launched yet.
Everything happens on the host side. The host side software is preparing
the device for the VM to use.

The consequence of this bug is that user space code will think INTx is
available while in fact it is not.

VFIO itself doesn't care much. I noticed the bug because our VMM (Cloud
Hypervisor) initializes INTx whenever it is available.

> 
> >  I believe that this fix is correct.  (And I'm frankly surprised that this bug didn't
> > cause a problem before this.  It's been there since I first wrote the code.)
> > -- Jake Oshins
> 
> I suppose it didn't cause any issue because the PCI device drivers use MSI/MSI-X,
> so they don't care about the values of the 'PIN' and 'LINE' registers.

I suspect the same. Drivers almost always prefer MSI / MSI-X over INTx.
No one else triggered that code path before.

Thanks,
Wei.

> 
> Thanks,
> Dexuan
>