Re: [Xen-devel] [PATCH] x86: re-enable NX if disabled

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.12.15 at 19:25, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 04/12/15 16:31, Jan Beulich wrote:
>> @@ -56,6 +60,9 @@ trampoline_gdt:
>>          .long   trampoline_gdt + BOOT_PSEUDORM_DS + 2 - .
>>          .popsection
>>  
>> +GLOBAL(trampoline_misc_enable_off)
>> +        .quad   0
>> +
> 
> For clarity, I would name this trampoline_misc_enable_mask and invert
> its representation to make the asm easier.

Easier? 

>> +        mov     bootsym_rel(trampoline_misc_enable_off,4,%esi)
>> +        mov     bootsym_rel(trampoline_misc_enable_off+4,4,%edi)
>> +        mov     %esi,%eax
>> +        or      %edi,%eax
>> +        jz      1f
>> +        mov     $MSR_IA32_MISC_ENABLE,%ecx
>> +        rdmsr
>> +        not     %esi
>> +        not     %edi
>> +        and     %esi,%eax
>> +        and     %edi,%edx
>> +        wrmsr
>> +1:

We could save the two NOTs, at the expense of the conditional
requiring a comparison. Not a whole lot of a win I would say, and
I generally prefer zero-initialized data items in the trampoline.

>> --- a/xen/arch/x86/cpu/intel.c
>> +++ b/xen/arch/x86/cpu/intel.c
>> @@ -162,19 +162,29 @@ static void __devinit early_init_intel(s
>>      if (c->x86 == 15 && c->x86_cache_alignment == 64)
>>              c->x86_cache_alignment = 128;
>>  
>> -    /* Unmask CPUID levels if masked: */
>> +    /* Unmask CPUID levels and NX if masked: */
>>      if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) {
>> -            u64 misc_enable;
>> +            u64 misc_enable, disable;
>>  
>>              rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
>>  
>> -            if (misc_enable & MSR_IA32_MISC_ENABLE_LIMIT_CPUID) {
>> -                    misc_enable &= ~MSR_IA32_MISC_ENABLE_LIMIT_CPUID;
>> -                    wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
>> -                    c->cpuid_level = cpuid_eax(0);
>> -                    if (opt_cpu_info || c == &boot_cpu_data)
>> -                            printk(KERN_INFO "revised cpuid level: %d\n",
>> -                                   c->cpuid_level);
>> +            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;
>> +            }
>> +
>> +            if (disable & MSR_IA32_MISC_ENABLE_LIMIT_CPUID)
>> +                    printk(KERN_INFO "revised cpuid level: %d\n",
>> +                           cpuid_eax(0));
>> +            if (disable & MSR_IA32_MISC_ENABLE_XD_DISABLE) {
>> +                    u64 efer;
>> +
>> +                    rdmsrl(MSR_EFER, efer);
>> +                    wrmsrl(MSR_EFER, efer | EFER_NX);
> 
> {read,write}_efer(), which also update this_cpu(efer).

Oh, indeed.

>> +                    printk(KERN_INFO
>> +                           "re-enabled NX (Execute Disable) protection\n");
>>              }
> 
> Because of the asm adjustment of $MSR_IA32_MISC_ENABLE, this code will
> only ever run on the BSP.
> 
> As such, it would better live somewhere like early_cpu_detect() (or a
> brand new early_cpu_workaround()), making it __init.

Well, I considered this but dropped the idea to avoid scattering
around of these workarounds even further. But now that you ask
for such - if at all I'd consider moving it into intel_cpu_init(). Otoh
the place it's now at allows for us noticing if the adjustment in the
trampoline didn't take effect (because then early_init_intel() would
still result in messages logged, even if in the NX case the AP would
be unlikely to make it that far).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel