Re: [PATCH v2 1/2] x86/boot: Clear XD_DISABLE from the early boot path

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On Thu, Jun 22, 2023 at 09:54:01AM +0200, Jan Beulich wrote:
> On 21.06.2023 18:43, Alejandro Vallejo wrote:
> > Sure, to everything before this
> > 
> >>> diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
> >>> index 168cd58f36..46b0cd8dbb 100644
> >>> --- a/xen/arch/x86/cpu/intel.c
> >>> +++ b/xen/arch/x86/cpu/intel.c
> >>> @@ -305,23 +305,23 @@ static void cf_check early_init_intel(struct 
> >>> cpuinfo_x86 *c)
> >>>           c->x86_cache_alignment = 128;
> >>>  
> >>>   /* Unmask CPUID levels and NX if masked: */
> >>> - rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> >>> -
> >>> - disable = misc_enable & (MSR_IA32_MISC_ENABLE_LIMIT_CPUID |
> >>> -                          MSR_IA32_MISC_ENABLE_XD_DISABLE);
> >>> - if (disable) {
> >>> -         wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable & ~disable);
> >>> -         bootsym(trampoline_misc_enable_off) |= disable;
> >>> -         bootsym(trampoline_efer) |= EFER_NXE;
> >>> - }
> >>> + if (rdmsr_safe(MSR_IA32_MISC_ENABLE, misc_enable) == 0) {
> >>
> >> There's no need to change rdmsrl() to rdmsr_safe(),
> > I thought we established before some hypervisors might not implement it. In
> > that case this function would crash. More gracefully than a triple fault,
> > sure, but still not a very friendly thing to do.
> 
> Yet then in early boot code you also don't (and can't) recover from getting
> #GP(0), in case that might really happen.
> 
> Jan
That's true, strictly speaking, but that case is restricted to the
incredibly unlikely case of "no NX and no MSR". As is, if we ever boot
on an Intel machine without that MSR we'll hit #GP(0), regardless of NX.

Not that it matters a whole lot though. It's pretty unlikely we'll ever
trip there.

Alejandro