From: Gleb Natapov <gleb@redhat.com>
To: "Zhang, Yang Z" <yang.z.zhang@intel.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
"Shan, Haitao" <haitao.shan@intel.com>,
"mtosatti@redhat.com" <mtosatti@redhat.com>,
"Zhang, Xiantao" <xiantao.zhang@intel.com>,
"hpa@linux.intel.com" <hpa@linux.intel.com>,
"Nakajima, Jun" <jun.nakajima@intel.com>
Subject: Re: [PATCH v3] KVM: VMX: enable acknowledge interupt on vmexit
Date: Tue, 29 Jan 2013 08:48:43 +0200 [thread overview]
Message-ID: <20130129064843.GE22871@redhat.com> (raw)
In-Reply-To: <A9667DDFB95DB7438FA9D7D576C3D87E31E52F@SHSMSX101.ccr.corp.intel.com>
On Mon, Jan 28, 2013 at 11:57:09PM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-01-28:
> > On Mon, Jan 28, 2013 at 08:54:07AM +0800, Yang Zhang wrote:
> >> From: Yang Zhang <yang.z.zhang@Intel.com>
> >>
> >> The "acknowledge interrupt on exit" feature controls processor behavior
> >> for external interrupt acknowledgement. When this control is set, the
> >> processor acknowledges the interrupt controller to acquire the
> >> interrupt vector on VM exit.
> >>
> >> After enabling this feature, an interrupt which arrived when target cpu
> >> is running in vmx non-root mode will be handled by vmx handler instead
> >> of handler in idt. Currently, vmx handler only fakes an interrupt stack
> >> and jump to idt table to let real handler to handle it. Further, we
> >> will recognize the interrupt and only delivery the interrupt which not
> >> belong to current vcpu through idt table. The interrupt which belonged
> >> to current vcpu will be handled inside vmx handler. This will reduce
> >> the interrupt handle cost of KVM.
> >>
> >> Refer to Intel SDM volum 3, chapter 33.2.
> >>
> >> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> >> ---
> >> arch/x86/include/asm/kvm_host.h | 2 + arch/x86/kvm/svm.c
> >> | 6 ++++ arch/x86/kvm/vmx.c | 61
> >> ++++++++++++++++++++++++++++++++++++-- arch/x86/kvm/x86.c
> >> | 3 +- 4 files changed, 67 insertions(+), 5 deletions(-)
> >> diff --git a/arch/x86/include/asm/kvm_host.h
> >> b/arch/x86/include/asm/kvm_host.h index 77d56a4..07daf10 100644 ---
> >> a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h
> >> @@ -340,6 +340,7 @@ struct kvm_vcpu_arch {
> >> unsigned long cr8;
> >> u32 hflags;
> >> u64 efer;
> >> + unsigned long host_idt_base;
> > Should be in vmx.
> >
> >> u64 apic_base; struct kvm_lapic *apic; /* kernel irqchip context
> >> */ unsigned long apic_attention; @@ -725,6 +726,7 @@ struct
> >> kvm_x86_ops { int (*check_intercept)(struct kvm_vcpu *vcpu,
> >> struct x86_instruction_info *info, enum x86_intercept_stage
> >> stage); + void (*handle_external_intr)(struct kvm_vcpu *vcpu); };
> >>
> >> struct kvm_arch_async_pf {
> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> >> index d29d3cd..c283185 100644
> >> --- a/arch/x86/kvm/svm.c
> >> +++ b/arch/x86/kvm/svm.c
> >> @@ -4227,6 +4227,11 @@ out:
> >> return ret;
> >> }
> >> +static void svm_handle_external_intr(struct kvm_vcpu *vcpu)
> >> +{
> >> + local_irq_enable();
> >> +}
> >> +
> >> static struct kvm_x86_ops svm_x86_ops = { .cpu_has_kvm_support =
> >> has_svm, .disabled_by_bios = is_disabled, @@ -4318,6 +4323,7 @@
> >> static struct kvm_x86_ops svm_x86_ops = { .set_tdp_cr3 = set_tdp_cr3,
> >>
> >> .check_intercept = svm_check_intercept, + .handle_external_intr =
> >> svm_handle_external_intr, };
> >>
> >> static int __init svm_init(void)
> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >> index 02eeba8..243ce45 100644
> >> --- a/arch/x86/kvm/vmx.c
> >> +++ b/arch/x86/kvm/vmx.c
> >> @@ -2565,7 +2565,8 @@ static __init int setup_vmcs_config(struct
> > vmcs_config *vmcs_conf)
> >> #ifdef CONFIG_X86_64
> >> min |= VM_EXIT_HOST_ADDR_SPACE_SIZE;
> >> #endif
> >> - opt = VM_EXIT_SAVE_IA32_PAT | VM_EXIT_LOAD_IA32_PAT;
> >> + opt = VM_EXIT_SAVE_IA32_PAT | VM_EXIT_LOAD_IA32_PAT |
> >> + VM_EXIT_ACK_INTR_ON_EXIT;
> >> if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_EXIT_CTLS,
> >> &_vmexit_control) < 0)
> >> return -EIO;
> >> @@ -3742,7 +3743,7 @@ static void vmx_disable_intercept_for_msr(u32 msr,
> > bool longmode_only)
> >> * Note that host-state that does change is set elsewhere. E.g., host-state
> >> * that is set differently for each CPU is set in vmx_vcpu_load(), not here.
> >> */
> >> -static void vmx_set_constant_host_state(void)
> >> +static void vmx_set_constant_host_state(struct kvm_vcpu *vcpu)
> >> {
> >> u32 low32, high32;
> >> unsigned long tmpl;
> >> @@ -3770,6 +3771,7 @@ static void vmx_set_constant_host_state(void)
> >>
> >> native_store_idt(&dt);
> >> vmcs_writel(HOST_IDTR_BASE, dt.address); /* 22.2.4 */
> >> + vcpu->arch.host_idt_base = dt.address;
> >>
> >> vmcs_writel(HOST_RIP, vmx_return); /* 22.2.5 */
> >> @@ -3884,7 +3886,7 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
> >>
> >> vmcs_write16(HOST_FS_SELECTOR, 0); /* 22.2.4 */
> >> vmcs_write16(HOST_GS_SELECTOR, 0); /* 22.2.4 */
> >> - vmx_set_constant_host_state();
> >> + vmx_set_constant_host_state(&vmx->vcpu);
> >> #ifdef CONFIG_X86_64
> >> rdmsrl(MSR_FS_BASE, a);
> >> vmcs_writel(HOST_FS_BASE, a); /* 22.2.4 */
> >> @@ -6094,6 +6096,56 @@ static void vmx_complete_atomic_exit(struct
> > vcpu_vmx *vmx)
> >> }
> >> }
> >> +static void vmx_handle_external_intr(struct kvm_vcpu *vcpu) +{ + u32
> >> exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO); + if ((exit_intr_info
> >> & (INTR_INFO_VALID_MASK | INTR_INFO_INTR_TYPE_MASK)) + ==
> >> (INTR_INFO_VALID_MASK | INTR_TYPE_EXT_INTR)) { + unsigned int vector;
> >> + unsigned long entry; + gate_desc *desc; + + vector =
> >> exit_intr_info & INTR_INFO_VECTOR_MASK; +#ifdef CONFIG_X86_64 + desc =
> >> (void *)vcpu->arch.host_idt_base + vector * 16; +#else + desc = (void
> >> *)vcpu->arch.host_idt_base + vector * 8; +#endif + + entry =
> >> gate_offset(*desc); + asm( + "mov %0, %%" _ASM_DX " \n\t" +#ifdef
> >> CONFIG_X86_64 + "mov %%" _ASM_SP ", %%" _ASM_BX " \n\t" + "and
> >> $0xfffffffffffffff0, %%" _ASM_SP " \n\t" + "mov %%ss, %%" _ASM_AX "
> >> \n\t" + "push %%" _ASM_AX " \n\t" + "push %%" _ASM_BX " \n\t"
> >> +#endif + "pushf \n\t" + "pop %%" _ASM_AX " \n\t" + "or $0x200,
> >> %%" _ASM_AX " \n\t" + "push %%" _ASM_AX " \n\t" + "mov %%cs, %%"
> >> _ASM_AX " \n\t" + "push %%" _ASM_AX " \n\t" + "push intr_return
> >> \n\t" + "jmp *%% " _ASM_DX " \n\t" + "1: \n\t" + ".pushsection
> >> .rodata \n\t" + ".global intr_return \n\t" + "intr_return: "
> >> _ASM_PTR " 1b \n\t" + ".popsection \n\t" + : : "m"(entry) : +#ifdef
> >> CONFIG_X86_64 + "rax", "rbx", "rdx" +#else + "eax", "edx" +#endif
> >> + ); + } else + local_irq_enable(); +} +
> >> static void vmx_recover_nmi_blocking(struct vcpu_vmx *vmx)
> >> {
> >> u32 exit_intr_info;
> >> @@ -6764,7 +6816,7 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu,
> > struct vmcs12 *vmcs12)
> >> * Other fields are different per CPU, and will be set later when
> >> * vmx_vcpu_load() is called, and when vmx_save_host_state() is called.
> >> */
> >> - vmx_set_constant_host_state();
> >> + vmx_set_constant_host_state(vcpu);
> >>
> >> /* * HOST_RSP is normally set correctly in vmx_vcpu_run() just
> >> before @@ -7361,6 +7413,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
> >> .set_tdp_cr3 = vmx_set_cr3,
> >>
> >> .check_intercept = vmx_check_intercept, + .handle_external_intr =
> >> vmx_handle_external_intr, };
> >>
> >> static int __init vmx_init(void)
> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >> index b9f5529..676b399 100644
> >> --- a/arch/x86/kvm/x86.c
> >> +++ b/arch/x86/kvm/x86.c
> >> @@ -5767,7 +5767,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> >>
> >> vcpu->mode = OUTSIDE_GUEST_MODE;
> >> smp_wmb();
> >> - local_irq_enable();
> >> +
> > I asked to add comment how interrupt is enabled here.
> How about the following comments:
> /*
> * Interrupt enable logic is changed if 'ack intr on vmexit' available:
No need to describe how current code is different from previous one.
This kind of explanation belongs to commit message.
> * IF bit is set from popping stack when returning from interrupt
> * handler. If no external interrupt, call local_irq_enable() to enable it.
> */
>
Here just add a comment that says that interrupts are enabled by the
callback. In vmx_handle_external_intr() inside "external interrupt" if()
add a comment that IF bit is set in rflags on the interrupt stack frame,
so interrupts will be enabled on a return from irq.
> >> + kvm_x86_ops->handle_external_intr(vcpu);
> >>
> >> ++vcpu->stat.exits;
> >> --
> >> 1.7.1
> >
> > --
> > Gleb.
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
> Best regards,
> Yang
--
Gleb.
prev parent reply other threads:[~2013-01-29 6:48 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-01-28 0:54 [PATCH v3] KVM: VMX: enable acknowledge interupt on vmexit Yang Zhang
2013-01-28 10:16 ` Gleb Natapov
2013-01-28 11:13 ` Zhang, Yang Z
2013-01-28 23:57 ` Zhang, Yang Z
2013-01-29 6:48 ` Gleb Natapov [this message]
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=20130129064843.GE22871@redhat.com \
--to=gleb@redhat.com \
--cc=haitao.shan@intel.com \
--cc=hpa@linux.intel.com \
--cc=jun.nakajima@intel.com \
--cc=kvm@vger.kernel.org \
--cc=mtosatti@redhat.com \
--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