[PATCH 1/2] x86/layout: Correct Xen's idea of its own memory layout

[PATCH 1/2] x86/layout: Correct Xen's idea of its own memory layout

[Andrew Cooper]

c/s b4cd59fe "x86: reorder .data and .init when linking" had an unintended
side effect, where xen_in_range() and the tboot S3 MAC were no longer correct.

In practice, it means that Xen's .data section is excluded from consideration,
which means:
 1) Default IOMMU construction for the hardware domain could create mappings.
 2) .data isn't included in the tboot MAC checked on resume from S3.

Adjust the comments and virtual address anchors used to define the regions.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/setup.c | 14 +++++++-------
 xen/arch/x86/tboot.c | 12 +++++++-----
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 66b7aba..dab67d5 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1685,7 +1685,7 @@ int __hwdom_init xen_in_range(unsigned long mfn)
     paddr_t start, end;
     int i;
 
-    enum { region_s3, region_text, region_bss, nr_regions };
+    enum { region_s3, region_ro, region_rw, nr_regions };
     static struct {
         paddr_t s, e;
     } xen_regions[nr_regions] __hwdom_initdata;
@@ -1696,12 +1696,12 @@ int __hwdom_init xen_in_range(unsigned long mfn)
         /* S3 resume code (and other real mode trampoline code) */
         xen_regions[region_s3].s = bootsym_phys(trampoline_start);
         xen_regions[region_s3].e = bootsym_phys(trampoline_end);
-        /* hypervisor code + data */
-        xen_regions[region_text].s =__pa(&_stext);
-        xen_regions[region_text].e = __pa(&__init_begin);
-        /* bss */
-        xen_regions[region_bss].s = __pa(&__bss_start);
-        xen_regions[region_bss].e = __pa(&__bss_end);
+        /* hypervisor .text + .rodata */
+        xen_regions[region_ro].s = __pa(&_stext);
+        xen_regions[region_ro].e = __pa(&__2M_rodata_end);
+        /* hypervisor .data + .bss */
+        xen_regions[region_rw].s = __pa(&__2M_rwdata_start);
+        xen_regions[region_rw].e = __pa(&__2M_rwdata_end);
     }
 
     start = (paddr_t)mfn << PAGE_SHIFT;
diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
index eac182d..a0d1d0f 100644
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -11,6 +11,7 @@
 #include <asm/processor.h>
 #include <asm/e820.h>
 #include <asm/tboot.h>
+#include <asm/setup.h>
 #include <crypto/vmac.h>
 
 /* tboot=<physical address of shared page> */
@@ -369,13 +370,14 @@ void tboot_shutdown(uint32_t shutdown_type)
         g_tboot_shared->mac_regions[0].start = bootsym_phys(trampoline_start);
         g_tboot_shared->mac_regions[0].size = bootsym_phys(trampoline_end) -
                                               bootsym_phys(trampoline_start);
-        /* hypervisor code + data */
+        /* hypervisor .text + .rodata */
         g_tboot_shared->mac_regions[1].start = (uint64_t)__pa(&_stext);
-        g_tboot_shared->mac_regions[1].size = __pa(&__init_begin) -
+        g_tboot_shared->mac_regions[1].size = __pa(&__2M_rodata_end) -
                                               __pa(&_stext);
-        /* bss */
-        g_tboot_shared->mac_regions[2].start = (uint64_t)__pa(&__bss_start);
-        g_tboot_shared->mac_regions[2].size = __pa(&__bss_end) - __pa(&__bss_start);
+        /* hypervisor .data + .bss */
+        g_tboot_shared->mac_regions[2].start = (uint64_t)__pa(&__2M_rwdata_start);
+        g_tboot_shared->mac_regions[2].size = __pa(&__2M_rwdata_end) -
+                                              __pa(&__2M_rwdata_start);
 
         /*
          * MAC domains and other Xen memory
-- 
2.1.4


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

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

[Andrew Cooper]

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.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/common/efi/boot.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index b6cbdad..82bf15b 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -120,11 +120,10 @@ static CHAR16 __initdata newline[] = L"\r\n";
  *     have to be disabled in xen/arch/arm/arm64/head.S; though BSS
  *     should be initialized somehow before use of variables living there,
  *   - use ebmalloc() in ARM/common EFI boot code,
- *   - call free_ebmalloc_unused_mem() somewhere in init code.
  */
 #define EBMALLOC_SIZE	MB(0)
 #else
