Re: [Xen-devel] [PATCH 1/3] x86/boot: Remove cached CPUID data from the trampoline

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.11.2019 21:25, Andrew Cooper wrote:
> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -270,6 +270,7 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>       if (disable) {
>               wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable & ~disable);
>               bootsym(trampoline_misc_enable_off) |= disable;
> +             bootsym(trampoline_efer) |= EFER_NX;
>       }

I'm fine with all other changes here, just this one concerns me:
Before your change we latch a value into trampoline_misc_enable_off
just to be used for subsequent IA32_MISC_ENABLE writes we do. This
means that, on a hypervisor (like Xen itself) simply discarding
guest writes to the MSR (which is "fine" with the described usage
of ours up to now), with your change we'd now end up trying to set
EFER.NX when the feature may not actually be enabled in
IA32_MISC_ENABLE. Wouldn't such an EFER write be liable to #GP(0)?
I.e. don't we need to read back the MSR value here, and verify
we actually managed to clear the bit before actually OR-ing in
EFER_NX?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel