Re: [PATCH-for-9.0 v2 06/19] hw/pci/msi: Restrict xen_is_pirq_msi() call to Xen

[David Woodhouse]

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!