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

[Maxim Levitsky <mlevitsk@redhat.com>]

On Fri, 2023-12-01 at 16:36 +0800, Yang, Weijiang wrote:
> 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?

I don't know if we want to add these comments to the source - I made them
up for myself/you to understand the subtle differences between each of these variables.

There is some documentation on the struct fields, it is reasonable, but
I do think that it will help a lot to add documentation to each of
fpu_kernel_cfg, fpu_user_cfg and fpu_guest_cfg.


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

It's all up to you to decide. The thing is one big mess, IMHO no comment can really make it understandable
without hours of research.

However as usual, the more comments the better, comments do help.

Best regards,
	Maxim Levitsky


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