| Message ID | 1413951525-17657-1-git-send-email-roy.franz@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, 2014-10-21 at 21:18 -0700, Roy Franz wrote: > efi_arch_allocate_mmap_buffer() allocates a buffer for the EFI memory map, and > for ARM it allocates a larger buffer than requested. This is done to account > for the increase in the map size that may occur when the allocation is made. > The previous code allocated a larger buffer, but did not adjust the size to > match. This caused the later call to GetMemoryMap() to fail with a > BUFFER_TOO_SMALL error, since the original, smaller size was used. This patch > changes the argument to efi_arch_allocate_mmap_buffer() to be a pointer to > UINTN, and the ARM version updates the size on a successful allocation. > The x86 version uses a different allocation method, so only the function > argument type is changed. > Also add decode of the BUFFER_TOO_SMALL error code to PrintErrMesg(). > > Signed-off-by: Roy Franz <roy.franz@linaro.org> For the arm side: Acked-by: Ian Campbell <ian.campbell@citrix.com> > --- > xen/arch/arm/efi/efi-boot.h | 6 ++++-- > xen/arch/x86/efi/efi-boot.h | 4 ++-- > xen/common/efi/boot.c | 5 ++++- > 3 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h > index d40d8b2..639942d 100644 > --- a/xen/arch/arm/efi/efi-boot.h > +++ b/xen/arch/arm/efi/efi-boot.h > @@ -370,14 +370,16 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *sect > { > } > > -static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) > +static void *__init efi_arch_allocate_mmap_buffer(UINTN *map_size) > { > void *ptr; > EFI_STATUS status; > + UINTN map_size_alloc = *map_size + EFI_PAGE_SIZE; > > - status = efi_bs->AllocatePool(EfiLoaderData, map_size + EFI_PAGE_SIZE, &ptr); > + status = efi_bs->AllocatePool(EfiLoaderData, map_size_alloc, &ptr); > if ( status != EFI_SUCCESS ) > return NULL; > + *map_size = map_size_alloc; > return ptr; > } > > diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h > index 4348cfe..454ffb6 100644 > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -190,10 +190,10 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, > > } > > -static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) > +static void *__init efi_arch_allocate_mmap_buffer(UINTN *map_size) > { > place_string(&mbi.mem_upper, NULL); > - mbi.mem_upper -= map_size; > + mbi.mem_upper -= *map_size; > mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR); > if ( mbi.mem_upper < xen_phys_start ) > return NULL; > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index f272171..4257341 100644 > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -271,6 +271,9 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) > case EFI_COMPROMISED_DATA: > mesg = L"Compromised data"; > break; > + case EFI_BUFFER_TOO_SMALL: > + mesg = L"Buffer too small"; > + break; > default: > PrintErr(L"ErrCode: "); > DisplayUint(ErrCode, 0); > @@ -1038,7 +1041,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > > efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key, > &efi_mdesc_size, &mdesc_ver); > - efi_memmap = efi_arch_allocate_mmap_buffer(efi_memmap_size); > + efi_memmap = efi_arch_allocate_mmap_buffer(&efi_memmap_size); > if ( !efi_memmap ) > blexit(L"Unable to allocate memory for EFI memory map"); >
>>> On 22.10.14 at 06:18, <roy.franz@linaro.org> wrote: > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -271,6 +271,9 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) > case EFI_COMPROMISED_DATA: > mesg = L"Compromised data"; > break; > + case EFI_BUFFER_TOO_SMALL: > + mesg = L"Buffer too small"; > + break; I don't really mind this change, but ... > @@ -1038,7 +1041,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE > *SystemTable) > > efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key, > &efi_mdesc_size, &mdesc_ver); > - efi_memmap = efi_arch_allocate_mmap_buffer(efi_memmap_size); > + efi_memmap = efi_arch_allocate_mmap_buffer(&efi_memmap_size); > if ( !efi_memmap ) > blexit(L"Unable to allocate memory for EFI memory map"); ... it would not have helped here, as you don't even get to see the error code here. Jan
On Wed, 2014-10-22 at 11:07 +0100, Jan Beulich wrote: > >>> On 22.10.14 at 06:18, <roy.franz@linaro.org> wrote: > > --- a/xen/common/efi/boot.c > > +++ b/xen/common/efi/boot.c > > @@ -271,6 +271,9 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) > > case EFI_COMPROMISED_DATA: > > mesg = L"Compromised data"; > > break; > > + case EFI_BUFFER_TOO_SMALL: > > + mesg = L"Buffer too small"; > > + break; > > I don't really mind this change, but ... > > > @@ -1038,7 +1041,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE > > *SystemTable) > > > > efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key, > > &efi_mdesc_size, &mdesc_ver); > > - efi_memmap = efi_arch_allocate_mmap_buffer(efi_memmap_size); > > + efi_memmap = efi_arch_allocate_mmap_buffer(&efi_memmap_size); > > if ( !efi_memmap ) > > blexit(L"Unable to allocate memory for EFI memory map"); > > ... it would not have helped here, as you don't even get to see the > error code here. This patch came about because I was seeing: Cannot obtain memory map: ErrCode: 0x8000000000000005 which I suppose is a different path. I don't think the allocation above would have ever returned EFI_BUFFER_TOO_SMALL, would it? Ian.
On Wed, Oct 22, 2014 at 3:42 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Wed, 2014-10-22 at 11:07 +0100, Jan Beulich wrote: >> >>> On 22.10.14 at 06:18, <roy.franz@linaro.org> wrote: >> > --- a/xen/common/efi/boot.c >> > +++ b/xen/common/efi/boot.c >> > @@ -271,6 +271,9 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) >> > case EFI_COMPROMISED_DATA: >> > mesg = L"Compromised data"; >> > break; >> > + case EFI_BUFFER_TOO_SMALL: >> > + mesg = L"Buffer too small"; >> > + break; >> >> I don't really mind this change, but ... >> >> > @@ -1038,7 +1041,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE >> > *SystemTable) >> > >> > efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key, >> > &efi_mdesc_size, &mdesc_ver); >> > - efi_memmap = efi_arch_allocate_mmap_buffer(efi_memmap_size); >> > + efi_memmap = efi_arch_allocate_mmap_buffer(&efi_memmap_size); >> > if ( !efi_memmap ) >> > blexit(L"Unable to allocate memory for EFI memory map"); >> >> ... it would not have helped here, as you don't even get to see the >> error code here. > > This patch came about because I was seeing: > Cannot obtain memory map: ErrCode: 0x8000000000000005 > which I suppose is a different path. I don't think the allocation above > would have ever returned EFI_BUFFER_TOO_SMALL, would it? > > Ian. The error there is generic since for ARM we do an AllocatePool(), and for x86 we use the string buffer that is allocated before the _start symbol. We don't have an EFI error code to decode at this point - the EFI error code only exists in the ARM efi_arch_allocate_mmap_buffer() function. Roy
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h index d40d8b2..639942d 100644 --- a/xen/arch/arm/efi/efi-boot.h +++ b/xen/arch/arm/efi/efi-boot.h @@ -370,14 +370,16 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *sect { } -static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) +static void *__init efi_arch_allocate_mmap_buffer(UINTN *map_size) { void *ptr; EFI_STATUS status; + UINTN map_size_alloc = *map_size + EFI_PAGE_SIZE; - status = efi_bs->AllocatePool(EfiLoaderData, map_size + EFI_PAGE_SIZE, &ptr); + status = efi_bs->AllocatePool(EfiLoaderData, map_size_alloc, &ptr); if ( status != EFI_SUCCESS ) return NULL; + *map_size = map_size_alloc; return ptr; } diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 4348cfe..454ffb6 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -190,10 +190,10 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, } -static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) +static void *__init efi_arch_allocate_mmap_buffer(UINTN *map_size) { place_string(&mbi.mem_upper, NULL); - mbi.mem_upper -= map_size; + mbi.mem_upper -= *map_size; mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR); if ( mbi.mem_upper < xen_phys_start ) return NULL; diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index f272171..4257341 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -271,6 +271,9 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) case EFI_COMPROMISED_DATA: mesg = L"Compromised data"; break; + case EFI_BUFFER_TOO_SMALL: + mesg = L"Buffer too small"; + break; default: PrintErr(L"ErrCode: "); DisplayUint(ErrCode, 0); @@ -1038,7 +1041,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key, &efi_mdesc_size, &mdesc_ver); - efi_memmap = efi_arch_allocate_mmap_buffer(efi_memmap_size); + efi_memmap = efi_arch_allocate_mmap_buffer(&efi_memmap_size); if ( !efi_memmap ) blexit(L"Unable to allocate memory for EFI memory map");
efi_arch_allocate_mmap_buffer() allocates a buffer for the EFI memory map, and for ARM it allocates a larger buffer than requested. This is done to account for the increase in the map size that may occur when the allocation is made. The previous code allocated a larger buffer, but did not adjust the size to match. This caused the later call to GetMemoryMap() to fail with a BUFFER_TOO_SMALL error, since the original, smaller size was used. This patch changes the argument to efi_arch_allocate_mmap_buffer() to be a pointer to UINTN, and the ARM version updates the size on a successful allocation. The x86 version uses a different allocation method, so only the function argument type is changed. Also add decode of the BUFFER_TOO_SMALL error code to PrintErrMesg(). Signed-off-by: Roy Franz <roy.franz@linaro.org> --- xen/arch/arm/efi/efi-boot.h | 6 ++++-- xen/arch/x86/efi/efi-boot.h | 4 ++-- xen/common/efi/boot.c | 5 ++++- 3 files changed, 10 insertions(+), 5 deletions(-)