If this VM assist is available (to PV guests only), use it to
- avoid issuing an IBPB ourselves upon entry from user mode (which the
  hypervisor would then have to emulate, as the MSR write traps),
- suppress the IBPB in the hypervisor if we don't mean to have one
  issued.

As there's no good place to have xen_vm_assist_ibpb() as an inline
function, make it an init-only out-of-line one.

While adjusting the Xen public header, drop the unused and no longer
applicable MAX_VMASST_TYPE (instead of modifying its value).

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

--- a/arch/x86/include/asm/xen/hypervisor.h
+++ b/arch/x86/include/asm/xen/hypervisor.h
@@ -43,6 +43,8 @@ static inline uint32_t xen_cpuid_base(vo
 	return hypervisor_cpuid_base("XenVMMXenVMM", 2);
 }
 
+int xen_vm_assist_ibpb(bool enable);
+
 struct pci_dev;
 
 #ifdef CONFIG_XEN_PV_DOM0
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -18,6 +18,8 @@
 #include <linux/pgtable.h>
 #include <linux/bpf.h>
 
+#include <xen/xen.h>
+
 #include <asm/spec-ctrl.h>
 #include <asm/cmdline.h>
 #include <asm/bugs.h>
@@ -32,6 +34,7 @@
 #include <asm/intel-family.h>
 #include <asm/e820/api.h>
 #include <asm/hypervisor.h>
+#include <asm/xen/hypervisor.h>
 #include <asm/tlbflush.h>
 
 #include "cpu.h"
@@ -934,7 +937,8 @@ do_cmd_auto:
 		break;
 
 	case RETBLEED_MITIGATION_IBPB:
-		setup_force_cpu_cap(X86_FEATURE_ENTRY_IBPB);
+		if (!xen_pv_domain() || xen_vm_assist_ibpb(true))
+			setup_force_cpu_cap(X86_FEATURE_ENTRY_IBPB);
 		mitigate_smt = true;
 		break;
 
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -294,6 +294,17 @@ int xen_panic_handler_init(void)
 	return 0;
 }
 
