From: Marcelo Tosatti <mtosatti@redhat.com>
To: "Zhang, Yang Z" <yang.z.zhang@intel.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
"gleb@redhat.com" <gleb@redhat.com>,
"Shan, Haitao" <haitao.shan@intel.com>,
"Zhang, Xiantao" <xiantao.zhang@intel.com>,
"Nakajima, Jun" <jun.nakajima@intel.com>,
"Anvin, H Peter" <h.peter.anvin@intel.com>
Subject: Re: [PATCH 2/2] x86, apicv: Add Posted Interrupt supporting
Date: Wed, 30 Jan 2013 21:03:11 -0200 [thread overview]
Message-ID: <20130130230311.GA19081@amt.cnet> (raw)
In-Reply-To: <A9667DDFB95DB7438FA9D7D576C3D87E31AB93@SHSMSX101.ccr.corp.intel.com>
On Fri, Jan 25, 2013 at 12:40:21AM +0000, Zhang, Yang Z wrote:
> 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).
Some comments:
Enable ack-on-exit feature patch:
1) Value of register VM_EXIT_INTR_INFO is available at
vmx->exit_intr_info. See commit
887864758580c80710947c38a4692032163777df.
Posted interrupt patch:
2) Must move IN_GUEST_MODE assignment after local_irq_disable, in
vcpu_enter_guest function. Otherwise:
cpu0 vcpu1<->cpu1
vcpu->mode = IN_GUEST_MODE
if IN_GUEST_MODE == true
send IPI
local_irq_disable
PIR not transferred to VIRR, misses interrupt.
3) Must check outstanding PIR notification bit unconditionally on
every VM-entry, because:
1. local_irq_disable
2. vcpu->mode = IN_GUEST_MODE
3. vmenter
4. vmexit
5. vcpu->mode = OUTSIDE_GUEST_MODE
If PIR-IPI-interrupt is sent between an event which triggers VM-exit
(for example, an external interrupt due to a device), and step 5
(assignment of vcpu->mode), the PIR->VIRR transfer before vmentry must
be made.
4) Today, an interrupt notification is cached on IRR until its delivered - further
interrupt injection is not generating further interrupt notification
bits. With PIR, behaviour changes: Its possible to have one bit in PIR and another
on IRR APIC page (if timing is right). Is this harmless? Why?
next prev parent reply other threads:[~2013-01-30 23:04 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-12-13 7:29 [PATCH 0/2] KVM: Add posted interrupt supporting Yang Zhang
2012-12-13 7:29 ` [PATCH 1/2] x86: Enable ack interrupt on vmexit Yang Zhang
2012-12-13 7:51 ` Gleb Natapov
2012-12-13 7:54 ` Zhang, Yang Z
2012-12-13 7:58 ` Gleb Natapov
2012-12-13 8:03 ` Zhang, Yang Z
2012-12-13 8:05 ` Gleb Natapov
2012-12-13 8:19 ` Zhang, Yang Z
2012-12-13 8:22 ` Gleb Natapov
2012-12-13 8:23 ` Zhang, Yang Z
2012-12-16 13:26 ` Zhang, Yang Z
2012-12-18 9:11 ` Gleb Natapov
2012-12-13 7:29 ` [PATCH 2/2] x86, apicv: Add Posted Interrupt supporting Yang Zhang
2013-01-22 22:59 ` Marcelo Tosatti
2013-01-23 5:09 ` Zhang, Yang Z
2013-01-24 23:43 ` Marcelo Tosatti
2013-01-25 0:40 ` Zhang, Yang Z
2013-01-30 23:03 ` Marcelo Tosatti [this message]
2013-01-30 23:57 ` Marcelo Tosatti
2013-01-31 7:35 ` Gleb Natapov
2013-01-31 9:43 ` Gleb Natapov
2013-01-31 13:32 ` Marcelo Tosatti
2013-01-31 13:38 ` Gleb Natapov
2013-01-31 13:44 ` Marcelo Tosatti
2013-01-31 13:55 ` Gleb Natapov
2013-02-04 0:57 ` Marcelo Tosatti
2013-02-04 9:10 ` Zhang, Yang Z
2013-02-04 9:55 ` Gleb Natapov
2013-02-04 14:43 ` Marcelo Tosatti
2013-02-04 17:13 ` Gleb Natapov
2013-02-04 19:59 ` Marcelo Tosatti
2013-02-04 20:47 ` Marcelo Tosatti
2013-02-05 5:57 ` Zhang, Yang Z
2013-02-05 8:00 ` Gleb Natapov
2013-02-05 10:35 ` Zhang, Yang Z
2013-02-05 10:54 ` Gleb Natapov
2013-02-05 10:58 ` Zhang, Yang Z
2013-02-05 11:16 ` Gleb Natapov
2013-02-05 13:26 ` Zhang, Yang Z
2013-02-05 13:29 ` Gleb Natapov
2013-02-05 13:40 ` Zhang, Yang Z
2013-02-05 13:43 ` Gleb Natapov
2013-02-07 1:23 ` Marcelo Tosatti
2013-02-05 7:32 ` Gleb Natapov
2013-02-06 22:49 ` Marcelo Tosatti
2013-02-07 0:24 ` Marcelo Tosatti
2013-02-07 13:52 ` Gleb Natapov
2013-02-08 2:07 ` Marcelo Tosatti
2013-02-08 12:18 ` Gleb Natapov
2013-02-07 14:01 ` Gleb Natapov
2013-02-07 21:49 ` Marcelo Tosatti
2013-02-08 12:28 ` Gleb Natapov
2013-02-08 13:46 ` Marcelo Tosatti
2013-01-31 9:37 ` Gleb Natapov
2013-02-04 9:11 ` Zhang, Yang Z
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130130230311.GA19081@amt.cnet \
--to=mtosatti@redhat.com \
--cc=gleb@redhat.com \
--cc=h.peter.anvin@intel.com \
--cc=haitao.shan@intel.com \
--cc=jun.nakajima@intel.com \
--cc=kvm@vger.kernel.org \
--cc=xiantao.zhang@intel.com \
--cc=yang.z.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox