Re: [PATCH 2/2] x86/efi: Unlock NX if necessary

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/07/2024 6:04 pm, Andrew Cooper wrote:
> On 22/07/2024 11:43 am, Jan Beulich wrote:
>> On 22.07.2024 12:18, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/efi/efi-boot.h
>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>> @@ -736,13 +736,33 @@ static void __init efi_arch_handle_module(const 
>>> struct file *file,
>>>      efi_bs->FreePool(ptr);
>>>  }
>>>  
>>> +static bool __init intel_unlock_nx(void)
>>> +{
>>> +    uint64_t val, disable;
>>> +
>>> +    rdmsrl(MSR_IA32_MISC_ENABLE, val);
>>> +
>>> +    disable = val & MSR_IA32_MISC_ENABLE_XD_DISABLE;
>>> +
>>> +    if ( !disable )
>>> +        return false;
>>> +
>>> +    wrmsrl(MSR_IA32_MISC_ENABLE, val & ~disable);
>> The base ISA not having ANDN or NAND (and a prereq to my patch to add
>> minimum-ABI-level control to the build machinery still sitting there
>> unreviewed), using "val ^ disable" here would likely produce slightly
>> better code for the time being.
> While that might technically be true, you're assuming that everyone
> reading the code can de-obfuscate ^ back into &~, and that the compiler
> hasn't made its own alternative arrangements.

In fact, the compiler sees through this "clever" trick and undoes the XOR.

Swapping &~ for ^ makes no change in the compiled binary, because in
both cases GCC chooses a BTR instruction instead.


While BTR might be a poor choice of instruction for this purpose, it
reinforces my opinion that trickery such as this is not something we
want to do.

If you want a more useful optimisation task, we should figure out how to
write rdmsrl()/wrmsrl() better so GCC is happy working on %edx in
isolation, rather than always merging it into %rax to be operated on. 
The rdmsr()/wrmsr() helpers taking a split hi and lo generate far better
code, even if they are much more awkward to use at a C level.

~Andrew