RE: [PATCH 2/2] x86, apicv: Add Posted Interrupt supporting

Zhang, Yang Z
2013-01-25T00:40:26Z

Marcelo Tosatti wrote on 2013-01-25:
> On Thu, Dec 13, 2012 at 03:29:40PM +0800, Yang Zhang wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>
>> 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 <yang.z.zhang@Intel.com>
>> ---
>> 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
>> +#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();
>> +
>> + 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);
>> -
>> + if (kvm_x86_ops->has_posted_interrupt(vcpu)) {
>> + result = 1;
>> + apic->irr_pending = true;
>> + kvm_x86_ops->send_nv(vcpu, vector);
>> + goto out;
>> + }
>
> Hi,
>
> Steps 4, 5 and 6 of section 29.6 are executed in both VMX root/non-root
> modes, or only non-root mode?
SDM doesn't tell. But we don't need know this in software level.

>
>
> If only non-root mode, there is a problem if target vcpu<->pcpu vm-exits
> before receiving and acking the interrupt. In that case PIR set bits are
> not transferred to VIRR.
>
> It would be necessary to read notification bit on VM-exit and, if set,
> do PIR->VIRR transfer in software. The downside, is lack of an atomic
In current implementation, it will sync PIR to VIRR before vmentry.

> (VIRR |= PIR; PIR = 0) in software. So it would require synchronization
> to KVM APIC injection (which ATM relies on atomic test_and_set of IRR).

Best regards,
Yang