Hi Simon,
Thank you for your review. I submitted five commits as requested. I believe
they are in the queue of the moderator since I am not a member of the mailing
list.
Regards,
Jeremy
Simon Glass <s...@chromium.org> writes:
> Hi Jeremy,
>
> On Fri, 14 Mar 2025 at 22:59, Jeremy Compostella
> <jeremy.composte...@intel.com> wrote:
>>
>> This commit introduces a new configuration option X86_RUN_64BIT_NO_SPL
>> to allow building U-Boot as a 64-bit binary without using the SPL
>> (Secondary Program Loader). The motivation is to simplify the boot
>> process for specific x86-based platforms that do not require SPL, such
>> as those booting directly from a 64-bit coreboot firmware.
>>
>> Key changes include:
>>
>> - Updated arch/x86/Kconfig to add a new choice for 64-bit U-Boot without
>> SPL.
>>
>> - Modified logic in arch/x86/cpu/coreboot/coreboot.c and
>> arch/x86/lib/zimage.c to use CONFIG_X86_64 instead of
>> CONFIG_X86_RUN_64BIT for consistency.
>>
>> - Adjusted the arch/x86/cpu/coreboot/Kconfig to select BINMAN based on
>> the CONFIG_X86_RUN_64BIT configuration.
>>
>> - Created a new defconfig file configs/coreboot64-no-spl_defconfig to
>> provide default configurations for building U-Boot in this new
>> mode. This includes enabling 64-bit operation, setting appropriate
>> memory addresses, and specifying boot arguments and commands.
>>
>> - Updated include/config_distro_bootcmd.h to determine the correct boot
>> EFI file name based on the new configuration.
>
> I think this patch would be better as separate commits, where each
> does one thing as you have listed above.
>
> Also, please use imperative tense, i.e. 'Update xxx to ...'
>
> With those changes you can add:
>
> Reviewed-by: Simon Glass <s...@chromium.org>
>
> to each.
>
>>
>> These changes are aimed at enhancing the flexibility and compatibility
>> of U-Boot on x86 platforms, particularly for systems utilizing 64-bit
>> coreboot without the need for a separate SPL stage.
>>
>> TEST=Built and booted U-Boot using the new configuration option on a
>> coreboot-64-bit-supported device. Verified successful boot process and
>> execution of U-Boot in 64-bit mode without SPL.
>>
>> Signed-off-by: Jeremy Compostella <jeremy.composte...@intel.com>
>> ---
>> arch/x86/Kconfig | 8 +++-
>> arch/x86/cpu/coreboot/Kconfig | 2 +-
>> arch/x86/cpu/coreboot/coreboot.c | 2 +-
>> arch/x86/lib/zimage.c | 2 +-
>> configs/coreboot64-no-spl_defconfig | 62 +++++++++++++++++++++++++++++
>> include/config_distro_bootcmd.h | 2 +-
>> 6 files changed, 73 insertions(+), 5 deletions(-)
>> create mode 100644 configs/coreboot64-no-spl_defconfig
>
> Thanks for looking at this.
>
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index 006a59d6fa6..ecccad692e6 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -30,7 +30,7 @@ config X86_RUN_32BIT
>> arch_phys_memset() can be used for basic access to other memory.
>>
>> config X86_RUN_64BIT
>> - bool "64-bit"
>> + bool "32-bit SPL followed by 64-bit U-Boot"
>> select X86_64
>> select SPL if !EFI_APP
>> select SPL_SEPARATE_BSS if !EFI_APP
>> @@ -40,6 +40,12 @@ config X86_RUN_64BIT
>> runs through the 16-bit and 32-bit init, then switches to 64-bit
>> mode and jumps to U-Boot proper.
>>
>> +config X86_RUN_64BIT_NO_SPL
>> + bool "64-bit"
>> + select X86_64
>> + help
>> + Build U-Boot as a 64-bit binary without SPL.
>
> Can you expand the help a little? You can say that with this option,
> U-Boot must be entered in 64-bit mode directly, meaning that a
> previous phase must have done the CPU setup (MP, page tables, etc.).
>
>> +
>> endchoice
>>
>> config X86_64
>> diff --git a/arch/x86/cpu/coreboot/Kconfig b/arch/x86/cpu/coreboot/Kconfig
>> index 085302c0482..66f25533b97 100644
>> --- a/arch/x86/cpu/coreboot/Kconfig
>> +++ b/arch/x86/cpu/coreboot/Kconfig
>> @@ -26,7 +26,7 @@ config SYS_COREBOOT
>> imply CBMEM_CONSOLE
>> imply X86_TSC_READ_BASE
>> imply USE_PREBOOT
>> - select BINMAN if X86_64
>> + select BINMAN if X86_RUN_64BIT
>> select SYSINFO
>> imply SYSINFO_EXTRA
>>
>> diff --git a/arch/x86/cpu/coreboot/coreboot.c
>> b/arch/x86/cpu/coreboot/coreboot.c
>> index fa7430b436f..d0719d1a405 100644
>> --- a/arch/x86/cpu/coreboot/coreboot.c
>> +++ b/arch/x86/cpu/coreboot/coreboot.c
>> @@ -22,7 +22,7 @@ int arch_cpu_init(void)
>> {
>> int ret;
>>
>> - ret = IS_ENABLED(CONFIG_X86_RUN_64BIT) ? x86_cpu_reinit_f() :
>> + ret = IS_ENABLED(CONFIG_X86_64) ? x86_cpu_reinit_f() :
>> x86_cpu_init_f();
>
> This seems to change the behavior for the coreboot32->U-Boot
> SPL->U-Boot 64 case. But your change seems correct to me.
>
>> if (ret)
>> return ret;
>> diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c
>> index 2eece34a073..ca3bd713ff2 100644
>> --- a/arch/x86/lib/zimage.c
>> +++ b/arch/x86/lib/zimage.c
>> @@ -424,7 +424,7 @@ int zboot_go(void)
>>
>> entry = state.load_address;
>> image_64bit = false;
>> - if (IS_ENABLED(CONFIG_X86_RUN_64BIT) &&
>> + if (IS_ENABLED(CONFIG_X86_64) &&
>> (hdr->xloadflags & XLF_KERNEL_64)) {
>> image_64bit = true;
>> }
>> diff --git a/configs/coreboot64-no-spl_defconfig
>> b/configs/coreboot64-no-spl_defconfig
>> new file mode 100644
>> index 00000000000..dd07524560b
>> --- /dev/null
>> +++ b/configs/coreboot64-no-spl_defconfig
>> @@ -0,0 +1,62 @@
>> +CONFIG_X86=y
>> +CONFIG_TEXT_BASE=0x1110000
>> +CONFIG_SYS_MALLOC_LEN=0x2000000
>> +CONFIG_NR_DRAM_BANKS=8
>> +CONFIG_ENV_SIZE=0x1000
>> +CONFIG_DEFAULT_DEVICE_TREE="coreboot"
>> +CONFIG_PRE_CON_BUF_ADDR=0x100000
>> +CONFIG_X86_RUN_64BIT_NO_SPL=y
>> +CONFIG_VENDOR_COREBOOT=y
>> +CONFIG_TARGET_COREBOOT=y
>> +CONFIG_FIT=y
>> +CONFIG_FIT_SIGNATURE=y
>> +CONFIG_BOOTSTD_FULL=y
>> +CONFIG_BOOTSTD_DEFAULTS=y
>> +CONFIG_SYS_MONITOR_BASE=0x01110000
>> +CONFIG_SHOW_BOOT_PROGRESS=y
>> +CONFIG_USE_BOOTARGS=y
>> +CONFIG_BOOTARGS="root=/dev/sdb3 init=/sbin/init rootwait ro"
>> +CONFIG_BOOTCOMMAND="bootflow scan -l; if bootflow menu; then bootflow boot;
>> fi"
>> +CONFIG_PRE_CONSOLE_BUFFER=y
>> +CONFIG_SYS_CONSOLE_INFO_QUIET=y
>> +CONFIG_LOG=y
>> +CONFIG_LOGF_LINE=y
>> +CONFIG_LOGF_FUNC=y
>> +CONFIG_DISPLAY_BOARDINFO_LATE=y
>> +CONFIG_CMD_IDE=y
>> +CONFIG_CMD_MMC=y
>> +CONFIG_CMD_SATA=y
>> +CONFIG_CMD_USB=y
>> +# CONFIG_CMD_SETEXPR is not set
>> +CONFIG_BOOTP_BOOTFILESIZE=y
>> +CONFIG_CMD_TIME=y
>> +CONFIG_CMD_SOUND=y
>> +CONFIG_CMD_EXT4_WRITE=y
>> +CONFIG_MAC_PARTITION=y
>> +CONFIG_ENV_OVERWRITE=y
>> +CONFIG_SYS_RELOC_GD_ENV_ADDR=y
>> +CONFIG_USE_BOOTFILE=y
>> +CONFIG_BOOTFILE="bzImage"
>> +CONFIG_TFTP_TSIZE=y
>> +CONFIG_USE_ROOTPATH=y
>> +CONFIG_REGMAP=y
>> +CONFIG_SYSCON=y
>> +# CONFIG_ACPIGEN is not set
>> +CONFIG_SYS_IDE_MAXDEVICE=4
>> +CONFIG_SYS_ATA_DATA_OFFSET=0
>> +CONFIG_SYS_ATA_REG_OFFSET=0
>> +CONFIG_SYS_ATA_ALT_OFFSET=0
>> +CONFIG_ATAPI=y
>> +CONFIG_LBA48=y
>> +CONFIG_SYS_64BIT_LBA=y
>> +CONFIG_NVME_PCI=y
>> +# CONFIG_PCI_PNP is not set
>> +CONFIG_SYS_NS16550_MEM32=y
>> +CONFIG_SOUND=y
>> +CONFIG_SOUND_I8254=y
>> +CONFIG_VIDEO_COPY=y
>> +CONFIG_CONSOLE_TRUETYPE=y
>> +CONFIG_CONSOLE_SCROLL_LINES=5
>> +CONFIG_CMD_DHRYSTONE=y
>> +# CONFIG_GZIP is not set
>> +CONFIG_SMBIOS_PARSER=y
>> diff --git a/include/config_distro_bootcmd.h
>> b/include/config_distro_bootcmd.h
>> index 0a4e4b8ff85..8ac3a4feeb8 100644
>> --- a/include/config_distro_bootcmd.h
>> +++ b/include/config_distro_bootcmd.h
>> @@ -112,7 +112,7 @@
>> #define BOOTEFI_NAME "bootarm.efi"
>> #elif defined(CONFIG_X86_RUN_32BIT)
>> #define BOOTEFI_NAME "bootia32.efi"
>> -#elif defined(CONFIG_X86_RUN_64BIT)
>> +#elif defined(CONFIG_X86_64)
>> #define BOOTEFI_NAME "bootx64.efi"
>> #elif defined(CONFIG_ARCH_RV32I)
>> #define BOOTEFI_NAME "bootriscv32.efi"
>> --
>> 2.48.1
>>
>
> We don't use the distro scripts anymore on x86, but this change seems fine.
>
> Regards,
> Simon
--
Jeremy
One Emacs to rule them all
