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
