Re: [PATCH] xen/riscv: PE/COFF image header for RISC-V target

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.06.2024 18:54, milandjokic1995@xxxxxxxxx wrote:
> --- a/xen/arch/riscv/Kconfig
> +++ b/xen/arch/riscv/Kconfig
> @@ -9,6 +9,15 @@ config ARCH_DEFCONFIG
>       string
>       default "arch/riscv/configs/tiny64_defconfig"
>  
> +config RISCV_EFI
> +     bool "UEFI boot service support"
> +     depends on RISCV_64
> +     default n
> +     help
> +       This option provides support for boot services through
> +       UEFI firmware. A UEFI stub is provided to allow Xen to
> +       be booted as an EFI application.

Hmm, all of this promises quite a bit more than you actually add. If
there are future plans, please clarify in the description. Otherwise
please consider adjusting name, prompt, and help text to actually
cover just what's actually done.

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/image.h

This is pretty generic a name for something pretty specific.

> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +
> +#ifndef _ASM_RISCV_IMAGE_H
> +#define _ASM_RISCV_IMAGE_H
> +
> +#define RISCV_IMAGE_MAGIC    "RISCV\0\0\0"
> +#define RISCV_IMAGE_MAGIC2   "RSC\x05"
> +
> +#define RISCV_IMAGE_FLAG_BE_SHIFT    0
> +#define RISCV_IMAGE_FLAG_BE_MASK     0x1
> +
> +#define RISCV_IMAGE_FLAG_LE          0
> +#define RISCV_IMAGE_FLAG_BE          1
> +
> +#ifdef CONFIG_CPU_BIG_ENDIAN

I don't think we have such a Kconfig control.

> +#error conversion of header fields to LE not yet implemented
> +#else
> +#define __HEAD_FLAG_BE               RISCV_IMAGE_FLAG_LE
> +#endif
> +
> +#define __HEAD_FLAG(field)   (__HEAD_FLAG_##field << \
> +                             RISCV_IMAGE_FLAG_##field##_SHIFT)
> +
> +#define __HEAD_FLAGS         (__HEAD_FLAG(BE))
> +
> +#define RISCV_HEADER_VERSION_MAJOR 0
> +#define RISCV_HEADER_VERSION_MINOR 2
> +
> +#define RISCV_HEADER_VERSION (RISCV_HEADER_VERSION_MAJOR << 16 | \
> +                           RISCV_HEADER_VERSION_MINOR)

Nit: Indentation of this 2nd line wants to result in the two RISCV_
to be properly aligned.

> +#ifndef __ASSEMBLY__
> +/*
> + * struct riscv_image_header - riscv xen image header
> + *
> + * @code0:           Executable code
> + * @code1:           Executable code
> + * @text_offset:     Image load offset
> + * @image_size:              Effective Image size
> + * @reserved:                reserved
> + * @reserved:                reserved
> + * @reserved:                reserved
> + * @magic:           Magic number
> + * @reserved:                reserved
> + * @reserved:                reserved (will be used for PE COFF offset)
> + */
> +
> +struct riscv_image_header {
> +     u32 code0;
> +     u32 code1;
> +     u64 text_offset;
> +     u64 image_size;
> +     u64 res1;
> +     u64 res2;
> +     u64 res3;
> +     u64 magic;
> +     u32 res4;
> +     u32 res5;

No new uses of u32 / u64 anymore, please. We're in the process of fully
moving to uint<N>_t.

> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -1,14 +1,40 @@
>  #include <asm/asm.h>
>  #include <asm/riscv_encoding.h>
> +#include <asm/image.h>
>  
>          .section .text.header, "ax", %progbits
>  
>          /*
>           * OpenSBI pass to start():
>           *   a0 -> hart_id ( bootcpu_id )
> -         *   a1 -> dtb_base 
> +         *   a1 -> dtb_base
>           */
>  FUNC(start)
> +#ifdef CONFIG_RISCV_EFI
> +        j xen_start

Comparing with what Arm does, shouldn't this similarly resolve to
the MZ pattern in the binary? In which case likely it needs to be
an entirely different insn, if such an insn even exists on RISC-V?
Otherwise the lack of MZ would clearly need explaining in the
description.

> +        /* -----------  Header -------------- */
> +     .word 0

Nit: Please use consistent indentation - either always tabs or
always blanks (matching what existing code uses).

> +     .balign 8
> +#if __riscv_xlen == 64

Wouldn't this better be CONFIG_RISCV_64? We do have #if-s like
this, but in different contexts. Even there I wonder of the
mix - Cc-ing Oleksii to possible comment (you probably should
have Cc-ed him anyway).

> +     /* Image load offset(2MB) from start of RAM */
> +     .dword 0x200000
> +#else
> +     /* Image load offset(4MB) from start of RAM */
> +     .dword 0x400000
> +#endif
> +     /* Effective size of xen image */
> +     .dword _end - _start
> +     .dword __HEAD_FLAGS
> +     .word RISCV_HEADER_VERSION
> +     .word 0
> +     .dword 0
> +     .ascii RISCV_IMAGE_MAGIC
> +     .balign 4
> +     .ascii RISCV_IMAGE_MAGIC2

There's only one "magic" in the struct further up.

This also isn't quite enough for a PE/COFF image, see again Arm
code. If the other header parts aren't needed, that too would
want mentioning / explaining in the description.

> +FUNC(xen_start)
> +#endif
>          /* Mask all interrupts */
>          csrw    CSR_SIE, zero
>  
> @@ -60,6 +86,11 @@ FUNC(start)
>          mv      a1, s1
>  
>          tail    start_xen
> +
> +#ifdef CONFIG_RISCV_EFI
> +END(xen_start)
> +#endif
> +
>  END(start)

I'm not convinced it is a good idea to have two functions nested
within one another, ELF-annotation-wise.

Jan