| Message ID | 20250226200134.29759-1-jason.andryuk@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/pciback: Make missing GSI non-fatal | expand |
On 2025/2/27 04:01, Jason Andryuk wrote: > A PCI may not have a legacy IRQ. In that case, do not fail assigning > to the pciback stub. Instead just skip xen_pvh_setup_gsi(). > > This will leave psdev->gsi == -1. In that case, when reading the value > via IOCTL_PRIVCMD_PCIDEV_GET_GSI, return -ENOENT. Userspace can used > this to distinquish from other errors. > > Fixes: b166b8ab4189 ("xen/pvh: Setup gsi for passthrough device") > Cc: stable@vger.kernel.org > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> > --- > drivers/xen/acpi.c | 4 ++-- > drivers/xen/xen-pciback/pci_stub.c | 17 ++++++++++------- > 2 files changed, 12 insertions(+), 9 deletions(-) > > diff --git a/drivers/xen/acpi.c b/drivers/xen/acpi.c > index d2ee605c5ca1..d6ab0cb3ba3f 100644 > --- a/drivers/xen/acpi.c > +++ b/drivers/xen/acpi.c > @@ -101,7 +101,7 @@ int xen_acpi_get_gsi_info(struct pci_dev *dev, > > pin = dev->pin; > if (!pin) > - return -EINVAL; > + return -ENOENT; > > entry = acpi_pci_irq_lookup(dev, pin); > if (entry) { > @@ -116,7 +116,7 @@ int xen_acpi_get_gsi_info(struct pci_dev *dev, > gsi = -1; > > if (gsi < 0) > - return -EINVAL; > + return -ENOENT; > > *gsi_out = gsi; > *trigger_out = trigger; > diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c > index b616b7768c3b..9715c2f70586 100644 > --- a/drivers/xen/xen-pciback/pci_stub.c > +++ b/drivers/xen/xen-pciback/pci_stub.c > @@ -240,6 +240,9 @@ static int pcistub_get_gsi_from_sbdf(unsigned int sbdf) > if (!psdev) > return -ENODEV; > > + if (psdev->gsi == -1) > + return -ENOENT; > + > return psdev->gsi; > } > #endif > @@ -475,14 +478,14 @@ static int pcistub_init_device(struct pcistub_device *psdev) > #ifdef CONFIG_XEN_ACPI > if (xen_initial_domain() && xen_pvh_domain()) { > err = xen_acpi_get_gsi_info(dev, &gsi, &trigger, &polarity); > - if (err) { > - dev_err(&dev->dev, "Fail to get gsi info!\n"); > - goto config_release; > + if (err && err != -ENOENT) { > + dev_err(&dev->dev, "Failed to get gsi info! %d\n", err); I think here needs " goto config_release;" since it is not ENOENT error. > + } else if (!err) { > + err = xen_pvh_setup_gsi(gsi, trigger, polarity); > + if (err) > + goto config_release; > + psdev->gsi = gsi; > } > - err = xen_pvh_setup_gsi(gsi, trigger, polarity); > - if (err) > - goto config_release; > - psdev->gsi = gsi; > } > #endif >
On 26.02.2025 21:01, Jason Andryuk wrote: > A PCI may not have a legacy IRQ. In that case, do not fail assigning Nit: Missing "device". > to the pciback stub. Instead just skip xen_pvh_setup_gsi(). > > This will leave psdev->gsi == -1. In that case, when reading the value > via IOCTL_PRIVCMD_PCIDEV_GET_GSI, return -ENOENT. Userspace can used Nit: "use". > this to distinquish from other errors. Nit: "distinguish". > --- a/drivers/xen/acpi.c > +++ b/drivers/xen/acpi.c > @@ -101,7 +101,7 @@ int xen_acpi_get_gsi_info(struct pci_dev *dev, > > pin = dev->pin; > if (!pin) > - return -EINVAL; > + return -ENOENT; > > entry = acpi_pci_irq_lookup(dev, pin); > if (entry) { While I can understand this change, ... > @@ -116,7 +116,7 @@ int xen_acpi_get_gsi_info(struct pci_dev *dev, > gsi = -1; > > if (gsi < 0) > - return -EINVAL; > + return -ENOENT; > > *gsi_out = gsi; > *trigger_out = trigger; ... I'd expect this needs to keep using an error code other than ENOENT. Aiui this path means the device has a pin-based interrupt, just that it's not configured correctly. In which case we'd better not allow the device to be handed to a guest. Unless there's logic in place (somewhere) to make sure it then would get to see a device without pin-based interrupt. > --- a/drivers/xen/xen-pciback/pci_stub.c > +++ b/drivers/xen/xen-pciback/pci_stub.c > @@ -240,6 +240,9 @@ static int pcistub_get_gsi_from_sbdf(unsigned int sbdf) > if (!psdev) > return -ENODEV; > > + if (psdev->gsi == -1) > + return -ENOENT; This may, aiui, mean either of the two situations above. They would then need distinguishing, too, if user space is intended to derive decisions from the error code it gets. Jan
On 2025-02-26 22:36, Chen, Jiqian wrote: > On 2025/2/27 04:01, Jason Andryuk wrote: >> @@ -475,14 +478,14 @@ static int pcistub_init_device(struct pcistub_device *psdev) >> #ifdef CONFIG_XEN_ACPI >> if (xen_initial_domain() && xen_pvh_domain()) { >> err = xen_acpi_get_gsi_info(dev, &gsi, &trigger, &polarity); >> - if (err) { >> - dev_err(&dev->dev, "Fail to get gsi info!\n"); >> - goto config_release; >> + if (err && err != -ENOENT) { >> + dev_err(&dev->dev, "Failed to get gsi info! %d\n", err); > I think here needs " goto config_release;" since it is not ENOENT error. Yes, thank you for catching that. Regards, Jason
On 2025-02-27 03:23, Jan Beulich wrote: > On 26.02.2025 21:01, Jason Andryuk wrote: > >> --- a/drivers/xen/acpi.c >> +++ b/drivers/xen/acpi.c >> @@ -101,7 +101,7 @@ int xen_acpi_get_gsi_info(struct pci_dev *dev, >> >> pin = dev->pin; >> if (!pin) >> - return -EINVAL; >> + return -ENOENT; >> >> entry = acpi_pci_irq_lookup(dev, pin); >> if (entry) { > > While I can understand this change, ... > >> @@ -116,7 +116,7 @@ int xen_acpi_get_gsi_info(struct pci_dev *dev, >> gsi = -1; >> >> if (gsi < 0) >> - return -EINVAL; >> + return -ENOENT; >> >> *gsi_out = gsi; >> *trigger_out = trigger; > > ... I'd expect this needs to keep using an error code other than ENOENT. > Aiui this path means the device has a pin-based interrupt, just that it's > not configured correctly. In which case we'd better not allow the device > to be handed to a guest. Unless there's logic in place (somewhere) to > make sure it then would get to see a device without pin-based interrupt. This is actually the case that fails for me. 05:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 06) Interrupt: pin A routed to IRQ -2147483648 Interrupt Pin is 0x01, and Interrupt Line is 0xff I'll have to check the existing code to see what it does. Also, thanks for catching the commit message typos. Regards, Jason
diff --git a/drivers/xen/acpi.c b/drivers/xen/acpi.c index d2ee605c5ca1..d6ab0cb3ba3f 100644 --- a/drivers/xen/acpi.c +++ b/drivers/xen/acpi.c @@ -101,7 +101,7 @@ int xen_acpi_get_gsi_info(struct pci_dev *dev, pin = dev->pin; if (!pin) - return -EINVAL; + return -ENOENT; entry = acpi_pci_irq_lookup(dev, pin); if (entry) { @@ -116,7 +116,7 @@ int xen_acpi_get_gsi_info(struct pci_dev *dev, gsi = -1; if (gsi < 0) - return -EINVAL; + return -ENOENT; *gsi_out = gsi; *trigger_out = trigger; diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c index b616b7768c3b..9715c2f70586 100644 --- a/drivers/xen/xen-pciback/pci_stub.c +++ b/drivers/xen/xen-pciback/pci_stub.c @@ -240,6 +240,9 @@ static int pcistub_get_gsi_from_sbdf(unsigned int sbdf) if (!psdev) return -ENODEV; + if (psdev->gsi == -1) + return -ENOENT; + return psdev->gsi; } #endif @@ -475,14 +478,14 @@ static int pcistub_init_device(struct pcistub_device *psdev) #ifdef CONFIG_XEN_ACPI if (xen_initial_domain() && xen_pvh_domain()) { err = xen_acpi_get_gsi_info(dev, &gsi, &trigger, &polarity); - if (err) { - dev_err(&dev->dev, "Fail to get gsi info!\n"); - goto config_release; + if (err && err != -ENOENT) { + dev_err(&dev->dev, "Failed to get gsi info! %d\n", err); + } else if (!err) { + err = xen_pvh_setup_gsi(gsi, trigger, polarity); + if (err) + goto config_release; + psdev->gsi = gsi; } - err = xen_pvh_setup_gsi(gsi, trigger, polarity); - if (err) - goto config_release; - psdev->gsi = gsi; } #endif
A PCI may not have a legacy IRQ. In that case, do not fail assigning to the pciback stub. Instead just skip xen_pvh_setup_gsi(). This will leave psdev->gsi == -1. In that case, when reading the value via IOCTL_PRIVCMD_PCIDEV_GET_GSI, return -ENOENT. Userspace can used this to distinquish from other errors. Fixes: b166b8ab4189 ("xen/pvh: Setup gsi for passthrough device") Cc: stable@vger.kernel.org Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> --- drivers/xen/acpi.c | 4 ++-- drivers/xen/xen-pciback/pci_stub.c | 17 ++++++++++------- 2 files changed, 12 insertions(+), 9 deletions(-)