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?