[PATCH] efi: get_memory_map: add sufficient slack for memory descriptors

[Mark Rutland]

On Thu, Feb 12, 2015 at 03:31:02PM +0000, Ard Biesheuvel wrote:
> On 12 February 2015 at 23:16, Mark Rutland <mark.rutland at arm.com> wrote:
> > On Thu, Feb 12, 2015 at 02:56:51PM +0000, Ard Biesheuvel wrote:
> >> On 12 February 2015 at 22:47, Matt Fleming <matt at codeblueprint.co.uk> wrote:
> >> > On Thu, 12 Feb, at 06:39:46PM, Ard Biesheuvel wrote:
> >> >>
> >> >> I don't see how doing a single allocation could result in a single
> >> >> free region to be split into more than 1 occupied region + 2 free
> >> >> regions.
> >> >> So no, I don't think it is ...
> >> >
> >> > I don't think that's a guarantee we can make, nor is it something we
> >> > should rely upon.
> >> >
> >> > Please explain the user-visible failure that this patch fixes. Does your
> >> > machine refuse to boot?
> >>
> >> I am running UEFI under QEMU and Xen primarily at the moment, and
> >> experimenting with various build options in Tianocore, One of the
> >> options is preallocating and freeing blocks of various memory types,
> >> in a way that should result in the final number of distinct regions to
> >> be much lower. It could result however in a free memory region to be
> >> carved up in three instead of two, and that is a failure I have seen
> >> occur.
> >
> > The simple answer is that the machine will fail to boot, beause the
> > efi_get_memory_map helper will give up after one go, and propagate the
> > error. The arm-stub will give up when the error is encountered.
> >
> 
> Indeed.
> 
> >> > Why is the 'goto again' loop insufficient in
> >> > handling this scenario?
> >> >
> >>
> >> Yes, that should solve it as well, so if you prefer I reinstate that,
> >> I can respin the patch. There is a theoretical possibility that it
> >> would take more than just one more iteration, but that is highly
> >> unlikely and it should still always complete.
> >
> > Please reinstate the loop. It will make this far less fragile.
> >
> 
> 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.

In general we have no idea how many iterations will be required,
depending on the layout and fragmentation of memory.

The important part is that with the original loop we will either manage
to get a big enough allocation for the memory map or we will fail to
allocate memory and we'll terminate.

Thanks,
Mark.