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

[Sean Christopherson]

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@xxxxxxxxxx>
> Reviewed-by: Liran Alon <liran.alon@xxxxxxxxxx>
> 
> ---
> 
> 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
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.

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index c030c96fc81a..e0111a271a5e 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7372,7 +7372,9 @@ static int vmx_update_pi_irte(struct kvm *kvm, unsigned int host_irq,
                 */

                kvm_set_msi_irq(kvm, e, &irq);
-               if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
+               if ((irq.delivery_mode != dest_Fixed &&
+                    irq.delivery_mode != dest_LowestPrio) ||
+                   !kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
                        /*
                         * Make sure the IRTE is in remapped mode if
                         * we don't handle it in posted mode.

> +		}
> +
>  		vcpu_info.pi_desc_addr = __pa(vcpu_to_pi_desc(vcpu));
>  		vcpu_info.vector = irq.vector;
>  
> -- 
> 2.17.1
> 
> 
> 
> 
> 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
> 
> 
>