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

[Jan Beulich <jbeulich@xxxxxxxx>]

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