Re: [PATCH v5 16/16] KVM: x86/xen: Add event channel interrupt vector upcall

[Paolo Bonzini <pbonzini@redhat.com>]

On 11/01/21 20:57, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> It turns out that we can't handle event channels *entirely* in userspace
> by delivering them as ExtINT, because KVM is a bit picky about when it
> accepts ExtINT interrupts from a legacy PIC. The in-kernel local APIC
> has to have LVT0 configured in APIC_MODE_EXTINT and unmasked, which
> isn't necessarily the case for Xen guests especially on secondary CPUs.
> 
> To cope with this, add kvm_xen_get_interrupt() which checks the
> evtchn_pending_upcall field in the Xen vcpu_info, and delivers the Xen
> upcall vector (configured by KVM_XEN_ATTR_TYPE_UPCALL_VECTOR) if it's
> set regardless of LAPIC LVT0 configuration. This gives us the minimum
> support we need for completely userspace-based implementation of event
> channels.
> 
> This does mean that vcpu_enter_guest() needs to check for the
> evtchn_pending_upcall flag being set, because it can't rely on someone
> having set KVM_REQ_EVENT unless we were to add some way for userspace to
> do so manually.
> 
> But actually, I don't quite see how that works reliably for interrupts
> injected with KVM_INTERRUPT either. In kvm_vcpu_ioctl_interrupt() the
> KVM_REQ_EVENT request is set once, but that'll get cleared the first time
> through vcpu_enter_guest(). So if the first exit is for something *else*
> without interrupts being enabled yet, won't the KVM_REQ_EVENT request
> have been consumed already and just be lost?

If the call is for something else and there is an interrupt, 
inject_pending_event sets *req_immediate_exit to true.  This causes an 
immediate vmexit after vmentry, and also schedules another KVM_REQ_EVENT 
processing.

If the call is for an interrupt but you cannot process it yet (IF=0, 
STI/MOVSS window, etc.), inject_pending_event calls 
kvm_x86_ops.enable_irq_window and this will cause KVM_REQ_EVENT to be 
sent again.

How do you inject the interrupt from userspace?  IIRC 
evtchn_upcall_pending is written by the hypervisor upon receiving a 
hypercall, so wouldn't you need the "dom0" to invoke a KVM_INTERRUPT 
ioctl (e.g. with irq == 256)?  That KVM_INTERRUPT will set KVM_REQ_EVENT.

If you want to write a testcase without having to write all the 
interrupt stuff in the selftests framework, you could set an IDT that 
has room only for 128 vectors and use interrupt 128 as the upcall 
vector.  Then successful delivery of the vector will cause a triple fault.

Independent of the answer to the above, this is really the only place 
where you're adding Xen code to a hot path.  Can you please use a 
STATIC_KEY_FALSE kvm_has_xen_vcpu (and a static inline function) to 
quickly return from kvm_xen_has_interrupt() if no vCPU has a shared info 
set up?

That is, something like

- when first setting vcpu_info_set to true, 
static_key_slow_inc(&kvm_has_xen_vcpu.key)

- when destroying a vCPU with vcpu_info_set to true, 
static_key_slow_dec_deferred(&kvm_has_xen_vcpu)

- rename kvm_xen_has_interrupt to __kvm__xen_has_interrupt

- add a wrapper that usese the static key to avoid the function call

static int kvm_xen_has_interrupt(struct kvm_vcpu *v)
{
	if (static_branch_unlikely(&kvm_has_xen_vcpu.key))
		return __kvm_xen_has_interrupt(v);

	return false;
}

Paolo