linux-kernel - Re: [PATCH v2 4/6] x86/fpu/xstate: Introduce fpu_guest

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: <Z8gSDhr1ZfIEVVgU@intel.com>
Date: Wed, 5 Mar 2025 16:57:50 +0800
From: Chao Gao <chao.gao@...el.com>
To: Dave Hansen <dave.hansen@...el.com>
CC: <tglx@...utronix.de>, <x86@...nel.org>, <seanjc@...gle.com>,
	<pbonzini@...hat.com>, <linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>,
	<peterz@...radead.org>, <rick.p.edgecombe@...el.com>, <mlevitsk@...hat.com>,
	<weijiang.yang@...el.com>, <john.allen@....com>
Subject: Re: [PATCH v2 4/6] x86/fpu/xstate: Introduce fpu_guest_cfg for guest
 FPU configuration

On Tue, Mar 04, 2025 at 02:50:48PM -0800, Dave Hansen wrote:
>On 11/26/24 02:17, Chao Gao wrote:
>> From: Yang Weijiang <weijiang.yang@...el.com>
>> 
>> Define new fpu_guest_cfg to hold all guest FPU settings so that it can
>> differ from generic kernel FPU settings, e.g., enabling CET supervisor
>> xstate by default for guest fpstate while it's remained disabled in
>> kernel FPU config.
>> 
>> The kernel dynamic xfeatures are specifically used by guest fpstate now,
>> add the mask for guest fpstate so that guest_perm.__state_perm ==
>> (fpu_kernel_cfg.default_xfeature | XFEATURE_MASK_KERNEL_DYNAMIC). And
>> if guest fpstate is re-allocated to hold user dynamic xfeatures, the
>> resulting permissions are consumed before calculate new guest fpstate.
>
>This kinda restates what the code does, but I don't think it matches the
>code.

Actually, it does (see more below)

>
>> With new guest FPU config added, there're 3 categories of FPU configs in
>> kernel, the usages and key fields are recapped as below.
>
>This changelog is pretty rough. It's got a lot of words but not much
>substance.

Will revise the changelog.

[...]

>
>
>> 
>>   @fpu_guest_cfg.default_size
>>   - size of compacted buffer with 'fpu_guest_cfg.default_features'
>
>This looks like kerneldoc, not changelog. Are you sure you want it _here_?

No, I agree this should be a comment above fpu_guest/kernel/user_cfg.

>
>
>> @@ -829,6 +835,10 @@ void __init fpu__init_system_xstate(unsigned int legacy_size)
>>  	fpu_user_cfg.default_features = fpu_user_cfg.max_features;
>>  	fpu_user_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC;
>>  
>> +	fpu_guest_cfg.max_features = fpu_kernel_cfg.max_features;
>> +	fpu_guest_cfg.default_features = fpu_guest_cfg.max_features;
>> +	fpu_guest_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC;
>
>I thought this was saying above that it was _setting_ dynamic features.
>
>Why _not_ set them by default?

At first glance, I had the same question.

XFEATURE_MASK_*KERNEL*_DYNAMIC is not excluded here, so it is enabled by
default. I believe the confusion arises partly from the order of the
patches. I will reorder the patches as you suggested in patch 5.