On Thu, 2020-11-26 at 18:59 +0100, Paolo Bonzini wrote: > On 26/11/20 18:29, David Woodhouse wrote: > > On Thu, 2020-11-26 at 11:10 +0000, David Woodhouse wrote: > > > > > > > whether or not there's an IRQ in the > > > > LAPIC should be irrelevant when deciding to exit to userspace. Note, the > > > > reinjection check covers vcpu->arch.interrupt.injected for the case where LAPIC > > > > is in userspace. > > > > > > > > return kvm_arch_interrupt_allowed(vcpu) && > > > > (!lapic_in_kernel(vcpu) || !kvm_cpu_has_extint(vcpu)) && > > > > !kvm_event_needs_reinjection(vcpu) && > > > > kvm_cpu_accept_dm_intr(vcpu); > > > > } > > > > > > Makes sense. I'm putting this version through some testing and will > > > post it later... > > > > Hm, that survived enough test iterations to persuade me to post it, but > > then seems to have fallen over later. I'm reverting to the > > kvm_cpu_has_injectable_intr() version to leave that one running too and > > be sure it's gone in that. FWIW I've just reproduced that hang on one of the iterations *without* "noapic" on its command line at all; this one was just with 'clearcpuid=450'. That's clearing the ARAT bit to force it to use the HPET+MSI for timers. The earlier one that had failed was 'noapic clearcpuid=450'. So that one looks like a separate bug, and I get to go frown at our HPET emulation instead. It probably wasn't a failure of the fix we're looking at here. I'm going to go and check if I can reproduce that even with the in- kernel irqchip mode, and claim it's someone else's problem for now :) > !kvm_cpu_has_injectable_intr(vcpu) boils down (assuming no nested virt) to > > if (!lapic_in_kernel(v)) > return !v->arch.interrupt.injected; > > if (kvm_cpu_has_extint(v)) > return 0; > > return 1; > > and Sean's proposal instead is the same indeed (the first "if" doesn't > matter), so there may be more than one bug. > > But it turns out that with some more inlining and Boolean algebra, we > can actually figure out what the code does. :) I had just finished > writing a looong review of your patch starting from that idea, so I'll > post it. Neat. Your version, once I made it build, ought to be functionally identical to the one I posted; just a bit neater. Although I do kind of like the symmetry of my original version using kvm_cpu_has_injectable_intr(), which is the condition used in vcpu_enter_guest() for enabling the interrupt window vmexit in the first place. It makes sense for those to match. We enable the irq window if kvm_cpu_has_injectable_intr() or if userspace asks. And when the exit happens, we feed it to userspace unless kvm_cpu_has_injectable_intr(). If we go with your simpler version, I wonder if it makes sense to make similar changes to the conditions in vcpu_enter_guest() to make it clearer that they match?
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
