Re: [PATCH v2 1/2] KVM: VMX: Disable posted interrupts for odd IRQs

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Wed, Sep 04, 2019 at 05:36:39PM +0200, Alexander Graf wrote:
> 
> 
> On 04.09.19 16:40, Sean Christopherson wrote:
> >On Wed, Sep 04, 2019 at 03:35:10PM +0200, Alexander Graf wrote:
> >>We can easily route hardware interrupts directly into VM context when
> >>they target the "Fixed" or "LowPriority" delivery modes.
> >>
> >>However, on modes such as "SMI" or "Init", we need to go via KVM code
> >>to actually put the vCPU into a different mode of operation, so we can
> >>not post the interrupt
> >>
> >>Add code in the VMX PI logic to explicitly refuse to establish posted
> >>mappings for advanced IRQ deliver modes. This reflects the logic in
> >>__apic_accept_irq() which also only ever passes Fixed and LowPriority
> >>interrupts as posted interrupts into the guest.
> >>
> >>This fixes a bug I have with code which configures real hardware to
> >>inject virtual SMIs into my guest.
> >>
> >>Signed-off-by: Alexander Graf <graf@amazon.com>
> >>Reviewed-by: Liran Alon <liran.alon@oracle.com>
> >>
> >>---
> >>
> >>v1 -> v2:
> >>
> >>   - Make error message more unique
> >>   - Update commit message to point to __apic_accept_irq()
> >>---
> >>  arch/x86/kvm/vmx/vmx.c | 22 ++++++++++++++++++++++
> >>  1 file changed, 22 insertions(+)
> >>
> >>diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> >>index 570a233e272b..8029fe658c30 100644
> >>--- a/arch/x86/kvm/vmx/vmx.c
> >>+++ b/arch/x86/kvm/vmx/vmx.c
> >>@@ -7401,6 +7401,28 @@ static int vmx_update_pi_irte(struct kvm *kvm, unsigned int host_irq,
> >>  			continue;
> >>  		}
> >>+		switch (irq.delivery_mode) {
> >>+		case dest_Fixed:
> >>+		case dest_LowestPrio:
> >>+			break;
> >>+		default:
> >>+			/*
> >>+			 * For non-trivial interrupt events, we need to go
> >>+			 * through the full KVM IRQ code, so refuse to take
> >>+			 * any direct PI assignments here.
> >>+			 */
> >
> >IMO, a beefy comment is unnecessary, anyone that is digging through this
> >code has hopefully read the PI spec or at least understands the basic
> >concepts.  I.e. it should be obvious that PI can't be used for SMI, etc...
> >
> >>+			ret = irq_set_vcpu_affinity(host_irq, NULL);
> >>+			if (ret < 0) {
> >>+				printk(KERN_INFO
> >>+				    "non-std IRQ failed to recover, irq: %u\n",
> >>+				    host_irq);
> >>+				goto out;
> >>+			}
> >>+
> >>+			continue;
> >
> >Using a switch to filter out two types is a bit of overkill.  It also
> 
> The switch should compile into the same as the if() below, it's just a
> matter of being more verbose in code.
> 
> >probably makes sense to perform the deliver_mode checks before calling
> >kvm_intr_is_single_vcpu().  Why not simply something like this?  The
> >existing comment and error message are even generic enough to keep as is.
> 
> Ok, so how about this, even though it goes against Liran's comment on the
> combined debug print?

I missed that comment.

How often do we expect irq_set_vcpu_affinity() to fail?  If it's frequent
enough that the debug message matters, maybe it should be a tracepoint.
 
> If you think it's reasonable despite the broken formatting, I'll be happy to
> fold the patches and submit as v3.
> 
> 
> Alex
> 
> 
> diff --git a/arch/x86/include/asm/kvm_host.h
> b/arch/x86/include/asm/kvm_host.h
> index 44a5ce57a905..55f68fb0d791 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1581,6 +1581,12 @@ bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct
> kvm_lapic_irq *irq,
>  void kvm_set_msi_irq(struct kvm *kvm, struct kvm_kernel_irq_routing_entry
> *e,
>  		     struct kvm_lapic_irq *irq);
> 
> +static inline bool kvm_irq_is_generic(struct kvm_lapic_irq *irq)
> +{
> +	return (irq->delivery_mode == dest_Fixed ||
> +		irq->delivery_mode == dest_LowestPrio);
> +}
> +
>  static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu)
>  {
>  	if (kvm_x86_ops->vcpu_blocking)
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 1f220a85514f..34cc59518cbb 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -5260,7 +5260,8 @@ get_pi_vcpu_info(struct kvm *kvm, struct
> kvm_kernel_irq_routing_entry *e,
> 
>  	kvm_set_msi_irq(kvm, e, &irq);
> 
> -	if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
> +	if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu) ||
> +	    !kvm_irq_is_generic(&irq)) {

I've never heard/seen the term generic used to describe x86 interrupts.
Maybe kvm_irq_is_intr() or kvm_irq_is_vectored_intr()?

>  		pr_debug("SVM: %s: use legacy intr remap mode for irq %u\n",
>  			 __func__, irq.vector);
>  		return -1;
> @@ -5314,6 +5315,7 @@ static int svm_update_pi_irte(struct kvm *kvm,
> unsigned int host_irq,
>  		 * 1. When cannot target interrupt to a specific vcpu.
>  		 * 2. Unsetting posted interrupt.
>  		 * 3. APIC virtialization is disabled for the vcpu.
> +		 * 4. IRQ has extended delivery mode (SMI, INIT, etc)

Similarly, 'extended delivery mode' isn't really a thing, it's simply the
delivery mode.

	4. IRQ is not a vectored interrupt.

>  		 */
>  		if (!get_pi_vcpu_info(kvm, e, &vcpu_info, &svm) && set &&
>  		    kvm_vcpu_apicv_active(&svm->vcpu)) {
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 570a233e272b..69f53809c7bb 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7382,10 +7382,14 @@ static int vmx_update_pi_irte(struct kvm *kvm,
> unsigned int host_irq,
>  		 * irqbalance to make the interrupts single-CPU.
>  		 *
>  		 * We will support full lowest-priority interrupt later.
> +		 *
> +		 * In addition, we can only inject generic interrupts using
> +		 * the PI mechanism, refuse to route others through it.
>  		 */
> 
>  		kvm_set_msi_irq(kvm, e, &irq);
> -		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
> +		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu) ||
> +		    !kvm_irq_is_generic(&irq)) {
>  			/*
>  			 * Make sure the IRTE is in remapped mode if
>  			 * we don't handle it in posted mode.
> 
> 
> 
> 
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879
> 
>