Re: [PATCH v3] KVM: x86/xen: Inject vCPU upcall vector when local APIC is enabled

[Sean Christopherson <seanjc@google.com>]

On Tue, Jan 16, 2024, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> 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.
 
> Specifically, when the local APIC is *disabled*, delivering the MSI
> will fail. Xen checks the vcpu_info->evtchn_upcall_pending flag when
> enabling the local APIC for a vCPU and injects the vector immediately
> if so.
> 
> Since userspace doesn't get to notice when the guest enables a local
> APIC which is emulated in KVM, KVM needs to do the same.
> 
> Astute reviewers may note that kvm_xen_inject_vcpu_vector() function has
> a WARN_ON_ONCE() in the case where kvm_irq_delivery_to_apic_fast() fails
> and returns false. In the case where the MSI is not delivered due to the
> local APIC being disabled, kvm_irq_delivery_to_apic_fast() still returns
> true but the value in *r is zero. So the WARN_ON_ONCE() remains correct,
> as that case should still never happen.
> 
> Fixes: fde0451be8fb3 ("KVM: x86/xen: Support per-vCPU event channel upcall via local APIC")
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Reviewed-by: Paul Durrant <paul@xen.org>
> Cc: stable@vger.kernel.org
> ---
>  v3: Repost, add Cc:stable
>  v2: Add Fixes: tag.
> 
>  arch/x86/kvm/lapic.c |  5 ++++-
>  arch/x86/kvm/xen.c   |  2 +-
>  arch/x86/kvm/xen.h   | 18 ++++++++++++++++++
>  3 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 3242f3da2457..1e715ca717bc 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -41,6 +41,7 @@
>  #include "ioapic.h"
>  #include "trace.h"
>  #include "x86.h"
> +#include "xen.h"
>  #include "cpuid.h"
>  #include "hyperv.h"
>  #include "smm.h"
> @@ -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?