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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Jan 23, 2025 at 05:14:39PM +0100, Jan Beulich wrote:
> On 23.01.2025 15:24, Roger Pau Monné wrote:
> > 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.
> 
> Prior to IBT it wasn't foreseeable any pruning might be needed. We're
> in a similar position now: We simply can't know whether anything else
> is going to be needed there.
> 
> > 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.
> 
> As indicated, I'd prefer the IBT check to remain in the function. But
> yes, I'll see about renaming. If ever other stuff wants adding there,
> we can surely rename another time.
> 
> >>>  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.
> 
> In this case I view the redundancy as necessary. You want to know what
> to add to the functions when you look at them, irrespective of whether
> you also look at their caller.

OK, I won't oppose to it, but I do think function names need
adjusting.

Thanks, Roger.