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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan