xen/arch/arm/efi/efi-boot.h | 7 ------- xen/arch/x86/efi/efi-boot.h | 2 +- xen/common/efi/boot.c | 22 ++++++++++++++-------- 3 files changed, 15 insertions(+), 16 deletions(-)
PrintMessage function print a message string followed by a
new line.
Move the function from ARM specific code to common code.
Reuse it in EFI code.
No functional changes.
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
xen/arch/arm/efi/efi-boot.h | 7 -------
xen/arch/x86/efi/efi-boot.h | 2 +-
xen/common/efi/boot.c | 22 ++++++++++++++--------
3 files changed, 15 insertions(+), 16 deletions(-)
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 7dd2528a17..3dbeed3f89 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -48,7 +48,6 @@ void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
void __flush_dcache_area(const void *vaddr, unsigned long size);
static int get_module_file_index(const char *name, unsigned int name_len);
-static void PrintMessage(const CHAR16 *s);
#define DEVICE_TREE_GUID \
{0xb1b621d5U, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
@@ -634,12 +633,6 @@ static int __init get_module_file_index(const char *name,
return ret;
}
-static void __init PrintMessage(const CHAR16 *s)
-{
- PrintStr(s);
- PrintStr(newline);
-}
-
/*
* This function allocates a binary and keeps track of its name, it returns the
* index of the file in the modules array or a negative number on error.
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 0194720003..d256310619 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -646,7 +646,7 @@ static void __init efi_arch_memory_setup(void)
else
{
cfg.addr = 0;
- PrintStr(L"Trampoline space cannot be allocated; will try fallback.\r\n");
+ PrintMessage(L"Trampoline space cannot be allocated; will try fallback.");
}
if ( !efi_enabled(EFI_LOADER) )
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index aad9f4db1e..6c2ef13bc5 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -174,6 +174,12 @@ static void __init PrintErr(const CHAR16 *s)
StdErr->OutputString(StdErr, (CHAR16 *)s );
}
+static void __init PrintMessage(const CHAR16 *s)
+{
+ PrintStr(s);
+ PrintStr(newline);
+}
+
static CHAR16 *__init FormatDec(UINT64 Val, CHAR16 *Buffer)
{
if ( Val >= 10 )
@@ -452,7 +458,7 @@ static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_i
*leaf = NULL;
if ( !loaded_image->DeviceHandle )
{
- PrintStr(L"Xen image loaded without providing a device\r\n");
+ PrintMessage(L"Xen image loaded without providing a device");
return NULL;
}
@@ -485,7 +491,7 @@ static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_i
* For instance this can happen if Xen was loaded using GRUB2
* "chainloader" command and the file was not from ESP.
*/
- PrintStr(L"Unsupported device path component\r\n");
+ PrintMessage(L"Unsupported device path component");
return NULL;
}
@@ -1378,11 +1384,11 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
else if ( wstrcmp(ptr + 1, L"help") == 0 ||
(ptr[1] == L'?' && !ptr[2]) )
{
- PrintStr(L"Xen EFI Loader options:\r\n");
- PrintStr(L"-basevideo retain current video mode\r\n");
- PrintStr(L"-mapbs map EfiBootServices{Code,Data}\r\n");
- PrintStr(L"-cfg=<file> specify configuration file\r\n");
- PrintStr(L"-help, -? display this help\r\n");
+ PrintMessage(L"Xen EFI Loader options:");
+ PrintMessage(L"-basevideo retain current video mode");
+ PrintMessage(L"-mapbs map EfiBootServices{Code,Data}");
+ PrintMessage(L"-cfg=<file> specify configuration file");
+ PrintMessage(L"-help, -? display this help");
blexit(NULL);
}
else
@@ -1433,7 +1439,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
/* Read and parse the config file. */
if ( read_section(loaded_image, L"config", &cfg, NULL) )
- PrintStr(L"Using builtin config file\r\n");
+ PrintMessage(L"Using builtin config file");
else if ( !cfg_file_name && file_name )
{
CHAR16 *tail;
--
2.43.0On 18/07/2025 10:41 am, Frediano Ziglio wrote: > PrintMessage function print a message string followed by a > new line. > Move the function from ARM specific code to common code. > Reuse it in EFI code. > No functional changes. > > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> Please no. Hiding \n (or \r\n) in strings is an antipattern and a source of confusion that we do not want in Xen. Instead, delete PrintMessage() and convert ARM to use PrintStr(). That looks like it was premature anti-optimisation. You save 4 bytes per string, but it costs 12 byte minimum in .text to set up the function call, let alone the function call itself. ~Andrew
On Fri, Jul 18, 2025 at 12:12 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 18/07/2025 10:41 am, Frediano Ziglio wrote: > > PrintMessage function print a message string followed by a > > new line. > > Move the function from ARM specific code to common code. > > Reuse it in EFI code. > > No functional changes. > > > > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> > > Please no. > > Hiding \n (or \r\n) in strings is an antipattern and a source of > confusion that we do not want in Xen. > I didn't realize it was not a Xen pattern. I found the thing not that coherent between x86 and arm and attempted to fix it. > Instead, delete PrintMessage() and convert ARM to use PrintStr(). That > looks like it was premature anti-optimisation. You save 4 bytes per > string, but it costs 12 byte minimum in .text to set up the function > call, let alone the function call itself. > To be honest I didn't consider this aspect and I don't feel it matters much. Looking at other languages printing an implicit newline is pretty common, Pascal has "Wtite" and "WtiteLn", Ocaml has "print_string" and "print_endline", Python has an "end" parameter which by default is the newline, Basic (if I remember) has the possibility of an implicit empty field. In C is not so common although there's "puts" and some functions (like "syslog") have some kind of implicit newline. But obviously Xen is not any of them. As I said above however my main reason is coherency so I'll post your suggestion. > ~Andrew Frediano
On 18/07/2025 1:00 pm, Frediano Ziglio wrote: > On Fri, Jul 18, 2025 at 12:12 PM Andrew Cooper > <andrew.cooper3@citrix.com> wrote: >> On 18/07/2025 10:41 am, Frediano Ziglio wrote: >>> PrintMessage function print a message string followed by a >>> new line. >>> Move the function from ARM specific code to common code. >>> Reuse it in EFI code. >>> No functional changes. >>> >>> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> >> Please no. >> >> Hiding \n (or \r\n) in strings is an antipattern and a source of >> confusion that we do not want in Xen. >> > I didn't realize it was not a Xen pattern. We went through a phase where it was inconsistent and we kept on having problems on the console. Missing a newline causes the next message to be appended with <0> and it's very confusion to read. > I found the thing not that > coherent between x86 and arm and attempted to fix it. Sure, and making things consistent is very welcome. > >> Instead, delete PrintMessage() and convert ARM to use PrintStr(). That >> looks like it was premature anti-optimisation. You save 4 bytes per >> string, but it costs 12 byte minimum in .text to set up the function >> call, let alone the function call itself. >> > To be honest I didn't consider this aspect and I don't feel it matters much. > Looking at other languages printing an implicit newline is pretty > common, Pascal has "Wtite" and "WtiteLn", Ocaml has "print_string" and > "print_endline", Right, and in both cases these are clear from context (i.e. the function name) that there's a newline. PrintStr() and PrintMessage() do not have the same kind of clarity. > Python has an "end" parameter which by default is the > newline, Basic (if I remember) has the possibility of an implicit > empty field. In C is not so common although there's "puts" and some > functions (like "syslog") have some kind of implicit newline. > But obviously Xen is not any of them. > > As I said above however my main reason is coherency so I'll post your > suggestion. Thankyou. I'm sure ARM will be better off as a result. ~Andrew
© 2016 - 2025 Red Hat, Inc.