Re: [PATCH] x86/Xen: make use of IBPB controlling VM assist

[Juergen Gross <jgross@xxxxxxxx>]

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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature