diff mbox series

[v3,2/4] efi/boot.c: add file.need_to_free and split display_file_info()

Message ID 20200907190027.669086-3-hudson@trmm.net
State Superseded
Headers show
Series efi: Unified Xen hypervisor/kernel/initrd images | expand

Commit Message

Trammell Hudson Sept. 7, 2020, 7 p.m. UTC
From: Trammell hudson <hudson@trmm.net>

Signed-off-by: Trammell hudson <hudson@trmm.net>
---
 xen/common/efi/boot.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

Comments

Roger Pau Monné Sept. 14, 2020, 9:05 a.m. UTC | #1
Thanks! Being picky you likely wan to split this into two separate
commits: one for adding need_to_free and the other for
display_file_info.  There's no relation between the two that would
require them to be on the same commit.

On Mon, Sep 07, 2020 at 03:00:25PM -0400, Trammell Hudson wrote:
> From: Trammell hudson <hudson@trmm.net>
> 
> Signed-off-by: Trammell hudson <hudson@trmm.net>
> ---
>  xen/common/efi/boot.c | 36 ++++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 4022a672c9..f5bdc4b1df 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -102,6 +102,7 @@ union string {
>  
>  struct file {
>      UINTN size;
> +    bool need_to_free;
>      union {
>          EFI_PHYSICAL_ADDRESS addr;
>          void *ptr;
> @@ -279,13 +280,13 @@ void __init noreturn blexit(const CHAR16 *str)
>      if ( !efi_bs )
>          efi_arch_halt();
>  
> -    if ( cfg.addr )
> +    if ( cfg.addr && cfg.need_to_free )
>          efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
> -    if ( kernel.addr )
> +    if ( kernel.addr && kernel.need_to_free )
>          efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
> -    if ( ramdisk.addr )
> +    if ( ramdisk.addr && ramdisk.need_to_free )
>          efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
> -    if ( xsm.addr )
> +    if ( xsm.addr && xsm.need_to_free )
>          efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));
>  
>      efi_arch_blexit();
> @@ -538,6 +539,21 @@ static char * __init split_string(char *s)
>      return NULL;
>  }
>  
> +static void __init display_file_info(CHAR16 *name, struct file *file, char *options)

I think name at least could be constified?

Also efi_arch_handle_module seem to do more than just printing file
info, hence I would likely rename this to handle_file_info to be more
representative of what it does.

Roger.
Trammell Hudson Sept. 14, 2020, 10:45 a.m. UTC | #2
On Monday, September 14, 2020 5:05 AM, Roger Pau Monné <roger.pau@citrix.com> wrote:

> Thanks! Being picky you likely wan to split this into two separate
> commits: one for adding need_to_free and the other for
> display_file_info. There's no relation between the two that would
> require them to be on the same commit.

Ok.  I'll address that in the v4 of the patch.

> [...]
> > +static void __init display_file_info(CHAR16 *name, struct file *file, char *options)
>
> I think name at least could be constified?

Oh, I wish.  There should be a major pass to constify the EFI functions.
So many of them are not const-correct and it worries me that
literal strings are passed to those functions.

> Also efi_arch_handle_module seem to do more than just printing file
> info, hence I would likely rename this to handle_file_info to be more
> representative of what it does.

Ok. Fixed in v4.

--
Trammell
diff mbox series

Patch

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 4022a672c9..f5bdc4b1df 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -102,6 +102,7 @@  union string {
 
 struct file {
     UINTN size;
+    bool need_to_free;
     union {
         EFI_PHYSICAL_ADDRESS addr;
         void *ptr;
@@ -279,13 +280,13 @@  void __init noreturn blexit(const CHAR16 *str)
     if ( !efi_bs )
         efi_arch_halt();
 
-    if ( cfg.addr )
+    if ( cfg.addr && cfg.need_to_free )
         efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
-    if ( kernel.addr )
+    if ( kernel.addr && kernel.need_to_free )
         efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
-    if ( ramdisk.addr )
+    if ( ramdisk.addr && ramdisk.need_to_free )
         efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
-    if ( xsm.addr )
+    if ( xsm.addr && xsm.need_to_free )
         efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));
 
     efi_arch_blexit();
@@ -538,6 +539,21 @@  static char * __init split_string(char *s)
     return NULL;
 }
 
+static void __init display_file_info(CHAR16 *name, struct file *file, char *options)
+{
+    if ( file == &cfg )
+        return;
+
+    PrintStr(name);
+    PrintStr(L": ");
+    DisplayUint(file->addr, 2 * sizeof(file->addr));
+    PrintStr(L"-");
+    DisplayUint(file->addr + file->size, 2 * sizeof(file->addr));
+    PrintStr(newline);
+
+    efi_arch_handle_module(file, name, options);
+}
+
 static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
                              struct file *file, char *options)
 {
@@ -572,6 +588,7 @@  static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
                          HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
         ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
                                     PFN_UP(size), &file->addr);
+        file->need_to_free = true;
     }
     if ( EFI_ERROR(ret) )
     {
@@ -581,16 +598,7 @@  static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
     else
     {
         file->size = size;
-        if ( file != &cfg )
-        {
-            PrintStr(name);
-            PrintStr(L": ");
-            DisplayUint(file->addr, 2 * sizeof(file->addr));
-            PrintStr(L"-");
-            DisplayUint(file->addr + size, 2 * sizeof(file->addr));
-            PrintStr(newline);
-            efi_arch_handle_module(file, name, options);
-        }
+        display_file_info(name, file, options);
 
         ret = FileHandle->Read(FileHandle, &file->size, file->ptr);
         if ( !EFI_ERROR(ret) && file->size != size )