Messages in this thread | | | Subject | Re: [PATCH v2 1/2] KVM: VMX: Disable posted interrupts for odd IRQs | From | Alexander Graf <> | Date | Wed, 4 Sep 2019 17:58:08 +0200 |
| |
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
| |