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

[Alexander Graf <graf@amazon.com>]

On 04.09.19 17:51, Sean Christopherson wrote:
> 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.

I don't expect to ever hit that debug print, so I don't think it matters 
really.

>   
>> 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()?

I was trying to come up with any name that describes "interrupt that we 
can post". If "intr" is that, I'll be happy to take it. Vectored_intr 
sounds even worse IMHO :).

> 
>>   		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.

s/extended/incompatible/ maybe?


Alex



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