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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox