diff mbox

[PART1,RFC,v3,10/12] svm: Do not expose x2APIC when enable AVIC

Message ID 1458281388-14452-11-git-send-email-Suravee.Suthikulpanit@amd.com (mailing list archive)
State New, archived
Headers show

Commit Message

Suravee Suthikulpanit March 18, 2016, 6:09 a.m. UTC 
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Since AVIC only virtualizes xAPIC hardware for the guest, we need to:
    * Intercept APIC BAR msr accesses to disable x2APIC
    * Intercept CPUID access to not advertise x2APIC support
    * Hide x2APIC support when checking via KVM ioctl

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kvm/svm.c | 49 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 7 deletions(-)

Comments

Radim Krčmář March 18, 2016, 8:59 p.m. UTC | #1 
2016-03-18 01:09-0500, Suravee Suthikulpanit:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Since AVIC only virtualizes xAPIC hardware for the guest, we need to:
>     * Intercept APIC BAR msr accesses to disable x2APIC
>     * Intercept CPUID access to not advertise x2APIC support
>     * Hide x2APIC support when checking via KVM ioctl
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/kvm/svm.c | 49 ++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 6303147..ba84d57 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -209,6 +209,7 @@ static const struct svm_direct_access_msrs {
>  	{ .index = MSR_IA32_LASTBRANCHTOIP,		.always = false },
>  	{ .index = MSR_IA32_LASTINTFROMIP,		.always = false },
>  	{ .index = MSR_IA32_LASTINTTOIP,		.always = false },
> +	{ .index = MSR_IA32_APICBASE,			.always = false },
>  	{ .index = MSR_INVALID,				.always = false },
>  };
>  
> @@ -853,6 +854,9 @@ static void svm_vcpu_init_msrpm(u32 *msrpm)
>  
>  		set_msr_interception(msrpm, direct_access_msrs[i].index, 1, 1);
>  	}
> +
> +	if (svm_vcpu_avic_enabled(svm))
> +		set_msr_interception(msrpm, MSR_IA32_APICBASE, 1, 1);

AVIC really won't exit on writes to MSR_IA32_APICBASE otherwise?

> @@ -3308,6 +3312,18 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  			msr_info->data = 0x1E;
>  		}
>  		break;
> +	case MSR_IA32_APICBASE:
> +		if (svm_vcpu_avic_enabled(svm)) {
> +			/* Note:
> +			 * For AVIC, we need to disable X2APIC
> +			 * and enable XAPIC
> +			 */
> +			kvm_get_msr_common(vcpu, msr_info);
> +			msr_info->data &= ~X2APIC_ENABLE;
> +			msr_info->data |= XAPIC_ENABLE;
> +			break;

No.  This won't make the guest switch to xAPIC.
x2APIC can only be enabled if CPUID has that flag and it's impossible to
toggle that CPUID flag it during runtime.

> +		}
> +		/* Follow through if not AVIC */
>  	default:
>  		return kvm_get_msr_common(vcpu, msr_info);
>  	}
> @@ -3436,6 +3452,10 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  	case MSR_VM_IGNNE:
>  		vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data);
>  		break;
> +	case MSR_IA32_APICBASE:
> +		if (svm_vcpu_avic_enabled(svm))
> +			avic_update_vapic_bar(to_svm(vcpu), data);

There is no connection to x2APIC, please do it in a different patch.

