diff mbox series

[v1,05/18] x86: refactor xen cmdline into general framework

Message ID 20220706210454.30096-6-dpsmith@apertussolutions.com (mailing list archive)
State New, archived
Headers show
Series Hyperlaunch | expand

Commit Message

Daniel P. Smith July 6, 2022, 9:04 p.m. UTC
This refactors xen cmdline processing into a general framework
under the new boot info abstraction.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Christopher Clark <christopher.clark@starlab.io>
---
 xen/arch/x86/include/asm/bootinfo.h | 49 ++++++++++++++++++++++++
 xen/arch/x86/setup.c                | 58 ++++-------------------------
 xen/include/xen/bootinfo.h          | 11 ++++++
 3 files changed, 68 insertions(+), 50 deletions(-)

Comments

Jan Beulich July 19, 2022, 1:26 p.m. UTC | #1
On 06.07.2022 23:04, Daniel P. Smith wrote:
> --- a/xen/include/xen/bootinfo.h
> +++ b/xen/include/xen/bootinfo.h
> @@ -53,6 +53,17 @@ struct __packed boot_info {
>  
>  extern struct boot_info *boot_info;
>  
> +static inline char *bootinfo_prepare_cmdline(struct boot_info *bi)
> +{
> +    bi->cmdline = arch_bootinfo_prepare_cmdline(bi->cmdline, bi->arch);
> +
> +    if ( *bi->cmdline == ' ' )
> +        printk(XENLOG_WARNING "%s: leading whitespace left on cmdline\n",
> +               __func__);

Just a remark and a question on this one: I don't view the use of
__func__ here (and in fact in many other cases as well) as very
useful. And why do we need such a warning all of the sudden in the
first place?

Jan
Daniel P. Smith July 22, 2022, 1:12 p.m. UTC | #2
On 7/19/22 09:26, Jan Beulich wrote:
> On 06.07.2022 23:04, Daniel P. Smith wrote:
>> --- a/xen/include/xen/bootinfo.h
>> +++ b/xen/include/xen/bootinfo.h
>> @@ -53,6 +53,17 @@ struct __packed boot_info {
>>  
>>  extern struct boot_info *boot_info;
>>  
>> +static inline char *bootinfo_prepare_cmdline(struct boot_info *bi)
>> +{
>> +    bi->cmdline = arch_bootinfo_prepare_cmdline(bi->cmdline, bi->arch);
>> +
>> +    if ( *bi->cmdline == ' ' )
>> +        printk(XENLOG_WARNING "%s: leading whitespace left on cmdline\n",
>> +               __func__);
> 
> Just a remark and a question on this one: I don't view the use of
> __func__ here (and in fact in many other cases as well) as very
> useful. And why do we need such a warning all of the sudden in the
> first place?

This started as just a debug print, thus why __func__ is in place, but
later decided to leave it. This is because after this point, the code
assumes that all leading space was stripped, but there was never a check
that logic did the job correct. I don't believe a failure to do so
warranted a halt to the boot process, but at least provide a warning
into the log should the trimming fail. Doing so would allow an admin to
have a clue should an unexpected behavior occur as a result of leading
space making it through and breaking the fully trimmed assumption made
elsewhere.

v/r,
dps
Jan Beulich July 25, 2022, 7:09 a.m. UTC | #3
On 22.07.2022 15:12, Daniel P. Smith wrote:
> On 7/19/22 09:26, Jan Beulich wrote:
>> On 06.07.2022 23:04, Daniel P. Smith wrote:
>>> --- a/xen/include/xen/bootinfo.h
>>> +++ b/xen/include/xen/bootinfo.h
>>> @@ -53,6 +53,17 @@ struct __packed boot_info {
>>>  
>>>  extern struct boot_info *boot_info;
>>>  
>>> +static inline char *bootinfo_prepare_cmdline(struct boot_info *bi)
>>> +{
>>> +    bi->cmdline = arch_bootinfo_prepare_cmdline(bi->cmdline, bi->arch);
>>> +
>>> +    if ( *bi->cmdline == ' ' )
>>> +        printk(XENLOG_WARNING "%s: leading whitespace left on cmdline\n",
>>> +               __func__);
>>
>> Just a remark and a question on this one: I don't view the use of
>> __func__ here (and in fact in many other cases as well) as very
>> useful. And why do we need such a warning all of the sudden in the
>> first place?
> 
> This started as just a debug print, thus why __func__ is in place, but
> later decided to leave it. This is because after this point, the code
> assumes that all leading space was stripped, but there was never a check
> that logic did the job correct. I don't believe a failure to do so
> warranted a halt to the boot process, but at least provide a warning
> into the log should the trimming fail. Doing so would allow an admin to
> have a clue should an unexpected behavior occur as a result of leading
> space making it through and breaking the fully trimmed assumption made
> elsewhere.

All fine, but then such a change wants doing on its own, not in the middle
of pretty involved refactoring work.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index e5135e402b..2fcd576023 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -45,4 +45,53 @@  struct __packed mb_memmap {
     uint32_t type;
 };
 
+static inline bool loader_is_grub2(const char *loader_name)
+{
+    /* GRUB1="GNU GRUB 0.xx"; GRUB2="GRUB 1.xx" */
+    const char *p = strstr(loader_name, "GRUB ");
+    return (p != NULL) && (p[5] != '0');
+}
+
+static inline char *arch_prepare_cmdline(char *p, struct arch_boot_info *arch)
+{
+    p = p ? : "";
+
+    /* Strip leading whitespace. */
+    while ( *p == ' ' )
+        p++;
+
+    /* GRUB2 and PVH don't not include image name as first item on command line. */
+    if ( !(arch->xenguest || loader_is_grub2(arch->boot_loader_name)) )
+    {
+        /* Strip image name plus whitespace. */
+        while ( (*p != ' ') && (*p != '\0') )
+            p++;
+        while ( *p == ' ' )
+            p++;
+    }
+
+    return p;
+}
+
+static inline char *arch_bootinfo_prepare_cmdline(
+    char *cmdline, struct arch_boot_info *arch)
+{
+    if ( !cmdline )
+        return "";
+
+    if ( (arch->kextra = strstr(cmdline, " -- ")) != NULL )
+    {
+        /*
+         * Options after ' -- ' separator belong to dom0.
+         *  1. Orphan dom0's options from Xen's command line.
+         *  2. Skip all but final leading space from dom0's options.
+         */
+        *arch->kextra = '\0';
+        arch->kextra += 3;
+        while ( arch->kextra[1] == ' ' ) arch->kextra++;
+    }
+
+
+    return arch_prepare_cmdline(cmdline, arch);
+}
 #endif
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index ad37f4a658..e4060d6219 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -716,34 +716,6 @@  ignore_param("edid");
  */
 ignore_param("placeholder");
 
-static bool __init loader_is_grub2(const char *loader_name)
-{
-    /* GRUB1="GNU GRUB 0.xx"; GRUB2="GRUB 1.xx" */
-    const char *p = strstr(loader_name, "GRUB ");
-    return (p != NULL) && (p[5] != '0');
-}
-
-static char * __init cmdline_cook(char *p, const char *loader_name)
-{
-    p = p ? : "";
-
-    /* Strip leading whitespace. */
-    while ( *p == ' ' )
-        p++;
-
-    /* GRUB2 and PVH don't not include image name as first item on command line. */
-    if ( xen_guest || loader_is_grub2(loader_name) )
-        return p;
-
-    /* Strip image name plus whitespace. */
-    while ( (*p != ' ') && (*p != '\0') )
-        p++;
-    while ( *p == ' ' )
-        p++;
-
-    return p;
-}
-
 static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int limit)
 {
     unsigned int n = min(bootsym(bios_e820nr), limit);
@@ -754,8 +726,7 @@  static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
     return n;
 }
 
-static struct domain *__init create_dom0(
-    const struct boot_info *bi, const char *kextra, const char *loader)
+static struct domain *__init create_dom0(const struct boot_info *bi)
 {
     struct xen_domctl_createdomain dom0_cfg = {
         .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
@@ -804,16 +775,16 @@  static struct domain *__init create_dom0(
     /* Grab the DOM0 command line. */
     cmdline = (image->string.kind == BOOTSTR_CMDLINE) ?
               image->string.bytes : NULL;
-    if ( cmdline || kextra )
+    if ( cmdline || bi->arch->kextra )
     {
         static char __initdata dom0_cmdline[MAX_GUEST_CMDLINE];
 
-        cmdline = cmdline_cook(cmdline, loader);
+        cmdline = arch_prepare_cmdline(cmdline, bi->arch);
         safe_strcpy(dom0_cmdline, cmdline);
 
-        if ( kextra )
+        if ( bi->arch->kextra )
             /* kextra always includes exactly one leading space. */
-            safe_strcat(dom0_cmdline, kextra);
+            safe_strcat(dom0_cmdline, bi->arch->kextra);
 
         /* Append any extra parameters. */
         if ( skip_ioapic_setup && !strstr(dom0_cmdline, "noapic") )
@@ -861,7 +832,7 @@  static struct domain *__init create_dom0(
 void __init noreturn __start_xen(unsigned long bi_p)
 {
     char *memmap_type = NULL;
-    char *cmdline, *kextra, *loader;
+    char *cmdline, *loader;
     void *bsp_stack;
     struct cpu_info *info = get_cpu_info(), *bsp_info;
     unsigned int initrdidx, num_parked = 0;
@@ -929,20 +900,7 @@  void __init noreturn __start_xen(unsigned long bi_p)
         ? boot_info->arch->boot_loader_name : "unknown";
 
     /* Parse the command-line options. */
-    cmdline = cmdline_cook((boot_info->arch->flags & BOOTINFO_FLAG_X86_CMDLINE) ?
-                            boot_info->cmdline : NULL,
-                           loader);
-    if ( (kextra = strstr(cmdline, " -- ")) != NULL )
-    {
-        /*
-         * Options after ' -- ' separator belong to dom0.
-         *  1. Orphan dom0's options from Xen's command line.
-         *  2. Skip all but final leading space from dom0's options.
-         */
-        *kextra = '\0';
-        kextra += 3;
-        while ( kextra[1] == ' ' ) kextra++;
-    }
+    cmdline = bootinfo_prepare_cmdline(boot_info);
     cmdline_parse(cmdline);
 
     /* Must be after command line argument parsing and before
@@ -1951,7 +1909,7 @@  void __init noreturn __start_xen(unsigned long bi_p)
      * We're going to setup domain0 using the module(s) that we stashed safely
      * above our heap. The second module, if present, is an initrd ramdisk.
      */
-    dom0 = create_dom0(boot_info, kextra, loader);
+    dom0 = create_dom0(boot_info);
     if ( !dom0 )
         panic("Could not set up DOM0 guest OS\n");
 
diff --git a/xen/include/xen/bootinfo.h b/xen/include/xen/bootinfo.h
index dde8202f62..477294dc10 100644
--- a/xen/include/xen/bootinfo.h
+++ b/xen/include/xen/bootinfo.h
@@ -53,6 +53,17 @@  struct __packed boot_info {
 
 extern struct boot_info *boot_info;
 
+static inline char *bootinfo_prepare_cmdline(struct boot_info *bi)
+{
+    bi->cmdline = arch_bootinfo_prepare_cmdline(bi->cmdline, bi->arch);
+
+    if ( *bi->cmdline == ' ' )
+        printk(XENLOG_WARNING "%s: leading whitespace left on cmdline\n",
+               __func__);
+
+    return bi->cmdline;
+}
+
 static inline unsigned long bootmodule_next_idx_by_kind(
     const struct boot_info *bi, bootmodule_kind kind, unsigned long start)
 {