From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web11.1771.1626887369524973935 for ; Wed, 21 Jul 2021 10:09:30 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=HpBiWWS6; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout02.posteo.de (Postfix) with ESMTPS id 6B43C240104 for ; Wed, 21 Jul 2021 19:09:27 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1626887367; bh=dS9hPsPl0Z6WajwfIceYMhGMdUZ79MUCOKQfg8dyNbw=; h=Subject:To:Cc:From:Date:From; b=HpBiWWS616X3NA5L06vzZyTGsgoDtCGp+cowsm7xlRjukF/WggaYASMF50Wn6od64 ySYWV83H/+C+6QHiQMIaS1nGaYlhFruHfBd96/XoZ4eMTg3UIJqXXK0sdaYVjqcOED g9q7IJi1rsdfcsPf1Jm7I9/kuET8CJG65PP7GkqE6ksI21VOzBQgml0w1Z5a7TK8wW nnSxxSgR74l7vva/1AoFGbAI9+8ZwLKZaf3kONq5ApyUTWg+/2UkmTqoTDBAtnmWzo oyW/0BxNqfLC952ZDo9osiXPkqv5rAKsGKK4osfyBzXhoLHQBhj+L6D1pGi2InB7J1 VttRnSxZD0YKw== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4GVMWV2s59z9rxF; Wed, 21 Jul 2021 19:09:26 +0200 (CEST) Subject: Re: [edk2-devel] [PATCH v1 6/6] UefiPayloadPkg: LinuxBoot: use a text format for the configuration block. To: devel@edk2.groups.io, chengchieh@google.com Cc: Trammell Hudson References: <20210721132328.1415485-1-chengchieh@google.com> <20210721132328.1415485-7-chengchieh@google.com> From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-ID: <6c7c62de-3af2-652f-a252-7e96193b4c58@posteo.de> Date: Wed, 21 Jul 2021 17:09:25 +0000 MIME-Version: 1.0 In-Reply-To: <20210721132328.1415485-7-chengchieh@google.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-GB On 21.07.21 15:23, Cheng-Chieh Huang via groups.io wrote: > From: Trammell Hudson > > This adds a text command line for the UefiPayloadPkg that uses > a textual magic number 'LnxBoot1' and a series of white-space > separated key=value[,value...] pairs for the parameters. > > The v1 binary configuration structure is used if it is present instead > for backwards compatability. > > Currently supported text command line arguments are are: > > * `serial=baud,base,width,type,hz,pci` > (only `baud` needs to be specified, the rest have reasonable > defaults) > > * `mem=start,end,type` > Types are based on `/sys/firmware/memmaps/*/types`: > 1 == "System RAM" > 3 == "ACPI Tables" > 4 == "ACPI Non-volatile Storage" > 5 == "Reserved" > > * `ACPI20=base` pointer to RDSP (from `/sys/firmware/efi/systab`) > > * `SMBIOS=base` pointer to the SMBIOS table (also from the EFI table) > > Signed-off-by: Cheng-Chieh Huang > --- > UefiPayloadPkg/Include/Linuxboot.h | 17 +- > UefiPayloadPkg/Library/LbParseLib/LbParseLib.c | 252 +++++++++++++++++--- > 2 files changed, 230 insertions(+), 39 deletions(-) > > diff --git a/UefiPayloadPkg/Include/Linuxboot.h b/UefiPayloadPkg/Include/Linuxboot.h > index 34ca18069983..56b39b5a09ff 100644 > --- a/UefiPayloadPkg/Include/Linuxboot.h > +++ b/UefiPayloadPkg/Include/Linuxboot.h > @@ -24,8 +24,7 @@ typedef struct MemoryMapEntryStruct { > UINT32 Type; > } MemoryMapEntry; > > -typedef struct UefiPayloadConfigStruct { > - UINT64 Version; > +typedef struct { > UINT64 AcpiBase; > UINT64 AcpiSize; > UINT64 SmbiosBase; > @@ -33,10 +32,22 @@ typedef struct UefiPayloadConfigStruct { > SerialPortConfig SerialConfig; > UINT32 NumMemoryMapEntries; > MemoryMapEntry MemoryMapEntries[0]; > +} UefiPayloadConfigV1; > + > +typedef struct UefiPayloadConfigStruct { > + UINT64 Version; > + union { > + UefiPayloadConfigV1 v1; > + struct { > + char cmdline[0]; // up to 64 KB > + } v2; > + } config; > } UefiPayloadConfig; > #pragma pack() > > -#define UEFI_PAYLOAD_CONFIG_VERSION 1 > +// magic version config is "LnxBoot1" > +#define UEFI_PAYLOAD_CONFIG_VERSION1 1 > +#define UEFI_PAYLOAD_CONFIG_VERSION2 0x31746f6f42786e4cULL > > #define LINUXBOOT_MEM_RAM 1 > #define LINUXBOOT_MEM_DEFAULT 2 > diff --git a/UefiPayloadPkg/Library/LbParseLib/LbParseLib.c b/UefiPayloadPkg/Library/LbParseLib/LbParseLib.c > index 34bfb6a1073f..5e68091cac91 100644 > --- a/UefiPayloadPkg/Library/LbParseLib/LbParseLib.c > +++ b/UefiPayloadPkg/Library/LbParseLib/LbParseLib.c > @@ -1,13 +1,12 @@ > /** @file > - This library will parse the linuxboot table in memory and extract those required > - information. > + This library will parse the linuxboot table in memory and extract those > +required information. > > Copyright (c) 2021, the u-root Authors. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > > - > #include > #include > #include > @@ -18,17 +17,42 @@ > #include > #include > #include > +#include > +#include > +//#include > +//#include > + > +#define strncmp(a, b, n) AsciiStrnCmp((a), (b), (n)) > + > +static uint64_t parse_int(const char* s, char** end) { > + UINT64 x; > + > + if (s[0] == '0' && s[1] == 'x') > + AsciiStrHexToUint64S(s, end, &x); > + else > + AsciiStrDecimalToUint64S(s, end, &x); > + > + return x; > +} > + > +static int isspace(const char c) { return c == ' ' || c == '\t' || c == '\n'; } > > // Retrieve UefiPayloadConfig from Linuxboot's uefiboot > -UefiPayloadConfig* GetUefiPayLoadConfig() { > - UefiPayloadConfig* config = > +const UefiPayloadConfig* GetUefiPayLoadConfig() { > + const UefiPayloadConfig* config = > (UefiPayloadConfig*)(UINTN)(PcdGet32(PcdPayloadFdMemBase) - SIZE_64KB); > - if (config->Version != UEFI_PAYLOAD_CONFIG_VERSION) { > - DEBUG((DEBUG_ERROR, "Expect payload config version: %d, but get %d\n", > - UEFI_PAYLOAD_CONFIG_VERSION, config->Version)); > - CpuDeadLoop (); > - } > - return config; > + > + if (config->Version == UEFI_PAYLOAD_CONFIG_VERSION1 || > + config->Version == UEFI_PAYLOAD_CONFIG_VERSION2) > + return config; > + > + DEBUG((DEBUG_ERROR, > + "Expect payload config version %016lx or %016lx, but get %016lx\n", > + UEFI_PAYLOAD_CONFIG_VERSION1, UEFI_PAYLOAD_CONFIG_VERSION2, > + config->Version)); > + CpuDeadLoop(); > + while (1) > + ; > } > > // Align the address and add memory rang to MemInfoCallback > @@ -54,6 +78,57 @@ void AddMemoryRange(IN BL_MEM_INFO_CALLBACK MemInfoCallback, IN UINTN start, > MemInfoCallback(&MemoryMap, NULL); > } > > +const char* cmdline_next(const char* cmdline, const char** option) { > + // at the end of the string, we're done > + if (!cmdline || *cmdline == '\0') return NULL; > + > + // skip any leading whitespace > + while (isspace(*cmdline)) cmdline++; > + > + // if we've hit the end of the string, we're done > + if (*cmdline == '\0') return NULL; > + > + *option = cmdline; > + > + // find the end of this option or the string > + while (!isspace(*cmdline) && *cmdline != '\0') cmdline++; > + > + // cmdline points to the whitespace or end of string > + return cmdline; > +} > + > +int cmdline_ints(const char* option, uint64_t args[], int max) { > + // skip any leading text up to an '=' > + const char* s = option; > + while (1) { > + const char c = *s++; > + if (c == '=') break; > + > + if (c == '\0' || isspace(c)) { > + s = option; > + break; > + } > + } > + > + for (int i = 0; i < max; i++) { > + char* end; > + args[i] = parse_int(s, &end); > + > + // end of string or end of the option? > + if (*end == '\0' || isspace(*end)) return i + 1; > + > + // not separator? signal an error if we have consumed any ints, > + // otherwise return 0 saying that none were found > + if (*end != ',') return i == 0 ? 0 : -1; > + > + // skip the , and get the next value > + s = end + 1; > + } > + > + // too many values! > + return -1; > +} > + > /** > Acquire the memory information from the linuxboot table in memory. > > @@ -67,20 +142,50 @@ void AddMemoryRange(IN BL_MEM_INFO_CALLBACK MemInfoCallback, IN UINTN start, > RETURN_STATUS > EFIAPI > ParseMemoryInfo(IN BL_MEM_INFO_CALLBACK MemInfoCallback, IN VOID* Params) { > - UefiPayloadConfig* config; > - int i; > + const UefiPayloadConfig* config = GetUefiPayLoadConfig(); > + if (!config) { > + DEBUG( > + (DEBUG_ERROR, "ParseMemoryInfo: Could not find UEFI Payload config\n")); > + return RETURN_SUCCESS; > + } > + > + if (config->Version == UEFI_PAYLOAD_CONFIG_VERSION1) { > + const UefiPayloadConfigV1* config1 = &config->config.v1; > + DEBUG( > + (DEBUG_INFO, "MemoryMap #entries: %d\n", config1->NumMemoryMapEntries)); > + > + for (int i = 0; i < config1->NumMemoryMapEntries; i++) { > + const MemoryMapEntry* entry = &config1->MemoryMapEntries[i]; > + DEBUG((DEBUG_INFO, "Start: 0x%lx End: 0x%lx Type:%d\n", entry->Start, > + entry->End, entry->Type)); > + AddMemoryRange(MemInfoCallback, entry->Start, entry->End, entry->Type); > + } > + } else > > - config = GetUefiPayLoadConfig(); > + if (config->Version == UEFI_PAYLOAD_CONFIG_VERSION2) { > + const char* cmdline = config->config.v2.cmdline; > + const char* option; > + uint64_t args[3]; > > - DEBUG((DEBUG_INFO, "MemoryMap #entries: %d\n", config->NumMemoryMapEntries)); > + // look for the mem=start,end,type > + while ((cmdline = cmdline_next(cmdline, &option))) { > + if (strncmp(option, "mem=", 4) != 0) continue; > > - MemoryMapEntry* entry = &config->MemoryMapEntries[0]; > - for (i = 0; i < config->NumMemoryMapEntries; i++) { > - DEBUG((DEBUG_INFO, "Start: 0x%lx End: 0x%lx Type:%d\n", entry->Start, > - entry->End, entry->Type)); > - AddMemoryRange(MemInfoCallback, entry->Start, entry->End, entry->Type); > - entry++; > + if (cmdline_ints(option, args, 3) != 3) { > + DEBUG((DEBUG_ERROR, "Parse error: '%a'\n", option)); > + continue; > + } > + > + const uint64_t start = args[0]; > + const uint64_t end = args[1]; > + const uint64_t type = args[2]; > + > + DEBUG( > + (DEBUG_INFO, "Start: 0x%lx End: 0x%lx Type:%d\n", start, end, type)); > + AddMemoryRange(MemInfoCallback, start, end, type); > + } > } > + > return RETURN_SUCCESS; > } > > @@ -96,14 +201,52 @@ ParseMemoryInfo(IN BL_MEM_INFO_CALLBACK MemInfoCallback, IN VOID* Params) { > RETURN_STATUS > EFIAPI > ParseSystemTable(OUT SYSTEM_TABLE_INFO* SystemTableInfo) { > - UefiPayloadConfig* config; > + const UefiPayloadConfig* config = GetUefiPayLoadConfig(); > + if (!config) { > + DEBUG((DEBUG_ERROR, > + "ParseSystemTable: Could not find UEFI Payload config\n")); > + return RETURN_SUCCESS; > + } > > - config = GetUefiPayLoadConfig(); > - SystemTableInfo->AcpiTableBase = config->AcpiBase; > - SystemTableInfo->AcpiTableSize = config->AcpiSize; > + if (config->Version == UEFI_PAYLOAD_CONFIG_VERSION1) { > + const UefiPayloadConfigV1* config1 = &config->config.v1; > + SystemTableInfo->AcpiTableBase = config1->AcpiBase; > + SystemTableInfo->AcpiTableSize = config1->AcpiSize; > > - SystemTableInfo->SmbiosTableBase = config->SmbiosBase; > - SystemTableInfo->SmbiosTableSize = config->SmbiosSize; > + SystemTableInfo->SmbiosTableBase = config1->SmbiosBase; > + SystemTableInfo->SmbiosTableSize = config1->SmbiosSize; > + } else > + > + if (config->Version == UEFI_PAYLOAD_CONFIG_VERSION2) { > + const char* cmdline = config->config.v2.cmdline; > + const char* option; > + uint64_t args[2]; > + > + // look for the acpi config > + while ((cmdline = cmdline_next(cmdline, &option))) { > + if (strncmp(option, "ACPI20=", 7) == 0) { > + const int count = cmdline_ints(option, args, 2); > + if (count < 0) { > + DEBUG((DEBUG_ERROR, "Parse error: '%a'\n", option)); > + continue; > + } > + > + if (count > 0) SystemTableInfo->AcpiTableBase = args[0]; > + if (count > 1) SystemTableInfo->AcpiTableSize = args[1]; > + } > + > + if (strncmp(option, "SMBIOS=", 7) == 0) { > + const int count = cmdline_ints(option, args, 2); > + if (count < 0) { > + DEBUG((DEBUG_ERROR, "Parse error: '%a'\n", option)); > + continue; > + } > + > + if (count > 0) SystemTableInfo->SmbiosTableBase = args[0]; > + if (count > 1) SystemTableInfo->SmbiosTableSize = args[1]; > + } Sorry, from only a quick peak, can this not leave {Acpi,Smbios}TableBase and {Acpi,Smbios}TableSize undefined entirely, while returning RETURN_SUCCESS? Even if not, it looks kind of odd a command-line argument can change the base address without changing the size, is there a documentation of this behaviour? What is the structure supposed to carry if no such arguments are given at all? Thanks for your time! Best regards, Marvin > + } > + } > > return RETURN_SUCCESS; > } > @@ -120,15 +263,52 @@ ParseSystemTable(OUT SYSTEM_TABLE_INFO* SystemTableInfo) { > RETURN_STATUS > EFIAPI > ParseSerialInfo(OUT SERIAL_PORT_INFO* SerialPortInfo) { > - UefiPayloadConfig* config; > - config = GetUefiPayLoadConfig(); > - > - SerialPortInfo->BaseAddr = config->SerialConfig.BaseAddr; > - SerialPortInfo->RegWidth = config->SerialConfig.RegWidth; > - SerialPortInfo->Type = config->SerialConfig.Type; > - SerialPortInfo->Baud = config->SerialConfig.Baud; > - SerialPortInfo->InputHertz = config->SerialConfig.InputHertz; > - SerialPortInfo->UartPciAddr = config->SerialConfig.UartPciAddr; > + // fill in some reasonable defaults > + SerialPortInfo->BaseAddr = 0x3f8; > + SerialPortInfo->RegWidth = 1; > + SerialPortInfo->Type = 1; // uefi.SerialPortTypeIO > + SerialPortInfo->Baud = 115200; > + SerialPortInfo->InputHertz = 1843200; > + SerialPortInfo->UartPciAddr = 0; > + > + const UefiPayloadConfig* config = GetUefiPayLoadConfig(); > + if (!config) { > + DEBUG((DEBUG_ERROR, "ParseSerialInfo: using default config\n")); > + return RETURN_SUCCESS; > + } > + > + if (config->Version == UEFI_PAYLOAD_CONFIG_VERSION1) { > + const UefiPayloadConfigV1* config1 = &config->config.v1; > + SerialPortInfo->BaseAddr = config1->SerialConfig.BaseAddr; > + SerialPortInfo->RegWidth = config1->SerialConfig.RegWidth; > + SerialPortInfo->Type = config1->SerialConfig.Type; > + SerialPortInfo->Baud = config1->SerialConfig.Baud; > + SerialPortInfo->InputHertz = config1->SerialConfig.InputHertz; > + SerialPortInfo->UartPciAddr = config1->SerialConfig.UartPciAddr; > + } else > + > + if (config->Version == UEFI_PAYLOAD_CONFIG_VERSION2) { > + const char* cmdline = config->config.v2.cmdline; > + const char* option; > + uint64_t args[6] = {}; > + > + while ((cmdline = cmdline_next(cmdline, &option))) { > + if (strncmp(option, "serial=", 7) != 0) continue; > + > + const int count = cmdline_ints(option, args, 6); > + if (count < 0) { > + DEBUG((DEBUG_ERROR, "Parse error: %a\n", option)); > + continue; > + } > + > + if (count > 0) SerialPortInfo->Baud = args[0]; > + if (count > 1) SerialPortInfo->BaseAddr = args[1]; > + if (count > 2) SerialPortInfo->RegWidth = args[2]; > + if (count > 3) SerialPortInfo->Type = args[3]; > + if (count > 4) SerialPortInfo->InputHertz = args[4]; > + if (count > 5) SerialPortInfo->UartPciAddr = args[5]; > + } > + } > > return RETURN_SUCCESS; > }