[v4,3/3] cmdline: parse multiple instances of the vga option

Commit Message

Roger Pau Monné July 5, 2023, 11:47 a.m. UTC

Parse all instances of the vga= option on the command line, in order
to always enforce the last selection on the command line.

Note that it's not safe to parse just the last occurrence of the vga=
option, as then a command line with `vga=current vga=keep` would
result in current being ignored.

Adjust the command line documentation to describe the new behavior.

Signed-off-by: Roger Pau Monné <[e-mail]>
---
Changes since v3:
 - Remove xen-command-line doc addition.
 - Set mode to 'ask' by default.

Changes since v2:
 - New in this version.
---
Build tested only, as I don't have a system that does legacy boot and
has VGA output I can check.

It's mostly encapsulating the current code inside of a while loop and
adding an extra else if for the "ask" option, there's a lot of
indentation changes.
---
 xen/arch/x86/boot/cmdline.c | 83 +++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 41 deletions(-)

Comments

Jan Beulich July 6, 2023, 10:45 a.m. UTC | #1

On 05.07.2023 13:47, Roger Pau Monne wrote:
> Parse all instances of the vga= option on the command line, in order
> to always enforce the last selection on the command line.
> 
> Note that it's not safe to parse just the last occurrence of the vga=
> option, as then a command line with `vga=current vga=keep` would
> result in current being ignored.
> 
> Adjust the command line documentation to describe the new behavior.

This was likely meant to be dropped along with ...

> Signed-off-by: Roger Pau Monné <[e-mail]>
> ---
> Changes since v3:
>  - Remove xen-command-line doc addition.

... this? Beyond that (easy to adjust while committing)
Reviewed-by: Jan Beulich <[e-mail]>

Jan

Roger Pau Monné July 7, 2023, 10:13 a.m. UTC | #2

On Thu, Jul 06, 2023 at 12:45:41PM +0200, Jan Beulich wrote:
> On 05.07.2023 13:47, Roger Pau Monne wrote:
> > Parse all instances of the vga= option on the command line, in order
> > to always enforce the last selection on the command line.
> > 
> > Note that it's not safe to parse just the last occurrence of the vga=
> > option, as then a command line with `vga=current vga=keep` would
> > result in current being ignored.
> > 
> > Adjust the command line documentation to describe the new behavior.
> 
> This was likely meant to be dropped along with ...
> 
> > Signed-off-by: Roger Pau Monné <[e-mail]>
> > ---
> > Changes since v3:
> >  - Remove xen-command-line doc addition.
> 
> ... this? Beyond that (easy to adjust while committing)
> Reviewed-by: Jan Beulich <[e-mail]>

Oh, indeed, I've added the changelog and didn't adjust the commit
message.

Thanks, Roger.

Patch

diff --git a/xen/arch/x86/boot/cmdline.c b/xen/arch/x86/boot/cmdline.c
index fc11c6d3c5c4..10dcc6142c85 100644
--- a/xen/arch/x86/boot/cmdline.c
+++ b/xen/arch/x86/boot/cmdline.c
@@ -277,59 +277,60 @@  static u16 rows2vmode(unsigned int rows)
 
 static void vga_parse(const char *cmdline, early_boot_opts_t *ebo)
 {
-    const char *c;
-    unsigned int tmp, vesa_depth, vesa_height, vesa_width;
-
-    c = find_opt(cmdline, "vga=", true);
-
-    if ( !c )
-        return;
+    const char *c = cmdline;
 
     ebo->boot_vid_mode = ASK_VGA;
 
-    if ( !strmaxcmp(c, "current", delim_chars_comma) )
-        ebo->boot_vid_mode = VIDEO_CURRENT_MODE;
-    else if ( !strsubcmp(c, "text-80x") )
-    {
-        c += strlen("text-80x");
-        ebo->boot_vid_mode = rows2vmode(strtoui(c, delim_chars_comma, NULL));
-    }
-    else if ( !strsubcmp(c, "gfx-") )
+    while ( (c = find_opt(c, "vga=", true)) != NULL )
     {
-        vesa_width = strtoui(c + strlen("gfx-"), "x", &c);
+        unsigned int tmp, vesa_depth, vesa_height, vesa_width;
+
+        if ( !strmaxcmp(c, "current", delim_chars_comma) )
+            ebo->boot_vid_mode = VIDEO_CURRENT_MODE;
+        else if ( !strsubcmp(c, "text-80x") )
+        {
+            c += strlen("text-80x");
+            ebo->boot_vid_mode = rows2vmode(strtoui(c, delim_chars_comma, NULL));
+        }
+        else if ( !strsubcmp(c, "gfx-") )
+        {
+            vesa_width = strtoui(c + strlen("gfx-"), "x", &c);
 
-        if ( vesa_width > U16_MAX )
-            return;
+            if ( vesa_width > U16_MAX )
+                return;
 
-        /*
-         * Increment c outside of strtoui() because otherwise some
-         * compiler may complain with following message:
-         * warning: operation on 'c' may be undefined.
-         */
-        ++c;
-        vesa_height = strtoui(c, "x", &c);
+            /*
+             * Increment c outside of strtoui() because otherwise some
+             * compiler may complain with following message:
+             * warning: operation on 'c' may be undefined.
+             */
+            ++c;
+            vesa_height = strtoui(c, "x", &c);
 
-        if ( vesa_height > U16_MAX )
-            return;
+            if ( vesa_height > U16_MAX )
+                return;
 
-        vesa_depth = strtoui(++c, delim_chars_comma, NULL);
+            vesa_depth = strtoui(++c, delim_chars_comma, NULL);
 
-        if ( vesa_depth > U16_MAX )
-            return;
+            if ( vesa_depth > U16_MAX )
+                return;
 
-        ebo->vesa_width = vesa_width;
-        ebo->vesa_height = vesa_height;
-        ebo->vesa_depth = vesa_depth;
-        ebo->boot_vid_mode = VIDEO_VESA_BY_SIZE;
-    }
-    else if ( !strsubcmp(c, "mode-") )
-    {
-        tmp = strtoui(c + strlen("mode-"), delim_chars_comma, NULL);
+            ebo->vesa_width = vesa_width;
+            ebo->vesa_height = vesa_height;
+            ebo->vesa_depth = vesa_depth;
+            ebo->boot_vid_mode = VIDEO_VESA_BY_SIZE;
+        }
+        else if ( !strsubcmp(c, "mode-") )
+        {
+            tmp = strtoui(c + strlen("mode-"), delim_chars_comma, NULL);
 
-        if ( tmp > U16_MAX )
-            return;
+            if ( tmp > U16_MAX )
+                return;
 
-        ebo->boot_vid_mode = tmp;
+            ebo->boot_vid_mode = tmp;
+        }
+        else if ( !strmaxcmp(c, "ask", delim_chars_comma) )
+            ebo->boot_vid_mode = ASK_VGA;
     }
 }
 #endif