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
