Re: [PATCH 4/4] arm64: head: tidy up the Image header definition

From: Robin Murphy <robin.murphy@arm.com>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, James Morse <james.morse@arm.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 4/4] arm64: head: tidy up the Image header definition
Date: Wed, 4 Nov 2020 11:29:59 +0000	[thread overview]
Message-ID: <c286eb6d-ae62-2dbb-8bd7-0bc8e597cd2a@arm.com> (raw)
In-Reply-To: <CAMj1kXFkC24u-P1C9HB8XyTGZMDR1d4v4QRr5ErskcwSe2oLqA@mail.gmail.com>

On 2020-11-03 07:13, Ard Biesheuvel wrote:
> On Thu, 29 Oct 2020 at 14:07, Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2020-10-29 07:30, Ard Biesheuvel wrote:
>>> On Wed, 28 Oct 2020 at 18:56, Robin Murphy <robin.murphy@arm.com> wrote:
>>>>
>>>> On 2020-10-28 14:17, Will Deacon wrote:
>>>>> On Tue, Oct 27, 2020 at 08:32:09AM +0100, Ard Biesheuvel wrote:
>>>>>> Even though support for EFI boot remains entirely optional for arm64,
>>>>>> it is unlikely that we will ever be able to repurpose the image header
>>>>>> fields that the EFI loader relies on, i.e., the magic NOP at offset
>>>>>> 0x0 and the PE header address at offset 0x3c.
>>>>>>
>>>>>> So let's factor out the differences into a 'magic_nop' macro and a local
>>>>>> symbol representing the PE header address, and move the conditional
>>>>>> definitions into efi-header.S, taking into account whether CONFIG_EFI is
>>>>>> enabled or not.
>>>>>
>>>>> How many architectures can claim to have both a "magic nop" and a
>>>>> "mysterious nop", hey?
>>>>
>>>> It's fun 'n'all, but putting my serious hat on for a moment, if the name
>>>> still requires a comment to explain it at point of use, it's not a very
>>>> good name :(
>>>>
>>>> At worst magic_nop is even potentially misleading, since it's not
>>>> necessarily a nop; there's no mention of the implicit dependency on a
>>>> context where the side-effect of executing it wouldn't affect anything
>>>> important.
>>>>
>>>> Could we call the macro itself something clear and self-explanatory like
>>>> efi_signature_insn please? I'm happy for it to be *commented* as "Magic
>>>> NOP" if you want parity with the VDSO :D
>>>>
>>>
>>> Will efi_pseudo_nop do?
>>
>> Again, what's the defining significance of the instruction that this
>> macro stands for - that it does nothing; that it does pseudo-nothing; or
>> that it has a specific signature encoding? I know this probably sounds
>> like bikeshedding to most, but I firmly believe that good, accurate
>> names really do matter :)
>>
> 
> Fair enough. But by that reasoning, putting NOP in the name is
> important, given that efi_signature_insn does not explain what the
> instruction does.
> 
> So, efi_signature_nop then?

Sure; I think arguing semantics beyond that point *would* be bikeshedding :)

>>> Also, do you think it would be better to use an opcode here that has
>>> no architectural side effects, such as a PRFM (literal) instruction?
>>> It is obviously not going to make a difference in practice, but it
>>> always annoyed me that the pseudo NOP is not a NOP.
>>
>> Yeah, it's a shame there's no way to get a guaranteed non-taken
>> conditional branch in A64, and nearly every good candidate for a
>> non-destructive operation with an arbitrary immediate seems to rely on
>> an rt=31 encoding... 'prfm PLIL3STRM, . + 2888' is utterly impenetrable,
>> but should indeed work; 'ccmp x18, #0, #0xd, pl' is probably the least
>> destructive ALU option (only a chance of changing the flags).
>>
> 
> I think ccmp is probably a better choice, given that PRFM PLI
> instructions issued with the MMU off are more likely to trigger
> something unexpected.

At least it happened to be PLI rather than PLD - the latter I'd 
definitely be scared of...

Robin.

>>>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>>>> ---
>>>>>>     arch/arm64/kernel/efi-header.S | 43 +++++++++++++++-----
>>>>>>     arch/arm64/kernel/head.S       | 19 +--------
>>>>>>     2 files changed, 35 insertions(+), 27 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm64/kernel/efi-header.S b/arch/arm64/kernel/efi-header.S
>>>>>> index ddaf57d825b5..7b7ac4316d95 100644
>>>>>> --- a/arch/arm64/kernel/efi-header.S
>>>>>> +++ b/arch/arm64/kernel/efi-header.S
>>>>>> @@ -7,7 +7,27 @@
>>>>>>     #include <linux/pe.h>
>>>>>>     #include <linux/sizes.h>
>>>>>>
>>>>>> +    .macro  magic_nop
>>>>>> +#ifdef CONFIG_EFI
>>>>>> +.L_head:
>>>>>> +    /*
>>>>>> +     * This add instruction has no meaningful effect except that
>>>>>> +     * its opcode forms the magic "MZ" signature required by UEFI.
>>>>>> +     */
>>>>>> +    add     x13, x18, #0x16
>>>>>
>>>>> It's probably faster too ;)
>>>>>
>>>>>> +#else
>>>>>> +    /*
>>>>>> +     * Bootloaders may inspect the opcode at the start of the kernel
>>>>>> +     * image to decide if the kernel is capable of booting via UEFI.
>>>>>> +     * So put an ordinary NOP here, not the "MZ.." pseudo-nop above.
>>>>>> +     */
>>>>>> +    nop
>>>>>
>>>>> Let's just hope nobody was decoding the branch instruction...
>>>>>
>>>>> Acked-by: Will Deacon >will@kernel.org>
>>>>>
>>>>> Will
>>>>>
>>>>> _______________________________________________
>>>>> linux-arm-kernel mailing list
>>>>> linux-arm-kernel@lists.infradead.org
>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel