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

[Juergen Gross <jgross@xxxxxxxx>]

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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature