xen: xen-pciback: Reset MSI-X state when exposing a device

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

[1]: https://lists.xenproject.org/archives/html/xen-devel/2017-09/

     msg02520.html

[2]: https://lists.xen.org/archives/html/xen-devel/2018-11/msg01616.html

Signed-off-by Chao Gao <[email protected]>

---
 drivers/xen/xen-pciback/pci_stub.c | 49 ++++++++++++++++++++++++++++++++++++++
 drivers/xen/xen-pciback/pciback.h  |  1 +
 drivers/xen/xen-pciback/xenbus.c   | 10 ++++++++
 3 files changed, 60 insertions(+)

drivers/xen/xen-pciback/pci_stub.c +16 -0

index 59661db..f8623d0 100644
@@ -87,6 +87,55 @@ static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
 	return psdev;
 }
 
+/*
+ * Reset Xen internal MSI-X state by invoking PHYSDEVOP_{release, prepare}_msix.
+ */
+int pcistub_msix_reset(struct pci_dev *dev)
+{
+#ifdef CONFIG_PCI_MSI
+	if (dev->msix_cap) {
+		struct physdev_pci_device ppdev = {
+			.seg = pci_domain_nr(dev->bus),
+			.bus = dev->bus->number,
+			.devfn = dev->devfn
+		};
+		int err;
+		u16 val;
+
+		/*

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

Quoted 40 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

Quoted 9 lines

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

But that may need taking care of: I don't think it is a good idea to have anything left from the prior owning domain when the device gets reset. I.e. left over IRQ bindings should perhaps be forcibly cleared before invoking the reset;

in fact I'd expect this to happen in the course of
domain destruction, and I'd expect the device reset to come after the domain was cleaned up. Perhaps simply an ordering issue in the tool stack?

Quoted 1 line

Quoted 1 line · On 12.12.18 at 16:18, <[email protected]> wrote:

Quoted 50 lines · Jan Beulich

Quoted 49 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

Quoted 4 lines

But that may need taking care of: I don't think it is a good idea to have anything left from the prior owning domain when the device gets reset. I.e. left over IRQ bindings should perhaps be forcibly cleared before invoking the reset;

Agree. How about pciback to track the established IRQ bindings? Then pciback can clear irq binding before invoking the reset.

Quoted 4 lines

in fact I'd expect this to happen in the course of
domain destruction, and I'd expect the device reset to come after the domain was cleaned up. Perhaps simply an ordering issue in the tool stack?

I don't think reversing the sequences of device reset and domain destruction would be simple. Furthermore, during device hot-unplug, device reset is done when the owner is alive. So if we use domain destruction to enforce all irq binding cleared, in theory, it won't be applicable to hot-unplug case (if qemu's hot-unplug logic is compromised).

Quoted 57 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 16:18, <[email protected]> wrote:

Quoted 54 lines · Jan Beulich

Quoted 40 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

Quoted 9 lines

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

But that may need taking care of: I don't think it is a good idea to have anything left from the prior owning domain when the device gets reset. I.e. left over IRQ bindings should perhaps be forcibly cleared before invoking the reset;

Quoted 2 lines

Agree. How about pciback to track the established IRQ bindings? Then pciback can clear irq binding before invoking the reset.

How would pciback even know of those mappings, when it's qemu who establishes (and manages) them?

Quoted 6 lines

Quoted 5 lines

in fact I'd expect this to happen in the course of
domain destruction, and I'd expect the device reset to come after the domain was cleaned up. Perhaps simply an ordering issue in the tool stack?

Quoted 6 lines

I don't think reversing the sequences of device reset and domain destruction would be simple. Furthermore, during device hot-unplug, device reset is done when the owner is alive. So if we use domain destruction to enforce all irq binding cleared, in theory, it won't be applicable to hot-unplug case (if qemu's hot-unplug logic is compromised).

Even in the hot-unplug case the tool stack could issue unbind requests, behind the back of the possibly compromised qemu, once neither the guest nor qemu have access to the device anymore.

Quoted 1 line

Quoted 1 line · On 13.12.18 at 04:46, <[email protected]> wrote:

Quoted 57 lines · Jan Beulich

Quoted 57 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 16:18, <[email protected]> wrote:

Quoted 54 lines · Jan Beulich

Quoted 40 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

Quoted 9 lines

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

But that may need taking care of: I don't think it is a good idea to have anything left from the prior owning domain when the device gets reset. I.e. left over IRQ bindings should perhaps be forcibly cleared before invoking the reset;

Quoted 5 lines

Quoted 2 lines

Agree. How about pciback to track the established IRQ bindings? Then pciback can clear irq binding before invoking the reset.

How would pciback even know of those mappings, when it's qemu who establishes (and manages) them?

I meant to expose some interfaces from pciback. And pciback serves as the proxy of IRQ (un)binding APIs.

Quoted 12 lines

Quoted 11 lines

Quoted 4 lines

in fact I'd expect this to happen in the course of
domain destruction, and I'd expect the device reset to come after the domain was cleaned up. Perhaps simply an ordering issue in the tool stack?

I don't think reversing the sequences of device reset and domain destruction would be simple. Furthermore, during device hot-unplug, device reset is done when the owner is alive. So if we use domain destruction to enforce all irq binding cleared, in theory, it won't be applicable to hot-unplug case (if qemu's hot-unplug logic is compromised).

Quoted 4 lines

Even in the hot-unplug case the tool stack could issue unbind requests, behind the back of the possibly compromised qemu, once neither the guest nor qemu have access to the device anymore.

But currently, tool stack doesn't know the remaining IRQ bindings. If tool stack can maintaine IRQ binding information of a pass-thru device (stored in Xenstore?), we can come up with a clean solution without modifying linux kernel and Xen.

Quoted 1 line

Quoted 1 line · On 13.12.18 at 04:46, <[email protected]> wrote:

Quoted 57 lines · Jan Beulich

Quoted 57 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 16:18, <[email protected]> wrote:

Quoted 54 lines · Jan Beulich

Quoted 40 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

Quoted 9 lines

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

But that may need taking care of: I don't think it is a good idea to have anything left from the prior owning domain when the device gets reset. I.e. left over IRQ bindings should perhaps be forcibly cleared before invoking the reset;

Quoted 5 lines

Quoted 2 lines

Agree. How about pciback to track the established IRQ bindings? Then pciback can clear irq binding before invoking the reset.

How would pciback even know of those mappings, when it's qemu who establishes (and manages) them?

I meant to expose some interfaces from pciback. And pciback serves as the proxy of IRQ (un)binding APIs.

Quoted 12 lines

Quoted 11 lines

Quoted 4 lines

in fact I'd expect this to happen in the course of
domain destruction, and I'd expect the device reset to come after the domain was cleaned up. Perhaps simply an ordering issue in the tool stack?

I don't think reversing the sequences of device reset and domain destruction would be simple. Furthermore, during device hot-unplug, device reset is done when the owner is alive. So if we use domain destruction to enforce all irq binding cleared, in theory, it won't be applicable to hot-unplug case (if qemu's hot-unplug logic is compromised).

Quoted 4 lines

Even in the hot-unplug case the tool stack could issue unbind requests, behind the back of the possibly compromised qemu, once neither the guest nor qemu have access to the device anymore.

But currently, tool stack doesn't know the remaining IRQ bindings. If tool stack can maintaine IRQ binding information of a pass-thru device (stored in Xenstore?), we can come up with a clean solution without modifying linux kernel and Xen.

Quoted 59 lines

Quoted 1 line

Quoted 1 line · On 13.12.18 at 04:46, <[email protected]> wrote:

Quoted 57 lines · Jan Beulich

Quoted 57 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 16:18, <[email protected]> wrote:

Quoted 54 lines · Jan Beulich

Quoted 40 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

Quoted 9 lines

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

But that may need taking care of: I don't think it is a good idea to have anything left from the prior owning domain when the device gets reset. I.e. left over IRQ bindings should perhaps be forcibly cleared before invoking the reset;

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Agree. How about pciback to track the established IRQ bindings? Then pciback can clear irq binding before invoking the reset.

How would pciback even know of those mappings, when it's qemu who establishes (and manages) them?

I meant to expose some interfaces from pciback. And pciback serves as the proxy of IRQ (un)binding APIs.

If at all possible we should avoid having to change more parties (qemu, libxc, kernel, hypervisor) than really necessary. Remember that such a bug fix may want backporting, and making sure affected people have all relevant components updated is increasingly difficult with their number growing.

Quoted 17 lines

Quoted 12 lines

Quoted 11 lines

Quoted 4 lines

in fact I'd expect this to happen in the course of
domain destruction, and I'd expect the device reset to come after the domain was cleaned up. Perhaps simply an ordering issue in the tool stack?

I don't think reversing the sequences of device reset and domain destruction would be simple. Furthermore, during device hot-unplug, device reset is done when the owner is alive. So if we use domain destruction to enforce all irq binding cleared, in theory, it won't be applicable to hot-unplug case (if qemu's hot-unplug logic is compromised).

Quoted 4 lines

Even in the hot-unplug case the tool stack could issue unbind requests, behind the back of the possibly compromised qemu, once neither the guest nor qemu have access to the device anymore.

Quoted 4 lines

But currently, tool stack doesn't know the remaining IRQ bindings. If tool stack can maintaine IRQ binding information of a pass-thru device (stored in Xenstore?), we can come up with a clean solution without modifying linux kernel and Xen.

If there's no way for the tool stack to either find out the bindings or "blindly" issue unbind requests (accepting them to fail), then a "wildcard" unbind operation may want adding. Or, perhaps even better, XEN_DOMCTL_deassign_device could unbind anything left in place for the specified device.

Quoted 59 lines

Quoted 1 line

Quoted 1 line · On 13.12.18 at 04:46, <[email protected]> wrote:

Quoted 57 lines · Jan Beulich

Quoted 57 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 16:18, <[email protected]> wrote:

Quoted 54 lines · Jan Beulich

Quoted 40 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

Quoted 9 lines

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

But that may need taking care of: I don't think it is a good idea to have anything left from the prior owning domain when the device gets reset. I.e. left over IRQ bindings should perhaps be forcibly cleared before invoking the reset;

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Agree. How about pciback to track the established IRQ bindings? Then pciback can clear irq binding before invoking the reset.

How would pciback even know of those mappings, when it's qemu who establishes (and manages) them?

I meant to expose some interfaces from pciback. And pciback serves as the proxy of IRQ (un)binding APIs.

If at all possible we should avoid having to change more parties (qemu, libxc, kernel, hypervisor) than really necessary. Remember that such a bug fix may want backporting, and making sure affected people have all relevant components updated is increasingly difficult with their number growing.

Quoted 17 lines

Quoted 12 lines

Quoted 11 lines

Quoted 4 lines

in fact I'd expect this to happen in the course of
domain destruction, and I'd expect the device reset to come after the domain was cleaned up. Perhaps simply an ordering issue in the tool stack?

I don't think reversing the sequences of device reset and domain destruction would be simple. Furthermore, during device hot-unplug, device reset is done when the owner is alive. So if we use domain destruction to enforce all irq binding cleared, in theory, it won't be applicable to hot-unplug case (if qemu's hot-unplug logic is compromised).

Quoted 4 lines

Even in the hot-unplug case the tool stack could issue unbind requests, behind the back of the possibly compromised qemu, once neither the guest nor qemu have access to the device anymore.

Quoted 4 lines

But currently, tool stack doesn't know the remaining IRQ bindings. If tool stack can maintaine IRQ binding information of a pass-thru device (stored in Xenstore?), we can come up with a clean solution without modifying linux kernel and Xen.

If there's no way for the tool stack to either find out the bindings or "blindly" issue unbind requests (accepting them to fail), then a "wildcard" unbind operation may want adding. Or, perhaps even better, XEN_DOMCTL_deassign_device could unbind anything left in place for the specified device.

Quoted 57 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 16:18, <[email protected]> wrote:

Quoted 54 lines · Jan Beulich

Quoted 40 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

Quoted 9 lines

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

But that may need taking care of: I don't think it is a good idea to have anything left from the prior owning domain when the device gets reset. I.e. left over IRQ bindings should perhaps be forcibly cleared before invoking the reset;

Quoted 2 lines

Agree. How about pciback to track the established IRQ bindings? Then pciback can clear irq binding before invoking the reset.

How would pciback even know of those mappings, when it's qemu who establishes (and manages) them?

Quoted 6 lines

Quoted 5 lines

in fact I'd expect this to happen in the course of
domain destruction, and I'd expect the device reset to come after the domain was cleaned up. Perhaps simply an ordering issue in the tool stack?

Quoted 6 lines

I don't think reversing the sequences of device reset and domain destruction would be simple. Furthermore, during device hot-unplug, device reset is done when the owner is alive. So if we use domain destruction to enforce all irq binding cleared, in theory, it won't be applicable to hot-unplug case (if qemu's hot-unplug logic is compromised).

Even in the hot-unplug case the tool stack could issue unbind requests, behind the back of the possibly compromised qemu, once neither the guest nor qemu have access to the device anymore.

Quoted 1 line

Quoted 1 line · On 12.12.18 at 16:18, <[email protected]> wrote:

Quoted 50 lines · Jan Beulich

Quoted 49 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

Quoted 4 lines

But that may need taking care of: I don't think it is a good idea to have anything left from the prior owning domain when the device gets reset. I.e. left over IRQ bindings should perhaps be forcibly cleared before invoking the reset;

Agree. How about pciback to track the established IRQ bindings? Then pciback can clear irq binding before invoking the reset.

Quoted 4 lines

in fact I'd expect this to happen in the course of
domain destruction, and I'd expect the device reset to come after the domain was cleaned up. Perhaps simply an ordering issue in the tool stack?

I don't think reversing the sequences of device reset and domain destruction would be simple. Furthermore, during device hot-unplug, device reset is done when the owner is alive. So if we use domain destruction to enforce all irq binding cleared, in theory, it won't be applicable to hot-unplug case (if qemu's hot-unplug logic is compromised).

Quoted 40 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

Quoted 9 lines

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

But that may need taking care of: I don't think it is a good idea to have anything left from the prior owning domain when the device gets reset. I.e. left over IRQ bindings should perhaps be forcibly cleared before invoking the reset;

in fact I'd expect this to happen in the course of
domain destruction, and I'd expect the device reset to come after the domain was cleaned up. Perhaps simply an ordering issue in the tool stack?

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 36 lines

[1]: https://lists.xenproject.org/archives/html/xen-devel/2017-09/

     msg02520.html

[2]: https://lists.xen.org/archives/html/xen-devel/2018-11/msg01616.html

Signed-off-by Chao Gao <[email protected]>

---
 drivers/xen/xen-pciback/pci_stub.c | 49 ++++++++++++++++++++++++++++++++++++++
 drivers/xen/xen-pciback/pciback.h  |  1 +
 drivers/xen/xen-pciback/xenbus.c   | 10 ++++++++
 3 files changed, 60 insertions(+)

drivers/xen/xen-pciback/pci_stub.c +16 -0

index 59661db..f8623d0 100644
@@ -87,6 +87,55 @@ static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
 	return psdev;
 }
 
+/*
+ * Reset Xen internal MSI-X state by invoking PHYSDEVOP_{release, prepare}_msix.
+ */
+int pcistub_msix_reset(struct pci_dev *dev)
+{
+#ifdef CONFIG_PCI_MSI
+	if (dev->msix_cap) {
+		struct physdev_pci_device ppdev = {
+			.seg = pci_domain_nr(dev->bus),
+			.bus = dev->bus->number,
+			.devfn = dev->devfn
+		};
+		int err;
+		u16 val;
+
+		/*

drivers/xen/xen-pciback/pci_stub.c static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev) · line 115

Quoted 10 lines

drivers/xen/xen-pciback/pci_stub.c +10 -0

+		 * Do a write first to flush Xen's internal state to hardware
+		 * such that the following read can infer whether MSI-X maskall
+		 * bit is set by Xen.
+		 */
+		pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &val);
+		pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, val);
+
+		pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &val);
+		if (!(val & PCI_MSIX_FLAGS_MASKALL))
+			return 0;

I would just perform a reset regardless of the maskall value, which would also allow you to skip the read/write dance that you do above.

ATM we are only concerned about the maskall bit, but there's no reason why prepare/release couldn't do more stuff in the future.

drivers/xen/xen-pciback/pci_stub.c static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev) · line 124

