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.
