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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <56FCA468.8090406@amd.com>
Date:	Thu, 31 Mar 2016 11:15:36 +0700
From:	Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
To:	Radim Krčmář <rkrcmar@...hat.com>
CC:	<pbonzini@...hat.com>, <joro@...tes.org>, <bp@...en8.de>,
	<gleb@...nel.org>, <alex.williamson@...hat.com>,
	<kvm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<wei@...hat.com>, <sherry.hurwitz@....com>
Subject: Re: [PART1 RFC v3 10/12] svm: Do not expose x2APIC when enable AVIC

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@....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@....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