From: Marcelo Tosatti <mtosatti@redhat.com>
To: Yang Zhang <yang.z.zhang@intel.com>
Cc: kvm@vger.kernel.org, gleb@redhat.com, haitao.shan@intel.com,
xiantao.zhang@intel.com, Kevin Tian <kevin.tian@intel.com>
Subject: Re: [PATCH v11 2/3] x86, apicv: add virtual x2apic support
Date: Mon, 21 Jan 2013 17:59:07 -0200 [thread overview]
Message-ID: <20130121195907.GA3561@amt.cnet> (raw)
In-Reply-To: <1358331672-32384-3-git-send-email-yang.z.zhang@intel.com>
On Wed, Jan 16, 2013 at 06:21:11PM +0800, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
>
> basically to benefit from apicv, we need to enable virtualized x2apic mode.
> Currently, we only enable it when guest is really using x2apic.
>
> Also, clear MSR bitmap for corresponding x2apic MSRs when guest enabled x2apic:
> 0x800 - 0x8ff: no read intercept for apicv register virtualization,
> except APIC ID and TMCCT which need software's assistance to
> get right value.
>
> Signed-off-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
> arch/x86/include/asm/kvm_host.h | 1 +
> arch/x86/include/asm/vmx.h | 1 +
> arch/x86/kvm/lapic.c | 20 ++--
> arch/x86/kvm/lapic.h | 5 +
> arch/x86/kvm/svm.c | 6 +
> arch/x86/kvm/vmx.c | 204 +++++++++++++++++++++++++++++++++++----
> 6 files changed, 209 insertions(+), 28 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index c431b33..35aa8e6 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -697,6 +697,7 @@ struct kvm_x86_ops {
> void (*enable_nmi_window)(struct kvm_vcpu *vcpu);
> void (*enable_irq_window)(struct kvm_vcpu *vcpu);
> void (*update_cr8_intercept)(struct kvm_vcpu *vcpu, int tpr, int irr);
> + void (*set_virtual_x2apic_mode)(struct kvm_vcpu *vcpu, bool set);
> 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 44c3f7e..0a54df0 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -139,6 +139,7 @@
> #define SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES 0x00000001
> #define SECONDARY_EXEC_ENABLE_EPT 0x00000002
> #define SECONDARY_EXEC_RDTSCP 0x00000008
> +#define SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE 0x00000010
> #define SECONDARY_EXEC_ENABLE_VPID 0x00000020
> #define SECONDARY_EXEC_WBINVD_EXITING 0x00000040
> #define SECONDARY_EXEC_UNRESTRICTED_GUEST 0x00000080
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 0664c13..f39aee3 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -140,11 +140,6 @@ static inline int apic_enabled(struct kvm_lapic *apic)
> (LVT_MASK | APIC_MODE_MASK | APIC_INPUT_POLARITY | \
> APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
>
> -static inline int apic_x2apic_mode(struct kvm_lapic *apic)
> -{
> - return apic->vcpu->arch.apic_base & X2APIC_ENABLE;
> -}
> -
> static inline int kvm_apic_id(struct kvm_lapic *apic)
> {
> return (kvm_apic_get_reg(apic, APIC_ID) >> 24) & 0xff;
> @@ -1323,12 +1318,17 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
> if (!kvm_vcpu_is_bsp(apic->vcpu))
> value &= ~MSR_IA32_APICBASE_BSP;
>
> - vcpu->arch.apic_base = value;
> - if (apic_x2apic_mode(apic)) {
> - u32 id = kvm_apic_id(apic);
> - u32 ldr = ((id >> 4) << 16) | (1 << (id & 0xf));
> - kvm_apic_set_ldr(apic, ldr);
> + if ((vcpu->arch.apic_base ^ value) & X2APIC_ENABLE) {
> + if (value & X2APIC_ENABLE) {
> + u32 id = kvm_apic_id(apic);
> + u32 ldr = ((id >> 4) << 16) | (1 << (id & 0xf));
> + kvm_apic_set_ldr(apic, ldr);
> + kvm_x86_ops->set_virtual_x2apic_mode(vcpu, true);
> + } else
> + kvm_x86_ops->set_virtual_x2apic_mode(vcpu, false);
> }
> +
> + vcpu->arch.apic_base = value;
Simpler to have
if (apic_x2apic_mode(apic)) {
...
kvm_x86_ops->set_virtual_x2apic_mode(vcpu, true);
} else {
kvm_x86_ops->set_virtual_x2apic_mode(vcpu, false);
}
Also it must be done after assignment of vcpu->arch.apic_base (this
patch has vcpu->arch.apic_base being read from
->set_virtual_x2apic_mode() path).
> +static void vmx_set_msr_bitmap(struct kvm_vcpu *vcpu)
> +{
> + unsigned long *msr_bitmap;
> +
> + if (apic_x2apic_mode(vcpu->arch.apic))
vcpu->arch.apic can be NULL.
> +static void vmx_intercept_for_msr_read_x2apic(u32 msr, bool set)
> +{
> + if (set) {
> + __vmx_enable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic,
> + msr, MSR_TYPE_R);
> + __vmx_enable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic,
> + msr, MSR_TYPE_R);
> + } else {
> + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic,
> + msr, MSR_TYPE_R);
> + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic,
> + msr, MSR_TYPE_R);
> + }
> +}
Please retain the enable_intercept/disable_intercept naming in the
function name, instead of a set parameter.
> +static void vmx_intercept_for_msr_write_x2apic(u32 msr, bool set)
> +{
> + if (set) {
> + __vmx_enable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic,
> + msr, MSR_TYPE_W);
> + __vmx_enable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic,
> + msr, MSR_TYPE_W);
> + } else {
> + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic,
> + msr, MSR_TYPE_W);
> + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic,
> + msr, MSR_TYPE_W);
> + }
> }
Same here.
> @@ -3848,6 +3950,7 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
> exec_control &= ~SECONDARY_EXEC_PAUSE_LOOP_EXITING;
> if (!enable_apicv_reg)
> exec_control &= ~SECONDARY_EXEC_APIC_REGISTER_VIRT;
> + exec_control &= ~SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
> return exec_control;
Unconditionally disabling SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE? Its
awkward.
> + struct vcpu_vmx *vmx = to_vmx(vcpu);
> +
> + /* There is not point to enable virtualize x2apic without enable
> + * apicv*/
> + if (!cpu_has_vmx_virtualize_x2apic_mode() || !enable_apicv_reg)
> + return;
> +
> + if (set) {
> + exec_control = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
> + /* virtualize x2apic mode relies on tpr shadow */
> + if (!(exec_control & CPU_BASED_TPR_SHADOW))
> + return;
> + }
> +
> + sec_exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
> +
> + if (set) {
> + sec_exec_control &= ~SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
> + sec_exec_control |= SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
> + } else {
> + sec_exec_control &= ~SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
> + if (vm_need_virtualize_apic_accesses(vmx->vcpu.kvm))
> + sec_exec_control |=
> + SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
> + }
> + vmcs_write32(SECONDARY_VM_EXEC_CONTROL, sec_exec_control);
> +
> + if (set) {
> + for (msr = 0x800; msr <= 0x8ff; msr++)
> + vmx_intercept_for_msr_read_x2apic(msr, false);
> +
> + /* According SDM, in x2apic mode, the whole id reg is used.
> + * But in KVM, it only use the highest eight bits. Need to
> + * intercept it */
> + vmx_intercept_for_msr_read_x2apic(0x802, true);
> + /* TMCCT */
> + vmx_intercept_for_msr_read_x2apic(0x839, true);
> + /* TPR */
> + vmx_intercept_for_msr_write_x2apic(0x808, false);
> + }
Why not disable write intercept for all MSRs which represent APIC registers
that are virtualized? Why TPR is special?
next prev parent reply other threads:[~2013-01-21 19:59 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-01-16 10:21 [PATCH v11 0/3] x86, apicv: Add APIC virtualization support Yang Zhang
2013-01-16 10:21 ` [PATCH v11 1/3] x86, apicv: add APICv register " Yang Zhang
2013-01-16 10:21 ` [PATCH v11 2/3] x86, apicv: add virtual x2apic support Yang Zhang
2013-01-17 13:22 ` Gleb Natapov
2013-01-18 1:49 ` Zhang, Yang Z
2013-01-21 19:59 ` Marcelo Tosatti [this message]
2013-01-21 20:21 ` Gleb Natapov
2013-01-21 21:21 ` Marcelo Tosatti
2013-01-21 21:34 ` Gleb Natapov
2013-01-21 22:16 ` Marcelo Tosatti
2013-01-22 0:33 ` Marcelo Tosatti
2013-01-22 3:37 ` Zhang, Yang Z
2013-01-22 9:45 ` Gleb Natapov
2013-01-22 9:42 ` Gleb Natapov
2013-01-22 12:21 ` Zhang, Yang Z
2013-01-22 15:55 ` Gleb Natapov
2013-01-22 23:13 ` Marcelo Tosatti
2013-01-23 0:35 ` Zhang, Yang Z
2013-01-23 10:29 ` Gleb Natapov
2013-01-16 10:21 ` [PATCH v11 3/3] x86, apicv: add virtual interrupt delivery support Yang Zhang
2013-01-17 1:26 ` Zhang, Yang Z
2013-01-20 12:51 ` Gleb Natapov
2013-01-21 0:49 ` Zhang, Yang Z
2013-01-21 5:03 ` Gleb Natapov
2013-01-21 7:18 ` Zhang, Yang Z
2013-01-21 21:08 ` Marcelo Tosatti
2013-01-23 0:45 ` Zhang, Yang Z
2013-01-23 10:33 ` Gleb Natapov
2013-01-23 10:52 ` Zhang, Yang Z
2013-01-21 21:05 ` Marcelo Tosatti
2013-01-21 21:18 ` Gleb Natapov
2013-01-21 21:54 ` Marcelo Tosatti
2013-01-22 3:38 ` Zhang, Yang Z
2013-01-16 10:27 ` [PATCH v11 0/3] x86, apicv: Add APIC virtualization support Gleb Natapov
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=20130121195907.GA3561@amt.cnet \
--to=mtosatti@redhat.com \
--cc=gleb@redhat.com \
--cc=haitao.shan@intel.com \
--cc=kevin.tian@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.