[PATCH 0/5] x86/HVM: misc tidying

[PATCH 0/5] x86/HVM: misc tidying

[Jan Beulich]

1: VMX: drop vmx_virt_exception and make vmx_vmfunc static
2: x86/HVM: hide SVM/VMX when their enabling is prohibited by firmware
3: VMX: don't run with CR4.VMXE set when VMX could not be enabled
4: x86/HVM: drop tsc_scaling.setup() hook
5: x86/HVM: improve CET-IBT pruning of ENDBR

Jan

[PATCH 1/5] VMX: drop vmx_virt_exception and make vmx_vmfunc static

[Jan Beulich]

The variable was introduced by 69b830e5ffb4 ("VMX: VMFUNC and #VE
definitions and detection") without any use and - violating Misra C:2012
rule 8.4 - without a declaration. Since no use has appeared, drop it.

For vmx_vmfunc the situation is similar, but not identical: It at least
has one use. Convert it to be static (and make style adjustments while
there).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
In how far the sole vmx_vmfunc use is actually meaningful (on its own)
I'm not really sure.

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -167,8 +167,7 @@ u32 vmx_secondary_exec_control __read_mo
 u32 vmx_vmexit_control __read_mostly;
 u32 vmx_vmentry_control __read_mostly;
 u64 vmx_ept_vpid_cap __read_mostly;
-u64 vmx_vmfunc __read_mostly;
-bool_t vmx_virt_exception __read_mostly;
+static uint64_t __read_mostly vmx_vmfunc;
 
 static DEFINE_PER_CPU_READ_MOSTLY(paddr_t, vmxon_region);
 static DEFINE_PER_CPU(paddr_t, current_vmcs);
@@ -475,8 +474,7 @@ static int vmx_init_vmcs_config(bool bsp
         vmx_basic_msr              = ((u64)vmx_basic_msr_high << 32) |
                                      vmx_basic_msr_low;
         vmx_vmfunc                 = _vmx_vmfunc;
-        vmx_virt_exception         = !!(_vmx_secondary_exec_control &
-                                       SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS);
+
         vmx_display_features();
 
         /* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */

Re: [PATCH 1/5] VMX: drop vmx_virt_exception and make vmx_vmfunc static

[Roger Pau Monné]

On Thu, Nov 16, 2023 at 02:30:41PM +0100, Jan Beulich wrote:
> The variable was introduced by 69b830e5ffb4 ("VMX: VMFUNC and #VE
> definitions and detection") without any use and - violating Misra C:2012
> rule 8.4 - without a declaration. Since no use has appeared, drop it.
> 
> For vmx_vmfunc the situation is similar, but not identical: It at least
> has one use. Convert it to be static (and make style adjustments while
> there).

I think you could also remove the sole user of vmx_vmfunc, as it's
just a cap_check() usage (unless there are more hidden usages).

> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

> ---
> In how far the sole vmx_vmfunc use is actually meaningful (on its own)
> I'm not really sure.
> 
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -167,8 +167,7 @@ u32 vmx_secondary_exec_control __read_mo
>  u32 vmx_vmexit_control __read_mostly;
>  u32 vmx_vmentry_control __read_mostly;
>  u64 vmx_ept_vpid_cap __read_mostly;
> -u64 vmx_vmfunc __read_mostly;
> -bool_t vmx_virt_exception __read_mostly;
> +static uint64_t __read_mostly vmx_vmfunc;

I'm quite sure this should be __ro_after_init, but I guess we cannot
be sure give the current code in vmx_init_vmcs_config().

Any CPU hot plugged that has a different set of VMX controls should
not be onlined, the more that migrating an already running VMCS to
such CPU will lead to failures if non-supported features are used.

Thanks, Roger.

Re: [PATCH 1/5] VMX: drop vmx_virt_exception and make vmx_vmfunc static

[Jan Beulich]

On 21.11.2023 16:48, Roger Pau Monné wrote:
> On Thu, Nov 16, 2023 at 02:30:41PM +0100, Jan Beulich wrote:
>> The variable was introduced by 69b830e5ffb4 ("VMX: VMFUNC and #VE
>> definitions and detection") without any use and - violating Misra C:2012
>> rule 8.4 - without a declaration. Since no use has appeared, drop it.
>>
>> For vmx_vmfunc the situation is similar, but not identical: It at least
>> has one use. Convert it to be static (and make style adjustments while
>> there).
> 
> I think you could also remove the sole user of vmx_vmfunc, as it's
> just a cap_check() usage (unless there are more hidden usages).

Well, perhaps (and hence my post-commit-message remark in the original
submission). Yet then I thought we permitted vmfunc use for altp2m, at
which point the cap_check() is meaningful.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> In how far the sole vmx_vmfunc use is actually meaningful (on its own)
>> I'm not really sure.

(Here ^^^)

>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -167,8 +167,7 @@ u32 vmx_secondary_exec_control __read_mo
>>  u32 vmx_vmexit_control __read_mostly;
>>  u32 vmx_vmentry_control __read_mostly;
>>  u64 vmx_ept_vpid_cap __read_mostly;
>> -u64 vmx_vmfunc __read_mostly;
>> -bool_t vmx_virt_exception __read_mostly;
>> +static uint64_t __read_mostly vmx_vmfunc;
> 
> I'm quite sure this should be __ro_after_init, but I guess we cannot
> be sure give the current code in vmx_init_vmcs_config().

I think we can be sure. But if we were to switch, I think all the
related variables should also be switched at the same time.

> Any CPU hot plugged that has a different set of VMX controls should
> not be onlined, the more that migrating an already running VMCS to
> such CPU will lead to failures if non-supported features are used.

That's the intention of that code, yes.

Jan

Re: [PATCH 1/5] VMX: drop vmx_virt_exception and make vmx_vmfunc static

[Roger Pau Monné]

On Tue, Nov 21, 2023 at 06:22:37PM +0100, Jan Beulich wrote:
> On 21.11.2023 16:48, Roger Pau Monné wrote:
> > On Thu, Nov 16, 2023 at 02:30:41PM +0100, Jan Beulich wrote:
> >> The variable was introduced by 69b830e5ffb4 ("VMX: VMFUNC and #VE
> >> definitions and detection") without any use and - violating Misra C:2012
> >> rule 8.4 - without a declaration. Since no use has appeared, drop it.
> >>
> >> For vmx_vmfunc the situation is similar, but not identical: It at least
> >> has one use. Convert it to be static (and make style adjustments while
> >> there).
> > 
> > I think you could also remove the sole user of vmx_vmfunc, as it's
> > just a cap_check() usage (unless there are more hidden usages).
> 
> Well, perhaps (and hence my post-commit-message remark in the original
> submission). Yet then I thought we permitted vmfunc use for altp2m, at
> which point the cap_check() is meaningful.

Right, I see now that we do only enable VMFUNC if EPTP switching is
supported, and hence we need to assert it's present on any other
CPUs, so yes, we must keep vmx_vmfunc.

> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> >> ---
> >> In how far the sole vmx_vmfunc use is actually meaningful (on its own)
> >> I'm not really sure.
> 
> (Here ^^^)
> 
> >> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> >> @@ -167,8 +167,7 @@ u32 vmx_secondary_exec_control __read_mo
> >>  u32 vmx_vmexit_control __read_mostly;
> >>  u32 vmx_vmentry_control __read_mostly;
> >>  u64 vmx_ept_vpid_cap __read_mostly;
> >> -u64 vmx_vmfunc __read_mostly;
> >> -bool_t vmx_virt_exception __read_mostly;
> >> +static uint64_t __read_mostly vmx_vmfunc;
> > 
> > I'm quite sure this should be __ro_after_init, but I guess we cannot
> > be sure give the current code in vmx_init_vmcs_config().
> 
> I think we can be sure. But if we were to switch, I think all the
> related variables should also be switched at the same time.

OK, IIRC in the past we have switched those kind of attributes as we
changed the code for other reasons I think, but I won't insist.
Coherency of attributes might be more valuable here.

> 
> > Any CPU hot plugged that has a different set of VMX controls should
> > not be onlined, the more that migrating an already running VMCS to
> > such CPU will lead to failures if non-supported features are used.
> 
> That's the intention of that code, yes.

Hm, yes, since we do require PIN_BASED_EXT_INTR_MASK and
PIN_BASED_NMI_EXITING on pin_based_exec_control the setting of the
vmx_ fields is only done on the BSP, or else VMX is not enabled.  It
would be IMO clearer to do the initial setting of the vmx_ fields
based on the function `bsp` parameter.  Anyway, not for this patch.

Thanks, Roger.

[PATCH 2/5] x86/HVM: hide SVM/VMX when their enabling is prohibited by firmware

[Jan Beulich]

... or we fail to enable the functionality on the BSP for other reasons.
The only place where hardware announcing the feature is recorded is the
raw CPU policy/featureset.

Inspired by https://lore.kernel.org/all/20230921114940.957141-1-pbonzini@redhat.com/.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2543,6 +2543,7 @@ const struct hvm_function_table * __init
 
     if ( _svm_cpu_up(true) )
     {
+        setup_clear_cpu_cap(X86_FEATURE_SVM);
         printk("SVM: failed to initialise.\n");
         return NULL;
     }
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -2163,6 +2163,23 @@ int __init vmx_vmcs_init(void)
 
     if ( !ret )
         register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1);
+    else
+    {
+        setup_clear_cpu_cap(X86_FEATURE_VMX);
+
+        /*
+         * _vmx_vcpu_up() may have made it past feature identification.
+         * Make sure all dependent features are off as well.
+         */
+        vmx_basic_msr              = 0;
+        vmx_pin_based_exec_control = 0;
+        vmx_cpu_based_exec_control = 0;
+        vmx_secondary_exec_control = 0;
+        vmx_vmexit_control         = 0;
+        vmx_vmentry_control        = 0;
+        vmx_ept_vpid_cap           = 0;
+        vmx_vmfunc                 = 0;
+    }
 
     return ret;
 }

Re: [PATCH 2/5] x86/HVM: hide SVM/VMX when their enabling is prohibited by firmware

[Roger Pau Monné]

On Thu, Nov 16, 2023 at 02:31:05PM +0100, Jan Beulich wrote:
> ... or we fail to enable the functionality on the BSP for other reasons.
> The only place where hardware announcing the feature is recorded is the
> raw CPU policy/featureset.
> 
> Inspired by https://lore.kernel.org/all/20230921114940.957141-1-pbonzini@redhat.com/.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

> 
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2543,6 +2543,7 @@ const struct hvm_function_table * __init
>  
>      if ( _svm_cpu_up(true) )
>      {
> +        setup_clear_cpu_cap(X86_FEATURE_SVM);
>          printk("SVM: failed to initialise.\n");
>          return NULL;
>      }
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -2163,6 +2163,23 @@ int __init vmx_vmcs_init(void)
>  
>      if ( !ret )
>          register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1);
> +    else
> +    {
> +        setup_clear_cpu_cap(X86_FEATURE_VMX);
> +
> +        /*
> +         * _vmx_vcpu_up() may have made it past feature identification.
> +         * Make sure all dependent features are off as well.
> +         */
> +        vmx_basic_msr              = 0;
> +        vmx_pin_based_exec_control = 0;
> +        vmx_cpu_based_exec_control = 0;
> +        vmx_secondary_exec_control = 0;
> +        vmx_vmexit_control         = 0;
> +        vmx_vmentry_control        = 0;
> +        vmx_ept_vpid_cap           = 0;
> +        vmx_vmfunc                 = 0;

Are there really any usages of those variables if VMX is disabled in
CPUID?

Thanks, Roger.

Re: [PATCH 2/5] x86/HVM: hide SVM/VMX when their enabling is prohibited by firmware

[Jan Beulich]

On 21.11.2023 17:24, Roger Pau Monné wrote:
> On Thu, Nov 16, 2023 at 02:31:05PM +0100, Jan Beulich wrote:
>> ... or we fail to enable the functionality on the BSP for other reasons.
>> The only place where hardware announcing the feature is recorded is the
>> raw CPU policy/featureset.
>>
>> Inspired by https://lore.kernel.org/all/20230921114940.957141-1-pbonzini@redhat.com/.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -2163,6 +2163,23 @@ int __init vmx_vmcs_init(void)
>>  
>>      if ( !ret )
>>          register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1);
>> +    else
>> +    {
>> +        setup_clear_cpu_cap(X86_FEATURE_VMX);
>> +
>> +        /*
>> +         * _vmx_vcpu_up() may have made it past feature identification.
>> +         * Make sure all dependent features are off as well.
>> +         */
>> +        vmx_basic_msr              = 0;
>> +        vmx_pin_based_exec_control = 0;
>> +        vmx_cpu_based_exec_control = 0;
>> +        vmx_secondary_exec_control = 0;
>> +        vmx_vmexit_control         = 0;
>> +        vmx_vmentry_control        = 0;
>> +        vmx_ept_vpid_cap           = 0;
>> +        vmx_vmfunc                 = 0;
> 
> Are there really any usages of those variables if VMX is disabled in
> CPUID?

I wanted to be on the safe side, as to me the question was "Are there really
_no_ uses anywhere of those variables if VMX is disabled in CPUID?" And I
couldn't easily convince myself of this being the case, seeing how all of
vmcs.h's cpu_has_* are defined (and I'm pretty sure we have uses outside of
arch/x86/hvm/vmx/).

Jan

Re: [PATCH 2/5] x86/HVM: hide SVM/VMX when their enabling is prohibited by firmware

[Andrew Cooper]

On 21/11/2023 5:27 pm, Jan Beulich wrote:
> On 21.11.2023 17:24, Roger Pau Monné wrote:
>> On Thu, Nov 16, 2023 at 02:31:05PM +0100, Jan Beulich wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>> @@ -2163,6 +2163,23 @@ int __init vmx_vmcs_init(void)
>>>  
>>>      if ( !ret )
>>>          register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1);
>>> +    else
>>> +    {
>>> +        setup_clear_cpu_cap(X86_FEATURE_VMX);
>>> +
>>> +        /*
>>> +         * _vmx_vcpu_up() may have made it past feature identification.
>>> +         * Make sure all dependent features are off as well.
>>> +         */
>>> +        vmx_basic_msr              = 0;
>>> +        vmx_pin_based_exec_control = 0;
>>> +        vmx_cpu_based_exec_control = 0;
>>> +        vmx_secondary_exec_control = 0;
>>> +        vmx_vmexit_control         = 0;
>>> +        vmx_vmentry_control        = 0;
>>> +        vmx_ept_vpid_cap           = 0;
>>> +        vmx_vmfunc                 = 0;
>> Are there really any usages of those variables if VMX is disabled in
>> CPUID?
> I wanted to be on the safe side, as to me the question was "Are there really
> _no_ uses anywhere of those variables if VMX is disabled in CPUID?" And I
> couldn't easily convince myself of this being the case, seeing how all of
> vmcs.h's cpu_has_* are defined (and I'm pretty sure we have uses outside of
> arch/x86/hvm/vmx/).

Before you commit, are you sure that VT-d will continue to be happy
using IOMMU superpages when the EPT features are cleared like this?

That's the only linkage I'm aware of that might cause issues.

~Andrew

Re: [PATCH 2/5] x86/HVM: hide SVM/VMX when their enabling is prohibited by firmware

[Jan Beulich]

On 21.11.2023 18:31, Andrew Cooper wrote:
> On 21/11/2023 5:27 pm, Jan Beulich wrote:
>> On 21.11.2023 17:24, Roger Pau Monné wrote:
>>> On Thu, Nov 16, 2023 at 02:31:05PM +0100, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>>> @@ -2163,6 +2163,23 @@ int __init vmx_vmcs_init(void)
>>>>  
>>>>      if ( !ret )
>>>>          register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1);
>>>> +    else
>>>> +    {
>>>> +        setup_clear_cpu_cap(X86_FEATURE_VMX);
>>>> +
>>>> +        /*
>>>> +         * _vmx_vcpu_up() may have made it past feature identification.
>>>> +         * Make sure all dependent features are off as well.
>>>> +         */
>>>> +        vmx_basic_msr              = 0;
>>>> +        vmx_pin_based_exec_control = 0;
>>>> +        vmx_cpu_based_exec_control = 0;
>>>> +        vmx_secondary_exec_control = 0;
>>>> +        vmx_vmexit_control         = 0;
>>>> +        vmx_vmentry_control        = 0;
>>>> +        vmx_ept_vpid_cap           = 0;
>>>> +        vmx_vmfunc                 = 0;
>>> Are there really any usages of those variables if VMX is disabled in
>>> CPUID?
>> I wanted to be on the safe side, as to me the question was "Are there really
>> _no_ uses anywhere of those variables if VMX is disabled in CPUID?" And I
>> couldn't easily convince myself of this being the case, seeing how all of
>> vmcs.h's cpu_has_* are defined (and I'm pretty sure we have uses outside of
>> arch/x86/hvm/vmx/).
> 
> Before you commit, are you sure that VT-d will continue to be happy
> using IOMMU superpages when the EPT features are cleared like this?