> +		/* Follow through */
>  	default:
>  		return kvm_set_msr_common(vcpu, msr);
>  	}
> @@ -4554,11 +4574,26 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu)
>  
>  	/* Update nrips enabled cache */
>  	svm->nrips_enabled = !!guest_cpuid_has_nrips(&svm->vcpu);
> +
> +	/* Do not support X2APIC when enable AVIC */
> +	if (svm_vcpu_avic_enabled(svm)) {
> +		int i;
> +
> +		for (i = 0 ; i < vcpu->arch.cpuid_nent ; i++) {
> +			if (vcpu->arch.cpuid_entries[i].function == 1)

Please use kvm_find_cpuid_entry for the search.

> +				vcpu->arch.cpuid_entries[i].ecx &= ~(1 << 21);

and X86_FEATURE_X2APIC (or something with X2APIC in name) for the bit.

The code will become so obvious that the comment can be removed. :)

> +		}
> +	}
>  }
>  
>  static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
>  {
>  	switch (func) {
> +	case 0x00000001:
> +		/* Do not support X2APIC when enable AVIC */
> +		if (avic)
> +			entry->ecx &= ~(1 << 21);

I think this might be the right place for the code you have in
svm_cpuid_update.

Btw. how does x2APIC behave under AVIC?
We definitely shouldn't recommend/expose x2APIC with AVIC as AVIC
doesn't accelerate x2APIC guest-facing interface, but the MSR interface
is going to exit and host-side interrupt delivery will probably still
work, so I don't see a huge problem with it.

> +		break;
>  	case 0x80000001:
>  		if (nested)
>  			entry->ecx |= (1 << 2); /* Set SVM bit */
> -- 
> 1.9.1
> 
--
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
Suravee Suthikulpanit March 31, 2016, 4:15 a.m. UTC | #2 
Hi Radim,

On 03/19/2016 03:59 AM, Radim Kr?má? wrote:
> 2016-03-18 01:09-0500, Suravee Suthikulpanit:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> Since AVIC only virtualizes xAPIC hardware for the guest, we need to:
>>      * Intercept APIC BAR msr accesses to disable x2APIC
>>      * Intercept CPUID access to not advertise x2APIC support
>>      * Hide x2APIC support when checking via KVM ioctl
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> ---
>>   arch/x86/kvm/svm.c | 49 ++++++++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 42 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 6303147..ba84d57 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -209,6 +209,7 @@ static const struct svm_direct_access_msrs {
>>   	{ .index = MSR_IA32_LASTBRANCHTOIP,		.always = false },
>>   	{ .index = MSR_IA32_LASTINTFROMIP,		.always = false },
>>   	{ .index = MSR_IA32_LASTINTTOIP,		.always = false },
>> +	{ .index = MSR_IA32_APICBASE,			.always = false },
>>   	{ .index = MSR_INVALID,				.always = false },
>>   };
>>
>> @@ -853,6 +854,9 @@ static void svm_vcpu_init_msrpm(u32 *msrpm)
>>
>>   		set_msr_interception(msrpm, direct_access_msrs[i].index, 1, 1);
>>   	}
>> +
>> +	if (svm_vcpu_avic_enabled(svm))
>> +		set_msr_interception(msrpm, MSR_IA32_APICBASE, 1, 1);
>
> AVIC really won't exit on writes to MSR_IA32_APICBASE otherwise?

Actually, I got confused about this part. This should not be needed.

>
>> @@ -3308,6 +3312,18 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>   			msr_info->data = 0x1E;
>>   		}
>>   		break;
>> +	case MSR_IA32_APICBASE:
>> +		if (svm_vcpu_avic_enabled(svm)) {
>> +			/* Note:
>> +			 * For AVIC, we need to disable X2APIC
>> +			 * and enable XAPIC
>> +			 */
>> +			kvm_get_msr_common(vcpu, msr_info);
>> +			msr_info->data &= ~X2APIC_ENABLE;
>> +			msr_info->data |= XAPIC_ENABLE;
>> +			break;
>
> No.  This won't make the guest switch to xAPIC.
> x2APIC can only be enabled if CPUID has that flag and it's impossible to
> toggle that CPUID flag it during runtime.

This is also not needed since we already disable the x2APIC in the CPUID 
below.

>> +		}
>> +		/* Follow through if not AVIC */
>>   	default:
>>   		return kvm_get_msr_common(vcpu, msr_info);
>>   	}
>> @@ -3436,6 +3452,10 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>>   	case MSR_VM_IGNNE:
>>   		vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data);
>>   		break;
>> +	case MSR_IA32_APICBASE:
>> +		if (svm_vcpu_avic_enabled(svm))
>> +			avic_update_vapic_bar(to_svm(vcpu), data);
>
> There is no connection to x2APIC, please do it in a different patch.

Right. I'll move this.

