__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.
---
v4: Rename functions. Re-base.
v3: Re-base.
v2: Use cpu_has_xen_ibt in {svm,vmx}_fill_funcs().

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -160,10 +160,17 @@ static int __init cf_check hvm_enable(vo
     else if ( using_svm() )
         fns = start_svm();
 
+    if ( fns )
+        hvm_funcs = *fns;
+
+    if ( IS_ENABLED(CONFIG_INTEL_VMX) )
+        vmx_fill_funcs();
+    if ( IS_ENABLED(CONFIG_AMD_SVM) )
+        svm_fill_funcs();
+
     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
@@ -2530,6 +2530,19 @@ const struct hvm_function_table * __init
     return &svm_function_table;
 }
 
+void __init svm_fill_funcs(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
@@ -3064,6 +3064,30 @@ const struct hvm_function_table * __init
     return &vmx_function_table;
 }
 
+void __init vmx_fill_funcs(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
@@ -260,6 +260,9 @@ extern int8_t hvm_port80_allowed;
 extern const struct hvm_function_table *start_svm(void);
 extern const struct hvm_function_table *start_vmx(void);
 
+void svm_fill_funcs(void);
+void vmx_fill_funcs(void);
+
 int hvm_domain_initialise(struct domain *d,
                           const struct xen_domctl_createdomain *config);
 void hvm_domain_relinquish_resources(struct domain *d);

On 25/02/2025 11:37 am, 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>

I don't especially like this, but I can't suggest anything better right
now, so

Acked-by: Andrew Cooper <andrew.cooper3@citrix.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.

This is all mixed up in the breakage underpinning nested-virt (the false
believe that there is some kind of "global" idea of how a VMCS is
configured).

I suspect this might just disappear in due course.

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

Why does the link order matter?

~Andrew

On 05.03.2025 15:47, Andrew Cooper wrote:
> On 25/02/2025 11:37 am, 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>
> 
> I don't especially like this, but I can't suggest anything better right
> now, so
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

>> 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.
> 
> Why does the link order matter?

Because hvm_enable() is a pre-SMP initcall, and if the new vendor functions
also were such, they'd need to run later.

Jan

On Tue, Feb 25, 2025 at 12:37:00PM +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>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

However I find this filling slightly ugly, and prone to be forgotten
when further hooks are added.

Would it make sense to delay enabling of IBT until after alternatives
have been applied, and thus simply not use the cf_clobber attribute on
functions that are patched to not be indirectly called?

We could still enable IBT before starting the APs.

Thanks, Roger.

On 05.03.2025 15:48, Roger Pau Monné wrote:
> On Tue, Feb 25, 2025 at 12:37:00PM +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>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> However I find this filling slightly ugly, and prone to be forgotten
> when further hooks are added.

Indeed. Luckily, while undesirable, that wouldn't be an outright bug.

> Would it make sense to delay enabling of IBT until after alternatives
> have been applied, and thus simply not use the cf_clobber attribute on
> functions that are patched to not be indirectly called?
> 
> We could still enable IBT before starting the APs.

I'd prefer if Andrew answered this. It looks like it might be an option,
but it also feels as if this would (if only a little) complicate logic
overall.

Jan

On Wed, Mar 05, 2025 at 04:02:51PM +0100, Jan Beulich wrote:
> On 05.03.2025 15:48, Roger Pau Monné wrote:
> > On Tue, Feb 25, 2025 at 12:37:00PM +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>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> > However I find this filling slightly ugly, and prone to be forgotten
> > when further hooks are added.
> 
> Indeed. Luckily, while undesirable, that wouldn't be an outright bug.
> 
> > Would it make sense to delay enabling of IBT until after alternatives
> > have been applied, and thus simply not use the cf_clobber attribute on
> > functions that are patched to not be indirectly called?
> > 
> > We could still enable IBT before starting the APs.
> 
> I'd prefer if Andrew answered this. It looks like it might be an option,
> but it also feels as if this would (if only a little) complicate logic
> overall.

It would indeed move the enabling a bit later, but overall (if
possible) it would IMO seem simpler than all this patching and filling
of tables.

Thanks, Roger.

On 05.03.2025 16:39, Roger Pau Monné wrote:
> On Wed, Mar 05, 2025 at 04:02:51PM +0100, Jan Beulich wrote:
>> On 05.03.2025 15:48, Roger Pau Monné wrote:
>>> On Tue, Feb 25, 2025 at 12:37:00PM +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>
>>>
>>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Thanks.
>>
>>> However I find this filling slightly ugly, and prone to be forgotten
>>> when further hooks are added.
>>
>> Indeed. Luckily, while undesirable, that wouldn't be an outright bug.
>>
>>> Would it make sense to delay enabling of IBT until after alternatives
>>> have been applied, and thus simply not use the cf_clobber attribute on
>>> functions that are patched to not be indirectly called?

Hmm, wait - how would that work? cf_clobber is used on function pointer
tables; any function indirectly callable prior to patching still needs
marking with cf_check, for build-time analysis to not throw errors (with
the specially patched gcc that Andrew prepared with a patch of H.J.'s).

>>> We could still enable IBT before starting the APs.
>>
>> I'd prefer if Andrew answered this. It looks like it might be an option,
>> but it also feels as if this would (if only a little) complicate logic
>> overall.
> 
> It would indeed move the enabling a bit later, but overall (if
> possible) it would IMO seem simpler than all this patching and filling
> of tables.

As per above, I think the patching is going to be needed anyway. And
hence I fear the table filling will continue to be needed, too.

Jan

On Wed, Mar 05, 2025 at 05:23:05PM +0100, Jan Beulich wrote:
> On 05.03.2025 16:39, Roger Pau Monné wrote:
> > On Wed, Mar 05, 2025 at 04:02:51PM +0100, Jan Beulich wrote:
> >> On 05.03.2025 15:48, Roger Pau Monné wrote:
> >>> On Tue, Feb 25, 2025 at 12:37:00PM +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>
> >>>
> >>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Thanks.
> >>
> >>> However I find this filling slightly ugly, and prone to be forgotten
> >>> when further hooks are added.
> >>
> >> Indeed. Luckily, while undesirable, that wouldn't be an outright bug.
> >>
> >>> Would it make sense to delay enabling of IBT until after alternatives
> >>> have been applied, and thus simply not use the cf_clobber attribute on
> >>> functions that are patched to not be indirectly called?
> 
> Hmm, wait - how would that work? cf_clobber is used on function pointer
> tables; any function indirectly callable prior to patching still needs
> marking with cf_check, for build-time analysis to not throw errors (with
> the specially patched gcc that Andrew prepared with a patch of H.J.'s).

Yeah, we would need something there?

Maybe disable such detection around alternative_{,v}call() usages if
possible?

I assume the build-time detection is done based on call sites?  We
would need to figure out whether the detection can be disabled for
chunks of code.

Thanks, Roger.

On 05.03.2025 18:26, Roger Pau Monné wrote:
> On Wed, Mar 05, 2025 at 05:23:05PM +0100, Jan Beulich wrote:
>> On 05.03.2025 16:39, Roger Pau Monné wrote:
>>> On Wed, Mar 05, 2025 at 04:02:51PM +0100, Jan Beulich wrote:
>>>> On 05.03.2025 15:48, Roger Pau Monné wrote:
>>>>> On Tue, Feb 25, 2025 at 12:37:00PM +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>
>>>>>
>>>>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>
>>>> Thanks.
>>>>
>>>>> However I find this filling slightly ugly, and prone to be forgotten
>>>>> when further hooks are added.
>>>>
>>>> Indeed. Luckily, while undesirable, that wouldn't be an outright bug.
>>>>
>>>>> Would it make sense to delay enabling of IBT until after alternatives
>>>>> have been applied, and thus simply not use the cf_clobber attribute on
>>>>> functions that are patched to not be indirectly called?
>>
>> Hmm, wait - how would that work? cf_clobber is used on function pointer
>> tables; any function indirectly callable prior to patching still needs
>> marking with cf_check, for build-time analysis to not throw errors (with
>> the specially patched gcc that Andrew prepared with a patch of H.J.'s).
> 
> Yeah, we would need something there?
> 
> Maybe disable such detection around alternative_{,v}call() usages if
> possible?
> 
> I assume the build-time detection is done based on call sites?

I think the build-time detection is based on places where addresses of
functions are taken. Call sites are close to impossible to re-associate
with the possible set of functions being called. If at all, that would
require the compiler to have a view of the entire image. Whereas the
warnings are issued as individual objects are being built.

Jan

>  We
> would need to figure out whether the detection can be disabled for
> chunks of code.
> 
> Thanks, Roger.