Re: [Xen-devel] [PATCH 2/2] efi/boot: Don't free ebmalloc area at all

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, Mar 01, 2017 at 12:48:26AM -0700, Jan Beulich wrote:
> >>> On 28.02.17 at 19:45, <daniel.kiper@xxxxxxxxxx> wrote:
> > On Tue, Feb 28, 2017 at 10:24:36AM -0700, Jan Beulich wrote:
> >> >>> On 28.02.17 at 18:09, <daniel.kiper@xxxxxxxxxx> wrote:
> >> > On Tue, Feb 28, 2017 at 09:14:24AM -0700, Jan Beulich wrote:
> >> >> >>> On 28.02.17 at 17:08, <andrew.cooper3@xxxxxxxxxx> wrote:
> >> >> > On 28/02/17 16:03, Jan Beulich wrote:
> >> >> >>>>> On 28.02.17 at 16:20, <andrew.cooper3@xxxxxxxxxx> wrote:
> >> >> >>> Freeing part of the BSS back for general use proves to be 
> >> >> >>> problematic.  It
> >> >> > is
> >> >> >>> not accounted for in xen_in_range(), causing errors when 
> >> >> >>> constructing the
> >> >> >>> IOMMU tables, resulting in a failure to boot.
> >> >> >>>
> >> >> >>> Other smaller issues are that tboot treats the entire BSS as 
> >> >> >>> hypervisor
> >> >> > data,
> >> >> >>> creating and checking a MAC of it on S3, and that, by being 1MB in 
> >> >> >>> size,
> >> >> >>> freeing it guarentees to shatter the hypervisor superpage mappings.
> >> >> >>>
> >> >> >>> Judging by the content stored in it, 1MB is overkill on size.  Drop 
> >> >> >>> it to a
> >> >> >>> more-reasonable 32kB and keep the entire buffer around after boot.
> >> >> >> Well, that's just because right now there's only a single user. The
> >> >> >> reason I refused Daniel making it smaller than its predecessor is
> >> >> >> that we can't really give a good estimate of how much data may
> >> >> >> need storing there: The memory map can have hundreds of entries
> >> >> >> and command lines for modules may also be almost arbitrarily long.
> >> >> >>
> >> >> >> What I don't recall, Daniel: Why was it that we can't use EFI boot
> >> >> >> services allocations here?
> >> >> >
> >> >> > From the original commit message,
> >> >> >
> >> >> >     1) We could use native EFI allocation functions (e.g. 
> >> >> > AllocatePool()
> >> >> >        or AllocatePages()) to get memory chunk. However, later 
> >> >> > (somewhere
> >> >> >        in __start_xen()) we must copy its contents to safe place or 
> >> >> > reserve
> >> >> >        it in e820 memory map and map it in Xen virtual address space.
> >> >>
> >> >> Reading this again, I have to admit that I don't understand why any
> >> >> copying or reserving would need to be done. We'd need to do runtime
> >> >> allocations, sure, but I would have thought this goes without saying.
> >> >
> >> > We discussed this once but I do not remember the thread. In general Xen 
> >> > EFI
> >> > boot code should allocate memory as EfiLoaderData. However, later in
> >> > efi_arch_process_memory_map() we treat this memory type as free. This 
> >> > means
> >> > that it can be overwritten. So, that is why I mentioned copy or 
> >> > reservation.
> >>
> >> There's nothing disallowing runtime data allocations, afaik.
> >
> > Do you mean EfiRuntimeServicesData? Probably possible but not nice IMO.
> > AIUI, this is not intended to use in that way.
>
> And that you base on what statement(s) in the spec?

UEFI ver. 2.6, p. 154: In general, UEFI OS loaders and UEFI applications should
allocate memory (and pool) of type EfiLoaderData. UEFI boot service drivers must
allocate memory (and pool) of type EfiBootServicesData. UREFI runtime drivers
should allocate memory (and pool) of type EfiRuntimeServicesData (although such
allocation can only be made during boot services time).

UEFI ver. 2.6, p. 210, Default pool allocations from memory type row.

Xen is UEFI application, so, we should use EfiLoaderData. Of course we 
potentially
can use EfiRuntimeServicesData because it is not strictly forbidden. However, 
as you
can see above EfiRuntimeServicesData is targeted for UEFI runtime drivers and 
stuff
like that. So, that is why I said "not nice" in my earlier email.

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel