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

[Radim KrÄmÃÅ]

2016-03-18 01:09-0500, Suravee Suthikulpanit:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> 
> 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@xxxxxxx>
> ---
>  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
>