linux-kernel - Re: [PATCH v7 06/26] x86/fpu/xstate: Create guest fpstate with guest specific config

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: <0112b446-ee7e-4b78-b3a4-671d3ba67299@intel.com>
Date:   Fri, 1 Dec 2023 16:36:23 +0800
From:   "Yang, Weijiang" <weijiang.yang@...el.com>
To:     Maxim Levitsky <mlevitsk@...hat.com>
CC:     <dave.hansen@...el.com>, <pbonzini@...hat.com>,
        <seanjc@...gle.com>, <peterz@...radead.org>, <chao.gao@...el.com>,
        <rick.p.edgecombe@...el.com>, <john.allen@....com>,
        <kvm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v7 06/26] x86/fpu/xstate: Create guest fpstate with guest
 specific config

On 12/1/2023 1:36 AM, Maxim Levitsky wrote:

[...]

>> +	fpstate->user_size	= fpu_user_cfg.default_size;
>> +	fpstate->user_xfeatures	= fpu_user_cfg.default_features;
> The whole thing makes my head spin like the good old CD/DVD writers used to ....
>
> So just to summarize this is what we have:
>
>
> KERNEL FPU CONFIG
>
> /*
>     all known and CPU supported user and supervisor features except
>     - "dynamic" kernel features" (CET_S)
>     - "independent" kernel features (XFEATURE_LBR)
> */
> fpu_kernel_cfg.max_features;
>
> /*
>     all known and CPU supported user and supervisor features except
>      - "dynamic" kernel features" (CET_S)
>      - "independent" kernel features (arch LBRs)
>      - "dynamic" userspace features (AMX state)
> */
> fpu_kernel_cfg.default_features;
>
>
> // size of compacted buffer with 'fpu_kernel_cfg.max_features'
> fpu_kernel_cfg.max_size;
>
>
> // size of compacted buffer with 'fpu_kernel_cfg.default_features'
> fpu_kernel_cfg.default_size;
>
>
> USER FPU CONFIG
>
> /*
>     all known and CPU supported user features
> */
> fpu_user_cfg.max_features;
>
> /*
>     all known and CPU supported user features except
>     - "dynamic" userspace features (AMX state)
> */
> fpu_user_cfg.default_features;
>
> // size of non compacted buffer with 'fpu_user_cfg.max_features'
> fpu_user_cfg.max_size;
>
> // size of non compacted buffer with 'fpu_user_cfg.default_features'
> fpu_user_cfg.default_size;
>
>
> GUEST FPU CONFIG
> /*
>     all known and CPU supported user and supervisor features except
>     - "independent" kernel features (XFEATURE_LBR)
> */
> fpu_guest_cfg.max_features;
>
> /*
>     all known and CPU supported user and supervisor features except
>      - "independent" kernel features (arch LBRs)
>      - "dynamic" userspace features (AMX state)
> */
> fpu_guest_cfg.default_features;
>
> // size of compacted buffer with 'fpu_guest_cfg.max_features'
> fpu_guest_cfg.max_size;
>
> // size of compacted buffer with 'fpu_guest_cfg.default_features'
> fpu_guest_cfg.default_size;

Good suggestion! Thanks!
how about adding them in patch 5 to make the summaries manifested?

> ---
>
>
> So in essence, guest FPU config is guest kernel fpu config and that is why
> 'fpu_user_cfg.default_size' had to be used above.
>
> How about that we have fpu_guest_kernel_config and fpu_guest_user_config instead
> to make the whole horrible thing maybe even more complicated but at least a bit more orthogonal?

I think it becomes necessary when there were more guest user/kernel xfeaures requiring
special handling like CET-S MSRs, then it looks much reasonable to split guest config into two,
but now we only have one single outstanding xfeature for guest. IMHO, existing definitions still
work with a few comments.

But I really like your ideas of making things clean and tidy :-)