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

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

On Wed, Nov 22, 2023 at 01:11:36PM +0100, Jan Beulich wrote:
> On 22.11.2023 13:01, Roger Pau Monné wrote:
> > 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?
> 
> Maybe. Yet then perhaps cpu_has_xen_ibt might better include the build-time
> check already?

I was wondering about this, yes, might be a better route.

Thanks, Roger.