Re: [PATCH] x86/Xen: make use of IBPB controlling VM assist
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
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH] x86/Xen: make use of IBPB controlling VM assist
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
Re: [PATCH] x86/Xen: make use of IBPB controlling VM assist
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
#include
+#include
+
#include
#include
#include
@@ -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
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH] x86/Xen: make use of IBPB controlling VM assist
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
Re: [PATCH] x86/Xen: make use of IBPB controlling VM assist
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
> #include
>
> +#include
> +
> #include
> #include
> #include
> @@ -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
Re: [PATCH] x86/Xen: make use of IBPB controlling VM assist
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
#include
+#include
+
#include
#include
#include
@@ -32,6 +34,7 @@
#include
#include
#include
+#include
#include
#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
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH] x86/Xen: make use of IBPB controlling VM assist
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
> #include
> +#include
> +
> #include
> #include
> #include
> @@ -32,6 +34,7 @@
> #include
> #include
> #include
> +#include
> #include
> #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
Re: [PATCH] x86/Xen: make use of IBPB controlling VM assist
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
#include
+#include
+
#include
#include
#include
@@ -32,6 +34,7 @@
#include
#include
#include
+#include
#include
#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
#include
+#include
+
#include
#include
#include
@@ -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
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH] x86/Xen: make use of IBPB controlling VM assist
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 > #include > +#include > + > #include > #include > #include > @@ -32,6 +34,7 @@ > #include > #include > #include > +#include > #include > #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
Re: [PATCH] x86/Xen: make use of IBPB controlling VM assist
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 #include +#include + #include #include #include @@ -32,6 +34,7 @@ #include #include #include +#include #include #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
Re: [PATCH] x86/Xen: make use of IBPB controlling VM assist
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 >>> #include >>> +#include >>> + >>> #include >>> #include >>> #include >>> @@ -32,6 +34,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include >>> #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
Re: [PATCH] x86/Xen: make use of IBPB controlling VM assist
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 #include +#include + #include #include #include @@ -32,6 +34,7 @@ #include #include #include +#include #include #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
Re: [PATCH] x86/Xen: make use of IBPB controlling VM assist
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 #include +#include + #include #include #include @@ -32,6 +34,7 @@ #include #include #include +#include #include #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;
