Re: [edk2-devel] [PATCH v1 6/6] UefiPayloadPkg: LinuxBoot: use a text format for the configuration block.

["Guo Dong" <guo.dong@intel.com>]

Please run BaseTools\Scripts\PatchCheck.py to make sure it passed the check.

Update LbParseLib.c following https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/.
current code is more like Linux style. E.g.
+static uint64_t parse_int(const char* s, char** end) {
+  UINT64 x;

Removed unused comments
+//#include <string.h>
+//#include <ctype.h>


Thanks,
Guo
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Cheng-Chieh Huang via groups.io
Sent: Wednesday, July 21, 2021 6:23 AM
To: devel@edk2.groups.io
Cc: Trammell Hudson <hudson@trmm.net>; Cheng-Chieh Huang <chengchieh@google.com>
Subject: [edk2-devel] [PATCH v1 6/6] UefiPayloadPkg: LinuxBoot: use a text format for the configuration block.

From: Trammell Hudson <hudson@trmm.net>

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 <chengchieh@google.com>
---
 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.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
-
 #include <IndustryStandard/Acpi.h>
 #include <IndustryStandard/SmBios.h>
 #include <Library/BaseLib.h>
@@ -18,17 +17,42 @@
 #include <Library/PcdLib.h>
 #include <Linuxboot.h>
 #include <Uefi/UefiBaseType.h>
+#include <stdint.h>
+#include <stdlib.h>
+//#include <string.h>
+//#include <ctype.h>
+
+#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];
+      }
+    }
+  }
 
   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;
 }
--
2.32.0.402.g57bb445576-goog