Re: [PATCH v3 02/12] x86/HVM: improve CET-IBT pruning of ENDBR

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Jan 23, 2025 at 02:18:41PM +0100, Jan Beulich wrote:
> On 23.01.2025 13:41, Roger Pau Monné wrote:
> > On Mon, Feb 26, 2024 at 05:42:20PM +0100, Jan Beulich wrote:
> >> --- a/xen/arch/x86/hvm/hvm.c
> >> +++ b/xen/arch/x86/hvm/hvm.c
> >> @@ -161,10 +161,15 @@ static int __init cf_check hvm_enable(vo
> >>      else if ( cpu_has_svm )
> >>          fns = start_svm();
> >>  
> >> +    if ( fns )
> >> +        hvm_funcs = *fns;
> >> +
> >> +    prune_vmx();
> >> +    prune_svm();
> > 
> > Isn't it actually the opposite of pruning.  What the helpers do is
> > fill all the pointers in the structure.
> 
> With the goal of their ENDBR to then be pruned. I agree though that the
> functions don't do any pruning themselves. Yet
> {svm,vmx}_prepare_for_cet_ibt_pruning() is a little awkward for my taste
> (although it would properly document the purpose). Plus ...
> 
> >  I would rather name them {vmx,svm}_fill_hvm_funcs() or similar.
> 
> ... while I can use those names (perhaps without the "hvm" infix), the
> present names have the advantage that any other pruning that we may
> find desirable could also be put there. Hence also why the cpu_has_*
> checks live there.

Hm, I'm unsure.  What else do you see becoming part of those
functions?  It's hard for me to suggest a name when it's unclear what
future logic do you think they could contain.

Given the current code I still think something that contains 'fill' or
similar is way more appropriate, the more if the IBT check is pulled
out into the caller.

> >  And possibly pull the
> > cpu_has_xen_ibt check outside the functions:
> > 
> > if ( cpu_has_xen_ibt )
> > {
> >     /*
> >      * Now that svm_function_table was copied, populate all function pointers
> >      * which may have been left at NULL, for __initdata_cf_clobber to have as
> >      * much of an effect as possible.
> >      */
> >     vmx_fill_hvm_funcs();
> >     svm_fill_hvm_funcs();
> > }
> 
> Which would leave the SVM function entirely empty.

You could possible declare it as an static inline in the hvm.h header
for the time being?

> The intention was for
> that to not be the case, and also for the comment you have added above
> to also live in the per-vendor functions.

Isn't that a bit redundant?  I would prefer to not have duplicated
comments over the code, hence my suggestion to place part of the logic
in the caller.

> > I would be nice to avoid directly exporting more vmx and smv specific
> > helpers, as if we ever want to compile out vmx or svm it would be more
> > churn to deal with those.  I however cannot think of any good way to
> > do this here, so it's fine to export those functions.
> 
> It could be another hook, just that the hook pointer then would point
> into .init.text (i.e. become stale once we purge .init.*). We could zap
> it of course after invoking it ...

Yes, I also think this is not great, and prefer your current proposal.

> Note that the vendor function invocations have meanwhile, in the course
> of re-basing, gained "if ( IS_ENABLED(CONFIG_...) )", so no extra
> (future) churn for the (already available) option you talk about.

Oh, OK, that's better then.

Thanks, Roger.