There aren't going to be HVM guests in the case the clearing above happens.
And the only thing that happens when vtd_ept_page_compatible() returns
false is that page table sharing is suppressed, which is relevant only for
HVM guests.

Jan

Re: [PATCH 2/5] x86/HVM: hide SVM/VMX when their enabling is prohibited by firmware

[Roger Pau Monné]

On Tue, Nov 21, 2023 at 06:27:02PM +0100, Jan Beulich wrote:
> On 21.11.2023 17:24, Roger Pau Monné wrote:
> > On Thu, Nov 16, 2023 at 02:31:05PM +0100, Jan Beulich wrote:
> >> ... or we fail to enable the functionality on the BSP for other reasons.
> >> The only place where hardware announcing the feature is recorded is the
> >> raw CPU policy/featureset.
> >>
> >> Inspired by https://lore.kernel.org/all/20230921114940.957141-1-pbonzini@redhat.com/.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> >> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> >> @@ -2163,6 +2163,23 @@ int __init vmx_vmcs_init(void)
> >>  
> >>      if ( !ret )
> >>          register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1);
> >> +    else
> >> +    {
> >> +        setup_clear_cpu_cap(X86_FEATURE_VMX);
> >> +
> >> +        /*
> >> +         * _vmx_vcpu_up() may have made it past feature identification.
> >> +         * Make sure all dependent features are off as well.
> >> +         */
> >> +        vmx_basic_msr              = 0;
> >> +        vmx_pin_based_exec_control = 0;
> >> +        vmx_cpu_based_exec_control = 0;
> >> +        vmx_secondary_exec_control = 0;
> >> +        vmx_vmexit_control         = 0;
> >> +        vmx_vmentry_control        = 0;
> >> +        vmx_ept_vpid_cap           = 0;
> >> +        vmx_vmfunc                 = 0;
> > 
> > Are there really any usages of those variables if VMX is disabled in
> > CPUID?
> 
> I wanted to be on the safe side, as to me the question was "Are there really
> _no_ uses anywhere of those variables if VMX is disabled in CPUID?" And I
> couldn't easily convince myself of this being the case, seeing how all of
> vmcs.h's cpu_has_* are defined (and I'm pretty sure we have uses outside of
> arch/x86/hvm/vmx/).

