Re: [PATCH v7 3/3] x86, apicv: add virtual x2apic support

[Gleb Natapov <gleb@redhat.com>]

On Mon, Dec 24, 2012 at 02:35:35AM +0000, Zhang, Yang Z wrote:
> Zhang, Yang Z wrote on 2012-12-24:
> > Gleb Natapov wrote on 2012-12-20:
> >> On Mon, Dec 17, 2012 at 01:30:50PM +0800, Yang Zhang wrote:
> >>> basically to benefit from apicv, we need clear MSR bitmap for
> >>> corresponding x2apic MSRs:
> >>>     0x800 - 0x8ff: no read intercept for apicv register virtualization
> >>>     TPR,EOI,SELF-IPI: no write intercept for virtual interrupt delivery
> >> We do not set "Virtualize x2APIC mode" bit in secondary execution
> >> control. If I read the spec correctly without that those MSR read/writes
> >> will go straight to physical local APIC.
> > Right. Now it cannot get benefit, but we may enable it in future and then we can
> > benefit from it.
Without enabling it you cannot disable MSR intercept for x2apic MSRs.

> how about to add the following check:
> if (apicv_enabled && virtual_x2apic_enabled)
> 	clear_msr();
> 
I do not understand what do you mean here.

> 
> >>> Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> >>> Signed-off-by: Kevin Tian <kevin.tian@intel.com>
> >>> ---
> >>>  arch/x86/kvm/vmx.c |   62
> >>>  ++++++++++++++++++++++++++++++++++++++++++++++------ 1 files changed,
> >>>  55 insertions(+), 7 deletions(-)
> >>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >>> index be66c3e..9b5e7a2 100644
> >>> --- a/arch/x86/kvm/vmx.c
> >>> +++ b/arch/x86/kvm/vmx.c
> >>> @@ -3773,7 +3773,10 @@ static void free_vpid(struct vcpu_vmx *vmx)
> >>>  	spin_unlock(&vmx_vpid_lock);
> >>>  }
> >>> -static void __vmx_disable_intercept_for_msr(unsigned long
> >>> *msr_bitmap, u32 msr) +#define MSR_TYPE_R	1 +#define MSR_TYPE_W	2
> >>> +static void __vmx_disable_intercept_for_msr(unsigned long
> >>> *msr_bitmap, + 				u32 msr, int type)
> >>>  {
> >>>  	int f = sizeof(unsigned long);
> >>> @@ -3786,20 +3789,52 @@ static void
> >> __vmx_disable_intercept_for_msr(unsigned long *msr_bitmap, u32 msr)
> >>>  	 * We can control MSRs 0x00000000-0x00001fff and
> >>>  0xc0000000-0xc0001fff. 	 */ 	if (msr <= 0x1fff) {
> >>> -		__clear_bit(msr, msr_bitmap + 0x000 / f); /* read-low */
> >>> -		__clear_bit(msr, msr_bitmap + 0x800 / f); /* write-low */
> >>> +		if (type & MSR_TYPE_R)
> >>> +			/* read-low */
> >>> +			__clear_bit(msr, msr_bitmap + 0x000 / f);
> >>> +
> >>> +		if (type & MSR_TYPE_W)
> >>> +			/* write-low */
> >>> +			__clear_bit(msr, msr_bitmap + 0x800 / f);
> >>> +
> >>>  	} else if ((msr >= 0xc0000000) && (msr <= 0xc0001fff)) {
> >>>  		msr &= 0x1fff;
> >>> -		__clear_bit(msr, msr_bitmap + 0x400 / f); /* read-high */
> >>> -		__clear_bit(msr, msr_bitmap + 0xc00 / f); /* write-high */
> >>> +		if (type & MSR_TYPE_R)
> >>> +			/* read-high */
> >>> +			__clear_bit(msr, msr_bitmap + 0x400 / f);
> >>> +
> >>> +		if (type & MSR_TYPE_W)
> >>> +			/* write-high */
> >>> +			__clear_bit(msr, msr_bitmap + 0xc00 / f);
> >>> +
> >>>  	}
> >>>  }
> >>>  
> >>>  static void vmx_disable_intercept_for_msr(u32 msr, bool longmode_only)
> >>>  {
> >>>  	if (!longmode_only)
> >>> -		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy, msr);
> >>> -	__vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode, msr);
> >>> +		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy, + 					msr,
> >>> MSR_TYPE_R | MSR_TYPE_W);
> >>> +	__vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode, + 					msr,
> >>> MSR_TYPE_R | MSR_TYPE_W); +} + +static void
> >>> vmx_disable_intercept_for_msr_read(u32 msr, bool longmode_only) +{ +
> >>> 	if (!longmode_only)
> >>> +		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy, + 					msr,
> >>> MSR_TYPE_R); +
> >>> 	__vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode, +					msr,
> >>> MSR_TYPE_R); +} + +static void vmx_disable_intercept_for_msr_write(u32
> >>> msr, bool longmode_only) +{ +	if (!longmode_only)
> >>> +		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy, + 					msr,
> >>> MSR_TYPE_W); +
> >>> 	__vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode, +					msr,
> >>> MSR_TYPE_W);
> >>>  }
> >>>  
> >>>  /* @@ -7633,6 +7668,19 @@ static int __init vmx_init(void)
> >>>  	vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
> >>>  	vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
> >>> +	if (enable_apicv_reg_vid) {
> >>> +		int msr;
> >>> +		for (msr = 0x800; msr <= 0x8ff; msr++)
> >>> +			vmx_disable_intercept_for_msr_read(msr, false);
> >>> +
> >>> +		/* TPR */
> >>> +		vmx_disable_intercept_for_msr_write(0x808, false);
> >>> +		/* EOI */
> >>> +		vmx_disable_intercept_for_msr_write(0x80b, false);
> >>> +		/* SELF-IPI */
> >>> +		vmx_disable_intercept_for_msr_write(0x83f, false);
> >>> +	}
> >>> +
> >>>  	if (enable_ept) {
> >>>  		kvm_mmu_set_mask_ptes(0ull,
> >>>  			(enable_ept_ad_bits) ? VMX_EPT_ACCESS_BIT : 0ull,
> >>> --
> >>> 1.7.1
> >> 
> >> --
> >> 			Gleb.
> > 
> > 
> > Best regards,
> > Yang
> >
> 
> 
> Best regards,
> Yang
> 

--
			Gleb.