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

[Juergen Gross <jgross@xxxxxxxx>]

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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature