Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b1306db2-74c5-c207-7c6d-beba0f1593f4@canonical.com>
Date:   Fri, 7 Oct 2022 01:51:29 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.1
Subject: Re: [v4 PATCH 2/3] RISC-V: Update image header
Content-Language: en-US
To:     Atish Patra <atishp@rivosinc.com>
Cc:     Ard Biesheuvel <ardb@kernel.org>,
        Daniel Kiper <daniel.kiper@oracle.com>,
        Fu Wei <tekkamanninja@gmail.com>,
        Leif Lindholm <quic_llindhol@quicinc.com>,
        Nikita Ermakov <arei@altlinux.org>,
        Atish Patra <atishp@atishpatra.org>,
        Julian Andres Klode <julian.klode@canonical.com>,
        Ilias Apalodimas <ilias.apalodimas@linaro.org>,
        linux-kernel@vger.kernel.org
References: <20221006230051.185850-1-atishp@rivosinc.com>
 <20221006230051.185850-3-atishp@rivosinc.com>
From:   Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
In-Reply-To: <20221006230051.185850-3-atishp@rivosinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 10/7/22 01:00, Atish Patra wrote:
> Update the RISC-V Linux kernel image headers as per the current header.
> 
> Reference:
> <Linux kernel source>/Documentation/riscv/boot-image-header.rst
> 
> 474efecb65dc: ("riscv: modify the Image header to improve compatibility with the ARM64 header")
> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>   include/grub/riscv32/linux.h | 15 ++++++++-------
>   include/grub/riscv64/linux.h | 21 +++++++++++++--------
>   2 files changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/include/grub/riscv32/linux.h b/include/grub/riscv32/linux.h
> index 512b777c8a41..de0dbdcd1be5 100644
> --- a/include/grub/riscv32/linux.h
> +++ b/include/grub/riscv32/linux.h
> @@ -19,20 +19,21 @@
>   #ifndef GRUB_RISCV32_LINUX_HEADER
>   #define GRUB_RISCV32_LINUX_HEADER 1
>   
> -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */

Thanks for following up on this series.

Considering 69edb312056 ("loader/arm64/linux: Remove magic number header 
field check") why should this constant exist in GRUB at all?

In a follow up patch we could remove it together with 
GRUB_LINUX_ARM_MAGIC_SIGNATURE, GRUB_LINUX_ARMXX_MAGIC_SIGNATURE, and 
GRUB_LINUX_ARMXX_MAGIC_SIGNATURE.

>   
> -/* From linux/Documentation/riscv/booting.txt */
> +/* From linux/Documentation/riscv/boot-image-header.rst */
>   struct linux_riscv_kernel_header
>   {
>     grub_uint32_t code0;		/* Executable code */
>     grub_uint32_t code1;		/* Executable code */
> -  grub_uint64_t text_offset;	/* Image load offset */
> -  grub_uint64_t res0;		/* reserved */
> -  grub_uint64_t res1;		/* reserved */
> +  grub_uint64_t text_offset;	/* Image load offset, little endian */
> +  grub_uint64_t image_size;	/* Effective Image size, little endian */
> +  grub_uint64_t flags;		/* kernel flags, little endian */
> +  grub_uint32_t version;	/* Version of this header */
> +  grub_uint32_t res1;		/* reserved */
>     grub_uint64_t res2;		/* reserved */
>     grub_uint64_t res3;		/* reserved */

According to tag next-20221006 of 
Documentation/riscv/boot-image-header.rst and of 
arch/riscv/include/asm/image.h this field is called 'magic' and filled 
it with the string 'RISCV\0\0\0'.

> -  grub_uint64_t res4;		/* reserved */
> -  grub_uint32_t magic;		/* Magic number, little endian, "RSCV" */
> +  grub_uint32_t magic;		/* Magic number, little endian, "RSC\x05" */

The Linux kernel documentation calls this field magic2.

Of course this is functionally irrelevant as we don't care about the 
content of both fields.

>     grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
>   };
>   
> diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
> index 3630c30fbf1a..ea77f8718222 100644
> --- a/include/grub/riscv64/linux.h
> +++ b/include/grub/riscv64/linux.h
> @@ -19,23 +19,28 @@
>   #ifndef GRUB_RISCV64_LINUX_HEADER
>   #define GRUB_RISCV64_LINUX_HEADER 1
>   
> -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> +#include <grub/efi/pe32.h>
> +
> +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */

to be removed in future

Best regards

Heinrich

>   
>   #define GRUB_EFI_PE_MAGIC	0x5A4D
>   
> -/* From linux/Documentation/riscv/booting.txt */
> +/* From linux/Documentation/riscv/boot-image-header.rst */
>   struct linux_riscv_kernel_header
>   {
>     grub_uint32_t code0;		/* Executable code */
>     grub_uint32_t code1;		/* Executable code */
> -  grub_uint64_t text_offset;	/* Image load offset */
> -  grub_uint64_t res0;		/* reserved */
> -  grub_uint64_t res1;		/* reserved */
> +  grub_uint64_t text_offset;	/* Image load offset, little endian */
> +  grub_uint64_t image_size;	/* Effective Image size, little endian */
> +  grub_uint64_t flags;		/* kernel flags, little endian */
> +  grub_uint32_t version;	/* Version of this header */
> +  grub_uint32_t res1;		/* reserved */
>     grub_uint64_t res2;		/* reserved */
> -  grub_uint64_t res3;		/* reserved */
> -  grub_uint64_t res4;		/* reserved */
> -  grub_uint32_t magic;		/* Magic number, little endian, "RSCV" */
> +  grub_uint64_t magic;		/* magic (RISC-V specifc, deprecated)*/
> +  grub_uint32_t magic2;		/* Magic number 2 (to match the ARM64 'magic' field pos) */
>     grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
> +
> +  struct grub_coff_image_header coff_image_header;
>   };
>   
>   #define linux_arch_kernel_header linux_riscv_kernel_header