* [Qemu-devel] [PATCH] Xen PCI passthrough: fix passthrough failure when irq map failure @ 2018-10-16 2:14 Zhao Yan 2018-10-18 8:22 ` Zhao, Yan Y 0 siblings, 1 reply; 9+ messages in thread From: Zhao Yan @ 2018-10-16 2:14 UTC (permalink / raw) To: qemu-devel, sstabellini, anthony.perard, xen-devel; +Cc: Zhao Yan Commit 5a11d0f7 mistakenly converted a log message into an error condition when irq map is failed for the pci device being passed through. Revert that part of the commit. Signed-off-by: Zhao Yan <yan.y.zhao@intel.com> --- hw/xen/xen_pt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c index e5a6eff44f..840fd0f748 100644 --- a/hw/xen/xen_pt.c +++ b/hw/xen/xen_pt.c @@ -849,7 +849,7 @@ static void xen_pt_realize(PCIDevice *d, Error **errp) machine_irq = s->real_device.irq; rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq); if (rc < 0) { - error_setg_errno(errp, errno, "Mapping machine irq %u to" + XEN_PT_ERR(d, "Mapping machine irq %u to" " pirq %i failed", machine_irq, pirq); /* Disable PCI intx assertion (turn on bit10 of devctl) */ @@ -871,7 +871,7 @@ static void xen_pt_realize(PCIDevice *d, Error **errp) PCI_SLOT(d->devfn), e_intx); if (rc < 0) { - error_setg_errno(errp, errno, "Binding of interrupt %u failed", + XEN_PT_ERR(d, "Binding of interrupt %u failed", e_intx); /* Disable PCI intx assertion (turn on bit10 of devctl) */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Xen PCI passthrough: fix passthrough failure when irq map failure 2018-10-16 2:14 [Qemu-devel] [PATCH] Xen PCI passthrough: fix passthrough failure when irq map failure Zhao Yan @ 2018-10-18 8:22 ` Zhao, Yan Y 2018-10-18 14:56 ` Roger Pau Monné 0 siblings, 1 reply; 9+ messages in thread From: Zhao, Yan Y @ 2018-10-18 8:22 UTC (permalink / raw) To: sstabellini@kernel.org, anthony.perard@citrix.com Cc: qemu-devel@nongnu.org, xen-devel@lists.xenproject.org Hi The background for this patch is that: for some pci device, even it's PCI_INTERRUPT_PIN is not 0, it actually does not support INTx mode, so we should just report error, disable INTx mode and continue the passthrough. However, the commit 5a11d0f7 regards this as error condition and let qemu quit passthrough, which is too rigorous. Error message is below: libxl: error: libxl_qmp.c:287:qmp_handle_error_response: Domain 2:received an error message from QMP server: Mapping machine irq 0 to pirq -1 failed: Operation not permitted libxl: error: libxl_pci.c:1306:libxl__add_pcidevs: Domain 2:libxl_device_pci_add failed: -3 libxl: error: libxl_create.c:1458:domcreate_attach_devices: Domain 2:unable to add pci devices libxl: error: libxl_domain.c:1003:libxl__destroy_domid: Domain 2:Non-existant domain libxl: error: libxl_domain.c:962:domain_destroy_callback: Domain 2:Unable to destroy guest libxl: error: libxl_domain.c:889:domain_destroy_cb: Domain 2:Destruction of domain failed After partially revert 5a11d0f7, the device can be passed through into domU and running quite well using msi mode. > -----Original Message----- > From: Zhao, Yan Y > Sent: Tuesday, October 16, 2018 10:15 AM > To: qemu-devel@nongnu.org; sstabellini@kernel.org; > anthony.perard@citrix.com; xen-devel@lists.xenproject.org > Cc: Zhao, Yan Y <yan.y.zhao@intel.com> > Subject: [PATCH] Xen PCI passthrough: fix passthrough failure when irq map > failure > > Commit 5a11d0f7 mistakenly converted a log message into an error condition > when irq map is failed for the pci device being passed through. Revert that part > of the commit. > > Signed-off-by: Zhao Yan <yan.y.zhao@intel.com> > --- > hw/xen/xen_pt.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c index e5a6eff44f..840fd0f748 > 100644 > --- a/hw/xen/xen_pt.c > +++ b/hw/xen/xen_pt.c > @@ -849,7 +849,7 @@ static void xen_pt_realize(PCIDevice *d, Error **errp) > machine_irq = s->real_device.irq; > rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq); > if (rc < 0) { > - error_setg_errno(errp, errno, "Mapping machine irq %u to" > + XEN_PT_ERR(d, "Mapping machine irq %u to" > " pirq %i failed", machine_irq, pirq); > > /* Disable PCI intx assertion (turn on bit10 of devctl) */ @@ -871,7 +871,7 > @@ static void xen_pt_realize(PCIDevice *d, Error **errp) > PCI_SLOT(d->devfn), > e_intx); > if (rc < 0) { > - error_setg_errno(errp, errno, "Binding of interrupt %u failed", > + XEN_PT_ERR(d, "Binding of interrupt %u failed", > e_intx); > > /* Disable PCI intx assertion (turn on bit10 of devctl) */ > -- > 2.17.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Xen PCI passthrough: fix passthrough failure when irq map failure 2018-10-18 8:22 ` Zhao, Yan Y @ 2018-10-18 14:56 ` Roger Pau Monné 2018-11-22 13:11 ` Zhao Yan 0 siblings, 1 reply; 9+ messages in thread From: Roger Pau Monné @ 2018-10-18 14:56 UTC (permalink / raw) To: Zhao, Yan Y Cc: sstabellini@kernel.org, anthony.perard@citrix.com, xen-devel@lists.xenproject.org, qemu-devel@nongnu.org On Thu, Oct 18, 2018 at 08:22:41AM +0000, Zhao, Yan Y wrote: > Hi > The background for this patch is that: for some pci device, even it's PCI_INTERRUPT_PIN is not 0, it actually does not support INTx mode, so we should just report error, disable INTx mode and continue the passthrough. > However, the commit 5a11d0f7 regards this as error condition and let qemu quit passthrough, which is too rigorous. > > Error message is below: > libxl: error: libxl_qmp.c:287:qmp_handle_error_response: Domain 2:received an error message from QMP server: Mapping machine irq 0 to pirq -1 failed: Operation not permitted I'm having issues figuring out what's happening here. s->real_device.irq is 0, yet the PCI config space read of PCI_INTERRUPT_PIN returns something different than 0. AFAICT this is due to some kind of error in Linux, so that even when the device is supposed to have a valid IRQ the sysfs node it is set to 0, do you know the actual underlying cause of this? Thanks, Roger. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Xen PCI passthrough: fix passthrough failure when irq map failure 2018-10-18 14:56 ` Roger Pau Monné @ 2018-11-22 13:11 ` Zhao Yan 2018-11-22 14:18 ` Roger Pau Monné 0 siblings, 1 reply; 9+ messages in thread From: Zhao Yan @ 2018-11-22 13:11 UTC (permalink / raw) To: Roger Pau Monné; +Cc: sstabellini, anthony.perard, xen-devel, qemu-devel On Thu, Oct 18, 2018 at 03:56:36PM +0100, Roger Pau Monné wrote: > On Thu, Oct 18, 2018 at 08:22:41AM +0000, Zhao, Yan Y wrote: > > Hi > > The background for this patch is that: for some pci device, even it's PCI_INTERRUPT_PIN is not 0, it actually does not support INTx mode, so we should just report error, disable INTx mode and continue the passthrough. > > However, the commit 5a11d0f7 regards this as error condition and let qemu quit passthrough, which is too rigorous. > > > > Error message is below: > > libxl: error: libxl_qmp.c:287:qmp_handle_error_response: Domain 2:received an error message from QMP server: Mapping machine irq 0 to pirq -1 failed: Operation not permitted > > I'm having issues figuring out what's happening here. > s->real_device.irq is 0, yet the PCI config space read of > PCI_INTERRUPT_PIN returns something different than 0. > > AFAICT this is due to some kind of error in Linux, so that even when > the device is supposed to have a valid IRQ the sysfs node it is set to > 0, do you know the actual underlying cause of this? > > Thanks, Roger. Hi Roger Sorry for the later reply, I just missed this mail... On my side, it's because the hardware actually does not support INTx mode, but its configuration space does not report PCI_INTERRUPT_PIN to 0. It's a hardware bug, but previous version of qemu can tolerate it, switch to MSI and make passthrough work. Thanks Yan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Xen PCI passthrough: fix passthrough failure when irq map failure 2018-11-22 13:11 ` Zhao Yan @ 2018-11-22 14:18 ` Roger Pau Monné 2018-11-23 5:04 ` Zhao Yan 0 siblings, 1 reply; 9+ messages in thread From: Roger Pau Monné @ 2018-11-22 14:18 UTC (permalink / raw) To: Zhao Yan; +Cc: sstabellini, anthony.perard, xen-devel, qemu-devel On Thu, Nov 22, 2018 at 08:11:20AM -0500, Zhao Yan wrote: > On Thu, Oct 18, 2018 at 03:56:36PM +0100, Roger Pau Monné wrote: > > On Thu, Oct 18, 2018 at 08:22:41AM +0000, Zhao, Yan Y wrote: > > > Hi > > > The background for this patch is that: for some pci device, even it's PCI_INTERRUPT_PIN is not 0, it actually does not support INTx mode, so we should just report error, disable INTx mode and continue the passthrough. > > > However, the commit 5a11d0f7 regards this as error condition and let qemu quit passthrough, which is too rigorous. > > > > > > Error message is below: > > > libxl: error: libxl_qmp.c:287:qmp_handle_error_response: Domain 2:received an error message from QMP server: Mapping machine irq 0 to pirq -1 failed: Operation not permitted > > > > I'm having issues figuring out what's happening here. > > s->real_device.irq is 0, yet the PCI config space read of > > PCI_INTERRUPT_PIN returns something different than 0. > > > > AFAICT this is due to some kind of error in Linux, so that even when > > the device is supposed to have a valid IRQ the sysfs node it is set to > > 0, do you know the actual underlying cause of this? > > > > Thanks, Roger. > Hi Roger > Sorry for the later reply, I just missed this mail... > On my side, it's because the hardware actually does not support INTx mode, > but its configuration space does not report PCI_INTERRUPT_PIN to 0. It's a > hardware bug, but previous version of qemu can tolerate it, switch to MSI > and make passthrough work. Then I think it would be better to check both PCI_INTERRUPT_PIN and s->real_device.irq before attempting to map the IRQ. Making the error non-fatal would mean that a device with a valid IRQ could fail to be setup correctly but the guest will still be created, and things won't go as expected when the guest attempts to use it. Thanks, Roger. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Xen PCI passthrough: fix passthrough failure when irq map failure 2018-11-22 14:18 ` Roger Pau Monné @ 2018-11-23 5:04 ` Zhao Yan 2018-11-23 10:19 ` Roger Pau Monné 0 siblings, 1 reply; 9+ messages in thread From: Zhao Yan @ 2018-11-23 5:04 UTC (permalink / raw) To: Roger Pau Monné; +Cc: sstabellini, anthony.perard, xen-devel, qemu-devel On Thu, Nov 22, 2018 at 03:18:05PM +0100, Roger Pau Monné wrote: > On Thu, Nov 22, 2018 at 08:11:20AM -0500, Zhao Yan wrote: > > On Thu, Oct 18, 2018 at 03:56:36PM +0100, Roger Pau Monné wrote: > > > On Thu, Oct 18, 2018 at 08:22:41AM +0000, Zhao, Yan Y wrote: > > > > Hi > > > > The background for this patch is that: for some pci device, even it's PCI_INTERRUPT_PIN is not 0, it actually does not support INTx mode, so we should just report error, disable INTx mode and continue the passthrough. > > > > However, the commit 5a11d0f7 regards this as error condition and let qemu quit passthrough, which is too rigorous. > > > > > > > > Error message is below: > > > > libxl: error: libxl_qmp.c:287:qmp_handle_error_response: Domain 2:received an error message from QMP server: Mapping machine irq 0 to pirq -1 failed: Operation not permitted > > > > > > I'm having issues figuring out what's happening here. > > > s->real_device.irq is 0, yet the PCI config space read of > > > PCI_INTERRUPT_PIN returns something different than 0. > > > > > > AFAICT this is due to some kind of error in Linux, so that even when > > > the device is supposed to have a valid IRQ the sysfs node it is set to > > > 0, do you know the actual underlying cause of this? > > > > > > Thanks, Roger. > > Hi Roger > > Sorry for the later reply, I just missed this mail... > > On my side, it's because the hardware actually does not support INTx mode, > > but its configuration space does not report PCI_INTERRUPT_PIN to 0. It's a > > hardware bug, but previous version of qemu can tolerate it, switch to MSI > > and make passthrough work. > > Then I think it would be better to check both PCI_INTERRUPT_PIN and > s->real_device.irq before attempting to map the IRQ. > > Making the error non-fatal would mean that a device with a valid IRQ > could fail to be setup correctly but the guest will still be created, > and things won't go as expected when the guest attempts to use it. > > Thanks, Roger. hi roger thanks for your sugguestion. it's right that "s->real_device.irq" is needed to be checked before mapping, like if it's 0. but on the other hand, maybe xc_physdev_map_pirq() itself can serve as a checking of "s->real_device.irq" ? like in our case, it will fail and return -EPERM. then error hanling is still conducted ==>set INTX_DISABLE flag, eventhrough the error is not fatal. machine_irq = s->real_device.irq; rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq); if (rc < 0) { error_setg_errno(errp, errno, "Mapping machine irq %u to" " pirq %i failed", machine_irq, pirq); /* Disable PCI intx assertion (turn on bit10 of devctl) */ cmd |= PCI_COMMAND_INTX_DISABLE; machine_irq = 0; s->machine_irq = 0; So, do you think it's all right just converting fatal error to non-fatal? Thanks Yan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Xen PCI passthrough: fix passthrough failure when irq map failure 2018-11-23 5:04 ` Zhao Yan @ 2018-11-23 10:19 ` Roger Pau Monné 2018-11-23 10:26 ` Jan Beulich 0 siblings, 1 reply; 9+ messages in thread From: Roger Pau Monné @ 2018-11-23 10:19 UTC (permalink / raw) To: Zhao Yan; +Cc: sstabellini, anthony.perard, xen-devel, qemu-devel, JBeulich Adding Jan in case he has an opinion on my reply below. On Fri, Nov 23, 2018 at 12:04:51AM -0500, Zhao Yan wrote: > On Thu, Nov 22, 2018 at 03:18:05PM +0100, Roger Pau Monné wrote: > > On Thu, Nov 22, 2018 at 08:11:20AM -0500, Zhao Yan wrote: > > > On Thu, Oct 18, 2018 at 03:56:36PM +0100, Roger Pau Monné wrote: > > > > On Thu, Oct 18, 2018 at 08:22:41AM +0000, Zhao, Yan Y wrote: > > > > > Hi > > > > > The background for this patch is that: for some pci device, even it's PCI_INTERRUPT_PIN is not 0, it actually does not support INTx mode, so we should just report error, disable INTx mode and continue the passthrough. > > > > > However, the commit 5a11d0f7 regards this as error condition and let qemu quit passthrough, which is too rigorous. > > > > > > > > > > Error message is below: > > > > > libxl: error: libxl_qmp.c:287:qmp_handle_error_response: Domain 2:received an error message from QMP server: Mapping machine irq 0 to pirq -1 failed: Operation not permitted > > > > > > > > I'm having issues figuring out what's happening here. > > > > s->real_device.irq is 0, yet the PCI config space read of > > > > PCI_INTERRUPT_PIN returns something different than 0. > > > > > > > > AFAICT this is due to some kind of error in Linux, so that even when > > > > the device is supposed to have a valid IRQ the sysfs node it is set to > > > > 0, do you know the actual underlying cause of this? > > > > > > > > Thanks, Roger. > > > Hi Roger > > > Sorry for the later reply, I just missed this mail... > > > On my side, it's because the hardware actually does not support INTx mode, > > > but its configuration space does not report PCI_INTERRUPT_PIN to 0. It's a > > > hardware bug, but previous version of qemu can tolerate it, switch to MSI > > > and make passthrough work. > > > > Then I think it would be better to check both PCI_INTERRUPT_PIN and > > s->real_device.irq before attempting to map the IRQ. > > > > Making the error non-fatal would mean that a device with a valid IRQ > > could fail to be setup correctly but the guest will still be created, > > and things won't go as expected when the guest attempts to use it. > > > > Thanks, Roger. > hi roger > thanks for your sugguestion. it's right that "s->real_device.irq" is needed to be checked before mapping, like if it's 0. > but on the other hand, maybe xc_physdev_map_pirq() itself can serve as a checking of "s->real_device.irq" ? > like in our case, it will fail and return -EPERM. > then error hanling is still conducted ==>set INTX_DISABLE flag, eventhrough the error is not fatal. > > machine_irq = s->real_device.irq; > rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq); > if (rc < 0) { > error_setg_errno(errp, errno, "Mapping machine irq %u to" > " pirq %i failed", machine_irq, pirq); > > /* Disable PCI intx assertion (turn on bit10 of devctl) */ > cmd |= PCI_COMMAND_INTX_DISABLE; > machine_irq = 0; > s->machine_irq = 0; > So, do you think it's all right just converting fatal error to non-fatal? As I said above, I think it would be better to leave the error as fatal and avoid attempting a xc_physdev_map_pirq with a machine_irq == 0, which will fail. If we really want to go down the route of making the error non-fatal, I think you will also have to report PCI_INTERRUPT_PIN as 0 to the guest, so that it's clear to the guest that the device doesn't have legacy interrupt support. Exposing a device with PCI_INTERRUPT_PIN != 0 but then not allowing the guest to clear PCI_COMMAND_INTX_DISABLE is likely bogus. Thanks, Roger. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Xen PCI passthrough: fix passthrough failure when irq map failure 2018-11-23 10:19 ` Roger Pau Monné @ 2018-11-23 10:26 ` Jan Beulich 2018-11-26 1:58 ` Zhao Yan 0 siblings, 1 reply; 9+ messages in thread From: Jan Beulich @ 2018-11-23 10:26 UTC (permalink / raw) To: Roger Pau Monne, Zhao Yan Cc: Anthony Perard, Stefano Stabellini, xen-devel, qemu-devel >>> On 23.11.18 at 11:19, <roger.pau@citrix.com> wrote: > Adding Jan in case he has an opinion on my reply below. I agree, fwiw. Jan > On Fri, Nov 23, 2018 at 12:04:51AM -0500, Zhao Yan wrote: >> On Thu, Nov 22, 2018 at 03:18:05PM +0100, Roger Pau Monné wrote: >> > On Thu, Nov 22, 2018 at 08:11:20AM -0500, Zhao Yan wrote: >> > > On Thu, Oct 18, 2018 at 03:56:36PM +0100, Roger Pau Monné wrote: >> > > > On Thu, Oct 18, 2018 at 08:22:41AM +0000, Zhao, Yan Y wrote: >> > > > > Hi >> > > > > The background for this patch is that: for some pci device, even it's PCI_INTERRUPT_PIN is not 0, it actually does not support INTx mode, so we should just report error, disable INTx mode and continue the passthrough. >> > > > > However, the commit 5a11d0f7 regards this as error condition and let qemu quit passthrough, which is too rigorous. >> > > > > >> > > > > Error message is below: >> > > > > libxl: error: libxl_qmp.c:287:qmp_handle_error_response: Domain 2:received an error message from QMP server: Mapping machine irq 0 to pirq -1 failed: Operation not permitted >> > > > >> > > > I'm having issues figuring out what's happening here. >> > > > s->real_device.irq is 0, yet the PCI config space read of >> > > > PCI_INTERRUPT_PIN returns something different than 0. >> > > > >> > > > AFAICT this is due to some kind of error in Linux, so that even when >> > > > the device is supposed to have a valid IRQ the sysfs node it is set to >> > > > 0, do you know the actual underlying cause of this? >> > > > >> > > > Thanks, Roger. >> > > Hi Roger >> > > Sorry for the later reply, I just missed this mail... >> > > On my side, it's because the hardware actually does not support INTx mode, >> > > but its configuration space does not report PCI_INTERRUPT_PIN to 0. It's a >> > > hardware bug, but previous version of qemu can tolerate it, switch to MSI >> > > and make passthrough work. >> > >> > Then I think it would be better to check both PCI_INTERRUPT_PIN and >> > s->real_device.irq before attempting to map the IRQ. >> > >> > Making the error non-fatal would mean that a device with a valid IRQ >> > could fail to be setup correctly but the guest will still be created, >> > and things won't go as expected when the guest attempts to use it. >> > >> > Thanks, Roger. >> hi roger >> thanks for your sugguestion. it's right that "s->real_device.irq" is needed to be checked before mapping, like if it's 0. >> but on the other hand, maybe xc_physdev_map_pirq() itself can serve as a checking of "s->real_device.irq" ? >> like in our case, it will fail and return -EPERM. >> then error hanling is still conducted ==>set INTX_DISABLE flag, eventhrough the error is not fatal. >> >> machine_irq = s->real_device.irq; >> rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq); >> if (rc < 0) { >> error_setg_errno(errp, errno, "Mapping machine irq %u to" >> " pirq %i failed", machine_irq, pirq); >> >> /* Disable PCI intx assertion (turn on bit10 of devctl) */ >> cmd |= PCI_COMMAND_INTX_DISABLE; >> machine_irq = 0; >> s->machine_irq = 0; >> So, do you think it's all right just converting fatal error to non-fatal? > > As I said above, I think it would be better to leave the error as > fatal and avoid attempting a xc_physdev_map_pirq with a machine_irq == > 0, which will fail. > > If we really want to go down the route of making the error non-fatal, > I think you will also have to report PCI_INTERRUPT_PIN as 0 to the > guest, so that it's clear to the guest that the device doesn't have > legacy interrupt support. > > Exposing a device with PCI_INTERRUPT_PIN != 0 but then not allowing > the guest to clear PCI_COMMAND_INTX_DISABLE is likely bogus. > > Thanks, Roger. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Xen PCI passthrough: fix passthrough failure when irq map failure 2018-11-23 10:26 ` Jan Beulich @ 2018-11-26 1:58 ` Zhao Yan 0 siblings, 0 replies; 9+ messages in thread From: Zhao Yan @ 2018-11-26 1:58 UTC (permalink / raw) To: Jan Beulich Cc: Roger Pau Monne, Anthony Perard, Stefano Stabellini, xen-devel, qemu-devel Hi Roger and Jan, Thanks for your review. You are right. I'll try the way to report PCI_INTERRUPT_PIN as 0 to the guest. Thanks Yan On Fri, Nov 23, 2018 at 03:26:44AM -0700, Jan Beulich wrote: > >>> On 23.11.18 at 11:19, <roger.pau@citrix.com> wrote: > > Adding Jan in case he has an opinion on my reply below. > > I agree, fwiw. > > Jan > > > On Fri, Nov 23, 2018 at 12:04:51AM -0500, Zhao Yan wrote: > >> On Thu, Nov 22, 2018 at 03:18:05PM +0100, Roger Pau Monné wrote: > >> > On Thu, Nov 22, 2018 at 08:11:20AM -0500, Zhao Yan wrote: > >> > > On Thu, Oct 18, 2018 at 03:56:36PM +0100, Roger Pau Monné wrote: > >> > > > On Thu, Oct 18, 2018 at 08:22:41AM +0000, Zhao, Yan Y wrote: > >> > > > > Hi > >> > > > > The background for this patch is that: for some pci device, even it's PCI_INTERRUPT_PIN is not 0, it > actually does not support INTx mode, so we should just report error, disable INTx mode and continue the passthrough. > >> > > > > However, the commit 5a11d0f7 regards this as error condition and let qemu quit passthrough, which is too > rigorous. > >> > > > > > >> > > > > Error message is below: > >> > > > > libxl: error: libxl_qmp.c:287:qmp_handle_error_response: Domain 2:received an error message from QMP server: > Mapping machine irq 0 to pirq -1 failed: Operation not permitted > >> > > > > >> > > > I'm having issues figuring out what's happening here. > >> > > > s->real_device.irq is 0, yet the PCI config space read of > >> > > > PCI_INTERRUPT_PIN returns something different than 0. > >> > > > > >> > > > AFAICT this is due to some kind of error in Linux, so that even when > >> > > > the device is supposed to have a valid IRQ the sysfs node it is set to > >> > > > 0, do you know the actual underlying cause of this? > >> > > > > >> > > > Thanks, Roger. > >> > > Hi Roger > >> > > Sorry for the later reply, I just missed this mail... > >> > > On my side, it's because the hardware actually does not support INTx mode, > >> > > but its configuration space does not report PCI_INTERRUPT_PIN to 0. It's a > >> > > hardware bug, but previous version of qemu can tolerate it, switch to MSI > >> > > and make passthrough work. > >> > > >> > Then I think it would be better to check both PCI_INTERRUPT_PIN and > >> > s->real_device.irq before attempting to map the IRQ. > >> > > >> > Making the error non-fatal would mean that a device with a valid IRQ > >> > could fail to be setup correctly but the guest will still be created, > >> > and things won't go as expected when the guest attempts to use it. > >> > > >> > Thanks, Roger. > >> hi roger > >> thanks for your sugguestion. it's right that "s->real_device.irq" is needed to be checked before mapping, like if > it's 0. > >> but on the other hand, maybe xc_physdev_map_pirq() itself can serve as a checking of "s->real_device.irq" ? > >> like in our case, it will fail and return -EPERM. > >> then error hanling is still conducted ==>set INTX_DISABLE flag, eventhrough the error is not fatal. > >> > >> machine_irq = s->real_device.irq; > >> rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq); > >> if (rc < 0) { > >> error_setg_errno(errp, errno, "Mapping machine irq %u to" > >> " pirq %i failed", machine_irq, pirq); > >> > >> /* Disable PCI intx assertion (turn on bit10 of devctl) */ > >> cmd |= PCI_COMMAND_INTX_DISABLE; > >> machine_irq = 0; > >> s->machine_irq = 0; > >> So, do you think it's all right just converting fatal error to non-fatal? > > > > As I said above, I think it would be better to leave the error as > > fatal and avoid attempting a xc_physdev_map_pirq with a machine_irq == > > 0, which will fail. > > > > If we really want to go down the route of making the error non-fatal, > > I think you will also have to report PCI_INTERRUPT_PIN as 0 to the > > guest, so that it's clear to the guest that the device doesn't have > > legacy interrupt support. > > > > Exposing a device with PCI_INTERRUPT_PIN != 0 but then not allowing > > the guest to clear PCI_COMMAND_INTX_DISABLE is likely bogus. > > > > Thanks, Roger. > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-11-26 2:02 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-10-16 2:14 [Qemu-devel] [PATCH] Xen PCI passthrough: fix passthrough failure when irq map failure Zhao Yan 2018-10-18 8:22 ` Zhao, Yan Y 2018-10-18 14:56 ` Roger Pau Monné 2018-11-22 13:11 ` Zhao Yan 2018-11-22 14:18 ` Roger Pau Monné 2018-11-23 5:04 ` Zhao Yan 2018-11-23 10:19 ` Roger Pau Monné 2018-11-23 10:26 ` Jan Beulich 2018-11-26 1:58 ` Zhao Yan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).