Re: [PATCH v3 1/3] hw/loongarch/boot: Refactor EFI booting protocol generation

[Philippe Mathieu-Daudé]

Hi,

On 24/12/24 15:24, Jiaxun Yang wrote:
> Refector EFI style booting data structure generation to
> support 32bit EFI variant on LoongArch32 CPU.
>
> All data structs are filled with padding members if necessary
> and marked as QEMU_PACKED to avoid host ABI alignment impact.
>
> Host endian is being cared as well.
>
> It also fixed various problems in old implementation such
> as null pointer on empty string, memory desc map_size not set,
> incorrect memory map definition and so on.
>
> Reviewed-by: Song Gao <gaosong@loongson.cn>
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>   hw/loongarch/boot.c         | 220 ++++++++++++++++++++++++++++++--------------
>   include/hw/loongarch/boot.h | 106 +++++++++++++++++----
>   2 files changed, 238 insertions(+), 88 deletions(-)


> +#define EFI_BOOT_MEMMAP_TABLE_GEN(type)                                     \
> +static void init_efi_boot_memmap_##type(void *guidp, void **p)              \
> +{                                                                           \
> +    struct efi_boot_memmap_##type *boot_memmap = *p;                        \
> +    efi_guid_t tbl_guid = LINUX_EFI_BOOT_MEMMAP_GUID;                       \
> +                                                                            \
> +    /* efi_configuration_table 1 */                                         \
> +    guidcpy(guidp, &tbl_guid);                                              \
> +                                                                            \
> +    boot_memmap->desc_size = cpu_to_le##type(sizeof(efi_memory_desc_t));    \

I'd prefer we avoid macros and use the ld/stN API, passing the
size as argument:

       stn_le_p(&boot_memmap->desc_size, size,
                sizeof(efi_memory_desc_t));

Ideally splitting the patch in 3, first convert to use the API
as stn_le_p(ptr, 8, value), then second patch propagate the size,
and the third add the size=4 case.

> +    boot_memmap->desc_ver = cpu_to_le32(1);                                 \
> +    boot_memmap->map_size = cpu_to_le##type(boot_memmap->desc_size *        \
> +                                            memmap_entries);                \
> +    memmap_write_descs(boot_memmap->map);                                   \
> +    *p += sizeof(struct efi_boot_memmap_##type);                            \
> +}
>   
> -    init_efi_boot_memmap(systab, p, start);
> -    p += ROUND_UP(sizeof(struct efi_boot_memmap) +
> -                  sizeof(efi_memory_desc_t) * memmap_entries, 64 * KiB);
> -    init_efi_initrd_table(systab, p, start);
> -    p += ROUND_UP(sizeof(struct efi_initrd), 64 * KiB);
> -    init_efi_fdt_table(systab);
> +#define EFI_INITRD_TABLE_GEN(type)                                          \
> +static void init_efi_initrd_table_##type(void *guidp, void **p)             \
> +{                                                                           \
> +    efi_guid_t tbl_guid = LINUX_EFI_INITRD_MEDIA_GUID;                      \
> +    struct efi_initrd_##type *initrd_table = *p;                            \
> +                                                                            \
> +    /* efi_configuration_table  */                                          \
> +    guidcpy(guidp, &tbl_guid);                                              \
> +                                                                            \
> +    initrd_table->base = cpu_to_le##type(initrd_offset);                    \
> +    initrd_table->size = cpu_to_le##type(initrd_size);                      \
> +    *p += sizeof(struct efi_initrd_##type);                                 \
> +}


> +EFI_INIT_SYSTAB_GEN(32)
> +EFI_INIT_SYSTAB_GEN(64)


> -static void init_boot_rom(struct loongarch_boot_info *info, void *p)
> +static void init_boot_rom(struct loongarch_boot_info *info, void *p,
> +                          bool is_64bit)
>   {
>       void *start = p;
>   
>       init_cmdline(info, p, start);
>       p += COMMAND_LINE_SIZE;
>   
> -    init_systab(info, p, start);
> +    if (is_64bit)
> +        init_systab_64(info, p, start);
> +    else
> +        init_systab_32(info, p, start);
>   }