Quoted 9 lines

drivers/xen/xen-pciback/pci_stub.c +9 -0

+
+		pr_info("Reset MSI-X state for device %04x:%02x:%02x.%d\n",
+			ppdev.seg, ppdev.bus, PCI_SLOT(ppdev.devfn),
+			PCI_FUNC(ppdev.devfn));
+
+		err = HYPERVISOR_physdev_op(PHYSDEVOP_release_msix, &ppdev);
+		if (err) {
+			dev_warn(&dev->dev, "MSI-X release failed (%d)\n",
+				 err);

This is a warn, while the message below is an err, any reason for the difference in log level?

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 36 lines

[1]: https://lists.xenproject.org/archives/html/xen-devel/2017-09/

     msg02520.html

[2]: https://lists.xen.org/archives/html/xen-devel/2018-11/msg01616.html

Signed-off-by Chao Gao <[email protected]>

---
 drivers/xen/xen-pciback/pci_stub.c | 49 ++++++++++++++++++++++++++++++++++++++
 drivers/xen/xen-pciback/pciback.h  |  1 +
 drivers/xen/xen-pciback/xenbus.c   | 10 ++++++++
 3 files changed, 60 insertions(+)

drivers/xen/xen-pciback/pci_stub.c +16 -0

index 59661db..f8623d0 100644
@@ -87,6 +87,55 @@ static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
 	return psdev;
 }
 
