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

From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	Kevin Tian <kevin.tian@intel.com>,
	Jun Nakajima <jun.nakajima@intel.com>
Subject: Re: [PATCH 5/5] x86/HVM: improve CET-IBT pruning of ENDBR
Date: Wed, 22 Nov 2023 13:11:36 +0100	[thread overview]
Message-ID: <12100118-b442-4cba-a8c3-5bfd33de0081@suse.com> (raw)
In-Reply-To: <ZV3tg_FjPP3ybJFp@macbook>

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?

Jan