| Message ID | 20231114143816.71079-7-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/xen: Have most of Xen files become target-agnostic | expand |
On 14 November 2023 09:38:02 GMT-05:00, "Philippe Mathieu-Daudé" <philmd@linaro.org> wrote: >Similarly to the restriction in hw/pci/msix.c (see commit >e1e4bf2252 "msix: fix msix_vector_masked"), restrict the >xen_is_pirq_msi() call in msi_is_masked() to Xen. > >Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Hm, we do also support the Xen abomination of snooping on MSI table writes to see if they're targeted at a Xen PIRQ, then actually unmasking the MSI from QEMU when the guest binds the corresponding event channel to that PIRQ. I think this is going to break in CI as kvm_xen_guest.py does deliberately exercise that use case, doesn't it? I deliberately *didn't* switch to testing the Xen PV net device, with a comment that testing MSI and irqchip permutations was far more entertaining. So I hope it should catch this?
On 14/11/23 16:13, David Woodhouse wrote: > On 14 November 2023 09:38:02 GMT-05:00, "Philippe Mathieu-Daudé" <philmd@linaro.org> wrote: >> Similarly to the restriction in hw/pci/msix.c (see commit >> e1e4bf2252 "msix: fix msix_vector_masked"), restrict the >> xen_is_pirq_msi() call in msi_is_masked() to Xen. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > Hm, we do also support the Xen abomination of snooping on MSI table writes to see if they're targeted at a Xen PIRQ, then actually unmasking the MSI from QEMU when the guest binds the corresponding event channel to that PIRQ. > > I think this is going to break in CI as kvm_xen_guest.py does deliberately exercise that use case, doesn't it? Hmmm I see what you mean. So you mentioned these checks: - host Xen accel - Xen accel emulated to guest via KVM host accel Maybe we need here: - guest expected to run Xen Being ( Xen accel emulated to guest via KVM host accel OR host Xen accel ) If so, possibly few places incorrectly check 'xen_enabled()' instead of this 'xen_guest()'. "Xen on KVM" is a tricky case... > I deliberately *didn't* switch to testing the Xen PV net device, with a comment that testing MSI and irqchip permutations was far more entertaining. So I hope it should catch this? ¯\_(ツ)_/¯
On 14 November 2023 10:22:23 GMT-05:00, "Philippe Mathieu-Daudé" <philmd@linaro.org> wrote: >On 14/11/23 16:13, David Woodhouse wrote: >> On 14 November 2023 09:38:02 GMT-05:00, "Philippe Mathieu-Daudé" <philmd@linaro.org> wrote: >>> Similarly to the restriction in hw/pci/msix.c (see commit >>> e1e4bf2252 "msix: fix msix_vector_masked"), restrict the >>> xen_is_pirq_msi() call in msi_is_masked() to Xen. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> >> Hm, we do also support the Xen abomination of snooping on MSI table writes to see if they're targeted at a Xen PIRQ, then actually unmasking the MSI from QEMU when the guest binds the corresponding event channel to that PIRQ. >> >> I think this is going to break in CI as kvm_xen_guest.py does deliberately exercise that use case, doesn't it? > >Hmmm I see what you mean. > >So you mentioned these checks: > >- host Xen accel >- Xen accel emulated to guest via KVM host accel > >Maybe we need here: > >- guest expected to run Xen > > Being ( > Xen accel emulated to guest via KVM host accel > OR > host Xen accel > ) > >If so, possibly few places incorrectly check 'xen_enabled()' >instead of this 'xen_guest()'. I think xen_is_pirq_msi() had that test built in, didn't it? Adding a 'xen_enabled() &&' prefix was technically redundant? What's the actual problem we're trying to solve here? That we had two separate implementations of xen_is_pirq_msi() (three if you count an empty stub?) which are resolved at link time and prevent you from running Xen-accel and KVM-accel VMs within the same QEMU process? >"Xen on KVM" is a tricky case... > >> I deliberately *didn't* switch to testing the Xen PV net device, with a comment that testing MSI and irqchip permutations was far more entertaining. So I hope it should catch this? > >¯\_(ツ)_/¯ I believe that if you push your branch to a gitlab tree with the right CI variables defined, it'll run all the CI? And I *hope* it fails with this patch. It's precisely the kind of thing I was *intending* to catch with the testing!
On Tue, 2023-11-14 at 16:22 +0100, Philippe Mathieu-Daudé wrote: > > If so, possibly few places incorrectly check 'xen_enabled()' > instead of this 'xen_guest()'. Sorry, I meant to respond to that one directly. I don't *think* there are any cases of that. As I added the CONFIG_XEN_EMU support, I moved a bunch of stuff to live under CONFIG_XEN_BUS instead of CONFIG_XEN, fixing them up (and implementing the emulated versions of grant table operations, event channel operations etc. which no longer come from the actual Xen libraries). The existing cases of xen_enabled() really *are* being used to mean 'xen_accel_enabled()', AFAIK. Apart from that one you just tried to add :)
On Tue, 2023-11-14 at 10:44 -0500, David Woodhouse wrote: > > I believe that if you push your branch to a gitlab tree with the > right CI variables defined, it'll run all the CI? And I *hope* it > fails with this patch. It's precisely the kind of thing I was > *intending* to catch with the testing! So it doesn't fail. Because virtio-net-pci doesn't use MSI, we need to test with E1000E or something like that. And actually I'm not 100% convinced the avocado tests are even running in the gitlab CI. And also, if I test it manually... it doesn't fail because you didn't break it :) You didn't break the xen_evtchn_snoop_msi() call; that still works. What you were changing is the part which makes msi_is_masked() lie and unconditionally return false when it's mapped to a Xen PIRQ. And actually, even in the Xen-emulation case, xen_is_pirq_msi() is always returning zero. So your patch isn't changing anything. It *does*, however, make hw/pci/msi.c do this exactly the same way as hw/pci/msix.c does. That one does check xen_enabled() first. I need to double-check whether the msi{x,}_is_masked() checks ought to be doing the same for Xen-emu as they do on real Xen. That whole thing is a complete abomination. But that isn't your problem. For your patch, Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
diff --git a/hw/pci/msi.c b/hw/pci/msi.c index 041b0bdbec..8104ac1d91 100644 --- a/hw/pci/msi.c +++ b/hw/pci/msi.c @@ -23,6 +23,7 @@ #include "hw/xen/xen.h" #include "qemu/range.h" #include "qapi/error.h" +#include "sysemu/xen.h" #include "hw/i386/kvm/xen_evtchn.h" @@ -308,7 +309,7 @@ bool msi_is_masked(const PCIDevice *dev, unsigned int vector) } data = pci_get_word(dev->config + msi_data_off(dev, msi64bit)); - if (xen_is_pirq_msi(data)) { + if (xen_enabled() && xen_is_pirq_msi(data)) { return false; }
Similarly to the restriction in hw/pci/msix.c (see commit e1e4bf2252 "msix: fix msix_vector_masked"), restrict the xen_is_pirq_msi() call in msi_is_masked() to Xen. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/pci/msi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)