+int __init xen_vm_assist_ibpb(bool enable)
+{
+	/*
+	 * Note that the VM-assist is a disable, so a request to enable IBPB
+	 * on our behalf needs to turn the functionality off (and vice versa).
+	 */
+	return HYPERVISOR_vm_assist(enable ? VMASST_CMD_disable
+					   : VMASST_CMD_enable,
+				    VMASST_TYPE_mode_switch_no_ibpb);
+}
+
 void xen_pin_vcpu(int cpu)
 {
 	static bool disable_pinning;
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -940,6 +940,13 @@ static void __init xen_pvmmu_arch_setup(
 	HYPERVISOR_vm_assist(VMASST_CMD_enable,
 			     VMASST_TYPE_pae_extended_cr3);
 
+	/*
+	 * By default suppress the hypervisor issuing IBPB on our behalf.  In
+	 * the RETBLEED_MITIGATION_IBPB case the VM assist will be disengaged
+	 * again in retbleed_select_mitigation().
+	 */
+	xen_vm_assist_ibpb(false);
+
 	if (register_callback(CALLBACKTYPE_event,
 			      xen_asm_exc_xen_hypervisor_callback) ||
 	    register_callback(CALLBACKTYPE_failsafe, xen_failsafe_callback))
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -413,7 +413,15 @@ DEFINE_GUEST_HANDLE_STRUCT(mmuext_op);
  */
 #define VMASST_TYPE_runstate_update_flag 5
 
-#define MAX_VMASST_TYPE 5
+/*
+ * x86-64 guests: Suppress IBPB on guest-user to guest-kernel mode switch.
+ *
+ * By default (on affected and capable hardware) as a safety measure Xen,
+ * to cover for the fact that guest-kernel and guest-user modes are both
+ * running in ring 3 (and hence share prediction context), would issue a
+ * barrier for user->kernel mode switches of PV guests.
+ */
+#define VMASST_TYPE_mode_switch_no_ibpb  33
 
 #ifndef __ASSEMBLY__

On 2/14/23 11:13 AM, Jan Beulich wrote:

> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -18,6 +18,8 @@
>   #include <linux/pgtable.h>
>   #include <linux/bpf.h>
>   
> +#include <xen/xen.h>
> +
>   #include <asm/spec-ctrl.h>
>   #include <asm/cmdline.h>
>   #include <asm/bugs.h>
> @@ -32,6 +34,7 @@
>   #include <asm/intel-family.h>
>   #include <asm/e820/api.h>
>   #include <asm/hypervisor.h>
> +#include <asm/xen/hypervisor.h>
>   #include <asm/tlbflush.h>
>   
>   #include "cpu.h"
> @@ -934,7 +937,8 @@ do_cmd_auto:
>   		break;
>   
>   	case RETBLEED_MITIGATION_IBPB:
> -		setup_force_cpu_cap(X86_FEATURE_ENTRY_IBPB);
> +		if (!xen_pv_domain() || xen_vm_assist_ibpb(true))


Is this going to compile without CONFIG_XEN?


I also think these two conditions should be wrapped into something to limit exposure of non-Xen code to Xen-specific primitives.


-boris


> +			setup_force_cpu_cap(X86_FEATURE_ENTRY_IBPB);
>   		mitigate_smt = true;
>   		break;

On 2/14/23 6:53 PM, Boris Ostrovsky wrote:
>
> On 2/14/23 11:13 AM, Jan Beulich wrote:
>
>> --- a/arch/x86/kernel/cpu/bugs.c
>> +++ b/arch/x86/kernel/cpu/bugs.c
>> @@ -18,6 +18,8 @@
>>   #include <linux/pgtable.h>
>>   #include <linux/bpf.h>
>>   +#include <xen/xen.h>
>> +
>>   #include <asm/spec-ctrl.h>
>>   #include <asm/cmdline.h>
>>   #include <asm/bugs.h>
>> @@ -32,6 +34,7 @@
>>   #include <asm/intel-family.h>
>>   #include <asm/e820/api.h>
>>   #include <asm/hypervisor.h>
>> +#include <asm/xen/hypervisor.h>
>>   #include <asm/tlbflush.h>
>>     #include "cpu.h"
>> @@ -934,7 +937,8 @@ do_cmd_auto:
>>           break;
>>         case RETBLEED_MITIGATION_IBPB:
>> -        setup_force_cpu_cap(X86_FEATURE_ENTRY_IBPB);
>> +        if (!xen_pv_domain() || xen_vm_assist_ibpb(true))
>
>
> Is this going to compile without CONFIG_XEN?
>
>
> I also think these two conditions should be wrapped into something to limit exposure of non-Xen code to Xen-specific primitives.
>

Oh, and this needs x86 maintainers.


-boris

On 15.02.2023 01:07, Boris Ostrovsky wrote:
> 
> On 2/14/23 6:53 PM, Boris Ostrovsky wrote:
>>
>> On 2/14/23 11:13 AM, Jan Beulich wrote:
>>
>>> --- a/arch/x86/kernel/cpu/bugs.c
>>> +++ b/arch/x86/kernel/cpu/bugs.c
>>> @@ -18,6 +18,8 @@
>>>   #include <linux/pgtable.h>
>>>   #include <linux/bpf.h>
>>>   +#include <xen/xen.h>
>>> +
>>>   #include <asm/spec-ctrl.h>
>>>   #include <asm/cmdline.h>
>>>   #include <asm/bugs.h>
>>> @@ -32,6 +34,7 @@
>>>   #include <asm/intel-family.h>
>>>   #include <asm/e820/api.h>
>>>   #include <asm/hypervisor.h>
>>> +#include <asm/xen/hypervisor.h>
>>>   #include <asm/tlbflush.h>
>>>     #include "cpu.h"
>>> @@ -934,7 +937,8 @@ do_cmd_auto:
>>>           break;
>>>         case RETBLEED_MITIGATION_IBPB:
>>> -        setup_force_cpu_cap(X86_FEATURE_ENTRY_IBPB);
>>> +        if (!xen_pv_domain() || xen_vm_assist_ibpb(true))
>>
>>
>> Is this going to compile without CONFIG_XEN?

Yes. The declaration of xen_vm_assist_ibpb() is visible (satisfying
the compiler) and DCE will eliminate the call to the function due to
xen_pv_domain() being constant "false" in that case, avoiding any
linking issues. The interesting case here really is building with
XEN but without XEN_PV: That's why I needed to put the function in
enlighten.c. This wouldn't be needed if xen_pv_domain() was also
constant "false" in that case (just like xen_pvh_domain() is when
!XEN_PVH).

>> I also think these two conditions should be wrapped into something to limit exposure of non-Xen code to Xen-specific primitives.

I would have done so, if I had any halfway sensible idea on how to
go about doing so in this particular case. In the absence of that it
looked okay-ish to me to reference Xen functions directly here.

> Oh, and this needs x86 maintainers.

Eventually yes. But I would prefer to sort the above question first
(which I'm sure would have been raised by them, in perhaps more
harsh a way), hence the initially limited exposure.

Jan

On 15.02.23 09:31, Jan Beulich wrote:
> On 15.02.2023 01:07, Boris Ostrovsky wrote:
>>
>> On 2/14/23 6:53 PM, Boris Ostrovsky wrote:
>>>
>>> On 2/14/23 11:13 AM, Jan Beulich wrote:
>>>
>>>> --- a/arch/x86/kernel/cpu/bugs.c
>>>> +++ b/arch/x86/kernel/cpu/bugs.c
>>>> @@ -18,6 +18,8 @@
>>>>    #include <linux/pgtable.h>
>>>>    #include <linux/bpf.h>
>>>>    +#include <xen/xen.h>
>>>> +
>>>>    #include <asm/spec-ctrl.h>
>>>>    #include <asm/cmdline.h>
>>>>    #include <asm/bugs.h>
>>>> @@ -32,6 +34,7 @@
>>>>    #include <asm/intel-family.h>
>>>>    #include <asm/e820/api.h>
>>>>    #include <asm/hypervisor.h>
>>>> +#include <asm/xen/hypervisor.h>
>>>>    #include <asm/tlbflush.h>
>>>>      #include "cpu.h"
>>>> @@ -934,7 +937,8 @@ do_cmd_auto:
>>>>            break;
>>>>          case RETBLEED_MITIGATION_IBPB:
>>>> -        setup_force_cpu_cap(X86_FEATURE_ENTRY_IBPB);
>>>> +        if (!xen_pv_domain() || xen_vm_assist_ibpb(true))
>>>
>>>
>>> Is this going to compile without CONFIG_XEN?
> 
> Yes. The declaration of xen_vm_assist_ibpb() is visible (satisfying
> the compiler) and DCE will eliminate the call to the function due to
> xen_pv_domain() being constant "false" in that case, avoiding any
> linking issues. The interesting case here really is building with
> XEN but without XEN_PV: That's why I needed to put the function in
> enlighten.c. This wouldn't be needed if xen_pv_domain() was also
> constant "false" in that case (just like xen_pvh_domain() is when
> !XEN_PVH).
> 
>>> I also think these two conditions should be wrapped into something to limit exposure of non-Xen code to Xen-specific primitives.
> 
> I would have done so, if I had any halfway sensible idea on how to
> go about doing so in this particular case. In the absence of that it
> looked okay-ish to me to reference Xen functions directly here.
> 
>> Oh, and this needs x86 maintainers.
> 
> Eventually yes. But I would prefer to sort the above question first
> (which I'm sure would have been raised by them, in perhaps more
> harsh a way), hence the initially limited exposure.

I'd rather add _one_ hook for Xen-PV in check_bugs() just before the call
of arch_smt_update(). This can then correct any needed mitigation settings.

So something like (note that due to using cpu_feature_enabled(X86_FEATURE_XENPV)
DCE is happening in case CONFIG_XEN_PV isn't defined)":

--- a/arch/x86/include/asm/xen/hypervisor.h
+++ b/arch/x86/include/asm/xen/hypervisor.h
@@ -63,4 +63,7 @@ void __init xen_pvh_init(struct boot_params *boot_params);
  void __init mem_map_via_hcall(struct boot_params *boot_params_p);
  #endif

+int __init xen_vm_assist_ibpb(bool enable);
+void __init xen_pv_fix_mitigations(void);
+
  #endif /* _ASM_X86_XEN_HYPERVISOR_H */
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -18,6 +18,8 @@
  #include <linux/pgtable.h>
  #include <linux/bpf.h>

+#include <xen/xen.h>
+
  #include <asm/spec-ctrl.h>
  #include <asm/cmdline.h>
  #include <asm/bugs.h>
@@ -177,6 +179,9 @@ void __init check_bugs(void)
         srbds_select_mitigation();
         l1d_flush_select_mitigation();

+       if (cpu_feature_enabled(X86_FEATURE_XENPV))
+               xen_pv_fix_mitigations();
+
         arch_smt_update();

  #ifdef CONFIG_X86_32
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -1476,6 +1476,23 @@ static uint32_t __init xen_platform_pv(void)
         return 0;
  }

+int __init xen_vm_assist_ibpb(bool enable)
+{
+       /*
+        * Note that the VM-assist is a disable, so a request to enable IBPB
+        * on our behalf needs to turn the functionality off (and vice versa).
+        */
+       return HYPERVISOR_vm_assist(enable ? VMASST_CMD_disable
+                                          : VMASST_CMD_enable,
+                                   VMASST_TYPE_mode_switch_no_ibpb);
+}
+
+void __init xen_pv_fix_mitigations(void)
+{
+       if (!xen_vm_assist_ibpb(true))
+               setup_clear_cpu_cap(X86_FEATURE_ENTRY_IBPB);
+}
+
  const __initconst struct hypervisor_x86 x86_hyper_xen_pv = {
         .name                   = "Xen PV",
         .detect                 = xen_platform_pv,
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -413,7 +413,15 @@ DEFINE_GUEST_HANDLE_STRUCT(mmuext_op);
   */
  #define VMASST_TYPE_runstate_update_flag 5

-#define MAX_VMASST_TYPE 5
+/*
+ * x86-64 guests: Suppress IBPB on guest-user to guest-kernel mode switch.
+ *
+ * By default (on affected and capable hardware) as a safety measure Xen,
+ * to cover for the fact that guest-kernel and guest-user modes are both
+ * running in ring 3 (and hence share prediction context), would issue a
+ * barrier for user->kernel mode switches of PV guests.
+ */
+#define VMASST_TYPE_mode_switch_no_ibpb  33

  #ifndef __ASSEMBLY__


Juergen

On 17.03.2023 14:56, Juergen Gross wrote:
> On 15.02.23 09:31, Jan Beulich wrote:
>> Eventually yes. But I would prefer to sort the above question first
>> (which I'm sure would have been raised by them, in perhaps more
>> harsh a way), hence the initially limited exposure.
> 
> I'd rather add _one_ hook for Xen-PV in check_bugs() just before the call
> of arch_smt_update(). This can then correct any needed mitigation settings.

Doing this in single central place is what I was originally hoping I
could do. But that simply doesn't work (afaict): It is for a reason
that I apply the adjustment in the RETBLEED_MITIGATION_IBPB case, by
suppressing the setting of the feature bit. Not the least because ...

> So something like (note that due to using cpu_feature_enabled(X86_FEATURE_XENPV)
> DCE is happening in case CONFIG_XEN_PV isn't defined)":
> 
> --- a/arch/x86/include/asm/xen/hypervisor.h
> +++ b/arch/x86/include/asm/xen/hypervisor.h
> @@ -63,4 +63,7 @@ void __init xen_pvh_init(struct boot_params *boot_params);
>   void __init mem_map_via_hcall(struct boot_params *boot_params_p);
>   #endif
> 
> +int __init xen_vm_assist_ibpb(bool enable);
> +void __init xen_pv_fix_mitigations(void);
> +
>   #endif /* _ASM_X86_XEN_HYPERVISOR_H */
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -18,6 +18,8 @@
>   #include <linux/pgtable.h>
>   #include <linux/bpf.h>
> 
> +#include <xen/xen.h>
> +
>   #include <asm/spec-ctrl.h>
>   #include <asm/cmdline.h>
>   #include <asm/bugs.h>
> @@ -177,6 +179,9 @@ void __init check_bugs(void)
>          srbds_select_mitigation();
>          l1d_flush_select_mitigation();
> 
> +       if (cpu_feature_enabled(X86_FEATURE_XENPV))
> +               xen_pv_fix_mitigations();
> +
>          arch_smt_update();
> 
>   #ifdef CONFIG_X86_32
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -1476,6 +1476,23 @@ static uint32_t __init xen_platform_pv(void)
>          return 0;
>   }
> 
> +int __init xen_vm_assist_ibpb(bool enable)
> +{
> +       /*
> +        * Note that the VM-assist is a disable, so a request to enable IBPB
> +        * on our behalf needs to turn the functionality off (and vice versa).
> +        */
> +       return HYPERVISOR_vm_assist(enable ? VMASST_CMD_disable
> +                                          : VMASST_CMD_enable,
> +                                   VMASST_TYPE_mode_switch_no_ibpb);
> +}
> +
> +void __init xen_pv_fix_mitigations(void)
> +{
> +       if (!xen_vm_assist_ibpb(true))
> +               setup_clear_cpu_cap(X86_FEATURE_ENTRY_IBPB);

... using both setup_clear_cpu_cap() (here) and setup_force_cpu_cap()
(in retbleed_select_mitigation() won't work: The latter wins, due to
how apply_forced_caps() works.

But of course calling both functions for the same feature is bogus
anyway. In fact I think it is for a good reason that in Xen we log a
message in such an event.

A new helper could be introduced (and used in
retbleed_select_mitigation()) to check whether a feature was
previously cleared, but I did conclude that it's likely for a good
reason that such doesn't exist.

As to your use of cpu_feature_enabled(X86_FEATURE_XENPV) and DCE -
I can certainly switch to using that, which then ought allow to move
xen_vm_assist_ibpb() back to enlighten_pv.c (as you have it, and as
I first had it until noticing the build breakage with PVH=y and
PV=n).

Jan

On 20.03.23 11:19, Jan Beulich wrote:
> On 17.03.2023 14:56, Juergen Gross wrote:
>> On 15.02.23 09:31, Jan Beulich wrote:
>>> Eventually yes. But I would prefer to sort the above question first
>>> (which I'm sure would have been raised by them, in perhaps more
>>> harsh a way), hence the initially limited exposure.
>>
>> I'd rather add _one_ hook for Xen-PV in check_bugs() just before the call
>> of arch_smt_update(). This can then correct any needed mitigation settings.
> 
> Doing this in single central place is what I was originally hoping I
> could do. But that simply doesn't work (afaict): It is for a reason
> that I apply the adjustment in the RETBLEED_MITIGATION_IBPB case, by
> suppressing the setting of the feature bit. Not the least because ...
> 
>> So something like (note that due to using cpu_feature_enabled(X86_FEATURE_XENPV)
>> DCE is happening in case CONFIG_XEN_PV isn't defined)":
>>
>> --- a/arch/x86/include/asm/xen/hypervisor.h
>> +++ b/arch/x86/include/asm/xen/hypervisor.h
>> @@ -63,4 +63,7 @@ void __init xen_pvh_init(struct boot_params *boot_params);
>>    void __init mem_map_via_hcall(struct boot_params *boot_params_p);
>>    #endif
>>
>> +int __init xen_vm_assist_ibpb(bool enable);
>> +void __init xen_pv_fix_mitigations(void);
>> +
>>    #endif /* _ASM_X86_XEN_HYPERVISOR_H */
>> --- a/arch/x86/kernel/cpu/bugs.c
>> +++ b/arch/x86/kernel/cpu/bugs.c
>> @@ -18,6 +18,8 @@
>>    #include <linux/pgtable.h>
>>    #include <linux/bpf.h>
>>
>> +#include <xen/xen.h>
>> +
>>    #include <asm/spec-ctrl.h>
>>    #include <asm/cmdline.h>
>>    #include <asm/bugs.h>
>> @@ -177,6 +179,9 @@ void __init check_bugs(void)
>>           srbds_select_mitigation();
>>           l1d_flush_select_mitigation();
>>
>> +       if (cpu_feature_enabled(X86_FEATURE_XENPV))
>> +               xen_pv_fix_mitigations();
>> +
>>           arch_smt_update();
>>
>>    #ifdef CONFIG_X86_32
>> --- a/arch/x86/xen/enlighten_pv.c
>> +++ b/arch/x86/xen/enlighten_pv.c
>> @@ -1476,6 +1476,23 @@ static uint32_t __init xen_platform_pv(void)
>>           return 0;
>>    }
>>
>> +int __init xen_vm_assist_ibpb(bool enable)
>> +{
>> +       /*
>> +        * Note that the VM-assist is a disable, so a request to enable IBPB
>> +        * on our behalf needs to turn the functionality off (and vice versa).
>> +        */
>> +       return HYPERVISOR_vm_assist(enable ? VMASST_CMD_disable
>> +                                          : VMASST_CMD_enable,
>> +                                   VMASST_TYPE_mode_switch_no_ibpb);
>> +}
>> +
>> +void __init xen_pv_fix_mitigations(void)
>> +{
>> +       if (!xen_vm_assist_ibpb(true))
>> +               setup_clear_cpu_cap(X86_FEATURE_ENTRY_IBPB);
> 
> ... using both setup_clear_cpu_cap() (here) and setup_force_cpu_cap()
> (in retbleed_select_mitigation() won't work: The latter wins, due to
> how apply_forced_caps() works.

Oh, right.

Just a wild guess of mine: probably the x86 maintainers would still prefer
a single Xen hook plus something like a setup_unforce_cpu_cap() addition.

> But of course calling both functions for the same feature is bogus
> anyway. In fact I think it is for a good reason that in Xen we log a
> message in such an event.

Depends. For Xen we do so in the kernel for multiple features, see
xen_init_capabilities().


Juergen

On 20.03.2023 14:02, Juergen Gross wrote:
> On 20.03.23 11:19, Jan Beulich wrote:
>> On 17.03.2023 14:56, Juergen Gross wrote:
>>> +void __init xen_pv_fix_mitigations(void)
>>> +{
>>> +       if (!xen_vm_assist_ibpb(true))
>>> +               setup_clear_cpu_cap(X86_FEATURE_ENTRY_IBPB);
>>
>> ... using both setup_clear_cpu_cap() (here) and setup_force_cpu_cap()
>> (in retbleed_select_mitigation() won't work: The latter wins, due to
>> how apply_forced_caps() works.
> 
> Oh, right.
> 
> Just a wild guess of mine: probably the x86 maintainers would still prefer
> a single Xen hook plus something like a setup_unforce_cpu_cap() addition.

If so, I'm not willing to make such a patch. That's clearly more fragile
than the approach chosen. I guess once I've made the one adjustment you
have pointed out, I'll resubmit otherwise unchanged and include x86 folks.
We'll see what the responses are going to be, if any at all.

>> But of course calling both functions for the same feature is bogus
>> anyway. In fact I think it is for a good reason that in Xen we log a
>> message in such an event.
> 
> Depends. For Xen we do so in the kernel for multiple features, see
> xen_init_capabilities().

I don't see anything there which looks like it might be both "force"d
and "clear"ed in a single session.

Jan

On 20.03.23 14:17, Jan Beulich wrote:
> On 20.03.2023 14:02, Juergen Gross wrote:
>> On 20.03.23 11:19, Jan Beulich wrote:
>>> On 17.03.2023 14:56, Juergen Gross wrote:
>>>> +void __init xen_pv_fix_mitigations(void)
>>>> +{
>>>> +       if (!xen_vm_assist_ibpb(true))
>>>> +               setup_clear_cpu_cap(X86_FEATURE_ENTRY_IBPB);
>>>
>>> ... using both setup_clear_cpu_cap() (here) and setup_force_cpu_cap()
>>> (in retbleed_select_mitigation() won't work: The latter wins, due to
>>> how apply_forced_caps() works.
>>
>> Oh, right.
>>
>> Just a wild guess of mine: probably the x86 maintainers would still prefer
>> a single Xen hook plus something like a setup_unforce_cpu_cap() addition.
> 
> If so, I'm not willing to make such a patch. That's clearly more fragile
> than the approach chosen. I guess once I've made the one adjustment you
> have pointed out, I'll resubmit otherwise unchanged and include x86 folks.
> We'll see what the responses are going to be, if any at all.

Fine with me.

> 
>>> But of course calling both functions for the same feature is bogus
>>> anyway. In fact I think it is for a good reason that in Xen we log a
>>> message in such an event.
>>
>> Depends. For Xen we do so in the kernel for multiple features, see
>> xen_init_capabilities().
> 
> I don't see anything there which looks like it might be both "force"d
> and "clear"ed in a single session.

Oh, I misunderstood you then.


Juergen

On 17/03/2023 1:56 pm, Juergen Gross wrote:
> On 15.02.23 09:31, Jan Beulich wrote:
>> On 15.02.2023 01:07, Boris Ostrovsky wrote:
>>>
>>> On 2/14/23 6:53 PM, Boris Ostrovsky wrote:
>>>>
>>>> On 2/14/23 11:13 AM, Jan Beulich wrote:
>>>>
>>>>> --- a/arch/x86/kernel/cpu/bugs.c
>>>>> +++ b/arch/x86/kernel/cpu/bugs.c
>>>>> @@ -18,6 +18,8 @@
>>>>>    #include <linux/pgtable.h>
>>>>>    #include <linux/bpf.h>
>>>>>    +#include <xen/xen.h>
>>>>> +
>>>>>    #include <asm/spec-ctrl.h>
>>>>>    #include <asm/cmdline.h>
>>>>>    #include <asm/bugs.h>
>>>>> @@ -32,6 +34,7 @@
>>>>>    #include <asm/intel-family.h>
>>>>>    #include <asm/e820/api.h>
>>>>>    #include <asm/hypervisor.h>
>>>>> +#include <asm/xen/hypervisor.h>
>>>>>    #include <asm/tlbflush.h>
>>>>>      #include "cpu.h"
>>>>> @@ -934,7 +937,8 @@ do_cmd_auto:
>>>>>            break;
>>>>>          case RETBLEED_MITIGATION_IBPB:
>>>>> -        setup_force_cpu_cap(X86_FEATURE_ENTRY_IBPB);
>>>>> +        if (!xen_pv_domain() || xen_vm_assist_ibpb(true))
>>>>
>>>>
>>>> Is this going to compile without CONFIG_XEN?
>>
>> Yes. The declaration of xen_vm_assist_ibpb() is visible (satisfying
>> the compiler) and DCE will eliminate the call to the function due to
>> xen_pv_domain() being constant "false" in that case, avoiding any
>> linking issues. The interesting case here really is building with
>> XEN but without XEN_PV: That's why I needed to put the function in
>> enlighten.c. This wouldn't be needed if xen_pv_domain() was also
>> constant "false" in that case (just like xen_pvh_domain() is when
>> !XEN_PVH).
>>
>>>> I also think these two conditions should be wrapped into something
>>>> to limit exposure of non-Xen code to Xen-specific primitives.
>>
>> I would have done so, if I had any halfway sensible idea on how to
>> go about doing so in this particular case. In the absence of that it
>> looked okay-ish to me to reference Xen functions directly here.
>>
>>> Oh, and this needs x86 maintainers.
>>
>> Eventually yes. But I would prefer to sort the above question first
>> (which I'm sure would have been raised by them, in perhaps more
>> harsh a way), hence the initially limited exposure.
>
> I'd rather add _one_ hook for Xen-PV in check_bugs() just before the call
> of arch_smt_update(). This can then correct any needed mitigation
> settings.
>
> So something like (note that due to using
> cpu_feature_enabled(X86_FEATURE_XENPV)
> DCE is happening in case CONFIG_XEN_PV isn't defined)":
>
> --- a/arch/x86/include/asm/xen/hypervisor.h
> +++ b/arch/x86/include/asm/xen/hypervisor.h
> @@ -63,4 +63,7 @@ void __init xen_pvh_init(struct boot_params
> *boot_params);
>  void __init mem_map_via_hcall(struct boot_params *boot_params_p);
>  #endif
>
> +int __init xen_vm_assist_ibpb(bool enable);
> +void __init xen_pv_fix_mitigations(void);

I'd suggest 'adjust' in preference to 'fix', but this could equally be
xen_pv_mitigations().

XenPV has very legitimate reasons to adjust things owing to it running
in Ring3, but "fix" comes with other implications too.

> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -1476,6 +1476,23 @@ static uint32_t __init xen_platform_pv(void)
>         return 0;
>  }
>
> +int __init xen_vm_assist_ibpb(bool enable)
> +{
> +       /*
> +        * Note that the VM-assist is a disable, so a request to
> enable IBPB
> +        * on our behalf needs to turn the functionality off (and vice
> versa).
> +        */
> +       return HYPERVISOR_vm_assist(enable ? VMASST_CMD_disable
> +                                          : VMASST_CMD_enable,
> +                                   VMASST_TYPE_mode_switch_no_ibpb);
> +}
> +
> +void __init xen_pv_fix_mitigations(void)
> +{
> +       if (!xen_vm_assist_ibpb(true))
> +               setup_clear_cpu_cap(X86_FEATURE_ENTRY_IBPB);

If nothing else, this needs a comment explaining what's going on.

"Xen PV guest kernels run in ring3, so share the same prediction domain
as userspace.  Xen (since version $X) default to issuing an IBPB on
guest user -> guest kernel transitions on behalf of the guest kernel. 
If Linux isn't depending on mode based prediction separation, turn this
behaviour off".

But this does open the next question.  Yes, unilaterally turning turning
this off restores the prior behaviour, but is this really the best thing
to do ?

~Andrew

On 17.03.2023 15:21, Andrew Cooper wrote:
> On 17/03/2023 1:56 pm, Juergen Gross wrote:
>> --- a/arch/x86/xen/enlighten_pv.c
>> +++ b/arch/x86/xen/enlighten_pv.c
>> @@ -1476,6 +1476,23 @@ static uint32_t __init xen_platform_pv(void)
>>         return 0;
>>  }
>>
>> +int __init xen_vm_assist_ibpb(bool enable)
>> +{
>> +       /*
>> +        * Note that the VM-assist is a disable, so a request to
>> enable IBPB
>> +        * on our behalf needs to turn the functionality off (and vice
>> versa).
>> +        */
>> +       return HYPERVISOR_vm_assist(enable ? VMASST_CMD_disable
>> +                                          : VMASST_CMD_enable,
>> +                                   VMASST_TYPE_mode_switch_no_ibpb);
>> +}
>> +
>> +void __init xen_pv_fix_mitigations(void)
>> +{
>> +       if (!xen_vm_assist_ibpb(true))
>> +               setup_clear_cpu_cap(X86_FEATURE_ENTRY_IBPB);
> 
> If nothing else, this needs a comment explaining what's going on.
> 
> "Xen PV guest kernels run in ring3, so share the same prediction domain
> as userspace.  Xen (since version $X) default to issuing an IBPB on
> guest user -> guest kernel transitions on behalf of the guest kernel. 
> If Linux isn't depending on mode based prediction separation, turn this
> behaviour off".

I would have thought the comment in the public header - saying exactly
that - is sufficient.

> But this does open the next question.  Yes, unilaterally turning turning
> this off restores the prior behaviour, but is this really the best thing
> to do ?

Unless this is purely a question on Jürgen's suggested version (in which
case I'd let him answer) - what alternative do you suggest, within the
present policy used in the kernel?

Jan

On 17.03.23 15:21, Andrew Cooper wrote:
> On 17/03/2023 1:56 pm, Juergen Gross wrote:
>> On 15.02.23 09:31, Jan Beulich wrote:
>>> On 15.02.2023 01:07, Boris Ostrovsky wrote:
>>>>
>>>> On 2/14/23 6:53 PM, Boris Ostrovsky wrote:
>>>>>
>>>>> On 2/14/23 11:13 AM, Jan Beulich wrote:
>>>>>
>>>>>> --- a/arch/x86/kernel/cpu/bugs.c
>>>>>> +++ b/arch/x86/kernel/cpu/bugs.c
>>>>>> @@ -18,6 +18,8 @@
>>>>>>     #include <linux/pgtable.h>
>>>>>>     #include <linux/bpf.h>
>>>>>>     +#include <xen/xen.h>
>>>>>> +
>>>>>>     #include <asm/spec-ctrl.h>
>>>>>>     #include <asm/cmdline.h>
>>>>>>     #include <asm/bugs.h>
>>>>>> @@ -32,6 +34,7 @@
>>>>>>     #include <asm/intel-family.h>
>>>>>>     #include <asm/e820/api.h>
>>>>>>     #include <asm/hypervisor.h>
>>>>>> +#include <asm/xen/hypervisor.h>
>>>>>>     #include <asm/tlbflush.h>
>>>>>>       #include "cpu.h"
>>>>>> @@ -934,7 +937,8 @@ do_cmd_auto:
>>>>>>             break;
>>>>>>           case RETBLEED_MITIGATION_IBPB:
>>>>>> -        setup_force_cpu_cap(X86_FEATURE_ENTRY_IBPB);
>>>>>> +        if (!xen_pv_domain() || xen_vm_assist_ibpb(true))
>>>>>
>>>>>
>>>>> Is this going to compile without CONFIG_XEN?
>>>
>>> Yes. The declaration of xen_vm_assist_ibpb() is visible (satisfying
>>> the compiler) and DCE will eliminate the call to the function due to
>>> xen_pv_domain() being constant "false" in that case, avoiding any
>>> linking issues. The interesting case here really is building with
>>> XEN but without XEN_PV: That's why I needed to put the function in
>>> enlighten.c. This wouldn't be needed if xen_pv_domain() was also
>>> constant "false" in that case (just like xen_pvh_domain() is when
>>> !XEN_PVH).
>>>
>>>>> I also think these two conditions should be wrapped into something
>>>>> to limit exposure of non-Xen code to Xen-specific primitives.
>>>
>>> I would have done so, if I had any halfway sensible idea on how to
>>> go about doing so in this particular case. In the absence of that it
>>> looked okay-ish to me to reference Xen functions directly here.
>>>
>>>> Oh, and this needs x86 maintainers.
>>>
>>> Eventually yes. But I would prefer to sort the above question first
>>> (which I'm sure would have been raised by them, in perhaps more
>>> harsh a way), hence the initially limited exposure.
>>
>> I'd rather add _one_ hook for Xen-PV in check_bugs() just before the call
>> of arch_smt_update(). This can then correct any needed mitigation
>> settings.
>>
>> So something like (note that due to using
>> cpu_feature_enabled(X86_FEATURE_XENPV)
>> DCE is happening in case CONFIG_XEN_PV isn't defined)":
>>
>> --- a/arch/x86/include/asm/xen/hypervisor.h
>> +++ b/arch/x86/include/asm/xen/hypervisor.h
>> @@ -63,4 +63,7 @@ void __init xen_pvh_init(struct boot_params
>> *boot_params);
>>   void __init mem_map_via_hcall(struct boot_params *boot_params_p);
>>   #endif
>>
>> +int __init xen_vm_assist_ibpb(bool enable);
>> +void __init xen_pv_fix_mitigations(void);
> 
> I'd suggest 'adjust' in preference to 'fix', but this could equally be
> xen_pv_mitigations().
> 
> XenPV has very legitimate reasons to adjust things owing to it running
> in Ring3, but "fix" comes with other implications too.
> 
>> --- a/arch/x86/xen/enlighten_pv.c
>> +++ b/arch/x86/xen/enlighten_pv.c
>> @@ -1476,6 +1476,23 @@ static uint32_t __init xen_platform_pv(void)
>>          return 0;
>>   }
>>
>> +int __init xen_vm_assist_ibpb(bool enable)
>> +{
>> +       /*
>> +        * Note that the VM-assist is a disable, so a request to
>> enable IBPB
>> +        * on our behalf needs to turn the functionality off (and vice
>> versa).
>> +        */
>> +       return HYPERVISOR_vm_assist(enable ? VMASST_CMD_disable
>> +                                          : VMASST_CMD_enable,
>> +                                   VMASST_TYPE_mode_switch_no_ibpb);
>> +}
>> +
>> +void __init xen_pv_fix_mitigations(void)
>> +{
>> +       if (!xen_vm_assist_ibpb(true))
>> +               setup_clear_cpu_cap(X86_FEATURE_ENTRY_IBPB);
> 
> If nothing else, this needs a comment explaining what's going on.
> 
> "Xen PV guest kernels run in ring3, so share the same prediction domain
> as userspace.  Xen (since version $X) default to issuing an IBPB on
> guest user -> guest kernel transitions on behalf of the guest kernel.
> If Linux isn't depending on mode based prediction separation, turn this
> behaviour off".
> 
> But this does open the next question.  Yes, unilaterally turning turning
> this off restores the prior behaviour, but is this really the best thing
> to do ?

I believe this question is primarily for Jan, as he is the initial author of
the patch.

I was just suggesting a variant which is IMHO more probable to be accepted by
the x86 maintainers. :-)


Juergen

On 2/15/23 3:31 AM, Jan Beulich wrote:
> On 15.02.2023 01:07, Boris Ostrovsky wrote:
>> On 2/14/23 6:53 PM, Boris Ostrovsky wrote:
>>> On 2/14/23 11:13 AM, Jan Beulich wrote:
>>>
>>>> --- a/arch/x86/kernel/cpu/bugs.c
>>>> +++ b/arch/x86/kernel/cpu/bugs.c
>>>> @@ -18,6 +18,8 @@
>>>>    #include <linux/pgtable.h>
>>>>    #include <linux/bpf.h>
>>>>    +#include <xen/xen.h>
>>>> +
>>>>    #include <asm/spec-ctrl.h>
>>>>    #include <asm/cmdline.h>
>>>>    #include <asm/bugs.h>
>>>> @@ -32,6 +34,7 @@
>>>>    #include <asm/intel-family.h>
>>>>    #include <asm/e820/api.h>
>>>>    #include <asm/hypervisor.h>
>>>> +#include <asm/xen/hypervisor.h>
>>>>    #include <asm/tlbflush.h>
>>>>      #include "cpu.h"
>>>> @@ -934,7 +937,8 @@ do_cmd_auto:
>>>>            break;
>>>>          case RETBLEED_MITIGATION_IBPB:
>>>> -        setup_force_cpu_cap(X86_FEATURE_ENTRY_IBPB);
>>>> +        if (!xen_pv_domain() || xen_vm_assist_ibpb(true))
>>>
>>> Is this going to compile without CONFIG_XEN?
> Yes. The declaration of xen_vm_assist_ibpb() is visible (satisfying
> the compiler) and DCE will eliminate the call to the function due to
> xen_pv_domain() being constant "false" in that case, avoiding any
> linking issues. The interesting case here really is building with
> XEN but without XEN_PV: That's why I needed to put the function in
> enlighten.c. This wouldn't be needed if xen_pv_domain() was also
> constant "false" in that case (just like xen_pvh_domain() is when
> !XEN_PVH).
>
>>> I also think these two conditions should be wrapped into something to limit exposure of non-Xen code to Xen-specific primitives.
> I would have done so, if I had any halfway sensible idea on how to
> go about doing so in this particular case. In the absence of that it
> looked okay-ish to me to reference Xen functions directly here.
>
>> Oh, and this needs x86 maintainers.
> Eventually yes. But I would prefer to sort the above question first
> (which I'm sure would have been raised by them, in perhaps more
> harsh a way), hence the initially limited exposure.
>

I also think there is a bit of a disconnect between how the mitigation is reported in the log/sysfs (retbleed_mitigation is RETBLEED_MITIGATION_IBPB, so "Mitigation: IBPB") and, for example, lscpu (since X86_FEATURE_ENTRY_IBPB is not set anymore).


-boris

On 16.02.2023 00:22, Boris Ostrovsky wrote:
> 
> On 2/15/23 3:31 AM, Jan Beulich wrote:
>> On 15.02.2023 01:07, Boris Ostrovsky wrote:
>>> On 2/14/23 6:53 PM, Boris Ostrovsky wrote:
>>>> On 2/14/23 11:13 AM, Jan Beulich wrote:
>>>>
>>>>> --- a/arch/x86/kernel/cpu/bugs.c
>>>>> +++ b/arch/x86/kernel/cpu/bugs.c
>>>>> @@ -18,6 +18,8 @@
>>>>>    #include <linux/pgtable.h>
>>>>>    #include <linux/bpf.h>
>>>>>    +#include <xen/xen.h>
>>>>> +
>>>>>    #include <asm/spec-ctrl.h>
>>>>>    #include <asm/cmdline.h>
>>>>>    #include <asm/bugs.h>
>>>>> @@ -32,6 +34,7 @@
>>>>>    #include <asm/intel-family.h>
>>>>>    #include <asm/e820/api.h>
>>>>>    #include <asm/hypervisor.h>
>>>>> +#include <asm/xen/hypervisor.h>
>>>>>    #include <asm/tlbflush.h>
>>>>>      #include "cpu.h"
>>>>> @@ -934,7 +937,8 @@ do_cmd_auto:
>>>>>            break;
>>>>>          case RETBLEED_MITIGATION_IBPB:
>>>>> -        setup_force_cpu_cap(X86_FEATURE_ENTRY_IBPB);
>>>>> +        if (!xen_pv_domain() || xen_vm_assist_ibpb(true))
>>>>
>>>> Is this going to compile without CONFIG_XEN?
>> Yes. The declaration of xen_vm_assist_ibpb() is visible (satisfying
>> the compiler) and DCE will eliminate the call to the function due to
>> xen_pv_domain() being constant "false" in that case, avoiding any
>> linking issues. The interesting case here really is building with
>> XEN but without XEN_PV: That's why I needed to put the function in
>> enlighten.c. This wouldn't be needed if xen_pv_domain() was also
>> constant "false" in that case (just like xen_pvh_domain() is when
>> !XEN_PVH).
>>
>>>> I also think these two conditions should be wrapped into something to limit exposure of non-Xen code to Xen-specific primitives.
>> I would have done so, if I had any halfway sensible idea on how to
>> go about doing so in this particular case. In the absence of that it
>> looked okay-ish to me to reference Xen functions directly here.
>>
>>> Oh, and this needs x86 maintainers.
>> Eventually yes. But I would prefer to sort the above question first
>> (which I'm sure would have been raised by them, in perhaps more
>> harsh a way), hence the initially limited exposure.
>>
> 
> I also think there is a bit of a disconnect between how the mitigation is reported in the log/sysfs (retbleed_mitigation is RETBLEED_MITIGATION_IBPB, so "Mitigation: IBPB") and, for example, lscpu (since X86_FEATURE_ENTRY_IBPB is not set anymore).

Initially I too was worried about this, but ENTRY_IBPB is not exposed,
as per the empty double quotes in

#define X86_FEATURE_ENTRY_IBPB		(11*32+10) /* "" Issue an IBPB on kernel entry */

Jan