| Message ID | 20240621210018.350429-1-wei.liu@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] PCI: hv: fix reading of PCI_INTERRUPT_PIN | expand |
From: Wei Liu <wei.liu@kernel.org> Sent: Friday, June 21, 2024 2:00 PM > > The intent of the code snippet is to always return 0 for both > PCI_INTERRUPT_LINE and PCI_INTERRUPT_PIN. > > The check misses PCI_INTERRUPT_PIN. This patch fixes 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 it misses the check for > PCI_INTERRUPT_PIN. > > Fixes: 4daace0d8ce8 ("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> > --- > v2: > * Change the commit subject line and message > * Change the code according to feedback > --- > 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..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 > -- > 2.43.0 > Reviewed-by: Michael Kelley <mhklinux@outlook.com>
1) Capitalize subject to match history 2) Say something more specific than "fix reading ..." Apparently this returns garbage in some case where you want to return zero? On Fri, Jun 21, 2024 at 09:00:18PM +0000, Wei Liu wrote: > The intent of the code snippet is to always return 0 for both > PCI_INTERRUPT_LINE and PCI_INTERRUPT_PIN. > > The check misses PCI_INTERRUPT_PIN. This patch fixes 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 it misses the check for > PCI_INTERRUPT_PIN. > > Fixes: 4daace0d8ce8 ("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> > --- > v2: > * Change the commit subject line and message > * Change the code according to feedback > --- > 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..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 > -- > 2.43.0 >
On Wed, Jun 26, 2024 at 10:10:39AM -0500, Bjorn Helgaas wrote: > 1) Capitalize subject to match history What do you mean here? I got the "PCI: hv: ..." format from recent commits. "PCI" is capitalized. You want to to capitalize "fix"? > 2) Say something more specific than "fix reading ..." > > Apparently this returns garbage in some case where you want to return > zero? Yes. *val is not changed in the old code, so garbage is returned. Here is the updated commit message. I can resend once you confirm you're happy with it. PCI: hv: Fix reading of PCI_INTERRUPT_PIN The intent of the code snippet is to always return 0 for both PCI_INTERRUPT_LINE and PCI_INTERRUPT_PIN. The check misses PCI_INTERRUPT_PIN. This patch fixes 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 it misses the check for PCI_INTERRUPT_PIN. Garbage is returned in this case. Fixes: 4daace0d8ce8 ("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> Thanks, Wei. > > On Fri, Jun 21, 2024 at 09:00:18PM +0000, Wei Liu wrote: > > The intent of the code snippet is to always return 0 for both > > PCI_INTERRUPT_LINE and PCI_INTERRUPT_PIN. > > > > The check misses PCI_INTERRUPT_PIN. This patch fixes 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 it misses the check for > > PCI_INTERRUPT_PIN. > > > > Fixes: 4daace0d8ce8 ("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> > > --- > > v2: > > * Change the commit subject line and message > > * Change the code according to feedback > > --- > > 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..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 > > -- > > 2.43.0 > >
On Mon, Jul 01, 2024 at 06:16:18AM +0000, Wei Liu wrote: > On Wed, Jun 26, 2024 at 10:10:39AM -0500, Bjorn Helgaas wrote: > > 1) Capitalize subject to match history > > What do you mean here? I got the "PCI: hv: ..." format from recent > commits. "PCI" is capitalized. You want to to capitalize "fix"? Yes. Look at the history: $ git log --oneline --no-merges drivers/pci/controller/pci-hyperv.c b5ff74c1ef50 PCI: hv: Fix ring buffer size calculation 07e8f88568f5 x86/apic: Drop apic::delivery_mode f741bcadfe52 PCI: hv: Annotate struct hv_dr_state with __counted_by 04bbe863241a PCI: hv: Fix a crash in hv_pci_restore_msi_msg() during hibernation 067d6ec7ed5b PCI: hv: Add a per-bus mutex state_lock a847234e24d0 Revert "PCI: hv: Fix a timing issue which causes kdump to fail occasionally" add9195e69c9 PCI: hv: Remove the useless hv_pcichild_state from struct hv_pci_dev 2738d5ab7929 PCI: hv: Fix a race condition in hv_irq_unmask() that can cause panic ... > > 2) Say something more specific than "fix reading ..." > > > > Apparently this returns garbage in some case where you want to return > > zero? > > Yes. *val is not changed in the old code, so garbage is returned. > > Here is the updated commit message. I can resend once you confirm you're > happy with it. > > PCI: hv: Fix reading of PCI_INTERRUPT_PIN Maybe: PCI: hv: Return zero, not garbage, when reading PCI_INTERRUPT_PIN > The intent of the code snippet is to always return 0 for both > PCI_INTERRUPT_LINE and PCI_INTERRUPT_PIN. > > The check misses PCI_INTERRUPT_PIN. This patch fixes 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 it misses the check for > PCI_INTERRUPT_PIN. Garbage is returned in this case. > > Fixes: 4daace0d8ce8 ("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> Looks fine.
On Mon, Jul 01, 2024 at 12:20:53PM -0500, Bjorn Helgaas wrote: > On Mon, Jul 01, 2024 at 06:16:18AM +0000, Wei Liu wrote: > > On Wed, Jun 26, 2024 at 10:10:39AM -0500, Bjorn Helgaas wrote: > > > 1) Capitalize subject to match history > > > > What do you mean here? I got the "PCI: hv: ..." format from recent > > commits. "PCI" is capitalized. You want to to capitalize "fix"? > > Yes. Look at the history: > > $ git log --oneline --no-merges drivers/pci/controller/pci-hyperv.c > b5ff74c1ef50 PCI: hv: Fix ring buffer size calculation > 07e8f88568f5 x86/apic: Drop apic::delivery_mode > f741bcadfe52 PCI: hv: Annotate struct hv_dr_state with __counted_by > 04bbe863241a PCI: hv: Fix a crash in hv_pci_restore_msi_msg() during hibernation > 067d6ec7ed5b PCI: hv: Add a per-bus mutex state_lock > a847234e24d0 Revert "PCI: hv: Fix a timing issue which causes kdump to fail occasionally" > add9195e69c9 PCI: hv: Remove the useless hv_pcichild_state from struct hv_pci_dev > 2738d5ab7929 PCI: hv: Fix a race condition in hv_irq_unmask() that can cause panic > ... > > > > 2) Say something more specific than "fix reading ..." > > > > > > Apparently this returns garbage in some case where you want to return > > > zero? > > > > Yes. *val is not changed in the old code, so garbage is returned. > > > > Here is the updated commit message. I can resend once you confirm you're > > happy with it. > > > > PCI: hv: Fix reading of PCI_INTERRUPT_PIN > > Maybe: > > PCI: hv: Return zero, not garbage, when reading PCI_INTERRUPT_PIN > > > The intent of the code snippet is to always return 0 for both > > PCI_INTERRUPT_LINE and PCI_INTERRUPT_PIN. > > > > The check misses PCI_INTERRUPT_PIN. This patch fixes 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 it misses the check for > > PCI_INTERRUPT_PIN. Garbage is returned in this case. > > > > Fixes: 4daace0d8ce8 ("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> > > Looks fine. Thanks. I will resend with the updated commit message.
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
The intent of the code snippet is to always return 0 for both PCI_INTERRUPT_LINE and PCI_INTERRUPT_PIN. The check misses PCI_INTERRUPT_PIN. This patch fixes 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 it misses the check for PCI_INTERRUPT_PIN. Fixes: 4daace0d8ce8 ("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> --- v2: * Change the commit subject line and message * Change the code according to feedback --- drivers/pci/controller/pci-hyperv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)