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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.03.17 at 09:20, <daniel.kiper@xxxxxxxxxx> wrote:
> 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).

Note the "In general ... should ...", as opposed to the "must" later on.
The situation here simply falls outside of the general case, and there's
a reason the spec isn't strict here. ARM, btw., already has such a use
in efi_arch_handle_cmdline().

Jan


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