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 03:15:19AM +0000, Michael Kelley wrote:
> From: Wei Liu <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.
> > 
> > Fixes: 4daace0d8ce85 ("PCI: hv: Add paravirtual PCI front-end for Microsoft Hyper-V
> > VMs")
> > Cc: stable@kernel.org
> > Signed-off-by: Wei Liu <wei.liu@kernel.org>
> > ---
> >  drivers/pci/controller/pci-hyperv.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> > index 5992280e8110..eec087c8f670 100644
> > --- a/drivers/pci/controller/pci-hyperv.c
> > +++ b/drivers/pci/controller/pci-hyperv.c
> > @@ -1130,8 +1130,8 @@ static void _hv_pcifront_read_config(struct hv_pci_dev
> > *hpdev, int where,
> >  		   PCI_CAPABILITY_LIST) {
> >  		/* ROM BARs are unimplemented */
> >  		*val = 0;
> > -	} else if (where >= PCI_INTERRUPT_LINE && where + size <=
> > -		   PCI_INTERRUPT_PIN) {
> > +	} else if ((where == PCI_INTERRUPT_LINE || where == PCI_INTERRUPT_PIN) &&
> > +		   size == 1) {
> 
> Any reason not to continue the pattern of the rest of the function,
> and do the following to fix the bug?
> 
>    	} else if (where >= PCI_INTERRUPT_LINE && where + size <= 
>   		   PCI_MIN_GNT) {
> 
> Your fix doesn't allow PCI_INTERRUPT_LINE and PCI_INTERRUPT_PIN
> to be read together as a 2-byte access, though I don't know if that
> matters.

I don't think this is a sane use case. Someone calls
pci_read_config_word on PCI_INTERRUPT_LINE and then breaks the two
fields out from a word size value.

There is only one in-tree instance attempting to read both fields at the
same time.  And it gets away with it because it immediately writes the
same value back to another register.

(Search for PCI_INTERRUPT_LINE in sound/pci/ctxfi/cthw20k1.c)

The rest of the code always does a byte read.

I had a version that looked like this:

diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 5992280e8110..cdd5be16021d 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -1130,8 +1130,8 @@ static void _hv_pcifront_read_config(struct hv_pci_dev *hpdev, int where,
                   PCI_CAPABILITY_LIST) {
                /* ROM BARs are unimplemented */
                *val = 0;
-       } else if (where >= PCI_INTERRUPT_LINE && where + size <=
-                  PCI_INTERRUPT_PIN) {
+       } else if ((where >= PCI_INTERRUPT_LINE && where + size <= PCI_INTERRUPT_PIN) ||
+                  (where >= PCI_INTERRUPT_PIN && where + size <= PCI_MIN_GNT)) {
                /*
                 * Interrupt Line and Interrupt PIN are hard-wired to zero
                 * because this front-end only supports message-signaled

It was consistent with the old style. But I thought it was a bit too
tedious to read, so I changed to the current version.

At the very least I would like to enforce the separation of the two
fields. 

I can send out the older version tomorrow -- just waiting for others to
chime in.

Thanks,
Wei.


> 
> I have a slight preference for the more consistent approach, but
> don't really object to what you've done.  Treat my idea as a
> suggestion to consider, but if you want to go with your approach,
> that's OK too.
> 
> Michael
> 
> >  		/*
> >  		 * Interrupt Line and Interrupt PIN are hard-wired to zero
> >  		 * because this front-end only supports message-signaled
> > --
> > 2.43.0
> > 
> 
>