| Message ID | 20181016021439.6212-1-yan.y.zhao@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Xen PCI passthrough: fix passthrough failure when irq map failure | expand |
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
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.
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
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.
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
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.
>>> 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.
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. > > >
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) */
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(-)