Re: [RFC PATCH v2] efi: const correct EFI functions

[Trammell Hudson]

On Tuesday, September 15, 2020 12:36 PM, Jan Beulich  wrote:
> In order for these casts to be halfway safe, they need to happen in
> inline functions, not macros. That way it'll be sufficiently clear
> and certain that it's really only the const which gets changed,
> but not e.g. also the pointed-to type.

At some point it really looks easier to just add const to
the efi headers rather than having to recreate all of their
verbose APIs.  The efiapi.h header was added nine years ago
and has been edited once, seven years ago, so it isn't a source
of significant churn.

Meanwhile, if someone wants to delve into the cursed mines
of sourceforge, perhaps the gnuefi devs can be convinced to
update their sources.

> Apart from this I think the whole change wants splitting up, to
> (about) one basic change at a time.

Yeah, the string and file const changes ended up mixed in the
change set.  They are sort of related, but it would be easiest
if the EFI constness was fixed first so that the printed strings
don't need additional casts.

--
Trammell

Re: [RFC PATCH v2] efi: const correct EFI functions

[Jan Beulich]

On 15.09.2020 12:02, Trammell Hudson wrote:
> @@ -149,10 +150,23 @@ static struct file __initdata cfg;
>  static struct file __initdata kernel;
>  static struct file __initdata ramdisk;
>  static struct file __initdata xsm;
> -static CHAR16 __initdata newline[] = L"\r\n";
> -
> -#define PrintStr(s) StdOut->OutputString(StdOut, s)
> -#define PrintErr(s) StdErr->OutputString(StdErr, s)
> +static const CHAR16 __initconst newline[] = L"\r\n";
> +
> +/* Cast away const-ness on EFI entry points */
> +#define PrintStr(s) StdOut->OutputString(StdOut, (CHAR16 *)(s))
> +#define PrintErr(s) StdErr->OutputString(StdErr, (CHAR16 *)(s))
> +#define efi_file_open(file,handle,name,mode,attr) \
> +(file)->Open(file, handle, (CHAR16 *)(name), mode, attr)
> +#define efi_handle_protocol(handle, protocol, interface) \
> +efi_bs->HandleProtocol(handle, (EFI_GUID *)(protocol), interface)
> +#define efi_locate_protocol(protocol, registration, interface) \
> +efi_bs->LocateProtocol((EFI_GUID *)(protocol), (void *)(registration), \
> +   interface)
> +#define efi_locate_handle(type, protocol, key, size, buffer) \
> +efi_bs->LocateHandle(type, (EFI_GUID *)(protocol), (void *)(key), \
> + size, buffer)
> +#define shim_verify(shim, ptr, len) \
> +(shim)->Verify((void *)(ptr), len)

In order for these casts to be halfway safe, they need to happen in
inline functions, not macros. That way it'll be sufficiently clear
and certain that it's really _only_ the const which gets changed,
but not e.g. also the pointed-to type.

Apart from this I think the whole change wants splitting up, to
(about) one basic change at a time.

Jan

[RFC PATCH v2] efi: const correct EFI functions

[Trammell Hudson]

By wrapping the few EFI handler functions used by the Xen boot process
and casting away constness where safe, it is possible to allow most of
the rest of the EFI boot code to use constant strings and GUIDs.

There are a few places in the code that casts away the const that should
be reconsidered. For instance, the config parser code modifies the config
file in place, which would not work if it were in a read-only segment.

Signed-off-by: Trammell Hudson 
---
 xen/arch/arm/efi/efi-boot.h |   8 +-
 xen/arch/x86/efi/efi-boot.h |  40 -
 xen/common/efi/boot.c   | 157 
 3 files changed, 111 insertions(+), 94 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 6527cb0bdf..13666bc065 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -418,9 +418,9 @@ static void __init efi_arch_memory_setup(void)
 {
 }

