| Message ID | 20230331095946.45024-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gfx: improvements when using multiboot2 and EFI | expand |
On 31.03.2023 11:59, Roger Pau Monne wrote: > Only set the GOP mode if vga is selected in the console option, This particular aspect of the behavior is inconsistent with legacy boot behavior: There "vga=" isn't qualified by what "console=" has. > otherwise just fetch the information from the current mode in order to > make it available to dom0. > > Introduce support for passing the command line to the efi_multiboot2() > helper, and parse the console= and vga= options if present. > > Add support for the 'gfx' and 'current' vga options, ignore the 'keep' > option, and print a warning message about other options not being > currently implemented. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >[...] > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -786,7 +786,30 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) > > static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { } > > -void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > +/* Return the next occurrence of opt in cmd. */ > +static const char __init *get_option(const char *cmd, const char *opt) > +{ > + const char *s = cmd, *o = NULL; > + > + if ( !cmd || !opt ) I can see why you need to check "cmd", but there's no need to check "opt" I would say. > @@ -807,7 +830,60 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable > > if ( gop ) > { > - gop_mode = efi_find_gop_mode(gop, 0, 0, 0); > + const char *opt = NULL, *last = cmdline; > + /* Default console selection is "com1,vga". */ > + bool vga = true; > + > + /* For the console option the last occurrence is the enforced one. */ > + while ( (last = get_option(last, "console=")) != NULL ) > + opt = last; > + > + if ( opt ) > + { > + const char *s = strstr(opt, "vga"); > + > + if ( !s || s > strpbrk(opt, " ") ) Why strpbrk() and not the simpler strchr()? Or did you mean to also look for tabs, but then didn't include \t here (and in get_option())? (Legacy boot code also takes \r and \n as separators, btw, but I'm unconvinced of the need.) Also aiui this is UB when the function returns NULL, as relational operators (excluding equality ones) may only be applied when both addresses refer to the same object (or to the end of an involved array). > + vga = false; > + } > + > + if ( vga ) > + { > + unsigned int width = 0, height = 0, depth = 0; > + bool keep_current = false; > + > + last = cmdline; > + while ( (last = get_option(last, "vga=")) != NULL ) It's yet different for "vga=", I'm afraid: Early boot code (boot/cmdline.c) finds the first instance only. Normal command line handling respects the last instance only. So while "vga=gfx-... vga=keep" will have the expected effect, "vga=keep vga=gfx-..." won't (I think). It is certainly fine to be less inflexible here, but I think this then wants accompanying by an update to the command line doc, no matter that presently it doesn't really describe these peculiarities. Otoh it would end up being slightly cheaper to only look for the first instance here as well. In particular ... > + { > + if ( !strncmp(last, "gfx-", 4) ) > + { > + width = simple_strtoul(last + 4, &last, 10); > + if ( *last == 'x' ) > + height = simple_strtoul(last + 1, &last, 10); > + if ( *last == 'x' ) > + depth = simple_strtoul(last + 1, &last, 10); > + /* Allow depth to be 0 or unset. */ > + if ( !width || !height ) > + width = height = depth = 0; > + keep_current = false; > + } > + else if ( !strncmp(last, "current", 7) ) > + keep_current = true; > + else if ( !strncmp(last, "keep", 4) ) > + { > + /* Ignore. */ > + } > + else > + { > + /* Fallback to defaults if unimplemented. */ > + width = height = depth = 0; > + keep_current = false; ... this zapping of what was successfully parsed before would then not be needed in any event (else I would question why this is necessary). Jan
On Wed, Apr 05, 2023 at 12:15:26PM +0200, Jan Beulich wrote: > On 31.03.2023 11:59, Roger Pau Monne wrote: > > Only set the GOP mode if vga is selected in the console option, > > This particular aspect of the behavior is inconsistent with legacy > boot behavior: There "vga=" isn't qualified by what "console=" has. Hm, I find this very odd, why would we fiddle with the VGA (or the GOP here) if we don't intend to use it for output? > > otherwise just fetch the information from the current mode in order to > > make it available to dom0. > > > > Introduce support for passing the command line to the efi_multiboot2() > > helper, and parse the console= and vga= options if present. > > > > Add support for the 'gfx' and 'current' vga options, ignore the 'keep' > > option, and print a warning message about other options not being > > currently implemented. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > >[...] > > --- a/xen/arch/x86/efi/efi-boot.h > > +++ b/xen/arch/x86/efi/efi-boot.h > > @@ -786,7 +786,30 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) > > > > static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { } > > > > -void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > > +/* Return the next occurrence of opt in cmd. */ > > +static const char __init *get_option(const char *cmd, const char *opt) > > +{ > > + const char *s = cmd, *o = NULL; > > + > > + if ( !cmd || !opt ) > > I can see why you need to check "cmd", but there's no need to check "opt" > I would say. Given this is executed without a page-fault handler in place I thought it was best to be safe rather than sorry, and avoid any pointer dereferences. > > @@ -807,7 +830,60 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable > > > > if ( gop ) > > { > > - gop_mode = efi_find_gop_mode(gop, 0, 0, 0); > > + const char *opt = NULL, *last = cmdline; > > + /* Default console selection is "com1,vga". */ > > + bool vga = true; > > + > > + /* For the console option the last occurrence is the enforced one. */ > > + while ( (last = get_option(last, "console=")) != NULL ) > > + opt = last; > > + > > + if ( opt ) > > + { > > + const char *s = strstr(opt, "vga"); > > + > > + if ( !s || s > strpbrk(opt, " ") ) > > Why strpbrk() and not the simpler strchr()? Or did you mean to also look > for tabs, but then didn't include \t here (and in get_option())? (Legacy > boot code also takes \r and \n as separators, btw, but I'm unconvinced > of the need.) I was originally checking for more characters here and didn't switch when removing those. I will add \t. > Also aiui this is UB when the function returns NULL, as relational operators > (excluding equality ones) may only be applied when both addresses refer to > the same object (or to the end of an involved array). Hm, I see, thanks for spotting. So I would need to do: s > (strpbrk(opt, " ") ?: s) So that we don't compare against NULL. Also the original code was wrong AFAICT, as strpbrk() returning NULL should result in vga=true (as it would imply console= is the last option on the command line). > > + vga = false; > > + } > > + > > + if ( vga ) > > + { > > + unsigned int width = 0, height = 0, depth = 0; > > + bool keep_current = false; > > + > > + last = cmdline; > > + while ( (last = get_option(last, "vga=")) != NULL ) > > It's yet different for "vga=", I'm afraid: Early boot code (boot/cmdline.c) > finds the first instance only. Normal command line handling respects the > last instance only. So while "vga=gfx-... vga=keep" will have the expected > effect, "vga=keep vga=gfx-..." won't (I think). It is certainly fine to be > less inflexible here, but I think this then wants accompanying by an update > to the command line doc, no matter that presently it doesn't really > describe these peculiarities. But if we then describe this behavior in the documentation people could rely on it. Right now this is just an implementation detail (or a bug I would say), and that would justify fixing boot/cmdline.c to also respect the last instance only. > Otoh it would end up being slightly cheaper > to only look for the first instance here as well. In particular ... > > > + { > > + if ( !strncmp(last, "gfx-", 4) ) > > + { > > + width = simple_strtoul(last + 4, &last, 10); > > + if ( *last == 'x' ) > > + height = simple_strtoul(last + 1, &last, 10); > > + if ( *last == 'x' ) > > + depth = simple_strtoul(last + 1, &last, 10); > > + /* Allow depth to be 0 or unset. */ > > + if ( !width || !height ) > > + width = height = depth = 0; > > + keep_current = false; > > + } > > + else if ( !strncmp(last, "current", 7) ) > > + keep_current = true; > > + else if ( !strncmp(last, "keep", 4) ) > > + { > > + /* Ignore. */ > > + } > > + else > > + { > > + /* Fallback to defaults if unimplemented. */ > > + width = height = depth = 0; > > + keep_current = false; > > ... this zapping of what was successfully parsed before would then not be > needed in any event (else I would question why this is necessary). Hm, I don't have a strong opinion, the expected behavior I have of command line options is that the last one is the one that takes effect, but it would simplify the change if we only cared about the first one, albeit that's an odd behavior. My preference would be to leave the code here respecting the last instance only, and attempt to fix boot/cmdline.c so it does the same. Thanks, Roger.
On 30.05.2023 18:02, Roger Pau Monné wrote: > On Wed, Apr 05, 2023 at 12:15:26PM +0200, Jan Beulich wrote: >> On 31.03.2023 11:59, Roger Pau Monne wrote: >>> Only set the GOP mode if vga is selected in the console option, >> >> This particular aspect of the behavior is inconsistent with legacy >> boot behavior: There "vga=" isn't qualified by what "console=" has. > > Hm, I find this very odd, why would we fiddle with the VGA (or the GOP > here) if we don't intend to use it for output? Because we also need to arrange for what Dom0 possibly wants to use. It has no way of setting the mode the low-level (BIOS or EFI) way. >>> otherwise just fetch the information from the current mode in order to >>> make it available to dom0. >>> >>> Introduce support for passing the command line to the efi_multiboot2() >>> helper, and parse the console= and vga= options if present. >>> >>> Add support for the 'gfx' and 'current' vga options, ignore the 'keep' >>> option, and print a warning message about other options not being >>> currently implemented. >>> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>> [...] >>> --- a/xen/arch/x86/efi/efi-boot.h >>> +++ b/xen/arch/x86/efi/efi-boot.h >>> @@ -786,7 +786,30 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) >>> >>> static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { } >>> >>> -void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) >>> +/* Return the next occurrence of opt in cmd. */ >>> +static const char __init *get_option(const char *cmd, const char *opt) >>> +{ >>> + const char *s = cmd, *o = NULL; >>> + >>> + if ( !cmd || !opt ) >> >> I can see why you need to check "cmd", but there's no need to check "opt" >> I would say. > > Given this is executed without a page-fault handler in place I thought > it was best to be safe rather than sorry, and avoid any pointer > dereferences. Hmm, I see. We don't do so elsewhere, though, I think. >>> @@ -807,7 +830,60 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable >>> >>> if ( gop ) >>> { >>> - gop_mode = efi_find_gop_mode(gop, 0, 0, 0); >>> + const char *opt = NULL, *last = cmdline; >>> + /* Default console selection is "com1,vga". */ >>> + bool vga = true; >>> + >>> + /* For the console option the last occurrence is the enforced one. */ >>> + while ( (last = get_option(last, "console=")) != NULL ) >>> + opt = last; >>> + >>> + if ( opt ) >>> + { >>> + const char *s = strstr(opt, "vga"); >>> + >>> + if ( !s || s > strpbrk(opt, " ") ) >> >> Why strpbrk() and not the simpler strchr()? Or did you mean to also look >> for tabs, but then didn't include \t here (and in get_option())? (Legacy >> boot code also takes \r and \n as separators, btw, but I'm unconvinced >> of the need.) > > I was originally checking for more characters here and didn't switch > when removing those. I will add \t. > >> Also aiui this is UB when the function returns NULL, as relational operators >> (excluding equality ones) may only be applied when both addresses refer to >> the same object (or to the end of an involved array). > > Hm, I see, thanks for spotting. So I would need to do: > > s > (strpbrk(opt, " ") ?: s) > > So that we don't compare against NULL. > > Also the original code was wrong AFAICT, as strpbrk() returning NULL > should result in vga=true (as it would imply console= is the last > option on the command line). I'm afraid I'm in trouble what "original code" you're referring to here. Iirc you really only add code to the function. And boot/cmdline.c has no use of strpbrk() afaics. >>> + vga = false; >>> + } >>> + >>> + if ( vga ) >>> + { >>> + unsigned int width = 0, height = 0, depth = 0; >>> + bool keep_current = false; >>> + >>> + last = cmdline; >>> + while ( (last = get_option(last, "vga=")) != NULL ) >> >> It's yet different for "vga=", I'm afraid: Early boot code (boot/cmdline.c) >> finds the first instance only. Normal command line handling respects the >> last instance only. So while "vga=gfx-... vga=keep" will have the expected >> effect, "vga=keep vga=gfx-..." won't (I think). It is certainly fine to be >> less inflexible here, but I think this then wants accompanying by an update >> to the command line doc, no matter that presently it doesn't really >> describe these peculiarities. > > But if we then describe this behavior in the documentation people > could rely on it. Right now this is just an implementation detail (or > a bug I would say), and that would justify fixing boot/cmdline.c to > also respect the last instance only. Yes, fixing the non-EFI code is certainly an option (and then describing the hopefully consistent result in the doc). Jan >> Otoh it would end up being slightly cheaper >> to only look for the first instance here as well. In particular ... >> >>> + { >>> + if ( !strncmp(last, "gfx-", 4) ) >>> + { >>> + width = simple_strtoul(last + 4, &last, 10); >>> + if ( *last == 'x' ) >>> + height = simple_strtoul(last + 1, &last, 10); >>> + if ( *last == 'x' ) >>> + depth = simple_strtoul(last + 1, &last, 10); >>> + /* Allow depth to be 0 or unset. */ >>> + if ( !width || !height ) >>> + width = height = depth = 0; >>> + keep_current = false; >>> + } >>> + else if ( !strncmp(last, "current", 7) ) >>> + keep_current = true; >>> + else if ( !strncmp(last, "keep", 4) ) >>> + { >>> + /* Ignore. */ >>> + } >>> + else >>> + { >>> + /* Fallback to defaults if unimplemented. */ >>> + width = height = depth = 0; >>> + keep_current = false; >> >> ... this zapping of what was successfully parsed before would then not be >> needed in any event (else I would question why this is necessary). > > Hm, I don't have a strong opinion, the expected behavior I have of > command line options is that the last one is the one that takes > effect, but it would simplify the change if we only cared about the > first one, albeit that's an odd behavior. > > My preference would be to leave the code here respecting the last > instance only, and attempt to fix boot/cmdline.c so it does the same. > > Thanks, Roger.
On Wed, May 31, 2023 at 11:15:44AM +0200, Jan Beulich wrote: > On 30.05.2023 18:02, Roger Pau Monné wrote: > > On Wed, Apr 05, 2023 at 12:15:26PM +0200, Jan Beulich wrote: > >> On 31.03.2023 11:59, Roger Pau Monne wrote: > >>> Only set the GOP mode if vga is selected in the console option, > >> > >> This particular aspect of the behavior is inconsistent with legacy > >> boot behavior: There "vga=" isn't qualified by what "console=" has. > > > > Hm, I find this very odd, why would we fiddle with the VGA (or the GOP > > here) if we don't intend to use it for output? > > Because we also need to arrange for what Dom0 possibly wants to use. > It has no way of setting the mode the low-level (BIOS or EFI) way. I understand this might be needed when Xen is booted as an EFI application, but otherwise it should be the bootloader that takes care of setting such mode, as (most?) OSes are normally loaded with boot services already exited. I've removed the parsing of the console= option and unconditionally parse vga= now. We can always adjust later. > >>> otherwise just fetch the information from the current mode in order to > >>> make it available to dom0. > >>> > >>> Introduce support for passing the command line to the efi_multiboot2() > >>> helper, and parse the console= and vga= options if present. > >>> > >>> Add support for the 'gfx' and 'current' vga options, ignore the 'keep' > >>> option, and print a warning message about other options not being > >>> currently implemented. > >>> > >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > >>> [...] > >>> --- a/xen/arch/x86/efi/efi-boot.h > >>> +++ b/xen/arch/x86/efi/efi-boot.h > >>> @@ -786,7 +786,30 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) > >>> > >>> static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { } > >>> > >>> -void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > >>> +/* Return the next occurrence of opt in cmd. */ > >>> +static const char __init *get_option(const char *cmd, const char *opt) > >>> +{ > >>> + const char *s = cmd, *o = NULL; > >>> + > >>> + if ( !cmd || !opt ) > >> > >> I can see why you need to check "cmd", but there's no need to check "opt" > >> I would say. > > > > Given this is executed without a page-fault handler in place I thought > > it was best to be safe rather than sorry, and avoid any pointer > > dereferences. > > Hmm, I see. We don't do so elsewhere, though, I think. If you insist I can remove it, otherwise I will just leave as-is. > > >>> @@ -807,7 +830,60 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable > >>> > >>> if ( gop ) > >>> { > >>> - gop_mode = efi_find_gop_mode(gop, 0, 0, 0); > >>> + const char *opt = NULL, *last = cmdline; > >>> + /* Default console selection is "com1,vga". */ > >>> + bool vga = true; > >>> + > >>> + /* For the console option the last occurrence is the enforced one. */ > >>> + while ( (last = get_option(last, "console=")) != NULL ) > >>> + opt = last; > >>> + > >>> + if ( opt ) > >>> + { > >>> + const char *s = strstr(opt, "vga"); > >>> + > >>> + if ( !s || s > strpbrk(opt, " ") ) > >> > >> Why strpbrk() and not the simpler strchr()? Or did you mean to also look > >> for tabs, but then didn't include \t here (and in get_option())? (Legacy > >> boot code also takes \r and \n as separators, btw, but I'm unconvinced > >> of the need.) > > > > I was originally checking for more characters here and didn't switch > > when removing those. I will add \t. > > > >> Also aiui this is UB when the function returns NULL, as relational operators > >> (excluding equality ones) may only be applied when both addresses refer to > >> the same object (or to the end of an involved array). > > > > Hm, I see, thanks for spotting. So I would need to do: > > > > s > (strpbrk(opt, " ") ?: s) > > > > So that we don't compare against NULL. > > > > Also the original code was wrong AFAICT, as strpbrk() returning NULL > > should result in vga=true (as it would imply console= is the last > > option on the command line). > > I'm afraid I'm in trouble what "original code" you're referring to here. > Iirc you really only add code to the function. And boot/cmdline.c has no > use of strpbrk() afaics. Original code in the patch, anyway this is now gone because I no longer parse console=. > >>> + vga = false; > >>> + } > >>> + > >>> + if ( vga ) > >>> + { > >>> + unsigned int width = 0, height = 0, depth = 0; > >>> + bool keep_current = false; > >>> + > >>> + last = cmdline; > >>> + while ( (last = get_option(last, "vga=")) != NULL ) > >> > >> It's yet different for "vga=", I'm afraid: Early boot code (boot/cmdline.c) > >> finds the first instance only. Normal command line handling respects the > >> last instance only. So while "vga=gfx-... vga=keep" will have the expected > >> effect, "vga=keep vga=gfx-..." won't (I think). It is certainly fine to be > >> less inflexible here, but I think this then wants accompanying by an update > >> to the command line doc, no matter that presently it doesn't really > >> describe these peculiarities. > > > > But if we then describe this behavior in the documentation people > > could rely on it. Right now this is just an implementation detail (or > > a bug I would say), and that would justify fixing boot/cmdline.c to > > also respect the last instance only. > > Yes, fixing the non-EFI code is certainly an option (and then describing > the hopefully consistent result in the doc). OK, let me take a look, I expect that should be fairly trivial. Thanks, Roger.
On 31.05.2023 11:30, Roger Pau Monné wrote: > On Wed, May 31, 2023 at 11:15:44AM +0200, Jan Beulich wrote: >> On 30.05.2023 18:02, Roger Pau Monné wrote: >>> On Wed, Apr 05, 2023 at 12:15:26PM +0200, Jan Beulich wrote: >>>> On 31.03.2023 11:59, Roger Pau Monne wrote: >>>>> Only set the GOP mode if vga is selected in the console option, >>>> >>>> This particular aspect of the behavior is inconsistent with legacy >>>> boot behavior: There "vga=" isn't qualified by what "console=" has. >>> >>> Hm, I find this very odd, why would we fiddle with the VGA (or the GOP >>> here) if we don't intend to use it for output? >> >> Because we also need to arrange for what Dom0 possibly wants to use. >> It has no way of setting the mode the low-level (BIOS or EFI) way. > > I understand this might be needed when Xen is booted as an EFI > application, but otherwise it should be the bootloader that takes care > of setting such mode, as (most?) OSes are normally loaded with boot > services already exited. The bootloader doing this is a quirk imo. In the Linux case this implies knowing the inner structure of the binary to be booted, to communicate the necessary information, plus peeking into the kernel's command line. Furthermore I wasn't referring to the EFI-with-bootloader case, but the legacy BIOS one plus the (mentioned by you) EFI-application one. Even the MB2 protocol allows the bootloader to hand over with boot services not exited yet, iirc, so even in that case Xen would be in the position of using boot service functions (from efi_multiboot2()). Jan
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 0fb7dd3029..9d13eee50b 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -221,9 +221,10 @@ __efi64_mb2_start: jmp x86_32_switch .Lefi_multiboot2_proto: - /* Zero EFI SystemTable and EFI ImageHandle addresses. */ + /* Zero EFI SystemTable, EFI ImageHandle addresses and cmdline. */ xor %esi,%esi xor %edi,%edi + xor %edx,%edx /* Skip Multiboot2 information fixed part. */ lea (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%ecx @@ -261,6 +262,13 @@ __efi64_mb2_start: cmove MB2_efi64_ih(%rcx),%rdi je .Lefi_mb2_next_tag + /* Get command line from Multiboot2 information. */ + cmpl $MULTIBOOT2_TAG_TYPE_CMDLINE,MB2_tag_type(%rcx) + jne .Lno_cmdline + lea MB2_tag_string(%rcx),%rdx + jmp .Lefi_mb2_next_tag +.Lno_cmdline: + /* Is it the end of Multiboot2 information? */ cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx) je .Lrun_bs @@ -324,7 +332,8 @@ __efi64_mb2_start: /* * efi_multiboot2() is called according to System V AMD64 ABI: - * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable. + * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable, + * %rdx - MB2 cmdline */ call efi_multiboot2 diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index c94e53d139..f46c1f021e 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -786,7 +786,30 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { } -void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) +/* Return the next occurrence of opt in cmd. */ +static const char __init *get_option(const char *cmd, const char *opt) +{ + const char *s = cmd, *o = NULL; + + if ( !cmd || !opt ) + return NULL; + + while ( (s = strstr(s, opt)) != NULL ) + { + if ( s == cmd || *(s - 1) == ' ' ) + { + o = s + strlen(opt); + break; + } + + s += strlen(opt); + } + + return o; +} + +void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable, + const char *cmdline) { EFI_GRAPHICS_OUTPUT_PROTOCOL *gop; EFI_HANDLE gop_handle; @@ -807,7 +830,60 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable if ( gop ) { - gop_mode = efi_find_gop_mode(gop, 0, 0, 0); + const char *opt = NULL, *last = cmdline; + /* Default console selection is "com1,vga". */ + bool vga = true; + + /* For the console option the last occurrence is the enforced one. */ + while ( (last = get_option(last, "console=")) != NULL ) + opt = last; + + if ( opt ) + { + const char *s = strstr(opt, "vga"); + + if ( !s || s > strpbrk(opt, " ") ) + vga = false; + } + + if ( vga ) + { + unsigned int width = 0, height = 0, depth = 0; + bool keep_current = false; + + last = cmdline; + while ( (last = get_option(last, "vga=")) != NULL ) + { + if ( !strncmp(last, "gfx-", 4) ) + { + width = simple_strtoul(last + 4, &last, 10); + if ( *last == 'x' ) + height = simple_strtoul(last + 1, &last, 10); + if ( *last == 'x' ) + depth = simple_strtoul(last + 1, &last, 10); + /* Allow depth to be 0 or unset. */ + if ( !width || !height ) + width = height = depth = 0; + keep_current = false; + } + else if ( !strncmp(last, "current", 7) ) + keep_current = true; + else if ( !strncmp(last, "keep", 4) ) + { + /* Ignore. */ + } + else + { + /* Fallback to defaults if unimplemented. */ + width = height = depth = 0; + keep_current = false; + PrintStr(L"Warning: Cannot use selected vga option.\r\n"); + } + } + + if ( !keep_current ) + gop_mode = efi_find_gop_mode(gop, width, height, depth); + } efi_arch_edid(gop_handle); } diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c index 287dac101a..fbd6c54188 100644 --- a/xen/arch/x86/x86_64/asm-offsets.c +++ b/xen/arch/x86/x86_64/asm-offsets.c @@ -175,6 +175,7 @@ void __dummy__(void) OFFSET(MB2_mem_lower, multiboot2_tag_basic_meminfo_t, mem_lower); OFFSET(MB2_efi64_st, multiboot2_tag_efi64_t, pointer); OFFSET(MB2_efi64_ih, multiboot2_tag_efi64_ih_t, pointer); + OFFSET(MB2_tag_string, multiboot2_tag_string_t, string); BLANK(); OFFSET(DOMAIN_vm_assist, struct domain, vm_assist);
Only set the GOP mode if vga is selected in the console option, otherwise just fetch the information from the current mode in order to make it available to dom0. Introduce support for passing the command line to the efi_multiboot2() helper, and parse the console= and vga= options if present. Add support for the 'gfx' and 'current' vga options, ignore the 'keep' option, and print a warning message about other options not being currently implemented. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - Do not return the last occurrence of a command line. - Rearrange the code for assembly processing of the cmdline and use lea. - Merge patches handling console= and vga= together. --- xen/arch/x86/boot/head.S | 13 ++++- xen/arch/x86/efi/efi-boot.h | 80 ++++++++++++++++++++++++++++++- xen/arch/x86/x86_64/asm-offsets.c | 1 + 3 files changed, 90 insertions(+), 4 deletions(-)