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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

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.

> and not doing so will shrink this diff a lot.
This particular one, yes. That said, looking more carefully I'd say I
should remove the XD_DISABLE parts of those lines altogether. It is
unconditionally cleared on boot now and will never ever have a non-zero
value at this point.

> The only thing you need to alter the re-enable NX printk(), which
> probably wants to move ahead of the "if (disable)" and source itself
> from bootsym(trampoline_misc_enable_off) instead.
Alternatively I could just get rid of the printk. As it is after this patch
even the BSP starts off with NX enabled.  EFER is updated as early as the
trampoline, which is as early as it would've happened before regardless of
XD_DISABLE.

Alejandro