| Message ID | 20200914115013.814079-2-hudson@trmm.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi: Unified Xen hypervisor/kernel/initrd images | expand |
On Mon, Sep 14, 2020 at 07:50:10AM -0400, Trammell Hudson wrote: > The config file, kernel, initrd, etc should only be freed if they > are allocated with the UEFI allocator. > > Signed-off-by: Trammell Hudson <hudson@trmm.net> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/common/efi/boot.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index 4022a672c9..7156139174 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(); > @@ -572,6 +573,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; Strictly speaking, don't you need to set need_to_free only if AllocatePages has succeed? I guess it doesn't matter much because addr would be zapped to 0 if allocation fails. Thanks, Roger.
On 16.09.2020 08:43, Roger Pau Monné wrote: > On Mon, Sep 14, 2020 at 07:50:10AM -0400, Trammell Hudson wrote: >> @@ -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)); All these look to be able to become just "if ( xyz.need_to_free )" if ... >> @@ -572,6 +573,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; > > Strictly speaking, don't you need to set need_to_free only if > AllocatePages has succeed? ... this was followed, so I think the adjustment wants making. > I guess it doesn't matter much because addr > would be zapped to 0 if allocation fails. Perhaps this zapping then also becomes unnecessary, albeit I didn't look very closely yet. Jan
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 4022a672c9..7156139174 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(); @@ -572,6 +573,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) ) {
The config file, kernel, initrd, etc should only be freed if they are allocated with the UEFI allocator. Signed-off-by: Trammell Hudson <hudson@trmm.net> --- xen/common/efi/boot.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)