-#define EBMALLOC_SIZE	MB(1)
+#define EBMALLOC_SIZE	KB(32)
 #endif
 
 static char __section(".bss.page_aligned") __aligned(PAGE_SIZE)
@@ -144,19 +143,6 @@ static void __init __maybe_unused *ebmalloc(size_t size)
     return ptr;
 }
 
-static void __init __maybe_unused free_ebmalloc_unused_mem(void)
-{
-    unsigned long start, end;
-
-    start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated);
-    end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem);
-
-    destroy_xen_mappings(start, end);
-    init_xenheap_pages(__pa(start), __pa(end));
-
-    printk(XENLOG_INFO "Freed %lukB unused BSS memory\n", (end - start) >> 10);
-}
-
 /*
  * Include architecture specific implementation here, which references the
  * static globals defined above.
@@ -1310,8 +1296,6 @@ void __init efi_init_memory(void)
     } *extra, *extra_head = NULL;
 #endif
 
-    free_ebmalloc_unused_mem();
-
     if ( !efi_enabled(EFI_BOOT) )
         return;
 
-- 
2.1.4


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

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

[Jan Beulich]

>>> On 28.02.17 at 16:20, <andrew.cooper3@citrix.com> 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?

Jan


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

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

[Andrew Cooper]

On 28/02/17 16:03, Jan Beulich wrote:
>>>> On 28.02.17 at 16:20, <andrew.cooper3@citrix.com> 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. This
       means that the code referring to Xen command line, loaded modules and
       EFI memory map, mostly in __start_xen(), will be further complicated
       and diverge from legacy BIOS cases. Additionally, both former things
       have to be placed below 4 GiB because their addresses are stored in
       multiboot_info_t structure which has 32-bit relevant members.


One way or another, if we don't want to shatter superpages, we either
must keep the entire allocation, or copy the content out later into a
smaller location once other heap facilities are available.

If we are copying data out, we might as well use EFI heap facilities
rather than rolling our own.

~Andrew

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

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

[Jan Beulich]

>>> On 28.02.17 at 17:08, <andrew.cooper3@citrix.com> wrote:
> On 28/02/17 16:03, Jan Beulich wrote:
>>>>> On 28.02.17 at 16:20, <andrew.cooper3@citrix.com> 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.

> This
>        means that the code referring to Xen command line, loaded modules and
>        EFI memory map, mostly in __start_xen(), will be further complicated
>        and diverge from legacy BIOS cases. Additionally, both former things
>        have to be placed below 4 GiB because their addresses are stored in
>        multiboot_info_t structure which has 32-bit relevant members.
> 
> 
> One way or another, if we don't want to shatter superpages, we either
> must keep the entire allocation, or copy the content out later into a
> smaller location once other heap facilities are available.
> 
> If we are copying data out, we might as well use EFI heap facilities
> rather than rolling our own.

Well, copying data later won't work, as there are pointers stored to
the original allocation.

Jan


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

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

[Daniel Kiper]

On Tue, Feb 28, 2017 at 09:14:24AM -0700, Jan Beulich wrote:
> >>> On 28.02.17 at 17:08, <andrew.cooper3@citrix.com> wrote:
> > On 28/02/17 16:03, Jan Beulich wrote:
> >>>>> On 28.02.17 at 16:20, <andrew.cooper3@citrix.com> 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.

> > This
> >        means that the code referring to Xen command line, loaded modules and
> >        EFI memory map, mostly in __start_xen(), will be further complicated
> >        and diverge from legacy BIOS cases. Additionally, both former things
> >        have to be placed below 4 GiB because their addresses are stored in
> >        multiboot_info_t structure which has 32-bit relevant members.
> >
> >
> > One way or another, if we don't want to shatter superpages, we either
> > must keep the entire allocation, or copy the content out later into a
> > smaller location once other heap facilities are available.
> >
> > If we are copying data out, we might as well use EFI heap facilities
> > rather than rolling our own.
>
> Well, copying data later won't work, as there are pointers stored to
> the original allocation.

This is not impossible. Just requires fiddling with mbi struct.

Daniel

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

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

[Jan Beulich]

>>> On 28.02.17 at 18:09, <daniel.kiper@oracle.com> wrote:
> On Tue, Feb 28, 2017 at 09:14:24AM -0700, Jan Beulich wrote:
>> >>> On 28.02.17 at 17:08, <andrew.cooper3@citrix.com> wrote:
>> > On 28/02/17 16:03, Jan Beulich wrote:
>> >>>>> On 28.02.17 at 16:20, <andrew.cooper3@citrix.com> 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.

>> > This
>> >        means that the code referring to Xen command line, loaded modules and
>> >        EFI memory map, mostly in __start_xen(), will be further complicated
>> >        and diverge from legacy BIOS cases. Additionally, both former things
>> >        have to be placed below 4 GiB because their addresses are stored in
>> >        multiboot_info_t structure which has 32-bit relevant members.
>> >
>> >
>> > One way or another, if we don't want to shatter superpages, we either
>> > must keep the entire allocation, or copy the content out later into a
>> > smaller location once other heap facilities are available.
>> >
>> > If we are copying data out, we might as well use EFI heap facilities
>> > rather than rolling our own.
>>
>> Well, copying data later won't work, as there are pointers stored to
>> the original allocation.
> 
> This is not impossible. Just requires fiddling with mbi struct.

That would cover this one specific use only, and even then how
do you know there weren't any derived pointers stored elsewhere?

Jan


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

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

[Daniel Kiper]

On Tue, Feb 28, 2017 at 10:24:36AM -0700, Jan Beulich wrote:
> >>> On 28.02.17 at 18:09, <daniel.kiper@oracle.com> wrote:
> > On Tue, Feb 28, 2017 at 09:14:24AM -0700, Jan Beulich wrote:
> >> >>> On 28.02.17 at 17:08, <andrew.cooper3@citrix.com> wrote:
> >> > On 28/02/17 16:03, Jan Beulich wrote:
> >> >>>>> On 28.02.17 at 16:20, <andrew.cooper3@citrix.com> 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.

> >> > This
> >> >        means that the code referring to Xen command line, loaded modules and
> >> >        EFI memory map, mostly in __start_xen(), will be further complicated
> >> >        and diverge from legacy BIOS cases. Additionally, both former things
> >> >        have to be placed below 4 GiB because their addresses are stored in
> >> >        multiboot_info_t structure which has 32-bit relevant members.
> >> >
> >> >
> >> > One way or another, if we don't want to shatter superpages, we either
> >> > must keep the entire allocation, or copy the content out later into a
> >> > smaller location once other heap facilities are available.
> >> >
> >> > If we are copying data out, we might as well use EFI heap facilities
> >> > rather than rolling our own.
> >>
> >> Well, copying data later won't work, as there are pointers stored to
> >> the original allocation.
> >
> > This is not impossible. Just requires fiddling with mbi struct.
>
> That would cover this one specific use only, and even then how
> do you know there weren't any derived pointers stored elsewhere?

Right, we should avoid it. It is too fragile approach.

Daniel

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

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

[Jan Beulich]

>>> On 28.02.17 at 19:45, <daniel.kiper@oracle.com> wrote:
> On Tue, Feb 28, 2017 at 10:24:36AM -0700, Jan Beulich wrote:
>> >>> On 28.02.17 at 18:09, <daniel.kiper@oracle.com> wrote:
>> > On Tue, Feb 28, 2017 at 09:14:24AM -0700, Jan Beulich wrote:
>> >> >>> On 28.02.17 at 17:08, <andrew.cooper3@citrix.com> wrote:
>> >> > On 28/02/17 16:03, Jan Beulich wrote:
>> >> >>>>> On 28.02.17 at 16:20, <andrew.cooper3@citrix.com> 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?

Jan


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

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

[Daniel Kiper]

On Wed, Mar 01, 2017 at 12:48:26AM -0700, Jan Beulich wrote:
> >>> On 28.02.17 at 19:45, <daniel.kiper@oracle.com> wrote:
> > On Tue, Feb 28, 2017 at 10:24:36AM -0700, Jan Beulich wrote:
> >> >>> On 28.02.17 at 18:09, <daniel.kiper@oracle.com> wrote:
> >> > On Tue, Feb 28, 2017 at 09:14:24AM -0700, Jan Beulich wrote:
> >> >> >>> On 28.02.17 at 17:08, <andrew.cooper3@citrix.com> wrote:
> >> >> > On 28/02/17 16:03, Jan Beulich wrote:
> >> >> >>>>> On 28.02.17 at 16:20, <andrew.cooper3@citrix.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 01.03.17 at 09:20, <daniel.kiper@oracle.com> wrote:
> On Wed, Mar 01, 2017 at 12:48:26AM -0700, Jan Beulich wrote:
>> >>> On 28.02.17 at 19:45, <daniel.kiper@oracle.com> wrote:
>> > On Tue, Feb 28, 2017 at 10:24:36AM -0700, Jan Beulich wrote:
>> >> >>> On 28.02.17 at 18:09, <daniel.kiper@oracle.com> wrote:
>> >> > On Tue, Feb 28, 2017 at 09:14:24AM -0700, Jan Beulich wrote:
>> >> >> >>> On 28.02.17 at 17:08, <andrew.cooper3@citrix.com> wrote:
>> >> >> > On 28/02/17 16:03, Jan Beulich wrote:
>> >> >> >>>>> On 28.02.17 at 16:20, <andrew.cooper3@citrix.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Daniel Kiper]

On Wed, Mar 01, 2017 at 01:47:21AM -0700, Jan Beulich wrote:
> >>> On 01.03.17 at 09:20, <daniel.kiper@oracle.com> wrote:

[...]

> > 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().

Yes, I see that. And that is why I say "not nice" not fundamentally wrong.

Daniel

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

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

[Daniel Kiper]

On Tue, Feb 28, 2017 at 04:08:35PM +0000, Andrew Cooper wrote:
> On 28/02/17 16:03, Jan Beulich wrote:
> >>>> On 28.02.17 at 16:20, <andrew.cooper3@citrix.com> 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. This
>        means that the code referring to Xen command line, loaded modules and
>        EFI memory map, mostly in __start_xen(), will be further complicated
>        and diverge from legacy BIOS cases. Additionally, both former things
>        have to be placed below 4 GiB because their addresses are stored in
>        multiboot_info_t structure which has 32-bit relevant members.
>
>
> One way or another, if we don't want to shatter superpages, we either
> must keep the entire allocation, or copy the content out later into a
> smaller location once other heap facilities are available.

Hmmm... Why BSS free "shatter superpages" and .init.* sections free does not?

In regards to tboot I think that we should just take into account during
calculation ebmalloc_mem allocated part only. Maybe move of ebmalloc_mem
region to the end of BSS would help somehow here.

Daniel

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

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

[Andrew Cooper]

On 28/02/17 17:41, Daniel Kiper wrote:
> On Tue, Feb 28, 2017 at 04:08:35PM +0000, Andrew Cooper wrote:
>> On 28/02/17 16:03, Jan Beulich wrote:
>>>>>> On 28.02.17 at 16:20, <andrew.cooper3@citrix.com> 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. This
>>        means that the code referring to Xen command line, loaded modules and
>>        EFI memory map, mostly in __start_xen(), will be further complicated
>>        and diverge from legacy BIOS cases. Additionally, both former things
>>        have to be placed below 4 GiB because their addresses are stored in
>>        multiboot_info_t structure which has 32-bit relevant members.
>>
>>
>> One way or another, if we don't want to shatter superpages, we either
>> must keep the entire allocation, or copy the content out later into a
>> smaller location once other heap facilities are available.
> Hmmm... Why BSS free "shatter superpages" and .init.* sections free does not?

Xen is purposefully laid out like this:

.text, 2M aligned, R/X
.rodata, 2M aligned, R/NX
.init.*, 2M aligned, R/W/X (reclaimed)
.data + .bss, 2M aligned, R/W/NX

(In reality there is a syslinux bug which caused me to revert the 2M
alignment in non-EFI builds, so we are still using 4k alignment, but I
need to find a way to work around this and move back to enforcing the 2M
alignment.)

When .init gets reclaimed, this leaves a (deliberate) hole which is all
2M aligned, which has no impact on the adjacent 2M superpages.

When ebmalloc gets reclaimed, it must shatter one or two superpages
mapping the .data/.bss section.

> In regards to tboot I think that we should just take into account during
> calculation ebmalloc_mem allocated part only. Maybe move of ebmalloc_mem
> region to the end of BSS would help somehow here.

At the moment, all the ranges of Xen VA space are bounded by linker symbols.

It is certainly possible to could account for the ebmalloc object in the
middle of the BSS, but it is turning into a layering violation.

~Andrew

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

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

[Daniel Kiper]

On Tue, Feb 28, 2017 at 05:58:26PM +0000, Andrew Cooper wrote:
> On 28/02/17 17:41, Daniel Kiper wrote:
> > On Tue, Feb 28, 2017 at 04:08:35PM +0000, Andrew Cooper wrote:
> >> On 28/02/17 16:03, Jan Beulich wrote:
> >>>>>> On 28.02.17 at 16:20, <andrew.cooper3@citrix.com> 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. This
> >>        means that the code referring to Xen command line, loaded modules and
> >>        EFI memory map, mostly in __start_xen(), will be further complicated
> >>        and diverge from legacy BIOS cases. Additionally, both former things
> >>        have to be placed below 4 GiB because their addresses are stored in
> >>        multiboot_info_t structure which has 32-bit relevant members.
> >>
> >>
> >> One way or another, if we don't want to shatter superpages, we either
> >> must keep the entire allocation, or copy the content out later into a
> >> smaller location once other heap facilities are available.
> > Hmmm... Why BSS free "shatter superpages" and .init.* sections free does not?
>
> Xen is purposefully laid out like this:
>
> .text, 2M aligned, R/X
> .rodata, 2M aligned, R/NX
> .init.*, 2M aligned, R/W/X (reclaimed)
> .data + .bss, 2M aligned, R/W/NX

AIUI, the purpose is to have properly mapped (in terms of R/W/X/NX)
sections. Right?

> (In reality there is a syslinux bug which caused me to revert the 2M
> alignment in non-EFI builds, so we are still using 4k alignment, but I
> need to find a way to work around this and move back to enforcing the 2M
> alignment.)
>
> When .init gets reclaimed, this leaves a (deliberate) hole which is all
> 2M aligned, which has no impact on the adjacent 2M superpages.
>
> When ebmalloc gets reclaimed, it must shatter one or two superpages
> mapping the .data/.bss section.

Looking at this I do not have a better idea for fix off the top of my head.
So, I have a feeling that we should drop free_ebmalloc_unused_mem() and leave
comment around ebmalloc() why we do not reclaim unused ebmalloc_mem region.
Should I post a patch or whether you would like to do it?

> > In regards to tboot I think that we should just take into account during
> > calculation ebmalloc_mem allocated part only. Maybe move of ebmalloc_mem
> > region to the end of BSS would help somehow here.
>
> At the moment, all the ranges of Xen VA space are bounded by linker symbols.
>
> It is certainly possible to could account for the ebmalloc object in the
> middle of the BSS, but it is turning into a layering violation.

I am not sure what do you mean by "layering violation".

Daniel

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

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

[Andrew Cooper]

On 28/02/17 19:01, Daniel Kiper wrote:
> On Tue, Feb 28, 2017 at 05:58:26PM +0000, Andrew Cooper wrote:
>> On 28/02/17 17:41, Daniel Kiper wrote:
>>> On Tue, Feb 28, 2017 at 04:08:35PM +0000, Andrew Cooper wrote:
>>>> On 28/02/17 16:03, Jan Beulich wrote:
>>>>>>>> On 28.02.17 at 16:20, <andrew.cooper3@citrix.com> 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. This
>>>>        means that the code referring to Xen command line, loaded modules and
>>>>        EFI memory map, mostly in __start_xen(), will be further complicated
>>>>        and diverge from legacy BIOS cases. Additionally, both former things
>>>>        have to be placed below 4 GiB because their addresses are stored in
>>>>        multiboot_info_t structure which has 32-bit relevant members.
>>>>
>>>>
>>>> One way or another, if we don't want to shatter superpages, we either
>>>> must keep the entire allocation, or copy the content out later into a
>>>> smaller location once other heap facilities are available.
>>> Hmmm... Why BSS free "shatter superpages" and .init.* sections free does not?
>> Xen is purposefully laid out like this:
>>
>> .text, 2M aligned, R/X
>> .rodata, 2M aligned, R/NX
>> .init.*, 2M aligned, R/W/X (reclaimed)
>> .data + .bss, 2M aligned, R/W/NX
> AIUI, the purpose is to have properly mapped (in terms of R/W/X/NX)
> sections. Right?

When I did the original work, it was both to get proper pagetable
permissions, and to get all of the compiled bits of Xen living in 2M
superpages (which allows the entire hypervisor to operate in 5 TLB entries!)

>
>> (In reality there is a syslinux bug which caused me to revert the 2M
>> alignment in non-EFI builds, so we are still using 4k alignment, but I
>> need to find a way to work around this and move back to enforcing the 2M
>> alignment.)
>>
>> When .init gets reclaimed, this leaves a (deliberate) hole which is all
>> 2M aligned, which has no impact on the adjacent 2M superpages.
>>
>> When ebmalloc gets reclaimed, it must shatter one or two superpages
>> mapping the .data/.bss section.
> Looking at this I do not have a better idea for fix off the top of my head.
> So, I have a feeling that we should drop free_ebmalloc_unused_mem() and leave
> comment around ebmalloc() why we do not reclaim unused ebmalloc_mem region.
> Should I post a patch or whether you would like to do it?

The patch at the root of this thread is basically that.

As a stopgap solution to unblock staging, I think I should drop the
adjustment of MB(1) to KB(32) and submit the patch.

This satisfies Jan's request to not make the current buffer smaller than
it currently is, and will give us more time to discuss alternative
solutions, at the cost of wasting 1MB of RAM.  (Not exactly breaking the
bank these days, but definitely something which should be fixed before
4.9 ships.)

>
>>> In regards to tboot I think that we should just take into account during
>>> calculation ebmalloc_mem allocated part only. Maybe move of ebmalloc_mem
>>> region to the end of BSS would help somehow here.
>> At the moment, all the ranges of Xen VA space are bounded by linker symbols.
>>
>> It is certainly possible to could account for the ebmalloc object in the
>> middle of the BSS, but it is turning into a layering violation.
> I am not sure what do you mean by "layering violation".

It feels hacky, and the wrong way to solve the problem.

~Andrew

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

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

[Daniel Kiper]

On Tue, Feb 28, 2017 at 07:09:14PM +0000, Andrew Cooper wrote:
> On 28/02/17 19:01, Daniel Kiper wrote:
> > On Tue, Feb 28, 2017 at 05:58:26PM +0000, Andrew Cooper wrote:
> >> On 28/02/17 17:41, Daniel Kiper wrote:
> >>> On Tue, Feb 28, 2017 at 04:08:35PM +0000, Andrew Cooper wrote:
> >>>> On 28/02/17 16:03, Jan Beulich wrote:
> >>>>>>>> On 28.02.17 at 16:20, <andrew.cooper3@citrix.com> 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. This
> >>>>        means that the code referring to Xen command line, loaded modules and
> >>>>        EFI memory map, mostly in __start_xen(), will be further complicated
> >>>>        and diverge from legacy BIOS cases. Additionally, both former things
> >>>>        have to be placed below 4 GiB because their addresses are stored in
> >>>>        multiboot_info_t structure which has 32-bit relevant members.
> >>>>
> >>>>
> >>>> One way or another, if we don't want to shatter superpages, we either
> >>>> must keep the entire allocation, or copy the content out later into a
> >>>> smaller location once other heap facilities are available.
> >>> Hmmm... Why BSS free "shatter superpages" and .init.* sections free does not?
> >> Xen is purposefully laid out like this:
> >>
> >> .text, 2M aligned, R/X
> >> .rodata, 2M aligned, R/NX
> >> .init.*, 2M aligned, R/W/X (reclaimed)
> >> .data + .bss, 2M aligned, R/W/NX
> > AIUI, the purpose is to have properly mapped (in terms of R/W/X/NX)
> > sections. Right?
>
> When I did the original work, it was both to get proper pagetable
> permissions, and to get all of the compiled bits of Xen living in 2M
> superpages (which allows the entire hypervisor to operate in 5 TLB entries!)

Thanks for explanation.

> >> (In reality there is a syslinux bug which caused me to revert the 2M
> >> alignment in non-EFI builds, so we are still using 4k alignment, but I
> >> need to find a way to work around this and move back to enforcing the 2M
> >> alignment.)
> >>
> >> When .init gets reclaimed, this leaves a (deliberate) hole which is all
> >> 2M aligned, which has no impact on the adjacent 2M superpages.
> >>
> >> When ebmalloc gets reclaimed, it must shatter one or two superpages
> >> mapping the .data/.bss section.
> > Looking at this I do not have a better idea for fix off the top of my head.
> > So, I have a feeling that we should drop free_ebmalloc_unused_mem() and leave
> > comment around ebmalloc() why we do not reclaim unused ebmalloc_mem region.
> > Should I post a patch or whether you would like to do it?
>
> The patch at the root of this thread is basically that.

Yep, I know.

> As a stopgap solution to unblock staging, I think I should drop the
> adjustment of MB(1) to KB(32) and submit the patch.

Sounds like a plan.

> This satisfies Jan's request to not make the current buffer smaller than
> it currently is, and will give us more time to discuss alternative
> solutions, at the cost of wasting 1MB of RAM.  (Not exactly breaking the
> bank these days, but definitely something which should be fixed before
> 4.9 ships.)

Granted!

Daniel

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

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

[Jan Beulich]

>>> On 28.02.17 at 20:09, <andrew.cooper3@citrix.com> wrote:
> On 28/02/17 19:01, Daniel Kiper wrote:
>> Looking at this I do not have a better idea for fix off the top of my head.

Well, one better solution was already suggested: Put it in .init.data and
suppress the freeing of the used part of it. The other option is to put
a section along the lines of Linux'es .brk _after_ .bss (whether ahead
of or after _end would need to be seen), replacing the uses of
__2M_rwdata_end by a runtime calculated value (and freeing space
past the used portion accordingly, without shattering large pages if
used).

>> So, I have a feeling that we should drop free_ebmalloc_unused_mem() and leave
>> comment around ebmalloc() why we do not reclaim unused ebmalloc_mem region.
>> Should I post a patch or whether you would like to do it?
> 
> The patch at the root of this thread is basically that.
> 
> As a stopgap solution to unblock staging, I think I should drop the
> adjustment of MB(1) to KB(32) and submit the patch.
> 
> This satisfies Jan's request to not make the current buffer smaller than
> it currently is, and will give us more time to discuss alternative
> solutions, at the cost of wasting 1MB of RAM.  (Not exactly breaking the
> bank these days, but definitely something which should be fixed before
> 4.9 ships.)

Yes, as an immediate band aid I can live with this.

Jan


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

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

[Daniel Kiper]

On Wed, Mar 01, 2017 at 01:02:55AM -0700, Jan Beulich wrote:
> >>> On 28.02.17 at 20:09, <andrew.cooper3@citrix.com> wrote:
> > On 28/02/17 19:01, Daniel Kiper wrote:
> >> Looking at this I do not have a better idea for fix off the top of my head.
>
> Well, one better solution was already suggested: Put it in .init.data and

This will increase Xen binary size. So, I prefer to leave it in BSS if possible.

> suppress the freeing of the used part of it. The other option is to put
> a section along the lines of Linux'es .brk _after_ .bss (whether ahead
> of or after _end would need to be seen), replacing the uses of
> __2M_rwdata_end by a runtime calculated value (and freeing space
> past the used portion accordingly, without shattering large pages if
> used).

OK, make sense for me. However, from a bootloader point of view it should be
still part of BSS. Otherwise, the bootloader can put the end of Xen image too
close to reserved/no ram/etc. region. This does not seem important right now
but after applying the rest of my patch series it can make a pain.

Daniel

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

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

[Jan Beulich]

>>> On 01.03.17 at 10:00, <daniel.kiper@oracle.com> wrote:
> On Wed, Mar 01, 2017 at 01:02:55AM -0700, Jan Beulich wrote:
>> >>> On 28.02.17 at 20:09, <andrew.cooper3@citrix.com> wrote:
>> > On 28/02/17 19:01, Daniel Kiper wrote:
>> >> Looking at this I do not have a better idea for fix off the top of my head.
>>
>> Well, one better solution was already suggested: Put it in .init.data and
> 
> This will increase Xen binary size. So, I prefer to leave it in BSS if 
> possible.

I don't think that's very important a criteria.

>> suppress the freeing of the used part of it. The other option is to put
>> a section along the lines of Linux'es .brk _after_ .bss (whether ahead
>> of or after _end would need to be seen), replacing the uses of
>> __2M_rwdata_end by a runtime calculated value (and freeing space
>> past the used portion accordingly, without shattering large pages if
>> used).
> 
> OK, make sense for me. However, from a bootloader point of view it should be
> still part of BSS. Otherwise, the bootloader can put the end of Xen image too
> close to reserved/no ram/etc. region. This does not seem important right now
> but after applying the rest of my patch series it can make a pain.

I'm afraid I don't understand: It needs to be part of the ELF
load segments or PE32+ loaded sections, but whether that's
part of .bss or an adjacent @nobits section at the source level
shouldn't matter at all.

Jan


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

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

[Daniel Kiper]

On Wed, Mar 01, 2017 at 02:08:17AM -0700, Jan Beulich wrote:
> >>> On 01.03.17 at 10:00, <daniel.kiper@oracle.com> wrote:
> > On Wed, Mar 01, 2017 at 01:02:55AM -0700, Jan Beulich wrote:

[...]

> >> suppress the freeing of the used part of it. The other option is to put
> >> a section along the lines of Linux'es .brk _after_ .bss (whether ahead
> >> of or after _end would need to be seen), replacing the uses of
> >> __2M_rwdata_end by a runtime calculated value (and freeing space
> >> past the used portion accordingly, without shattering large pages if
> >> used).
> >
> > OK, make sense for me. However, from a bootloader point of view it should be
> > still part of BSS. Otherwise, the bootloader can put the end of Xen image too
> > close to reserved/no ram/etc. region. This does not seem important right now
> > but after applying the rest of my patch series it can make a pain.
>
> I'm afraid I don't understand: It needs to be part of the ELF
> load segments or PE32+ loaded sections, but whether that's
> part of .bss or an adjacent @nobits section at the source level
> shouldn't matter at all.

At source level I do not care much. And if it is a part "of the ELF load
segments" and "PE32+ loaded sections" then it is OK for me.

Daniel

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

Re: [PATCH 1/2] x86/layout: Correct Xen's idea of its own memory layout

[Jan Beulich]

>>> On 28.02.17 at 16:20, <andrew.cooper3@citrix.com> wrote:
> c/s b4cd59fe "x86: reorder .data and .init when linking" had an unintended
> side effect, where xen_in_range() and the tboot S3 MAC were no longer correct.
> 
> In practice, it means that Xen's .data section is excluded from 
> consideration,
> which means:
>  1) Default IOMMU construction for the hardware domain could create mappings.
>  2) .data isn't included in the tboot MAC checked on resume from S3.
> 
> Adjust the comments and virtual address anchors used to define the regions.

Reported-by: Jan Beulich <jbeulich@suse.com>

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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