Wouldn't that have exploded already if initialization of _vmx_cpu_up()
failed? (regardless of whether the CPUID flag is cleared or not)

My main concern is that it's very easy for the variables here getting
out of sync with the ones used by vmx_init_vmcs_config().

It might have been nice to place all those fields in an array that we
could just zero here without having to account for each individual
variable.

Thanks, Roger.

Re: [PATCH 2/5] x86/HVM: hide SVM/VMX when their enabling is prohibited by firmware

[Jan Beulich]

On 22.11.2023 09:22, Roger Pau Monné wrote:
> On Tue, Nov 21, 2023 at 06:27:02PM +0100, Jan Beulich wrote:
>> On 21.11.2023 17:24, Roger Pau Monné wrote:
>>> On Thu, Nov 16, 2023 at 02:31:05PM +0100, Jan Beulich wrote:
>>>> ... or we fail to enable the functionality on the BSP for other reasons.
>>>> The only place where hardware announcing the feature is recorded is the
>>>> raw CPU policy/featureset.
>>>>
>>>> Inspired by https://lore.kernel.org/all/20230921114940.957141-1-pbonzini@redhat.com/.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Thanks.
>>
>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>>> @@ -2163,6 +2163,23 @@ int __init vmx_vmcs_init(void)
>>>>  
>>>>      if ( !ret )
>>>>          register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1);
>>>> +    else
>>>> +    {
>>>> +        setup_clear_cpu_cap(X86_FEATURE_VMX);
>>>> +
>>>> +        /*
>>>> +         * _vmx_vcpu_up() may have made it past feature identification.
>>>> +         * Make sure all dependent features are off as well.
>>>> +         */
>>>> +        vmx_basic_msr              = 0;
>>>> +        vmx_pin_based_exec_control = 0;
>>>> +        vmx_cpu_based_exec_control = 0;
>>>> +        vmx_secondary_exec_control = 0;
>>>> +        vmx_vmexit_control         = 0;
>>>> +        vmx_vmentry_control        = 0;
>>>> +        vmx_ept_vpid_cap           = 0;
>>>> +        vmx_vmfunc                 = 0;
>>>
>>> Are there really any usages of those variables if VMX is disabled in
>>> CPUID?
>>
>> I wanted to be on the safe side, as to me the question was "Are there really
>> _no_ uses anywhere of those variables if VMX is disabled in CPUID?" And I
>> couldn't easily convince myself of this being the case, seeing how all of
>> vmcs.h's cpu_has_* are defined (and I'm pretty sure we have uses outside of
>> arch/x86/hvm/vmx/).
> 
> Wouldn't that have exploded already if initialization of _vmx_cpu_up()
> failed? (regardless of whether the CPUID flag is cleared or not)

