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' */
 
-/* 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_uint32_t magic;		/* Magic number, little endian, "RSC\x05" */
   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' */
 
 #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
-- 
2.25.1

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

On Fri, 7 Oct 2022 at 01:51, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> 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.
>

Indeed.

But by the same reasoning, why do we need per-arch kernel header
typedefs at all?  The only fields we access are generic PE/COFF header
fields.

> >
> > -/* 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

On 10/7/22 09:20, Ard Biesheuvel wrote:
> On Fri, 7 Oct 2022 at 01:51, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>>
>>
>> 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.
>>
> 
> Indeed.
> 
> But by the same reasoning, why do we need per-arch kernel header
> typedefs at all?  The only fields we access are generic PE/COFF header
> fields.

That said I would suggest to put the series in without any further 
iterations and clean up afterwards.

Acked-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

> 
>>>
>>> -/* 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

On Fri, Oct 7, 2022 at 12:50 AM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 10/7/22 09:20, Ard Biesheuvel wrote:
> > On Fri, 7 Oct 2022 at 01:51, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >>
> >>
> >> 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?
> >>

Yes. Those are not required. I will remove those.

> >> 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.
> >>
> >
GRUB_LINUX_ARM_MAGIC_SIGNATURE is still being used in ARM32 loaders.
The other one can be removed.

> > Indeed.
> >
> > But by the same reasoning, why do we need per-arch kernel header
> > typedefs at all?  The only fields we access are generic PE/COFF header
> > fields.
>
> That said I would suggest to put the series in without any further
> iterations and clean up afterwards.
>
> Acked-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>
> >
> >>>
> >>> -/* 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.
> >>

Yes. I forgot to update this one for rv32. RV64 header file has the
correct fields.

> >> 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' */
> >>

Done.

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



-- 
Regards,
Atish

On Fri, Oct 7, 2022 at 12:50 AM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 10/7/22 09:20, Ard Biesheuvel wrote:
> > On Fri, 7 Oct 2022 at 01:51, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >>
> >>
> >> 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.
> >>
> >
> > Indeed.
> >
> > But by the same reasoning, why do we need per-arch kernel header
> > typedefs at all?  The only fields we access are generic PE/COFF header
> > fields.
>
> That said I would suggest to put the series in without any further
> iterations and clean up afterwards.
>
> Acked-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>

Thanks for reviewing it. I will send the next version right now. I
screwed up my scripts and grub-devel was not in the CC also.

> >
> >>>
> >>> -/* 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



-- 
Regards,
Atish