+/*
+ * Reset Xen internal MSI-X state by invoking PHYSDEVOP_{release, prepare}_msix.
+ */
+int pcistub_msix_reset(struct pci_dev *dev)
+{
+#ifdef CONFIG_PCI_MSI
+	if (dev->msix_cap) {
+		struct physdev_pci_device ppdev = {
+			.seg = pci_domain_nr(dev->bus),
+			.bus = dev->bus->number,
+			.devfn = dev->devfn
+		};
+		int err;
+		u16 val;
+
+		/*

drivers/xen/xen-pciback/pci_stub.c static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev) · line 115

Quoted 10 lines

drivers/xen/xen-pciback/pci_stub.c +10 -0

+		 * Do a write first to flush Xen's internal state to hardware
+		 * such that the following read can infer whether MSI-X maskall
+		 * bit is set by Xen.
+		 */
+		pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &val);
+		pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, val);
+
+		pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &val);
+		if (!(val & PCI_MSIX_FLAGS_MASKALL))
+			return 0;

I would just perform a reset regardless of the maskall value, which would also allow you to skip the read/write dance that you do above.

ATM we are only concerned about the maskall bit, but there's no reason why prepare/release couldn't do more stuff in the future.

drivers/xen/xen-pciback/pci_stub.c static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev) · line 124