Quite likely, or in other words the clearing added here likely was missing
before already.

> My main concern is that it's very easy for the variables here getting
> out of sync with the ones used by vmx_init_vmcs_config().
> 
> It might have been nice to place all those fields in an array that we
> could just zero here without having to account for each individual
> variable.

Yeah, that might (have been) better. Indeed I already need to remember to
correctly deal with vmx_tertiary_exec_control either here or in the patch
introducing it. I guess I should make a follow-on patch converting to a
struct and at the same time moving to __ro_after_init.

Jan

[PATCH 3/5] VMX: don't run with CR4.VMXE set when VMX could not be enabled

[Jan Beulich]

While generally benign, doing so is still at best misleading.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Using set_in_cr4() seems favorable over updating mmu_cr4_features
despite the resulting redundant CR4 update. But I certainly could be
talked into going the alternative route.

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2959,7 +2959,7 @@ static bool __init has_if_pschange_mc(vo
 
 const struct hvm_function_table * __init start_vmx(void)
 {
-    set_in_cr4(X86_CR4_VMXE);
+    write_cr4(read_cr4() | X86_CR4_VMXE);
 
     if ( vmx_vmcs_init() )
     {
@@ -2967,6 +2967,9 @@ const struct hvm_function_table * __init
         return NULL;
     }
 
+    /* Arrange for APs to have CR4.VMXE set early on. */
+    set_in_cr4(X86_CR4_VMXE);
+
     vmx_function_table.singlestep_supported = cpu_has_monitor_trap_flag;
 
     if ( cpu_has_vmx_dt_exiting )

Re: [PATCH 3/5] VMX: don't run with CR4.VMXE set when VMX could not be enabled

[Roger Pau Monné]

On Thu, Nov 16, 2023 at 02:32:07PM +0100, Jan Beulich wrote:
> While generally benign, doing so is still at best misleading.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Using set_in_cr4() seems favorable over updating mmu_cr4_features
> despite the resulting redundant CR4 update. But I certainly could be
> talked into going the alternative route.

Hm, yes I wondered the same, overall I guess it's safer to just use
set_in_cr4() and do the redundant CR4 write. It's just for the BSP
anyway.

> 
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2959,7 +2959,7 @@ static bool __init has_if_pschange_mc(vo
>  
>  const struct hvm_function_table * __init start_vmx(void)
>  {
> -    set_in_cr4(X86_CR4_VMXE);
> +    write_cr4(read_cr4() | X86_CR4_VMXE);
>  
>      if ( vmx_vmcs_init() )
>      {
> @@ -2967,6 +2967,9 @@ const struct hvm_function_table * __init
>          return NULL;

Do we want to also clear VMXE from CR4 here?

Thanks, Roger.

Re: [PATCH 3/5] VMX: don't run with CR4.VMXE set when VMX could not be enabled

[Jan Beulich]

On 21.11.2023 18:30, Roger Pau Monné wrote:
> On Thu, Nov 16, 2023 at 02:32:07PM +0100, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -2959,7 +2959,7 @@ static bool __init has_if_pschange_mc(vo
>>  
>>  const struct hvm_function_table * __init start_vmx(void)
>>  {
>> -    set_in_cr4(X86_CR4_VMXE);
>> +    write_cr4(read_cr4() | X86_CR4_VMXE);
>>  
>>      if ( vmx_vmcs_init() )
>>      {
>> @@ -2967,6 +2967,9 @@ const struct hvm_function_table * __init
>>          return NULL;
> 
> Do we want to also clear VMXE from CR4 here?

Yes, definitely. That was the point of the patch (as far as the BSP is
concerned); I clearly meant to have that there, but then didn't put it
there.

Jan

[PATCH 4/5] x86/HVM: drop tsc_scaling.setup() hook

[Jan Beulich]

This was used by VMX only, and the intended VMCS write can as well
happen from vmx_set_tsc_offset(), invoked (directly or indirectly)
almost immediately after the present call sites of the hook.
vmx_set_tsc_offset() isn't invoked frequently elsewhere, so the extra
VMCS write shouldn't raise performance concerns.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1086,9 +1086,6 @@ static int cf_check hvm_load_cpu_ctxt(st
     v->arch.hvm.guest_cr[2] = ctxt.cr2;
     hvm_update_guest_cr(v, 2);
 
-    if ( hvm_funcs.tsc_scaling.setup )
-        alternative_vcall(hvm_funcs.tsc_scaling.setup, v);
-
     v->arch.msrs->tsc_aux = ctxt.msr_tsc_aux;
 
     hvm_set_guest_tsc_fixed(v, ctxt.tsc, d->arch.hvm.sync_tsc);
@@ -4033,9 +4030,6 @@ void hvm_vcpu_reset_state(struct vcpu *v
     hvm_set_segment_register(v, x86_seg_gdtr, &reg);
     hvm_set_segment_register(v, x86_seg_idtr, &reg);
 
-    if ( hvm_funcs.tsc_scaling.setup )
-        alternative_vcall(hvm_funcs.tsc_scaling.setup, v);
-
     /* Sync AP's TSC with BSP's. */
     v->arch.hvm.cache_tsc_offset =
         v->domain->vcpu[0]->arch.hvm.cache_tsc_offset;
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1454,20 +1454,13 @@ static void cf_check vmx_handle_cd(struc
     }
 }
 
-static void cf_check vmx_setup_tsc_scaling(struct vcpu *v)
-{
-    if ( v->domain->arch.vtsc )
-        return;
-
-    vmx_vmcs_enter(v);
-    __vmwrite(TSC_MULTIPLIER, hvm_tsc_scaling_ratio(v->domain));
-    vmx_vmcs_exit(v);
-}
-
 static void cf_check vmx_set_tsc_offset(struct vcpu *v, u64 offset, u64 at_tsc)
 {
     vmx_vmcs_enter(v);
 
+    if ( !v->domain->arch.vtsc && cpu_has_vmx_tsc_scaling )
+        __vmwrite(TSC_MULTIPLIER, hvm_tsc_scaling_ratio(v->domain));
+
     if ( nestedhvm_vcpu_in_guestmode(v) )
         offset += nvmx_get_tsc_offset(v);
 
@@ -3030,10 +3023,7 @@ const struct hvm_function_table * __init
     }
 
     if ( cpu_has_vmx_tsc_scaling )
-    {
         vmx_function_table.tsc_scaling.ratio_frac_bits = 48;
-        vmx_function_table.tsc_scaling.setup = vmx_setup_tsc_scaling;
-    }
 
     model_specific_lbr = get_model_specific_lbr();
     lbr_tsx_fixup_check();
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -240,9 +240,6 @@ struct hvm_function_table {
         uint8_t  ratio_frac_bits;
         /* maximum-allowed TSC scaling ratio */
         uint64_t max_ratio;
-
-        /* Architecture function to setup TSC scaling ratio */
-        void (*setup)(struct vcpu *v);
     } tsc_scaling;
 };
 

Re: [PATCH 4/5] x86/HVM: drop tsc_scaling.setup() hook

[Roger Pau Monné]

On Thu, Nov 16, 2023 at 02:32:47PM +0100, Jan Beulich wrote:
> This was used by VMX only, and the intended VMCS write can as well
> happen from vmx_set_tsc_offset(), invoked (directly or indirectly)
> almost immediately after the present call sites of the hook.
> vmx_set_tsc_offset() isn't invoked frequently elsewhere, so the extra
> VMCS write shouldn't raise performance concerns.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

I would be lying if I told you I understand guest TSC management in
Xen, but patch does keep the previous call sites with adding others
that are indeed not hot paths.

I do wonder whether it would be possible to set this before the domain
is started, as AFAICT the TSC scaling ratio is only set before the
domain is started (either by domain create or other toolstack
hypercalls).

Could we maybe further limit the write to the case where the vCPU is
not running?

Thanks, Roger.

Re: [PATCH 4/5] x86/HVM: drop tsc_scaling.setup() hook

[Jan Beulich]

On 22.11.2023 10:06, Roger Pau Monné wrote:
> On Thu, Nov 16, 2023 at 02:32:47PM +0100, Jan Beulich wrote:
>> This was used by VMX only, and the intended VMCS write can as well
>> happen from vmx_set_tsc_offset(), invoked (directly or indirectly)
>> almost immediately after the present call sites of the hook.
>> vmx_set_tsc_offset() isn't invoked frequently elsewhere, so the extra
>> VMCS write shouldn't raise performance concerns.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> I would be lying if I told you I understand guest TSC management in
> Xen, but patch does keep the previous call sites with adding others
> that are indeed not hot paths.
> 
> I do wonder whether it would be possible to set this before the domain
> is started, as AFAICT the TSC scaling ratio is only set before the
> domain is started (either by domain create or other toolstack
> hypercalls).
> 
> Could we maybe further limit the write to the case where the vCPU is
> not running?

Possibly, but not straightforwardly: E.g. I think we'd need to actually
enforce tsc_set_info() to only ever act on non-running (perhaps even
not-yet-started) domains. And I mean not by looking where it's being
called from at present (which may already provide such kind-of-
guarantees), but in the function itself.

Jan

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

[Jan Beulich]

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

--- 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 ( !IS_ENABLED(CONFIG_XEN_IBT) )
+        return;
+
+    /* Nothing at present. */
+}
+
 void 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
@@ -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;
+}
+
 /*
  * 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);

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

[Roger Pau Monné]

On Thu, Nov 16, 2023 at 02:33:14PM +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.
> 
> --- 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 ( !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.

> +        return;
> +
> +    /* Nothing at present. */
> +}
> +
>  void 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
> @@ -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;

Hm, I find this quite fragile, as it's easy to add a new handler
without realizing that addition here might also be required.

I don't really have good ideas about how to handle this, unless we
populate unused handlers with some poison and loop over the structure
as an array of pointers and choke on finding one of them pointing to
NULL.

Thanks, Roger.

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

[Jan Beulich]

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.

> Hm, I find this quite fragile, as it's easy to add a new handler
> without realizing that addition here might also be required.

Indeed, but that's not the end of the world (as much as so far it
wasn't deemed necessary at all to try and also purge unused hooks'
ENDBR).

> I don't really have good ideas about how to handle this, unless we
> populate unused handlers with some poison and loop over the structure
> as an array of pointers and choke on finding one of them pointing to
> NULL.

The looping over the resulting section is already somewhat fragile,
when considering other non-pointer data in there. A specific poison
value would further increase the risk of mistaking a value for what
it doesn't represent, even if just a tiny bit.

Populating unused handlers with some poison value would also have the
same problem of being easy to forget when adding a new hook.

Jan

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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

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

[Roger Pau Monné]

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.