| Message ID | 20200907190027.669086-3-hudson@trmm.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi: Unified Xen hypervisor/kernel/initrd images | expand |
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.
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 --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 )