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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

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.

Jan