| Message ID | 20150213160448.GA30567@codeblueprint.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> On 14 Feb 2015, at 00:04, Matt Fleming <matt@codeblueprint.co.uk> wrote: > >> On Thu, 12 Feb, at 11:31:02PM, Ard Biesheuvel wrote: >> >> Actually, looking again at the original patch, it appears that my >> analysis was incorrect regarding the possibility that the loop would >> never terminate. The only thing that could happen if desc_size > >> sizeof(efi_memory_desc_t) is that you need two iterations instead of >> one to get a pool allocation that is of sufficient size. >> So perhaps it is better to just revert the patch. >> >> My apologies for the hassle. > > This is what I've got queued up, > Acked-by: Ard Biesheuvel <ard@linaro.org> Thanks Matt > --- > > From 3f281b98ffc99e604a3988aa93304a3a591eeeb5 Mon Sep 17 00:00:00 2001 > From: Matt Fleming <matt.fleming@intel.com> > Date: Fri, 13 Feb 2015 15:46:56 +0000 > Subject: [PATCH] Revert "efi/libstub: Call get_memory_map() to obtain map and > desc sizes" > > This reverts commit d1a8d66b9177105e898e73716f97eb61842c457a. > > Ard reported a boot failure when running UEFI under Qemu and Xen and > experimenting with various Tianocore build options, > > "As it turns out, when allocating room for the UEFI memory map using > UEFI's AllocatePool (), it may result in two new memory map entries > being created, for instance, when using Tianocore's preallocated region > feature. For example, the following region > > 0x00005ead5000-0x00005ebfffff [Conventional Memory| | | | | |WB|WT|WC|UC] > > may be split like this > > 0x00005ead5000-0x00005eae2fff [Conventional Memory| | | | | |WB|WT|WC|UC] > 0x00005eae3000-0x00005eae4fff [Loader Data | | | | | |WB|WT|WC|UC] > 0x00005eae5000-0x00005ebfffff [Conventional Memory| | | | | |WB|WT|WC|UC] > > if the preallocated Loader Data region was chosen to be right in the > middle of the original free space. > > After patch d1a8d66b9177 ("efi/libstub: Call get_memory_map() to > obtain map and desc sizes"), this is not being dealt with correctly > anymore, as the existing logic to allocate room for a single additional > entry has become insufficient." > > Mark requested to reinstate the old loop we had before commit > d1a8d66b9177, which grows the memory map buffer until it's big enough to > hold the EFI memory map. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Matt Fleming <matt.fleming@intel.com> > --- > drivers/firmware/efi/libstub/efi-stub-helper.c | 16 ++++++---------- > 1 file changed, 6 insertions(+), 10 deletions(-) > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > index d073e3946383..9bd9fbb5bea8 100644 > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > @@ -66,29 +66,25 @@ efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg, > unsigned long key; > u32 desc_version; > > - *map_size = 0; > - *desc_size = 0; > - key = 0; > - status = efi_call_early(get_memory_map, map_size, NULL, > - &key, desc_size, &desc_version); > - if (status != EFI_BUFFER_TOO_SMALL) > - return EFI_LOAD_ERROR; > - > + *map_size = sizeof(*m) * 32; > +again: > /* > * Add an additional efi_memory_desc_t because we're doing an > * allocation which may be in a new descriptor region. > */ > - *map_size += *desc_size; > + *map_size += sizeof(*m); > status = efi_call_early(allocate_pool, EFI_LOADER_DATA, > *map_size, (void **)&m); > if (status != EFI_SUCCESS) > goto fail; > > + *desc_size = 0; > + key = 0; > status = efi_call_early(get_memory_map, map_size, m, > &key, desc_size, &desc_version); > if (status == EFI_BUFFER_TOO_SMALL) { > efi_call_early(free_pool, m); > - return EFI_LOAD_ERROR; > + goto again; > } > > if (status != EFI_SUCCESS) > -- > 1.9.3 > > -- > Matt Fleming, Intel Open Source Technology Center
On Fri, Feb 13, 2015 at 04:04:48PM +0000, Matt Fleming wrote: > On Thu, 12 Feb, at 11:31:02PM, Ard Biesheuvel wrote: > > > > Actually, looking again at the original patch, it appears that my > > analysis was incorrect regarding the possibility that the loop would > > never terminate. The only thing that could happen if desc_size > > > sizeof(efi_memory_desc_t) is that you need two iterations instead of > > one to get a pool allocation that is of sufficient size. > > So perhaps it is better to just revert the patch. > > > > My apologies for the hassle. > > This is what I've got queued up, Looks sane to me: Acked-by: Mark Rutland <mark.rutland@arm.com> Thanks, Mark. > > --- > > From 3f281b98ffc99e604a3988aa93304a3a591eeeb5 Mon Sep 17 00:00:00 2001 > From: Matt Fleming <matt.fleming@intel.com> > Date: Fri, 13 Feb 2015 15:46:56 +0000 > Subject: [PATCH] Revert "efi/libstub: Call get_memory_map() to obtain map and > desc sizes" > > This reverts commit d1a8d66b9177105e898e73716f97eb61842c457a. > > Ard reported a boot failure when running UEFI under Qemu and Xen and > experimenting with various Tianocore build options, > > "As it turns out, when allocating room for the UEFI memory map using > UEFI's AllocatePool (), it may result in two new memory map entries > being created, for instance, when using Tianocore's preallocated region > feature. For example, the following region > > 0x00005ead5000-0x00005ebfffff [Conventional Memory| | | | | |WB|WT|WC|UC] > > may be split like this > > 0x00005ead5000-0x00005eae2fff [Conventional Memory| | | | | |WB|WT|WC|UC] > 0x00005eae3000-0x00005eae4fff [Loader Data | | | | | |WB|WT|WC|UC] > 0x00005eae5000-0x00005ebfffff [Conventional Memory| | | | | |WB|WT|WC|UC] > > if the preallocated Loader Data region was chosen to be right in the > middle of the original free space. > > After patch d1a8d66b9177 ("efi/libstub: Call get_memory_map() to > obtain map and desc sizes"), this is not being dealt with correctly > anymore, as the existing logic to allocate room for a single additional > entry has become insufficient." > > Mark requested to reinstate the old loop we had before commit > d1a8d66b9177, which grows the memory map buffer until it's big enough to > hold the EFI memory map. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Matt Fleming <matt.fleming@intel.com> > --- > drivers/firmware/efi/libstub/efi-stub-helper.c | 16 ++++++---------- > 1 file changed, 6 insertions(+), 10 deletions(-) > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > index d073e3946383..9bd9fbb5bea8 100644 > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > @@ -66,29 +66,25 @@ efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg, > unsigned long key; > u32 desc_version; > > - *map_size = 0; > - *desc_size = 0; > - key = 0; > - status = efi_call_early(get_memory_map, map_size, NULL, > - &key, desc_size, &desc_version); > - if (status != EFI_BUFFER_TOO_SMALL) > - return EFI_LOAD_ERROR; > - > + *map_size = sizeof(*m) * 32; > +again: > /* > * Add an additional efi_memory_desc_t because we're doing an > * allocation which may be in a new descriptor region. > */ > - *map_size += *desc_size; > + *map_size += sizeof(*m); > status = efi_call_early(allocate_pool, EFI_LOADER_DATA, > *map_size, (void **)&m); > if (status != EFI_SUCCESS) > goto fail; > > + *desc_size = 0; > + key = 0; > status = efi_call_early(get_memory_map, map_size, m, > &key, desc_size, &desc_version); > if (status == EFI_BUFFER_TOO_SMALL) { > efi_call_early(free_pool, m); > - return EFI_LOAD_ERROR; > + goto again; > } > > if (status != EFI_SUCCESS) > -- > 1.9.3 > > -- > Matt Fleming, Intel Open Source Technology Center >
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index d073e3946383..9bd9fbb5bea8 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -66,29 +66,25 @@ efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg, unsigned long key; u32 desc_version; - *map_size = 0; - *desc_size = 0; - key = 0; - status = efi_call_early(get_memory_map, map_size, NULL, - &key, desc_size, &desc_version); - if (status != EFI_BUFFER_TOO_SMALL) - return EFI_LOAD_ERROR; - + *map_size = sizeof(*m) * 32; +again: /* * Add an additional efi_memory_desc_t because we're doing an * allocation which may be in a new descriptor region. */ - *map_size += *desc_size; + *map_size += sizeof(*m); status = efi_call_early(allocate_pool, EFI_LOADER_DATA, *map_size, (void **)&m); if (status != EFI_SUCCESS) goto fail; + *desc_size = 0; + key = 0; status = efi_call_early(get_memory_map, map_size, m, &key, desc_size, &desc_version); if (status == EFI_BUFFER_TOO_SMALL) { efi_call_early(free_pool, m); - return EFI_LOAD_ERROR; + goto again; } if (status != EFI_SUCCESS)