>
>> +		/* Follow through */
>>   	default:
>>   		return kvm_set_msr_common(vcpu, msr);
>>   	}
>> @@ -4554,11 +4574,26 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu)
>>
>>   	/* Update nrips enabled cache */
>>   	svm->nrips_enabled = !!guest_cpuid_has_nrips(&svm->vcpu);
>> +
>> +	/* Do not support X2APIC when enable AVIC */
>> +	if (svm_vcpu_avic_enabled(svm)) {
>> +		int i;
>> +
>> +		for (i = 0 ; i < vcpu->arch.cpuid_nent ; i++) {
>> +			if (vcpu->arch.cpuid_entries[i].function == 1)
>
> Please use kvm_find_cpuid_entry for the search.
>
>> +				vcpu->arch.cpuid_entries[i].ecx &= ~(1 << 21);
>
> and X86_FEATURE_X2APIC (or something with X2APIC in name) for the bit.
>
> The code will become so obvious that the comment can be removed. :)

Good point. I can only find example of using (X86_FEATURE_X2APIC % 32) 
== 21.

>> +		}
>> +	}
>>   }
>>
>>   static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
>>   {
>>   	switch (func) {
>> +	case 0x00000001:
>> +		/* Do not support X2APIC when enable AVIC */
>> +		if (avic)
>> +			entry->ecx &= ~(1 << 21);
>
> I think this might be the right place for the code you have in
> svm_cpuid_update.

Right. I'll also make change to use (X86_FEATURE_X2APIC % 32)

> Btw. how does x2APIC behave under AVIC?
> We definitely shouldn't recommend/expose x2APIC with AVIC as AVIC
> doesn't accelerate x2APIC guest-facing interface,

Access to offset 0x400+ would generate #VMEXIT no accel fault 
read/write.  So, we will need to handle and emulate this in the host.

> but the MSR interface is going to exit and host-side interrupt
 > delivery will probably still work, so I don't see
 > a huge problem with it.

Agree that it will still work. However, in such case, the guest code 
would likely default to using x2APIC interface, which will not be 
handled by the AVIC hardware, and resulting in no performance 
improvement that we are trying to introduce.

Thanks,
Suravee


--
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
Paolo Bonzini March 31, 2016, 11:23 a.m. UTC | #3 
On 31/03/2016 06:15, Suravee Suthikulpanit wrote:
>>> +                vcpu->arch.cpuid_entries[i].ecx &= ~(1 << 21);
>> 
>> and X86_FEATURE_X2APIC (or something with X2APIC in name) for the
>> bit.
>> 
>> The code will become so obvious that the comment can be removed.
>> :)
> 
> Good point. I can only find example of using (X86_FEATURE_X2APIC %
> 32) == 21.

You can use bit(X86_FEATURE_X2APIC), it is defined in arch/x86/kvm/x86.h.

>> but the MSR interface is going to exit and host-side interrupt 
>> delivery will probably still work, so I don't see a huge problem
>> with it.
> 
> Agree that it will still work. However, in such case, the guest code
> would likely default to using x2APIC interface, which will not be
> handled by the AVIC hardware, and resulting in no performance
> improvement that we are trying to introduce.

You would still get some improvement from exit-free interrupt delivery.

Paolo
--
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
Suravee Suthikulpanit April 5, 2016, 10:14 a.m. UTC | #4 
Hi Paolo,

On 3/31/16 18:23, Paolo Bonzini wrote:
>
>
> On 31/03/2016 06:15, Suravee Suthikulpanit wrote:
>>>> +                vcpu->arch.cpuid_entries[i].ecx &= ~(1 << 21);
>>>
>>> and X86_FEATURE_X2APIC (or something with X2APIC in name) for the
>>> bit.
>>>
>>> The code will become so obvious that the comment can be removed.
>>> :)
>>
>> Good point. I can only find example of using (X86_FEATURE_X2APIC %
>> 32) == 21.
>
> You can use bit(X86_FEATURE_X2APIC), it is defined in arch/x86/kvm/x86.h.

Ahh, thanks.

>
>>> but the MSR interface is going to exit and host-side interrupt
>>> delivery will probably still work, so I don't see a huge problem
>>> with it.
>>
>> Agree that it will still work. However, in such case, the guest code
>> would likely default to using x2APIC interface, which will not be
>> handled by the AVIC hardware, and resulting in no performance
>> improvement that we are trying to introduce.
>
> You would still get some improvement from exit-free interrupt delivery.

Let me look into this and investigate some more.

Thanks,
Suravee
--
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
diff mbox

