__init{const,data}_cf_clobber can have an effect only for pointers
actually populated in the respective tables. While not the case for SVM
right now, VMX installs a number of pointers only under certain
conditions. Hence the respective functions would have their ENDBR purged
only when those conditions are met. Invoke "pruning" functions after
having copied the respective tables, for them to install any "missing"
pointers.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
This is largely cosmetic for present hardware, which when supporting
CET-IBT likely also supports all of the advanced VMX features for which
hook pointers are installed conditionally. The only case this would make
a difference there is when use of respective features was suppressed via
command line option (where available). For future hooks it may end up
relevant even by default, and it also would be if AMD started supporting
CET-IBT; right now it matters only for .pi_update_irte, as iommu_intpost
continues to default to off.

Originally I had meant to put the SVM and VMX functions in presmp-
initcalls, but hvm/{svm,vmx}/built_in.o are linked into hvm/built_in.o
before hvm/hvm.o. And I don't think I want to fiddle with link order
here.
---
v3: Re-base.
v2: Use cpu_has_xen_ibt in prune_{svm,vmx}().

--- 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();
+
     if ( fns == NULL )
         return 0;
 
-    hvm_funcs = *fns;
     hvm_enabled = 1;
 
     printk("HVM: %s enabled\n", fns->name);
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2587,6 +2587,19 @@ const struct hvm_function_table * __init
     return &svm_function_table;
 }
 
+void __init prune_svm(void)
+{
+    /*
+     * 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.
+     */
+    if ( !cpu_has_xen_ibt )
+        return;
+
+    /* Nothing at present. */
+}
+
 void asmlinkage svm_vmexit_handler(void)
 {
     struct cpu_user_regs *regs = guest_cpu_user_regs();
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3042,6 +3042,30 @@ const struct hvm_function_table * __init
     return &vmx_function_table;
 }
 
+void __init prune_vmx(void)
+{
+    /*
+     * Now that vmx_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.
+     */
+    if ( !cpu_has_xen_ibt )
+        return;
+
+    vmx_function_table.set_descriptor_access_exiting =
+        vmx_set_descriptor_access_exiting;
+
+    vmx_function_table.update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap;
+    vmx_function_table.process_isr            = vmx_process_isr;
+    vmx_function_table.handle_eoi             = vmx_handle_eoi;
+
+    vmx_function_table.pi_update_irte = vmx_pi_update_irte;
+
+    vmx_function_table.deliver_posted_intr = vmx_deliver_posted_intr;
+    vmx_function_table.sync_pir_to_irr     = vmx_sync_pir_to_irr;
+    vmx_function_table.test_pir            = vmx_test_pir;
+}
+
 /*
  * Not all cases receive valid value in the VM-exit instruction length field.
  * Callers must know what they're doing!
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -250,6 +250,9 @@ extern s8 hvm_port80_allowed;
 extern const struct hvm_function_table *start_svm(void);
 extern const struct hvm_function_table *start_vmx(void);
 
+void prune_svm(void);
+void prune_vmx(void);
+
 int hvm_domain_initialise(struct domain *d,
                           const struct xen_domctl_createdomain *config);
 void hvm_domain_relinquish_resources(struct domain *d);

On Mon, Feb 26, 2024 at 05:42:20PM +0100, Jan Beulich wrote:
> __init{const,data}_cf_clobber can have an effect only for pointers
> actually populated in the respective tables. While not the case for SVM
> right now, VMX installs a number of pointers only under certain
> conditions. Hence the respective functions would have their ENDBR purged
> only when those conditions are met. Invoke "pruning" functions after
> having copied the respective tables, for them to install any "missing"
> pointers.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> This is largely cosmetic for present hardware, which when supporting
> CET-IBT likely also supports all of the advanced VMX features for which
> hook pointers are installed conditionally. The only case this would make
> a difference there is when use of respective features was suppressed via
> command line option (where available). For future hooks it may end up
> relevant even by default, and it also would be if AMD started supporting
> CET-IBT; right now it matters only for .pi_update_irte, as iommu_intpost
> continues to default to off.
> 
> Originally I had meant to put the SVM and VMX functions in presmp-
> initcalls, but hvm/{svm,vmx}/built_in.o are linked into hvm/built_in.o
> before hvm/hvm.o. And I don't think I want to fiddle with link order
> here.
> ---
> v3: Re-base.
> v2: Use cpu_has_xen_ibt in prune_{svm,vmx}().
> 
> --- 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.  I would rather name them
{vmx,svm}_fill_hvm_funcs() or similar.  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();
}

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.

Thanks, Roger.

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

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.

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

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.