Message-ID: <d7ef1d58-2c3f-ef58-b6aa-bb7ccfe162f6@wdc.com>
Date:   Thu, 23 May 2019 11:35:11 -0700
From:   Atish Patra <atish.patra@....com>
To:     Paul Walmsley <paul.walmsley@...ive.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "marek.vasut@...il.com" <marek.vasut@...il.com>,
        "trini@...sulko.com" <trini@...sulko.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Anup Patel <anup@...infault.org>,
        Palmer Dabbelt <palmer@...ive.com>,
        Zong Li <zong@...estech.com>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "will.deacon@....com" <will.deacon@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "merker@...ian.org" <merker@...ian.org>,
        "linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>
Subject: Re: [v2 PATCH] RISC-V: Add a PE/COFF compliant Image header.

On 5/16/19 9:20 AM, Paul Walmsley wrote:
> + ARM64 maintainers, Tom, Marek
> 
> Hi Atish,
> 
> On Mon, 13 May 2019, Atish Patra wrote:
> 
>> On 5/13/19 5:40 PM, Paul Walmsley wrote:
>>> On Mon, 13 May 2019, Atish Patra wrote:
>>>> On 5/13/19 5:09 PM, Paul Walmsley wrote:
>>>>
>>>>> What are the semantics of those reserved fields?
>>>>
>>>> +struct riscv_image_header {
>>>> +	u32 code0;
>>>> +	u32 code1;
>>>> +	u64 text_offset;
>>>> +	u64 image_size;
>>>> +	u64 res1;
>>>> +	u64 res2;
>>>> +	u64 res3;
>>>> +	u64 magic;
>>>> +	u32 res4; ---> We can use this for versioning when required
>>>> +	u32 res5; ---> This is reserved for PE/COFF header
>>>> +};
>>>
>>> I saw that in your patch.  The problem is that this doesn't describe what
>>> other software might expect in those fields.  Can anything at all be
>>> placed in those reserved fields?
>>
>> Yes. The reserved fields can be used for anything that boot loaders and Linux
>> kernel can agree with each other. If you look at the ARM64, they have
>> "Informative flags" in place of res1.
>>
>>>>>> Do we need to add it now or add it later when we actually need a
>>>>>> version
>>>>>> number. My preference is to add it later based on requirement.
>>>>>
>>>>> If it isn't added now, how would bootloaders know whether it was there
>>>>> or
>>>>> not?
>>>>>
>>>>>
>>>> Here is the corresponding U-Boot Patch
>>>> https://patchwork.ozlabs.org/patch/1096087/
>>>>
>>>> Currently, boot loader doesn't care about versioning. Since we are
>>>> updating a
>>>> reserved field, offsets will not change. If a boot loader want to use the
>>>> versioning, it should be patched along with the kernel patch.
>>>>
>>>> Any other boot loader that doesn't care about the version, it can continue
>>>> to
>>>> do so without any change.
>>>>
>>>> My idea is to enable the minimum required fields in this patch and keep
>>>> everything else as reserved so that it can be amended in future as
>>>> required.
>>>
>>> If those fields really are reserved for implementors to do whatever they
>>> want with them, then that might be a reasonable approach.  That seems
>>> unlikely, however, since specification authors usually reserve the right
>>> to use reserved fields for their own purposes in later versions.
>>>
>> Technically, we are just implementing the "DOS" header part of PE/COFF format
>> for now. It only mandates a magic string "MZ" at the top and a 32bit value at
>> offset 0x3c tells us offset of PE/COFF header in image.
>> Anything in between is implementation specific.
>>
>> For example, it will be updated to support EFI stub as described in the commit
>> text,
>> "In order to support EFI stub, code0 should be replaced with "MZ" magic string
>> and res5(at offset 0x3c) should point to the rest of the PE/COFF header (which
>> will be added during EFI support)."
> 
> OK.  I think we should try to share this header format with other
> architectures.  This one after all is copied from ARM64, and some of the
> core fields will be the same across multiple architectures.  That way we
> can try to avoid proliferating different boot header formats for each
> architecture, which should be better for both the kernel and the
> bootloaders.  ARM64 folks, would you be interested in working together on
> this?
> 
> Meanwhile, to unblock RISC-V, and to make this header durable for future
> extensions and to match the existing ARM64 usage, I think we should make
> the following technical changes to what you proposed:
> 
> 1. Reserve all of the existing ARM64 fields in the same way ARM64 does
>     now.  This keeps open the possibility that we can merge this format
>     with the one used with ARM64, and reuse the same bootloader code.
>     Based on our discussions, it sounds like the primary difference between
>     what you're proposing and the ARM64 format involves the flags/res1
>     field.  Let's keep that as a flag field, reuse ARM64's endianness bit
>     as architecture-independent, then define the rest of the flags in that
>     field as architecture-defined.
> 
> 2. Allocate another set of reserved bits for a format version number.
>     Probably 16 bits is sufficient.  This tells bootloaders how to
>     interpret the header fields in future extensions.  The goal is to
>     preserve compatibility across newer and older versions of the header.
>     The existing ARM64 header would be version 0.  This format that
>     incorporates these changes would be version 1.  The thought here is to
>     preserve all of the semantics of existing fields in newer versions
>     (except for any remaining reserved fields), since many people often do
>     not replace their bootloaders.
> 
> 3. Define a way to point to additional fields outside this existing
>     header.  Another 32 bits of previously reserved data can be defined as
>     a file offset to additional fields (defined as 32-bit words from the
>     beginning of the header).  This should make it technically simple to
>     add additional fields in the future.  For example, RISC-V, and probably
>     other architectures, will want to add some way to indicate which ISA
>     extensions are necessary to run the kernel image.  Right now there
>     won't be any fields defined, so we can leave the format undefined for
>     the moment also.  Let's stipulate for version 1 that this field
>     should be fixed at 0, indicating no additional fields.
> 
> 4. Document all of this, in this patch, in a file such as
>     Documentation/riscv/boot-image-header.txt.  If
>     we're able to reach agreement with other maintainers, then we
>     can move this file out into a common, non-architecture-specific
>     documentation location.
> 

I have sent out a v3 incorporating most of your suggestions. If ARM 
maintainers agree, we can move both the headers to a common place.

Just FYI: Marek also suggested to add unified support Image.gz for both 
U-Boot & RISC-V in U-Boot. I am working on that as well.

> 
> thanks
> 
> - Paul
> 


-- 
Regards,
Atish

Powered by blists - more mailing lists