On 26/11/20 13:05, David Woodhouse wrote:
|It looks like this was introduced in commit 782d422bcaee, when
dm_request_for_irq_injection() started returning true based purely on
the fact that userspace had requested the interrupt window, without heed
to kvm_cpu_has_interrupt() also being true. |
That patch had no semantic change, because
dm_request_for_irq_injection() was split in two and the problematic bit
was only split to kvm_vcpu_ready_for_interrupt_injection().
Even pre-patch there was a
if (kvm_cpu_has_interrupt(vcpu))
return false;
in dm_request_for_irq_injection() which your patch would have changed to
if (lapic_in_kernel(vcpu) && kvm_cpu_has_extint(vcpu))
return false;
Your patch certainly works, but _what_ does
!(lapic_in_kernel(vcpu) && kvm_cpu_has_extint(vcpu)) &&
kvm_cpu_accept_dm_intr(vcpu)
mean in terms of the vcpu's state? I have no idea, in fact at this
point I barely have an idea of what
kvm_vcpu_ready_for_interrupt_injection does. Let's figure it out.
First act
~~~~~~~~~
First of all let's take a step back from your patch. Let's just look at
kvm_cpu_has_interrupt(vcpu) and trivially remove the APIC case from
kvm_cpu_has_interrupt:
+static bool xxx(struct kvm_vcpu *vcpu)
+{
+ WARN_ON(pic_in_kernel(vcpu->kvm));
+ if (!lapic_in_kernel(vcpu))
+ return vcpu->arch.interrupt.injected;
+ else
+ return kvm_cpu_has_extint(vcpu);
+}
return kvm_arch_interrupt_allowed(vcpu) &&
- !kvm_cpu_has_interrupt(vcpu) &&
!kvm_event_needs_reinjection(vcpu) &&
+ !xxx(vcpu) &&
kvm_cpu_accept_dm_intr(vcpu);
Again, no idea does "xxx" do, much less its combination with
kvm_cpu_accept_dm_intr. We need to dive further down.
Second act
~~~~~~~~~~
kvm_cpu_accept_dm_intr can be rewritten like this:
if (!lapic_in_kernel(vcpu))
return true;
else
return kvm_apic_accept_pic_intr(vcpu));
Therefore, we can commonize the "if"s in our xxx function with those
from kvm_cpu_accept_dm_intr. Remembering that the first act used the
negation of xxx, the patch now takes this shape
+static int yyy(struct kvm_vcpu *vcpu)
+{
+ WARN_ON(pic_in_kernel(vcpu->kvm));
+ if (!lapic_in_kernel(vcpu))
+ return !vcpu->arch.interrupt.injected;
+ else
+ return (!kvm_cpu_has_extint(vcpu) &&
+ kvm_apic_accept_pic_intr(vcpu));
+}
return kvm_arch_interrupt_allowed(vcpu) &&
- !kvm_cpu_has_interrupt(vcpu) &&
!kvm_event_needs_reinjection(vcpu) &&
- kvm_cpu_accept_dm_intr(vcpu);
+ yyy(vcpu);
This doesn't seem like progress, but we're not done...
Third act
~~~~~~~~~
Let's look at the arms of yyy's "if" statement one by one.
If !lapic_in_kernel, the return statement will always be true because
the function is called under !kvm_event_needs_reinjection(vcpu). So
we're already at
static int yyy(struct kvm_vcpu *vcpu)
{
WARN_ON(pic_in_kernel(vcpu->kvm));
if (!lapic_in_kernel(vcpu))
return true;
return (!kvm_cpu_has_extint(vcpu) &&
kvm_apic_accept_pic_intr(vcpu));
}
As to the "else" branch, irqchip_split is true so
kvm_cpu_has_extint(vcpu) is "kvm_apic_accept_pic_intr(v) &&
pending_userspace_extint(v)". More simplifications ahead!
!(A && B) && A
=> (!A || !B) && A
=> A && !B
that is:
static int yyy(struct kvm_vcpu *vcpu)
{
WARN_ON(pic_in_kernel(vcpu->kvm));
if (!lapic_in_kernel(vcpu))
return true;
return (kvm_apic_accept_pic_intr(vcpu) &&
!pending_userspace_extint(vcpu));
}
which makes sense: focusing on ExtINT and ignoring event reinjection
(which is handled by the caller), the vCPU is ready for interrupt
injection if:
- there is no LAPIC (so ExtINT injection is in the hands of userspace), or
- PIC interrupts are being accepted, and userspace's last ExtINT isn't
still pending.
Thus, the final patch is:
static int kvm_vcpu_ready_for_interrupt_injection(struct kvm_vcpu *vcpu)
{
+ WARN_ON(pic_in_kernel(vcpu->kvm));
+
return kvm_arch_interrupt_allowed(vcpu) &&
- !kvm_cpu_has_interrupt(vcpu) &&
!kvm_event_needs_reinjection(vcpu) &&
- kvm_cpu_accept_dm_intr(vcpu);
+ (!lapic_in_kernel(vcpu)
+ || (kvm_apic_accept_pic_intr(vcpu)
+ && !pending_userspace_extint(v));
}
I'm wondering if this one fails as well...
Paolo