-static void __init efi_arch_handle_cmdline(CHAR16 *image_name,
-   CHAR16 *cmdline_options,
-   char *cfgfile_options)
+static void __init efi_arch_handle_cmdline(const CHAR16 *image_name,
+   const CHAR16 *cmdline_options,
+   const char *cfgfile_options)
 {
 union string name;
 char *buf;
@@ -482,7 +482,7 @@ static void __init efi_arch_handle_cmdline(CHAR16 
*image_name,
 }

 static void __init efi_arch_handle_module(struct file *file, const CHAR16 
*name,
-  char *options)
+  const char *options)
 {
 int node;
 int chosen;
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 7188c9a551..8e9811f3e0 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -280,10 +280,10 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE 
dir_handle, char *sect
 {
 union string name;

-name.s = get_value(&cfg, section, "ucode");
-if ( !name.s )
-name.s = get_value(&cfg, "global", "ucode");
-if ( name.s )
+name.cs = get_value(&cfg, section, "ucode");
+if ( !name.cs )
+name.cs = get_value(&cfg, "global", "ucode");
+if ( name.cs )
 {
 microcode_set_module(mbi.mods_count);
 split_string(name.s);
@@ -292,29 +292,29 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE 
dir_handle, char *sect
 }
 }

-static void __init efi_arch_handle_cmdline(CHAR16 *image_name,
-   CHAR16 *cmdline_options,
-   char *cfgfile_options)
+static void __init efi_arch_handle_cmdline(const CHAR16 *image_name,
+   const CHAR16 *cmdline_options,
+   const char *cfgfile_options)
 {
 union string name;

 if ( cmdline_options )
 {
-name.w = cmdline_options;
+name.cw = cmdline_options;
 w2s(&name);
-place_string(&mbi.cmdline, name.s);
+place_string(&mbi.cmdline, name.cs);
 }
 if ( cfgfile_options )
 place_string(&mbi.cmdline, cfgfile_options);
 /* Insert image name last, as it gets prefixed to the other options. */
 if ( image_name )
 {
-name.w = image_name;
+name.cw = image_name;
 w2s(&name);
 }
 else
-name.s = "xen";
-place_string(&mbi.cmdline, name.s);
+name.cs = "xen";
+place_string(&mbi.cmdline, name.cs);

 if ( mbi.cmdline )
 mbi.flags |= MBI_CMDLINE;
@@ -328,8 +328,8 @@ static void __init efi_arch_handle_cmdline(CHAR16 
*image_name,

 static void __init efi_arch_edd(void)
 {
-static EFI_GUID __initdata bio_guid = BLOCK_IO_PROTOCOL;
-static EFI_GUID __initdata devp_guid = DEVICE_PATH_PROTOCOL;
+static const EFI_GUID __initconst bio_guid = BLOCK_IO_PROTOCOL;
+static const EFI_GUID __initconst devp_guid = DEVICE_PATH_PROTOCOL;
 EFI_HANDLE *handles = NULL;
 unsigned int i;
 UINTN size;
@@ -339,12 +339,12 @@ static void __init efi_arch_edd(void)
 BUILD_BUG_ON(offsetof(struct edd_info, edd_device_params) != EDDEXTSIZE);
 BUILD_BUG_ON(sizeof(struct edd_device_params) != EDDPARMSIZE);
 size = 0;
-status = efi_bs->LocateHandle(ByProtocol, &bio_guid, NULL, &size, NULL);
+status = efi_locate_handle(ByProtocol, &bio_guid, NULL, &size, NULL);
 if ( status == EFI_BUFFER_TOO_SMALL )
 status = efi_bs->AllocatePool(EfiLoaderData, size, (void **)&handles);
 if ( !EFI_ERROR(status) )
-status = efi_bs->LocateHandle(ByProtocol, &bio_guid, NULL, &size,
-  handles);
+status = efi_locate_handle(ByProtocol, &bio_guid, NULL, &size,
+   handles);
 if ( EFI_ERROR(status) )
 size = 0;
 for ( i = 0; i < size / sizeof(*handles); ++i )