Re: [PATCH 5/5] x86/HVM: improve CET-IBT pruning of ENDBR

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

On Wed, Nov 22, 2023 at 11:42:16AM +0100, Jan Beulich wrote:
> On 22.11.2023 11:08, Roger Pau Monné wrote:
> > On Thu, Nov 16, 2023 at 02:33:14PM +0100, Jan Beulich wrote:
> >> --- 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 ( !IS_ENABLED(CONFIG_XEN_IBT) )
> > 
> > Shouldn't this better use cpu_has_xen_ibt?
> > 
> > Otherwise the clobbering done in _apply_alternatives() won't be
> > engaged, so it's pointless to set the extra fields.
> 
> That's better answered in the context of ...
> 
> >> --- a/xen/arch/x86/hvm/vmx/vmx.c
> >> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >> @@ -3032,6 +3032,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 ( !IS_ENABLED(CONFIG_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;
> 
> ... this: The goal of having a compile time conditional was to have the
> compiler eliminate the code when not needed. Otherwise there's no real
> reason to have a conditional there in the first place - we can as well
> always install all these pointers.

Maybe do:

if ( !IS_ENABLED(CONFIG_XEN_IBT) || !cpu_has_xen_ibt )

then?

Thanks, Roger.