On Tue, 2024-01-16 at 08:52 -0800, Sean Christopherson wrote:
> On Tue, Jan 16, 2024, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@xxxxxxxxxxxx>
> >
> > Linux guests since commit b1c3497e604d ("x86/xen: Add support for
> > HVMOP_set_evtchn_upcall_vector") in v6.0 onwards will use the per-vCPU
> > upcall vector when it's advertised in the Xen CPUID leaves.
> >
> > This upcall is injected through the local APIC as an MSI, unlike the
> > older system vector which was merely injected by the hypervisor any time
> > the CPU was able to receive an interrupt and the upcall_pending flags is
> > set in its vcpu_info.
> >
> > Effectively, that makes the per-CPU upcall edge triggered instead of
> > level triggered.
> >
> > We lose edges.
>
> Pronouns. And losing edges isn't wrong in and of itself. The only thing that
> is "wrong" is that KVM doesn't exactly follow Xen's behavior. How about smushing
> the above sentence and the next two paragraphs into:
>
> Effectively, that makes the per-CPU upcall edge triggered instead of
> level triggered, which results in the upcall being lost if the MSI is
> delivered when the local APIC is *disabled*.
>
> Xen checks the vcpu_info->evtchn_upcall_pending flag when enabling the
> local APIC for a vCPU and injects the vector immediately if so. Do the
> same in KVM since KVM doesn't provide a way for userspace to notice when
> the guest software enables a local APIC.
Yep, that works.
> > @@ -499,8 +500,10 @@ static inline void apic_set_spiv(struct kvm_lapic *apic, u32 val)
> > }
> >
> > /* Check if there are APF page ready requests pending */
> > - if (enabled)
> > + if (enabled) {
>
> Argh, not your code, but there's really no reason this check needs to be outside
> of the "if (enabled != apic->sw_enabled)" block. I don't care about optimizing
> anything, I just don't like that it implies that KVM always needs to take action
> if SPIV is written. Probably not worth trying to "fix" at this point though :-(
>
> > kvm_make_request(KVM_REQ_APF_READY, apic->vcpu);
> > + kvm_xen_enable_lapic(apic->vcpu);
>
> I think we should name the Xen helper kvm_xen_sw_enable_lapic(), to make it clear
> that the behavior doesn't apply to the APIC being hardware enabled via MSR.
> Unless it does apply to that path?
I believe Xen only injects the interrupts when the SPIV register is
written with bit 8 set (i.e. not SW disabled). It doesn't look like a
HW disable/enable will reinject the vector. So yes, renaming it makes
some sense.
It *shouldn't* be moved inside the 'if (enabled != apic->sw_enabled)'
block, if it's to emulate the Xen behaviour faithfully. With an upcall
pending, I believe that a HW enable followed by *any* write to the SPIV
register will inject the vector.
(The APIC starts up with the software bit in SPIV *enabled*).
I'll send a new version with updated commit message and that name
change. Thanks.
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
