From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH 2/2] x86, apicv: Add Posted Interrupt supporting Date: Thu, 31 Jan 2013 11:37:36 +0200 Message-ID: <20130131093736.GM15004@redhat.com> References: <1355383780-1367-1-git-send-email-yang.z.zhang@intel.com> <1355383780-1367-3-git-send-email-yang.z.zhang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, haitao.shan@intel.com, xiantao.zhang@intel.com, jun.nakajima@intel.com, h.peter.anvin@intel.com To: Yang Zhang Return-path: Received: from mx1.redhat.com ([209.132.183.28]:18832 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751408Ab3AaJhk (ORCPT ); Thu, 31 Jan 2013 04:37:40 -0500 Content-Disposition: inline In-Reply-To: <1355383780-1367-3-git-send-email-yang.z.zhang@intel.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Dec 13, 2012 at 03:29:40PM +0800, Yang Zhang wrote: > From: Yang Zhang > > Posted Interrupt allows APIC interrupts to inject into guest directly > without any vmexit. > > - When delivering a interrupt to guest, if target vcpu is running, > update Posted-interrupt requests bitmap and send a notification event > to the vcpu. Then the vcpu will handle this interrupt automatically, > without any software involvemnt. > > - If target vcpu is not running or there already a notification event > pending in the vcpu, do nothing. The interrupt will be handled by > next vm entry. > > Signed-off-by: Yang Zhang > --- > arch/x86/include/asm/entry_arch.h | 1 + > arch/x86/include/asm/hw_irq.h | 1 + > arch/x86/include/asm/irq.h | 1 + > arch/x86/include/asm/irq_vectors.h | 4 + > arch/x86/include/asm/kvm_host.h | 3 + > arch/x86/include/asm/vmx.h | 4 + > arch/x86/kernel/entry_64.S | 2 + > arch/x86/kernel/irq.c | 25 +++++++ > arch/x86/kernel/irqinit.c | 2 + > arch/x86/kvm/lapic.c | 16 +++- > arch/x86/kvm/lapic.h | 1 + > arch/x86/kvm/vmx.c | 133 +++++++++++++++++++++++++++++++++--- > 12 files changed, 180 insertions(+), 13 deletions(-) > > diff --git a/arch/x86/include/asm/entry_arch.h b/arch/x86/include/asm/entry_arch.h > index 40afa00..7b0a29e 100644 > --- a/arch/x86/include/asm/entry_arch.h > +++ b/arch/x86/include/asm/entry_arch.h > @@ -18,6 +18,7 @@ BUILD_INTERRUPT(reboot_interrupt,REBOOT_VECTOR) > #endif > > BUILD_INTERRUPT(x86_platform_ipi, X86_PLATFORM_IPI_VECTOR) > +BUILD_INTERRUPT(posted_intr_ipi, POSTED_INTR_VECTOR) > > /* > * every pentium local APIC has two 'local interrupts', with a > diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h > index eb92a6e..ee61af3 100644 > --- a/arch/x86/include/asm/hw_irq.h > +++ b/arch/x86/include/asm/hw_irq.h > @@ -28,6 +28,7 @@ > /* Interrupt handlers registered during init_IRQ */ > extern void apic_timer_interrupt(void); > extern void x86_platform_ipi(void); > +extern void posted_intr_ipi(void); > extern void error_interrupt(void); > extern void irq_work_interrupt(void); > > diff --git a/arch/x86/include/asm/irq.h b/arch/x86/include/asm/irq.h > index ba870bb..cff9933 100644 > --- a/arch/x86/include/asm/irq.h > +++ b/arch/x86/include/asm/irq.h > @@ -30,6 +30,7 @@ extern void irq_force_complete_move(int); > #endif > > extern void (*x86_platform_ipi_callback)(void); > +extern void (*posted_intr_callback)(void); > extern void native_init_IRQ(void); > extern bool handle_irq(unsigned irq, struct pt_regs *regs); > > diff --git a/arch/x86/include/asm/irq_vectors.h b/arch/x86/include/asm/irq_vectors.h > index 1508e51..8f2e383 100644 > --- a/arch/x86/include/asm/irq_vectors.h > +++ b/arch/x86/include/asm/irq_vectors.h > @@ -102,6 +102,10 @@ > */ > #define X86_PLATFORM_IPI_VECTOR 0xf7 > > +#ifdef CONFIG_HAVE_KVM Users of POSTED_INTR_VECTOR are not under ifdef, which means compilation will fails with kvm disabled. Test it please. > +#define POSTED_INTR_VECTOR 0xf2 > +#endif > + > /* > * IRQ work vector: > */ > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 7e26d1a..82423a8 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -700,6 +700,9 @@ struct kvm_x86_ops { > int (*has_virtual_interrupt_delivery)(struct kvm_vcpu *vcpu); > void (*update_irq)(struct kvm_vcpu *vcpu); > void (*update_eoi_exitmap)(struct kvm_vcpu *vcpu, int vector, bool set); > + int (*has_posted_interrupt)(struct kvm_vcpu *vcpu); > + int (*send_nv)(struct kvm_vcpu *vcpu, int vector); > + void (*update_irr)(struct kvm_vcpu *vcpu); > int (*set_tss_addr)(struct kvm *kvm, unsigned int addr); > int (*get_tdp_level)(void); > u64 (*get_mt_mask)(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio); > diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h > index 1003341..7b9e1d0 100644 > --- a/arch/x86/include/asm/vmx.h > +++ b/arch/x86/include/asm/vmx.h > @@ -152,6 +152,7 @@ > #define PIN_BASED_EXT_INTR_MASK 0x00000001 > #define PIN_BASED_NMI_EXITING 0x00000008 > #define PIN_BASED_VIRTUAL_NMIS 0x00000020 > +#define PIN_BASED_POSTED_INTR 0x00000080 > > #define VM_EXIT_SAVE_DEBUG_CONTROLS 0x00000002 > #define VM_EXIT_HOST_ADDR_SPACE_SIZE 0x00000200 > @@ -174,6 +175,7 @@ > /* VMCS Encodings */ > enum vmcs_field { > VIRTUAL_PROCESSOR_ID = 0x00000000, > + POSTED_INTR_NV = 0x00000002, > GUEST_ES_SELECTOR = 0x00000800, > GUEST_CS_SELECTOR = 0x00000802, > GUEST_SS_SELECTOR = 0x00000804, > @@ -208,6 +210,8 @@ enum vmcs_field { > VIRTUAL_APIC_PAGE_ADDR_HIGH = 0x00002013, > APIC_ACCESS_ADDR = 0x00002014, > APIC_ACCESS_ADDR_HIGH = 0x00002015, > + POSTED_INTR_DESC_ADDR = 0x00002016, > + POSTED_INTR_DESC_ADDR_HIGH = 0x00002017, > EPT_POINTER = 0x0000201a, > EPT_POINTER_HIGH = 0x0000201b, > EOI_EXIT_BITMAP0 = 0x0000201c, > diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S > index b51b2c7..d06eea1 100644 > --- a/arch/x86/kernel/entry_64.S > +++ b/arch/x86/kernel/entry_64.S > @@ -1160,6 +1160,8 @@ apicinterrupt LOCAL_TIMER_VECTOR \ > apic_timer_interrupt smp_apic_timer_interrupt > apicinterrupt X86_PLATFORM_IPI_VECTOR \ > x86_platform_ipi smp_x86_platform_ipi > +apicinterrupt POSTED_INTR_VECTOR \ > + posted_intr_ipi smp_posted_intr_ipi > > apicinterrupt THRESHOLD_APIC_VECTOR \ > threshold_interrupt smp_threshold_interrupt > diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c > index e4595f1..781d324 100644 > --- a/arch/x86/kernel/irq.c > +++ b/arch/x86/kernel/irq.c > @@ -22,6 +22,9 @@ atomic_t irq_err_count; > > /* Function pointer for generic interrupt vector handling */ > void (*x86_platform_ipi_callback)(void) = NULL; > +/* Function pointer for posted interrupt vector handling */ > +void (*posted_intr_callback)(void) = NULL; > +EXPORT_SYMBOL_GPL(posted_intr_callback); > > /* > * 'what should we do if we get a hw irq event on an illegal vector'. > @@ -228,6 +231,28 @@ void smp_x86_platform_ipi(struct pt_regs *regs) > set_irq_regs(old_regs); > } > > +/* > + * Handler for POSTED_INTERRUPT_VECTOR. > + */ > +void smp_posted_intr_ipi(struct pt_regs *regs) > +{ > + struct pt_regs *old_regs = set_irq_regs(regs); > + > + ack_APIC_irq(); > + > + irq_enter(); > + > + exit_idle(); > + > + if (posted_intr_callback) > + posted_intr_callback(); > + The callback does nothing. Drop it. > + irq_exit(); > + > + set_irq_regs(old_regs); > +} > + > + > EXPORT_SYMBOL_GPL(vector_used_by_percpu_irq); > > #ifdef CONFIG_HOTPLUG_CPU > diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c > index 6e03b0d..d15ca4f 100644 > --- a/arch/x86/kernel/irqinit.c > +++ b/arch/x86/kernel/irqinit.c > @@ -205,6 +205,8 @@ static void __init apic_intr_init(void) > > /* IPI for X86 platform specific use */ > alloc_intr_gate(X86_PLATFORM_IPI_VECTOR, x86_platform_ipi); > + /* IPI for posted interrupt use */ > + alloc_intr_gate(POSTED_INTR_VECTOR, posted_intr_ipi); > > /* IPI vectors for APIC spurious and error interrupts */ > alloc_intr_gate(SPURIOUS_APIC_VECTOR, spurious_interrupt); > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 2109a6a..d660b9d 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -350,6 +350,7 @@ static inline int apic_find_highest_irr(struct kvm_lapic *apic) > if (!apic->irr_pending) > return -1; > > + kvm_x86_ops->update_irr(apic->vcpu); > result = apic_search_irr(apic); > ASSERT(result == -1 || result >= 16); > > @@ -725,18 +726,25 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, > if (trig_mode) { > apic_debug("level trig mode for vector %d", vector); > apic_set_vector(vector, apic->regs + APIC_TMR); > - } else > + } else { > apic_clear_vector(vector, apic->regs + APIC_TMR); > - Why doing pi only for edge triggered interrupts? > + if (kvm_x86_ops->has_posted_interrupt(vcpu)) { Drop has_posted_interrupt(). Just call send_nv() directly. And give it more descriptive name. > + result = 1; > + apic->irr_pending = true; This is always true with vid anyway. > + kvm_x86_ops->send_nv(vcpu, vector); > + goto out; > + } > + } > result = !apic_test_and_set_irr(vector, apic); > - trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode, > - trig_mode, vector, !result); > if (!result) { > if (trig_mode) > apic_debug("level trig mode repeatedly for " > "vector %d", vector); > break; > } > +out: > + trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode, > + trig_mode, vector, !result); No trace if !result now. > > kvm_make_request(KVM_REQ_EVENT, vcpu); > kvm_vcpu_kick(vcpu); What is the point of sending notification vector to vcpu is you kick it out of a guest mode immediately after? > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h > index 10e3f66..0f8361e 100644 > --- a/arch/x86/kvm/lapic.h > +++ b/arch/x86/kvm/lapic.h > @@ -42,6 +42,7 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu); > int kvm_cpu_has_extint(struct kvm_vcpu *v); > int kvm_cpu_get_extint(struct kvm_vcpu *v); > int kvm_apic_get_highest_irr(struct kvm_vcpu *vcpu); > +void kvm_apic_update_irr(struct kvm_vcpu *vcpu, unsigned int *pir); > void kvm_lapic_reset(struct kvm_vcpu *vcpu); > u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu); > void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8); > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 6b6bd03..07dbde6 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -31,6 +31,7 @@ > #include > #include > #include > +#include > #include "kvm_cache_regs.h" > #include "x86.h" > > @@ -86,6 +87,8 @@ module_param(fasteoi, bool, S_IRUGO); > static bool __read_mostly enable_apicv_reg_vid = 1; > module_param(enable_apicv_reg_vid, bool, S_IRUGO); > > +static bool __read_mostly enable_apicv_pi = 1; > +module_param(enable_apicv_pi, bool, S_IRUGO); > /* > * If nested=1, nested virtualization is supported, i.e., guests may use > * VMX and be a hypervisor for its own guests. If nested=0, guests may not > @@ -369,6 +372,35 @@ struct nested_vmx { > struct page *apic_access_page; > }; > > +#define POSTED_INTR_ON 0 > +/* Posted-Interrupt Descriptor */ > +struct pi_desc { > + u32 pir[8]; /* Posted interrupt requested */ > + union { > + struct { > + u8 on:1, > + rsvd:7; > + } control; > + u32 rsvd[8]; > + } u; > +} __aligned(64); > + > +static void pi_clear_on(struct pi_desc *pi_desc) > +{ > + clear_bit(POSTED_INTR_ON, (unsigned long *)&pi_desc->u.control); > +} > + > +static u8 pi_test_and_set_on(struct pi_desc *pi_desc) > +{ > + return test_and_set_bit(POSTED_INTR_ON, > + (unsigned long *)&pi_desc->u.control); > +} > + > +static void pi_set_pir(int vector, struct pi_desc *pi_desc) > +{ > + set_bit(vector, (unsigned long *)pi_desc->pir); > +} > + > struct vcpu_vmx { > struct kvm_vcpu vcpu; > unsigned long host_rsp; > @@ -435,6 +467,9 @@ struct vcpu_vmx { > u8 eoi_exitmap_changed; > u32 eoi_exit_bitmap[8]; > > + /* Posted interrupt descriptor */ > + struct pi_desc *pi; > + I am not convinced we should try to save 46 bytes here when !pi. > /* Support for a guest hypervisor (nested VMX) */ > struct nested_vmx nested; > }; > @@ -779,6 +814,11 @@ static inline bool cpu_has_vmx_virtual_intr_delivery(void) > SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY; > } > > +static inline bool cpu_has_vmx_posted_intr(void) > +{ > + return vmcs_config.pin_based_exec_ctrl & PIN_BASED_POSTED_INTR; > +} > + > static inline bool cpu_has_vmx_flexpriority(void) > { > return cpu_has_vmx_tpr_shadow() && > @@ -2475,12 +2515,6 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf) > u32 _vmexit_control = 0; > u32 _vmentry_control = 0; > > - min = PIN_BASED_EXT_INTR_MASK | PIN_BASED_NMI_EXITING; > - opt = PIN_BASED_VIRTUAL_NMIS; > - if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_PINBASED_CTLS, > - &_pin_based_exec_control) < 0) > - return -EIO; > - > min = CPU_BASED_HLT_EXITING | > #ifdef CONFIG_X86_64 > CPU_BASED_CR8_LOAD_EXITING | > @@ -2554,6 +2588,17 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf) > &_vmexit_control) < 0) > return -EIO; > > + min = PIN_BASED_EXT_INTR_MASK | PIN_BASED_NMI_EXITING; > + opt = PIN_BASED_VIRTUAL_NMIS | PIN_BASED_POSTED_INTR; > + if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_PINBASED_CTLS, > + &_pin_based_exec_control) < 0) > + return -EIO; > + > + if (!(_cpu_based_2nd_exec_control & > + SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY) || > + !(_vmexit_control & VM_EXIT_ACK_INTR_ON_EXIT)) > + _pin_based_exec_control &= ~PIN_BASED_POSTED_INTR; > + > min = 0; > opt = VM_ENTRY_LOAD_IA32_PAT; > if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_ENTRY_CTLS, > @@ -2739,6 +2784,9 @@ static __init int hardware_setup(void) > if (enable_apicv_reg_vid) > kvm_x86_ops->update_cr8_intercept = NULL; > > + if (!cpu_has_vmx_posted_intr() || !enable_apicv_reg_vid) > + enable_apicv_pi = 0; > + > if (nested) > nested_vmx_setup_ctls_msrs(); > > @@ -3904,6 +3952,57 @@ static void ept_set_mmio_spte_mask(void) > kvm_mmu_set_mmio_spte_mask(0xffull << 49 | 0x6ull); > } > > +static void pi_handler(void) > +{ > + ; > +} > + > +static int vmx_has_posted_interrupt(struct kvm_vcpu *vcpu) > +{ > + return irqchip_in_kernel(vcpu->kvm) && enable_apicv_pi; > +} > + > +static int vmx_send_nv(struct kvm_vcpu *vcpu, > + int vector) > +{ > + struct vcpu_vmx *vmx = to_vmx(vcpu); > + > + pi_set_pir(vector, vmx->pi); > + if (!pi_test_and_set_on(vmx->pi) && (vcpu->mode == IN_GUEST_MODE)) { > + apic->send_IPI_mask(get_cpu_mask(vcpu->cpu), POSTED_INTR_VECTOR); > + return 1; > + } > + return 0; > +} Return value is not checked by the caller. > + > +static void vmx_update_irr(struct kvm_vcpu *vcpu) > +{ > + struct vcpu_vmx *vmx = to_vmx(vcpu); > + struct kvm_lapic *apic = vcpu->arch.apic; > + unsigned int i, old, new, val, irr_off; Spaces instead of tabs. Not only here. Run through checkpatch. > + > + if (!enable_apicv_pi) > + return; > + Why no check for pit->on before syncing? > + for (i = 0; i <= 7; i++) { > + if (vmx->pi->pir[i]) { > + irr_off = APIC_IRR + i * 0x10; > + do { > + old = kvm_apic_get_reg(apic, irr_off); > + new = old | vmx->pi->pir[i]; > + val = cmpxchg((u32 *)(apic->regs + irr_off), old, new); > + } while (unlikely (val != old)); > + vmx->pi->pir[i] = 0; > + } > + } > +} > + > +static void free_pi(struct vcpu_vmx *vmx) > +{ > + if (enable_apicv_pi) > + kfree(vmx->pi); > +} > + > /* > * Sets up the vmcs for emulated real mode. > */ > @@ -3913,6 +4012,7 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx) > unsigned long a; > #endif > int i; > + u32 pin_based_exec_ctrl = vmcs_config.pin_based_exec_ctrl; > u32 vmexit_ctrl = vmcs_config.vmexit_ctrl; > > /* I/O */ > @@ -3925,8 +4025,10 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx) > vmcs_write64(VMCS_LINK_POINTER, -1ull); /* 22.3.1.5 */ > > /* Control */ > - vmcs_write32(PIN_BASED_VM_EXEC_CONTROL, > - vmcs_config.pin_based_exec_ctrl); > + if (!enable_apicv_pi) > + pin_based_exec_ctrl &= ~PIN_BASED_POSTED_INTR; > + > + vmcs_write32(PIN_BASED_VM_EXEC_CONTROL, pin_based_exec_ctrl); > > vmcs_write32(CPU_BASED_VM_EXEC_CONTROL, vmx_exec_control(vmx)); > > @@ -3944,6 +4046,13 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx) > vmcs_write16(GUEST_INTR_STATUS, 0); > } > > + if (enable_apicv_pi) { > + vmx->pi = kmalloc(sizeof(struct pi_desc), > + GFP_KERNEL | __GFP_ZERO); kzalloc(). > + vmcs_write64(POSTED_INTR_NV, POSTED_INTR_VECTOR); > + vmcs_write64(POSTED_INTR_DESC_ADDR, __pa((vmx->pi))); > + } > + > if (ple_gap) { > vmcs_write32(PLE_GAP, ple_gap); > vmcs_write32(PLE_WINDOW, ple_window); > @@ -6220,6 +6329,8 @@ static void vmx_update_irq(struct kvm_vcpu *vcpu) > vmx->eoi_exit_bitmap[index]); > vmx->eoi_exitmap_changed = 0; > } > + if (enable_apicv_pi) > + pi_clear_on(vmx->pi); > } > > static void vmx_update_eoi_exitmap(struct kvm_vcpu *vcpu, > @@ -6626,6 +6737,7 @@ static void vmx_free_vcpu(struct kvm_vcpu *vcpu) > > free_vpid(vmx); > free_nested(vmx); > + free_pi(vmx); > free_loaded_vmcs(vmx->loaded_vmcs); > kfree(vmx->guest_msrs); > kvm_vcpu_uninit(vcpu); > @@ -7520,8 +7632,11 @@ static struct kvm_x86_ops vmx_x86_ops = { > .enable_irq_window = enable_irq_window, > .update_cr8_intercept = update_cr8_intercept, > .has_virtual_interrupt_delivery = vmx_has_virtual_interrupt_delivery, > + .has_posted_interrupt = vmx_has_posted_interrupt, > .update_irq = vmx_update_irq, > .update_eoi_exitmap = vmx_update_eoi_exitmap, > + .send_nv = vmx_send_nv, > + .update_irr = vmx_update_irr, > SVM? > .set_tss_addr = vmx_set_tss_addr, > .get_tdp_level = get_ept_level, > @@ -7618,7 +7733,7 @@ static int __init vmx_init(void) > /* SELF-IPI */ > vmx_disable_intercept_for_msr_write(0x83f, false); > } > - > + posted_intr_callback = pi_handler; > if (enable_ept) { > kvm_mmu_set_mask_ptes(0ull, > (enable_ept_ad_bits) ? VMX_EPT_ACCESS_BIT : 0ull, > -- > 1.7.1 -- Gleb.