linux-kernel - Re: [PATCH RFC 5/7] kvm: x86: Revise CPUID.D.1.EBX for alignment rule

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: <edb5ae3f-3c7f-0647-d586-98a9e3e15249@linux.intel.com>
Date:   Thu, 3 Jun 2021 12:45:48 +0800
From:   "Liu, Jing2" <jing2.liu@...ux.intel.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     pbonzini@...hat.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, jing2.liu@...el.com
Subject: Re: [PATCH RFC 5/7] kvm: x86: Revise CPUID.D.1.EBX for alignment rule



On 5/25/2021 5:28 AM, Sean Christopherson wrote:
> On Sun, Feb 07, 2021, Jing Liu wrote:
>> CPUID.0xD.1.EBX[1] is set if, when the compacted format of an XSAVE
>> area is used, this extended state component located on the next
>> 64-byte boundary following the preceding state component (otherwise,
>> it is located immediately following the preceding state component).
>>
>> AMX tileconfig and tiledata are the first to use 64B alignment.
>> Revise the runtime cpuid modification for this rule.
>>
>> Signed-off-by: Jing Liu<jing2.liu@...ux.intel.com>
>> ---
>>   arch/x86/kvm/cpuid.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 04a73c395c71..ee1fac0a865e 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -35,12 +35,17 @@ static u32 xstate_required_size(u64 xstate_bv, bool compacted)
>>   {
>>   	int feature_bit = 0;
>>   	u32 ret = XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET;
>> +	bool is_aligned = false;
>>   
>>   	xstate_bv &= XFEATURE_MASK_EXTEND;
>>   	while (xstate_bv) {
>>   		if (xstate_bv & 0x1) {
>>   		        u32 eax, ebx, ecx, edx, offset;
>>   		        cpuid_count(0xD, feature_bit, &eax, &ebx, &ecx, &edx);
>> +			/* ECX[2]: 64B alignment in compacted form */
>> +			is_aligned = !!(ecx & 2);
>> +			if (is_aligned && compacted)
> I'd forego the local is_aligned, and also check "compacted" first so that the
> uncompacted variant isn't required to evaluated ecx.
Non-compacted only works for XCR0 (user states), do we need add a check 
or simply do
'state_bv &= XFEATURE_MASK_USER_ENABLED'?

         xstate_bv &= XFEATURE_MASK_EXTEND;
+       /* Only user states use non-compacted format. */
+       if (!compacted)
+               xstate_bv &= XFEATURE_MASK_USER_SUPPORTED;
+
         while (xstate_bv) {
                 if (xstate_bv & 0x1) {
                         u32 eax, ebx, ecx, edx, offset;
                         cpuid_count(0xD, feature_bit, &eax, &ebx, &ecx, 
&edx);
-                       offset = compacted ? ret : ebx;
+                       offset = compacted ? ((ecx & 0x2) ? ALIGN(ret, 
64) : ret) : ebx;
                         ret = max(ret, offset + eax);
                 }

>
> And the real reason I am responding... can you post this as a standalone patch?
Sure. Let me separate it.
> I stumbled across the "aligned" flag when reading through the SDM for a completely
> unrelated reason, and also discovered that the flag has been documented since
> 2016.  While AMX may be the first to "officially" utilize the alignment flag,
> the flag itself is architectural and not strictly limited to AMX.
Yes, this is not a new feature, but seems no one use it before.

Thanks,
Jing
>> +				ret = ALIGN(ret, 64);
>>   			offset = compacted ? ret : ebx;
>>   			ret = max(ret, offset + eax);
>>   		}
>> -- 
>> 2.18.4
>>