Quoted 9 lines

drivers/xen/xen-pciback/pci_stub.c +9 -0

+
+		pr_info("Reset MSI-X state for device %04x:%02x:%02x.%d\n",
+			ppdev.seg, ppdev.bus, PCI_SLOT(ppdev.devfn),
+			PCI_FUNC(ppdev.devfn));
+
+		err = HYPERVISOR_physdev_op(PHYSDEVOP_release_msix, &ppdev);
+		if (err) {
+			dev_warn(&dev->dev, "MSI-X release failed (%d)\n",
+				 err);

This is a warn, while the message below is an err, any reason for the difference in log level?

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

[1]: https://lists.xenproject.org/archives/html/xen-devel/2017-09/

     msg02520.html

[2]: https://lists.xen.org/archives/html/xen-devel/2018-11/msg01616.html

Signed-off-by Chao Gao <[email protected]>

---
 drivers/xen/xen-pciback/pci_stub.c | 49 ++++++++++++++++++++++++++++++++++++++
 drivers/xen/xen-pciback/pciback.h  |  1 +
 drivers/xen/xen-pciback/xenbus.c   | 10 ++++++++
 3 files changed, 60 insertions(+)

drivers/xen/xen-pciback/pci_stub.c +16 -0

index 59661db..f8623d0 100644
@@ -87,6 +87,55 @@ static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
 	return psdev;
 }
 