Patch

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 6303147..ba84d57 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -209,6 +209,7 @@  static const struct svm_direct_access_msrs {
 	{ .index = MSR_IA32_LASTBRANCHTOIP,		.always = false },
 	{ .index = MSR_IA32_LASTINTFROMIP,		.always = false },
 	{ .index = MSR_IA32_LASTINTTOIP,		.always = false },
+	{ .index = MSR_IA32_APICBASE,			.always = false },
 	{ .index = MSR_INVALID,				.always = false },
 };
 
@@ -841,7 +842,7 @@  static void set_msr_interception(u32 *msrpm, unsigned msr,
 	msrpm[offset] = tmp;
 }
 
-static void svm_vcpu_init_msrpm(u32 *msrpm)
+static void svm_vcpu_init_msrpm(struct vcpu_svm *svm, u32 *msrpm)
 {
 	int i;
 
@@ -853,6 +854,9 @@  static void svm_vcpu_init_msrpm(u32 *msrpm)
 
 		set_msr_interception(msrpm, direct_access_msrs[i].index, 1, 1);
 	}
+
+	if (svm_vcpu_avic_enabled(svm))
+		set_msr_interception(msrpm, MSR_IA32_APICBASE, 1, 1);
 }
 
 static void add_msr_offset(u32 offset)
@@ -1394,18 +1398,18 @@  static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
 
 	svm->nested.hsave = page_address(hsave_page);
 
-	svm->msrpm = page_address(msrpm_pages);
-	svm_vcpu_init_msrpm(svm->msrpm);
-
-	svm->nested.msrpm = page_address(nested_msrpm_pages);
-	svm_vcpu_init_msrpm(svm->nested.msrpm);
-
 	svm->vmcb = page_address(page);
 	clear_page(svm->vmcb);
 	svm->vmcb_pa = page_to_pfn(page) << PAGE_SHIFT;
 	svm->asid_generation = 0;
 	init_vmcb(svm);
 
+	svm->msrpm = page_address(msrpm_pages);
+	svm_vcpu_init_msrpm(svm, svm->msrpm);
+
+	svm->nested.msrpm = page_address(nested_msrpm_pages);
+	svm_vcpu_init_msrpm(svm, svm->nested.msrpm);
+
 	svm_init_osvw(&svm->vcpu);
 
 	return &svm->vcpu;
@@ -3308,6 +3312,18 @@  static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			msr_info->data = 0x1E;
 		}
 		break;
+	case MSR_IA32_APICBASE:
+		if (svm_vcpu_avic_enabled(svm)) {
+			/* Note:
+			 * For AVIC, we need to disable X2APIC
+			 * and enable XAPIC
+			 */
+			kvm_get_msr_common(vcpu, msr_info);
+			msr_info->data &= ~X2APIC_ENABLE;
+			msr_info->data |= XAPIC_ENABLE;
+			break;
+		}
+		/* Follow through if not AVIC */
 	default:
 		return kvm_get_msr_common(vcpu, msr_info);
 	}
@@ -3436,6 +3452,10 @@  static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 	case MSR_VM_IGNNE:
 		vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data);
 		break;
+	case MSR_IA32_APICBASE:
+		if (svm_vcpu_avic_enabled(svm))
+			avic_update_vapic_bar(to_svm(vcpu), data);
+		/* Follow through */
 	default:
 		return kvm_set_msr_common(vcpu, msr);
 	}
@@ -4554,11 +4574,26 @@  static void svm_cpuid_update(struct kvm_vcpu *vcpu)
 
 	/* Update nrips enabled cache */
 	svm->nrips_enabled = !!guest_cpuid_has_nrips(&svm->vcpu);
+
+	/* Do not support X2APIC when enable AVIC */
+	if (svm_vcpu_avic_enabled(svm)) {
+		int i;
+
+		for (i = 0 ; i < vcpu->arch.cpuid_nent ; i++) {
+			if (vcpu->arch.cpuid_entries[i].function == 1)
+				vcpu->arch.cpuid_entries[i].ecx &= ~(1 << 21);
+		}
+	}
 }
 
 static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
 {
 	switch (func) {
+	case 0x00000001:
+		/* Do not support X2APIC when enable AVIC */
+		if (avic)
+			entry->ecx &= ~(1 << 21);
+		break;
 	case 0x80000001:
 		if (nested)
 			entry->ecx |= (1 << 2); /* Set SVM bit */