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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.07.2024 19:38, Andrew Cooper wrote:
> 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.

Oh, okay.

> 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.

Just to mention it: I wouldn't have considered this to be "trickery".

> 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.

That may end up quite challenging without actually fiddling with the
compiler itself. Plus rdmsrl()/wrmsrl() themselves won't know how the
values are used in surrounding code, so improving one set of cases
may make things worse for another set. Introducing yet another variant
of them may also not be very desirable.

Jan