+/*
+ * Reset Xen internal MSI-X state by invoking PHYSDEVOP_{release, prepare}_msix.
+ */
+int pcistub_msix_reset(struct pci_dev *dev)
+{
+#ifdef CONFIG_PCI_MSI
+	if (dev->msix_cap) {
+		struct physdev_pci_device ppdev = {
+			.seg = pci_domain_nr(dev->bus),
+			.bus = dev->bus->number,
+			.devfn = dev->devfn
+		};
+		int err;
+		u16 val;
+
+		/*

Quoted 1 line

Quoted 1 line · On 05.12.18 at 03:19, <[email protected]> wrote:

drivers/xen/xen-pciback/pci_stub.c +16 -0

@@ -87,6 +87,55 @@ static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
 	return psdev;
 }
 
+/*
+ * Reset Xen internal MSI-X state by invoking PHYSDEVOP_{release, prepare}_msix.
+ */
+int pcistub_msix_reset(struct pci_dev *dev)
+{
+#ifdef CONFIG_PCI_MSI
+	if (dev->msix_cap) {
+		struct physdev_pci_device ppdev = {
+			.seg = pci_domain_nr(dev->bus),
+			.bus = dev->bus->number,
+			.devfn = dev->devfn
+		};
+		int err;
+		u16 val;
+
+		/*

drivers/xen/xen-pciback/pci_stub.c +5 -0

+		pr_info("Reset MSI-X state for device %04x:%02x:%02x.%d\n",
+			ppdev.seg, ppdev.bus, PCI_SLOT(ppdev.devfn),
+			PCI_FUNC(ppdev.devfn));
+
+		err = HYPERVISOR_physdev_op(PHYSDEVOP_release_msix, &ppdev);

Quoted 1 line

Quoted 1 line · On 05.12.18 at 03:19, <[email protected]> wrote:

drivers/xen/xen-pciback/pci_stub.c +16 -0

@@ -87,6 +87,55 @@ static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
 	return psdev;
 }
 
+/*
+ * Reset Xen internal MSI-X state by invoking PHYSDEVOP_{release, prepare}_msix.
+ */
+int pcistub_msix_reset(struct pci_dev *dev)
+{
+#ifdef CONFIG_PCI_MSI
+	if (dev->msix_cap) {
+		struct physdev_pci_device ppdev = {
+			.seg = pci_domain_nr(dev->bus),
+			.bus = dev->bus->number,
+			.devfn = dev->devfn
+		};
+		int err;
+		u16 val;
+
+		/*

drivers/xen/xen-pciback/pci_stub.c +5 -0

+		pr_info("Reset MSI-X state for device %04x:%02x:%02x.%d\n",
+			ppdev.seg, ppdev.bus, PCI_SLOT(ppdev.devfn),
+			PCI_FUNC(ppdev.devfn));
+
+		err = HYPERVISOR_physdev_op(PHYSDEVOP_release_msix, &ppdev);

Quoted 96 lines · Jan Beulich

Quoted 59 lines

Quoted 1 line

Quoted 1 line · On 13.12.18 at 04:46, <[email protected]> wrote:

Quoted 57 lines · Jan Beulich

Quoted 57 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 16:18, <[email protected]> wrote:

Quoted 54 lines · Jan Beulich

Quoted 40 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

Quoted 9 lines

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

But that may need taking care of: I don't think it is a good idea to have anything left from the prior owning domain when the device gets reset. I.e. left over IRQ bindings should perhaps be forcibly cleared before invoking the reset;

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Agree. How about pciback to track the established IRQ bindings? Then pciback can clear irq binding before invoking the reset.

How would pciback even know of those mappings, when it's qemu who establishes (and manages) them?

I meant to expose some interfaces from pciback. And pciback serves as the proxy of IRQ (un)binding APIs.

If at all possible we should avoid having to change more parties (qemu, libxc, kernel, hypervisor) than really necessary. Remember that such a bug fix may want backporting, and making sure affected people have all relevant components updated is increasingly difficult with their number growing.

Quoted 17 lines

Quoted 12 lines

Quoted 11 lines

Quoted 4 lines

in fact I'd expect this to happen in the course of
domain destruction, and I'd expect the device reset to come after the domain was cleaned up. Perhaps simply an ordering issue in the tool stack?

I don't think reversing the sequences of device reset and domain destruction would be simple. Furthermore, during device hot-unplug, device reset is done when the owner is alive. So if we use domain destruction to enforce all irq binding cleared, in theory, it won't be applicable to hot-unplug case (if qemu's hot-unplug logic is compromised).

Quoted 4 lines

Even in the hot-unplug case the tool stack could issue unbind requests, behind the back of the possibly compromised qemu, once neither the guest nor qemu have access to the device anymore.

Quoted 4 lines

But currently, tool stack doesn't know the remaining IRQ bindings. If tool stack can maintaine IRQ binding information of a pass-thru device (stored in Xenstore?), we can come up with a clean solution without modifying linux kernel and Xen.

Quoted 5 lines

If there's no way for the tool stack to either find out the bindings or "blindly" issue unbind requests (accepting them to fail), then a "wildcard" unbind operation may want adding. Or, perhaps even better, XEN_DOMCTL_deassign_device could unbind anything left in place for the specified device.

Good idea. I will take this advice.

Thanks
Chao

Quoted 103 lines · Chao Gao

Quoted 101 lines · Jan Beulich

Quoted 59 lines

Quoted 1 line

Quoted 1 line · On 13.12.18 at 04:46, <[email protected]> wrote:

Quoted 57 lines · Jan Beulich

Quoted 57 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 16:18, <[email protected]> wrote:

Quoted 54 lines · Jan Beulich

Quoted 40 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

Quoted 9 lines

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

But that may need taking care of: I don't think it is a good idea to have anything left from the prior owning domain when the device gets reset. I.e. left over IRQ bindings should perhaps be forcibly cleared before invoking the reset;

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Agree. How about pciback to track the established IRQ bindings? Then pciback can clear irq binding before invoking the reset.

How would pciback even know of those mappings, when it's qemu who establishes (and manages) them?

I meant to expose some interfaces from pciback. And pciback serves as the proxy of IRQ (un)binding APIs.

If at all possible we should avoid having to change more parties (qemu, libxc, kernel, hypervisor) than really necessary. Remember that such a bug fix may want backporting, and making sure affected people have all relevant components updated is increasingly difficult with their number growing.

Quoted 17 lines

Quoted 12 lines

Quoted 11 lines

Quoted 4 lines

in fact I'd expect this to happen in the course of
domain destruction, and I'd expect the device reset to come after the domain was cleaned up. Perhaps simply an ordering issue in the tool stack?

I don't think reversing the sequences of device reset and domain destruction would be simple. Furthermore, during device hot-unplug, device reset is done when the owner is alive. So if we use domain destruction to enforce all irq binding cleared, in theory, it won't be applicable to hot-unplug case (if qemu's hot-unplug logic is compromised).

Quoted 4 lines

Even in the hot-unplug case the tool stack could issue unbind requests, behind the back of the possibly compromised qemu, once neither the guest nor qemu have access to the device anymore.

Quoted 4 lines

But currently, tool stack doesn't know the remaining IRQ bindings. If tool stack can maintaine IRQ binding information of a pass-thru device (stored in Xenstore?), we can come up with a clean solution without modifying linux kernel and Xen.

If there's no way for the tool stack to either find out the bindings or "blindly" issue unbind requests (accepting them to fail), then a "wildcard" unbind operation may want adding. Or, perhaps even better, XEN_DOMCTL_deassign_device could unbind anything left in place for the specified device.

Quoted 4 lines

Good idea. I will take this advice.

Thanks
Chao

I am having the same issue, and cannot find a fix in either xen-pciback or the Xen codebase. Was a solution ever pushed as a result of this thread?

Quoted 104 lines · Stanislav

Quoted 103 lines · Chao Gao

Quoted 101 lines · Jan Beulich

Quoted 59 lines

Quoted 1 line

Quoted 1 line · On 13.12.18 at 04:46, <[email protected]> wrote:

Quoted 57 lines · Jan Beulich

Quoted 57 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 16:18, <[email protected]> wrote:

Quoted 54 lines · Jan Beulich

Quoted 40 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 26 lines · Roger Pau Monné

Quoted 17 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot. Assigning it to another guest also meets the same issue. And the only way to make it work again is un-binding and binding it to pciback. Someone reported this issue one year ago [1]. More detail also can be found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly during reboot or re-assignment. In the above case, Xen set maskall bit to mask all MSI interrupts after it detected a potential security issue. Even after device reset, Xen didn't reset its internal maskall bit. As a result, maskall bit would be set again in next write to MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X internal state of a device, we employ it to fix this issue rather than introducing another dedicated sub-hypercall.

Quoted 5 lines

Note that PHYSDEVOPS_release_msix() will fail if the mapping between the device's msix and pirq has been created. This limitation prevents us calling this function when detaching a device from a guest during guest shutdown. Thus it is called right before calling PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

Quoted 9 lines

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

But that may need taking care of: I don't think it is a good idea to have anything left from the prior owning domain when the device gets reset. I.e. left over IRQ bindings should perhaps be forcibly cleared before invoking the reset;

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Agree. How about pciback to track the established IRQ bindings? Then pciback can clear irq binding before invoking the reset.

How would pciback even know of those mappings, when it's qemu who establishes (and manages) them?

I meant to expose some interfaces from pciback. And pciback serves as the proxy of IRQ (un)binding APIs.

If at all possible we should avoid having to change more parties (qemu, libxc, kernel, hypervisor) than really necessary. Remember that such a bug fix may want backporting, and making sure affected people have all relevant components updated is increasingly difficult with their number growing.

Quoted 17 lines

Quoted 12 lines

Quoted 11 lines

Quoted 4 lines

in fact I'd expect this to happen in the course of
domain destruction, and I'd expect the device reset to come after the domain was cleaned up. Perhaps simply an ordering issue in the tool stack?

I don't think reversing the sequences of device reset and domain destruction would be simple. Furthermore, during device hot-unplug, device reset is done when the owner is alive. So if we use domain destruction to enforce all irq binding cleared, in theory, it won't be applicable to hot-unplug case (if qemu's hot-unplug logic is compromised).

Quoted 4 lines

Even in the hot-unplug case the tool stack could issue unbind requests, behind the back of the possibly compromised qemu, once neither the guest nor qemu have access to the device anymore.

Quoted 4 lines

But currently, tool stack doesn't know the remaining IRQ bindings. If tool stack can maintaine IRQ binding information of a pass-thru device (stored in Xenstore?), we can come up with a clean solution without modifying linux kernel and Xen.

If there's no way for the tool stack to either find out the bindings or "blindly" issue unbind requests (accepting them to fail), then a "wildcard" unbind operation may want adding. Or, perhaps even better, XEN_DOMCTL_deassign_device could unbind anything left in place for the specified device.

Quoted 8 lines

Quoted 4 lines

Good idea. I will take this advice.

Thanks
Chao

I am having the same issue, and cannot find a fix in either xen-pciback or the Xen codebase. Was a solution ever pushed as a result of this thread?

I submitted patches [1] to Xen community. But I didn't get it merged. We made a change in device driver to disable MSI-X during guest OS shutdown to mitigate the issue. But when guest or qemu was crashed, we encountered this issue again. I have no plan to get back to these patches. But if you want to fix the issue completely along what the patches below did, please go ahead.

[1]: https://lists.xenproject.org/archives/html/xen-devel/2019-01/msg01227.html

Thanks
Chao

Hello Stanislav,

Quoted 191 lines · Chao Gao

Quoted 171 lines · Stanislav

Quoted 170 lines · Chao Gao

Quoted 168 lines · Jan Beulich

Quoted 109 lines

Quoted 1 line

Quoted 1 line · On 13.12.18 at 04:46, <[email protected]> wrote:

Quoted 105 lines · Jan Beulich

Quoted 104 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 16:18, <[email protected]> wrote:

Quoted 88 lines · Jan Beulich

Quoted 79 lines

Quoted 2 lines

Quoted 2 lines

On 12.12.18 at 08:06, <[email protected]>
wrote:

Quoted 58 lines · Boris Ostrovsky

Quoted 45 lines · Roger Pau Monné

Quoted 37 lines · Chao Gao

I find some pass-thru devices don't work any
more across guest reboot.
Assigning it to another guest also meets the
same issue. And the only
way to make it work again is un-binding and
binding it to pciback.
Someone reported this issue one year ago [1].
More detail also can be
found in [2].

The root-cause is Xen's internal MSI-X state
isn't reset properly
during reboot or re-assignment. In the above
case, Xen set maskall bit
to mask all MSI interrupts after it detected
a potential security
issue. Even after device reset, Xen didn't
reset its internal maskall
bit. As a result, maskall bit would be set
again in next write to
MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also
triggers Xen resetting MSI-X
internal state of a device, we employ it to
fix this issue rather than
introducing another dedicated sub-hypercall.

Note that PHYSDEVOPS_release_msix() will fail
if the mapping between
the device's msix and pirq has been created.
This limitation prevents
us calling this function when detaching a
device from a guest during
guest shutdown. Thus it is called right
before calling
PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then
I would also drop the
() at the end of the hypercall name since it's
not a function.

Quoted 11 lines

I'm also wondering why the release can't be
done when the device is
detached from the guest (or the guest has been
shut down). This makes
me worry about the raciness of the
attach/detach procedure: if there's
a state where pciback assumes the device has
been detached from the
guest, but there are still pirqs bound, an
attempt to attach to
another guest in such state will fail.

Quoted 6 lines

I wonder whether this additional reset
functionality could be done out
of xen_pcibk_xenbus_remove(). We first do a (best
effort) device reset
and then do the extra things that are not
properly done there.

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying
the handler of PHYSDEVOP_release_msix. To do a successful Xen internal
MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished
without error. But ATM, xen expects that no msi is bound to pirq when
doing PHYSDEVOP_release_msix. Otherwise it fails

Quoted 8 lines

with error code -EBUSY.
However, the expectation isn't guaranteed in
xen_pcibk_xenbus_remove().
In some cases, if qemu fails to unmap MSIs, MSIs
are unmapped by Xen
at last minute, which happens after device reset
in
xen_pcibk_xenbus_remove().

Quoted 7 lines

But that may need taking care of: I don't think it is a good idea to have
anything left from the prior owning domain when the device gets reset.
I.e. left over IRQ bindings should perhaps be forcibly cleared before
invoking the reset;

Agree. How about pciback to track the established IRQ bindings? Then
pciback can clear irq binding before invoking the reset.

Quoted 7 lines

Quoted 3 lines

How would pciback even know of those mappings, when it's qemu
who establishes (and manages) them?

I meant to expose some interfaces from pciback. And pciback serves
as the proxy of IRQ (un)binding APIs.

If at all possible we should avoid having to change more parties (qemu,
libxc, kernel, hypervisor) than really necessary. Remember that such
a bug fix may want backporting, and making sure affected people have
all relevant components updated is increasingly difficult with their
number growing.

Quoted 26 lines

Quoted 19 lines

Quoted 7 lines

Quoted 6 lines

in fact I'd expect this to happen in the course of domain destruction, and I'd expect the device reset to come after the
domain was cleaned up. Perhaps simply an ordering issue in the tool
stack?

Quoted 11 lines

I don't think reversing the sequences of device reset and domain
destruction would be simple. Furthermore, during device hot-unplug,
device reset is done when the owner is alive. So if we use domain
destruction to enforce all irq binding cleared, in theory, it won't be
applicable to hot-unplug case (if qemu's hot-unplug logic is
compromised).

Quoted 6 lines

Even in the hot-unplug case the tool stack could issue unbind
requests, behind the back of the possibly compromised qemu,
once neither the guest nor qemu have access to the device anymore.

Quoted 7 lines

But currently, tool stack doesn't know the remaining IRQ bindings.
If tool stack can maintaine IRQ binding information of a pass-thru
device (stored in Xenstore?), we can come up with a clean solution
without modifying linux kernel and Xen.

If there's no way for the tool stack to either find out the bindings
or "blindly" issue unbind requests (accepting them to fail), then a
"wildcard" unbind operation may want adding. Or, perhaps even better, XEN_DOMCTL_deassign_device could unbind anything left in place for the specified device.

Quoted 9 lines

Quoted 4 lines

Good idea. I will take this advice.

Thanks
Chao

I am having the same issue, and cannot find a fix in either xen- pciback or the Xen codebase.
Was a solution ever pushed as a result of this thread?

I submitted patches [1] to Xen community. But I didn't get it merged.
We made a change in device driver to disable MSI-X during guest OS shutdown to mitigate the issue. But when guest or qemu was crashed, we
encountered this issue again. I have no plan to get back to these patches. But if you want to fix the issue completely along what the patches below did, please go ahead.

Quoted 6 lines

[1]:
https://lists.xenproject.org/archives/html/xen-devel/2019-01/msg01227.html

Thanks
Chao

Stanislav: Are you able to continue the work with these patches, to get them merged?

Thanks,

-- Pasi

Hello Stanislav,

Quoted 122 lines · Chao Gao

Quoted 105 lines · Stanislav

Quoted 103 lines · Chao Gao

Quoted 101 lines · Jan Beulich

Quoted 59 lines

Quoted 1 line

Quoted 1 line · On 13.12.18 at 04:46, <[email protected]> wrote:

Quoted 57 lines · Jan Beulich

Quoted 57 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 16:18, <[email protected]> wrote:

Quoted 54 lines · Jan Beulich

Quoted 40 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 26 lines · Roger Pau Monné

Quoted 22 lines · Chao Gao

I find some pass-thru devices don't work any more across guest reboot.
Assigning it to another guest also meets the same issue. And the only
way to make it work again is un-binding and binding it to pciback.
Someone reported this issue one year ago [1]. More detail also can be
found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly
during reboot or re-assignment. In the above case, Xen set maskall bit
to mask all MSI interrupts after it detected a potential security
issue. Even after device reset, Xen didn't reset its internal maskall
bit. As a result, maskall bit would be set again in next write to
MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X
internal state of a device, we employ it to fix this issue rather than
introducing another dedicated sub-hypercall.

Note that PHYSDEVOPS_release_msix() will fail if the mapping between
the device's msix and pirq has been created. This limitation prevents
us calling this function when detaching a device from a guest during
guest shutdown. Thus it is called right before calling
PHYSDEVOPS_prepare_msix().

s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the () at the end of the hypercall name since it's not a function.

Quoted 6 lines

I'm also wondering why the release can't be done when the device is detached from the guest (or the guest has been shut down). This makes me worry about the raciness of the attach/detach procedure: if there's a state where pciback assumes the device has been detached from the guest, but there are still pirqs bound, an attempt to attach to another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

Quoted 9 lines

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

But that may need taking care of: I don't think it is a good idea to have anything left from the prior owning domain when the device gets reset. I.e. left over IRQ bindings should perhaps be forcibly cleared before invoking the reset;

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Agree. How about pciback to track the established IRQ bindings? Then pciback can clear irq binding before invoking the reset.

How would pciback even know of those mappings, when it's qemu who establishes (and manages) them?

I meant to expose some interfaces from pciback. And pciback serves as the proxy of IRQ (un)binding APIs.

If at all possible we should avoid having to change more parties (qemu, libxc, kernel, hypervisor) than really necessary. Remember that such a bug fix may want backporting, and making sure affected people have all relevant components updated is increasingly difficult with their number growing.

Quoted 17 lines

Quoted 12 lines

Quoted 11 lines

Quoted 4 lines

in fact I'd expect this to happen in the course of
domain destruction, and I'd expect the device reset to come after the domain was cleaned up. Perhaps simply an ordering issue in the tool stack?

I don't think reversing the sequences of device reset and domain destruction would be simple. Furthermore, during device hot-unplug, device reset is done when the owner is alive. So if we use domain destruction to enforce all irq binding cleared, in theory, it won't be applicable to hot-unplug case (if qemu's hot-unplug logic is compromised).

Quoted 4 lines

Even in the hot-unplug case the tool stack could issue unbind requests, behind the back of the possibly compromised qemu, once neither the guest nor qemu have access to the device anymore.

Quoted 4 lines

But currently, tool stack doesn't know the remaining IRQ bindings. If tool stack can maintaine IRQ binding information of a pass-thru device (stored in Xenstore?), we can come up with a clean solution without modifying linux kernel and Xen.

If there's no way for the tool stack to either find out the bindings or "blindly" issue unbind requests (accepting them to fail), then a "wildcard" unbind operation may want adding. Or, perhaps even better, XEN_DOMCTL_deassign_device could unbind anything left in place for the specified device.

Quoted 8 lines

Quoted 4 lines

Good idea. I will take this advice.

Thanks
Chao

I am having the same issue, and cannot find a fix in either xen-pciback or the Xen codebase. Was a solution ever pushed as a result of this thread?

I submitted patches [1] to Xen community. But I didn't get it merged. We made a change in device driver to disable MSI-X during guest OS shutdown to mitigate the issue. But when guest or qemu was crashed, we encountered this issue again. I have no plan to get back to these patches. But if you want to fix the issue completely along what the patches below did, please go ahead.

Quoted 5 lines

[1]: https://lists.xenproject.org/archives/html/xen-devel/2019-01/msg01227.html

Thanks
Chao

Stanislav: Are you able to continue the work with these patches, to get them merged?

Quoted 127 lines · Pasi Kärkkäinen

Hello Stanislav,

Quoted 124 lines · Chao Gao

Quoted 105 lines · Stanislav

Quoted 103 lines · Chao Gao

Quoted 101 lines · Jan Beulich

Quoted 59 lines

Quoted 1 line

Quoted 1 line · On 13.12.18 at 04:46, <[email protected]> wrote:

Quoted 57 lines · Jan Beulich

Quoted 57 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 16:18, <[email protected]> wrote:

Quoted 54 lines · Jan Beulich

Quoted 40 lines

Quoted 1 line

Quoted 1 line · On 12.12.18 at 08:06, <[email protected]> wrote:

Quoted 34 lines · Boris Ostrovsky

Quoted 32 lines · Roger Pau Monné

On Wed, Dec 05, 2018 at 10:19:17AM +0800, Chao Gao wrote:
I find some pass-thru devices don't work any more across guest reboot.
Assigning it to another guest also meets the same issue. And the only
way to make it work again is un-binding and binding it to pciback.
Someone reported this issue one year ago [1]. More detail also can be
found in [2].

The root-cause is Xen's internal MSI-X state isn't reset properly
during reboot or re-assignment. In the above case, Xen set maskall bit
to mask all MSI interrupts after it detected a potential security
issue. Even after device reset, Xen didn't reset its internal maskall
bit. As a result, maskall bit would be set again in next write to
MSI-X message control register.

Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X
internal state of a device, we employ it to fix this issue rather than
introducing another dedicated sub-hypercall.

Note that PHYSDEVOPS_release_msix() will fail if the mapping between
the device's msix and pirq has been created. This limitation prevents
us calling this function when detaching a device from a guest during
guest shutdown. Thus it is called right before calling
PHYSDEVOPS_prepare_msix().
s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the
() at the end of the hypercall name since it's not a function.

I'm also wondering why the release can't be done when the device is
detached from the guest (or the guest has been shut down). This makes
me worry about the raciness of the attach/detach procedure: if there's
a state where pciback assumes the device has been detached from the
guest, but there are still pirqs bound, an attempt to attach to
another guest in such state will fail.

Quoted 3 lines

I wonder whether this additional reset functionality could be done out of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset and then do the extra things that are not properly done there.

Quoted 9 lines

No. It cannot be done in xen_pcibk_xenbus_remove() without modifying the handler of PHYSDEVOP_release_msix. To do a successful Xen internal MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished without error. But ATM, xen expects that no msi is bound to pirq when doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY. However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove(). In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen at last minute, which happens after device reset in xen_pcibk_xenbus_remove().

But that may need taking care of: I don't think it is a good idea to have anything left from the prior owning domain when the device gets reset. I.e. left over IRQ bindings should perhaps be forcibly cleared before invoking the reset;

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Agree. How about pciback to track the established IRQ bindings? Then pciback can clear irq binding before invoking the reset.

How would pciback even know of those mappings, when it's qemu who establishes (and manages) them?

I meant to expose some interfaces from pciback. And pciback serves as the proxy of IRQ (un)binding APIs.

If at all possible we should avoid having to change more parties (qemu, libxc, kernel, hypervisor) than really necessary. Remember that such a bug fix may want backporting, and making sure affected people have all relevant components updated is increasingly difficult with their number growing.

Quoted 17 lines

Quoted 12 lines

Quoted 11 lines

Quoted 4 lines

in fact I'd expect this to happen in the course of
domain destruction, and I'd expect the device reset to come after the domain was cleaned up. Perhaps simply an ordering issue in the tool stack?

I don't think reversing the sequences of device reset and domain destruction would be simple. Furthermore, during device hot-unplug, device reset is done when the owner is alive. So if we use domain destruction to enforce all irq binding cleared, in theory, it won't be applicable to hot-unplug case (if qemu's hot-unplug logic is compromised).

Quoted 4 lines

Even in the hot-unplug case the tool stack could issue unbind requests, behind the back of the possibly compromised qemu, once neither the guest nor qemu have access to the device anymore.

Quoted 4 lines

But currently, tool stack doesn't know the remaining IRQ bindings. If tool stack can maintaine IRQ binding information of a pass-thru device (stored in Xenstore?), we can come up with a clean solution without modifying linux kernel and Xen.

If there's no way for the tool stack to either find out the bindings or "blindly" issue unbind requests (accepting them to fail), then a "wildcard" unbind operation may want adding. Or, perhaps even better, XEN_DOMCTL_deassign_device could unbind anything left in place for the specified device.

Quoted 8 lines

Quoted 4 lines

Good idea. I will take this advice.

Thanks
Chao

I am having the same issue, and cannot find a fix in either xen-pciback or the Xen codebase. Was a solution ever pushed as a result of this thread?

I submitted patches [1] to Xen community. But I didn't get it merged. We made a change in device driver to disable MSI-X during guest OS shutdown to mitigate the issue. But when guest or qemu was crashed, we encountered this issue again. I have no plan to get back to these patches. But if you want to fix the issue completely along what the patches below did, please go ahead.

[1]: https://lists.xenproject.org/archives/html/xen-devel/2019-01/msg01227.html

Quoted 5 lines

Quoted 3 lines

Thanks
Chao

Stanislav: Are you able to continue the work with these patches, to get them merged?

What further work is needed for these patches? Are they only needed for Intel i210 NIC PCI passthrough, or are other devices affected?