[PATCH] x86/setup: do not relocate below the end of current Xen image placement

[PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Daniel Kiper]

If it is possible we would like to have the Xen image higher than the
booloader put it and certainly do not overwrite the Xen code and data
during copy/relocation. Otherwise the Xen may crash silently at boot.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 xen/arch/x86/setup.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 32bb02e..be91d34 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -960,7 +960,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         }
         else
             end = 0;
-        if ( end > s )
+
+        /*
+         * Is the region size greater than zero?
+         * Does the region begins above or at the
+         * end of current Xen image placement?
+         */
+        if ( end > s && (end - reloc_size >= _end - _start) )
         {
             l4_pgentry_t *pl4e;
             l3_pgentry_t *pl3e;
-- 
1.7.10.4


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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Jan Beulich]

>>> On 27.11.17 at 16:41, <daniel.kiper@oracle.com> wrote:
> If it is possible we would like to have the Xen image higher than the
> booloader put it and certainly do not overwrite the Xen code and data
> during copy/relocation. Otherwise the Xen may crash silently at boot.

Is this something that you've actually observed happening? I'd be
curious about the particular numbers if so.

> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  xen/arch/x86/setup.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 32bb02e..be91d34 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -960,7 +960,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          }
>          else
>              end = 0;
> -        if ( end > s )
> +
> +        /*
> +         * Is the region size greater than zero?

Why "greater than zero"?

> +         * Does the region begins above or at the
> +         * end of current Xen image placement?
> +         */
> +        if ( end > s && (end - reloc_size >= _end - _start) )

And how does this added condition effect Xen only being moved
upwards? _end - _start after all is only the Xen image size, with
no consideration of its position. Plus I think you really mean
__2M_rwdata_end instead of _end.

Also as a more general remark: While we dislike too long lines,
too short ones aren't very nice either. Without trying it out I
can't even be sure the whole comment wouldn't perhaps fit on
two lines instead of the three it uses right now.

Finally - please use parentheses in expressions consistently.

Jan


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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Andrew Cooper]

On 27/11/17 15:41, Daniel Kiper wrote:
> If it is possible we would like to have the Xen image higher than the
> booloader put it and certainly do not overwrite the Xen code and data
> during copy/relocation. Otherwise the Xen may crash silently at boot.
>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

Actually, there is a second related bug which I've only just got to the
bottom of, and haven't had time to fix yet.

(XEN) Bootloader: GRUB 2.02
(XEN) Command line: hvm_fep com1=115200,8n1 console=com1,vga
dom0_mem=2048M,max:2048M watchdog ucode=scan dom0_max_vcpus=8
crashkernel=192M,below=4G
(XEN) Xen image load base address: 0xaba00000
(XEN) Video information:
(XEN)  VGA is text mode 80x25, font 8x16
(XEN)  VBE/DDC methods: none; EDID transfer time: 0 seconds
(XEN)  EDID info not retrieved because no DDC retrieval method detected
(XEN) Disc information:
(XEN)  Found 1 MBR signatures
(XEN)  Found 1 EDD information structures
(XEN) Xen-e820 RAM map:
(XEN)  0000000000000000 - 000000000009bc00 (usable)
(XEN)  000000000009bc00 - 00000000000a0000 (reserved)
(XEN)  00000000000e0000 - 0000000000100000 (reserved)
(XEN)  0000000000100000 - 00000000ac209000 (usable)
(XEN)  00000000ac209000 - 00000000aeb99000 (reserved)
(XEN)  00000000aeb99000 - 00000000aeb9d000 (ACPI NVS)
(XEN)  00000000aeb9d000 - 00000000aecd1000 (reserved)
(XEN)  00000000aecd1000 - 00000000aecd4000 (ACPI NVS)
(XEN)  00000000aecd4000 - 00000000aecf5000 (reserved)
(XEN)  00000000aecf5000 - 00000000aecf6000 (ACPI NVS)
(XEN)  00000000aecf6000 - 00000000aed24000 (reserved)
(XEN)  00000000aed24000 - 00000000aef2f000 (ACPI NVS)
(XEN)  00000000aef2f000 - 00000000aefed000 (ACPI data)
(XEN)  00000000aefed000 - 00000000af000000 (usable)
(XEN)  00000000af000000 - 00000000b0000000 (reserved)
(XEN)  00000000f8000000 - 00000000fc000000 (reserved)
(XEN)  00000000fec00000 - 00000000fec01000 (reserved)
(XEN)  00000000fed19000 - 00000000fed1a000 (reserved)
(XEN)  00000000fed1c000 - 00000000fed20000 (reserved)
(XEN)  00000000fee00000 - 00000000fee01000 (reserved)
(XEN)  00000000ff400000 - 0000000100000000 (reserved)
(XEN)  0000000100000000 - 0000000850000000 (usable)
(XEN) Kdump: DISABLED (failed to reserve 192MB (196608kB) at 0xa0200000)
(XEN) ACPI: RSDP 000F0410, 0024 (r2 INTEL )

When booting with Grub2 capable of locating Xen at its preferred
location, the calculation for the kexec region fails, because the
current location of Xen isn't taken into account.

The end of the chosen kexec region (0xa0200000 + 0x0c000000) overlaps
with the bottom of the Xen image.

~Andrew

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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Konrad Rzeszutek Wilk]

On Mon, Nov 27, 2017 at 04:58:52PM +0000, Andrew Cooper wrote:
> On 27/11/17 15:41, Daniel Kiper wrote:
> > If it is possible we would like to have the Xen image higher than the
> > booloader put it and certainly do not overwrite the Xen code and data
> > during copy/relocation. Otherwise the Xen may crash silently at boot.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> 
> Actually, there is a second related bug which I've only just got to the
> bottom of, and haven't had time to fix yet.
> 
> (XEN) Bootloader: GRUB 2.02
> (XEN) Command line: hvm_fep com1=115200,8n1 console=com1,vga
> dom0_mem=2048M,max:2048M watchdog ucode=scan dom0_max_vcpus=8
> crashkernel=192M,below=4G
> (XEN) Xen image load base address: 0xaba00000
> (XEN) Video information:
> (XEN)  VGA is text mode 80x25, font 8x16
> (XEN)  VBE/DDC methods: none; EDID transfer time: 0 seconds
> (XEN)  EDID info not retrieved because no DDC retrieval method detected
> (XEN) Disc information:
> (XEN)  Found 1 MBR signatures
> (XEN)  Found 1 EDD information structures
> (XEN) Xen-e820 RAM map:
> (XEN)  0000000000000000 - 000000000009bc00 (usable)
> (XEN)  000000000009bc00 - 00000000000a0000 (reserved)
> (XEN)  00000000000e0000 - 0000000000100000 (reserved)
> (XEN)  0000000000100000 - 00000000ac209000 (usable)
> (XEN)  00000000ac209000 - 00000000aeb99000 (reserved)
> (XEN)  00000000aeb99000 - 00000000aeb9d000 (ACPI NVS)
> (XEN)  00000000aeb9d000 - 00000000aecd1000 (reserved)
> (XEN)  00000000aecd1000 - 00000000aecd4000 (ACPI NVS)
> (XEN)  00000000aecd4000 - 00000000aecf5000 (reserved)
> (XEN)  00000000aecf5000 - 00000000aecf6000 (ACPI NVS)
> (XEN)  00000000aecf6000 - 00000000aed24000 (reserved)
> (XEN)  00000000aed24000 - 00000000aef2f000 (ACPI NVS)
> (XEN)  00000000aef2f000 - 00000000aefed000 (ACPI data)
> (XEN)  00000000aefed000 - 00000000af000000 (usable)
> (XEN)  00000000af000000 - 00000000b0000000 (reserved)
> (XEN)  00000000f8000000 - 00000000fc000000 (reserved)
> (XEN)  00000000fec00000 - 00000000fec01000 (reserved)
> (XEN)  00000000fed19000 - 00000000fed1a000 (reserved)
> (XEN)  00000000fed1c000 - 00000000fed20000 (reserved)
> (XEN)  00000000fee00000 - 00000000fee01000 (reserved)
> (XEN)  00000000ff400000 - 0000000100000000 (reserved)
> (XEN)  0000000100000000 - 0000000850000000 (usable)
> (XEN) Kdump: DISABLED (failed to reserve 192MB (196608kB) at 0xa0200000)
> (XEN) ACPI: RSDP 000F0410, 0024 (r2 INTEL )
> 
> When booting with Grub2 capable of locating Xen at its preferred
> location, the calculation for the kexec region fails, because the
> current location of Xen isn't taken into account.
> 
> The end of the chosen kexec region (0xa0200000 + 0x0c000000) overlaps
> with the bottom of the Xen image.

<blushes> Never got to upstream this, nor actually do the RFC thing
I mentioned..


commit 0350412917e7465fe5aaa3ba7616cf9bc6daa533
Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date:   Wed Dec 16 16:05:31 2015 -0500

    kexec/relocate: Change kexec location if relocation is in the way.
    
    The issue at hand is that GRUB2 puts us at the end of the
    E820_RAM that is under 4GB. It is aligned as well.
    For example :
    
    (XEN) Xen image base address: 0xeec00000
    ..
    (XEN) Xen-e820 RAM map:
    (XEN)  0000000000000000 - 000000000009fc00 (usable)
    (XEN)  000000000009fc00 - 00000000000a0000 (reserved)
    (XEN)  00000000000f0000 - 0000000000100000 (reserved)
    (XEN)  0000000000100000 - 00000000effff000 (usable)
    (XEN)  00000000effff000 - 00000000f0000000 (reserved)
    
    If the user decides to put the kexec crashkernel in the same
    area (so at the end of the E820_RAM) the relocation routines
    go haywire. For example with " crashkernel=512M@3327M"
    
    we would be usurping the end of the E820_RAM.
    
    This code doesn't actually fix the underlaying issue
    with the relocation routines (See below for explanation).
    Instead it just recomputes the location of where the kexec
    image should reside. With this patch we will have:
    
    (XEN) Kdump: 3327MB-3839MB overlaps with 0xeee00000(3822MB). Adjusting.
    (XEN) Kdump: 512MB (524288kB) at 0xcee00000->0xeee00000
    (XEN) New Xen image base address: 0xef800000
    
    The code assumes that the "new" relocation physical is always
    going to _after_ where GRUB has put the initial code.
    
    In other words - we always move it upwards in memory. But in this case
    there is no space (because kexec has grabbed it all) so we must move it
    downward (below where GRUB put us).
    
    That means subtracting the delta.. But since the value is
    an unsigned int that negative bit becomes 0xfffffff instead of -<some value>.
    Then the addition in the pagetables becomes quite large.
    
    However an RFC patch that fixes this didn't work right.
    
    OraBug: 22371625 - Xen hypervisor reallocation fails to allocate L3 pagetables
    Acked-by: Adnan Misherfi <adnan.misherfi@oracle.com>
    Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 21cbbce603..a5b4d6e427 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -473,12 +473,16 @@ static void __init parse_video_info(void)
         vga_console_info.u.vesa_lfb.mode_attrs = bvi->vesa_attrib;
     }
 }
-
+#define RELOCATE_BANDAID 1
 static void __init kexec_reserve_area(struct e820map *e820)
 {
     unsigned long kdump_start = kexec_crash_area.start;
     unsigned long kdump_size  = kexec_crash_area.size;
     static bool_t __initdata is_reserved = 0;
+#if RELOCATE_BANDAID
+    unsigned int i = 0;
+#endif
+    int rc = 0;
 
     kdump_size = (kdump_size + PAGE_SIZE - 1) & PAGE_MASK;
 
@@ -486,8 +490,37 @@ static void __init kexec_reserve_area(struct e820map *e820)
         return;
 
     is_reserved = 1;
+#if RELOCATE_BANDAID
+    while ( xen_img_base_phys_addr && i < e820->nr_map )
+    {
+        unsigned  long s = e820->map[i].addr;
+        unsigned  long e = (e820->map[i].addr + e820->map[i].size);
+
+        if ( e820->map[i++].type != E820_RAM )
+            continue;
+
+        if ( s <= xen_img_base_phys_addr && xen_img_base_phys_addr <= e )
+        {
+            unsigned long delta = xen_img_base_phys_addr - kdump_size;
+
+            if ( delta > s )
+            {
+                printk("Kdump: %luMB-%luMB overlaps with 0x%x(%uMB). Adjusting.\n",
+                       kdump_start >> 20 , (kdump_start + kdump_size) >> 20,
+                       xen_img_base_phys_addr, xen_img_base_phys_addr >> 20);
+                kexec_crash_area.start = delta;
+                kdump_start = delta;
+                break;
+            }
+            else
+                rc = -EINVAL;
+        }
+    }
+#endif
+    if ( rc == 0 )
+        rc = !reserve_e820_ram(e820, kdump_start, kdump_start + kdump_size);
 
-    if ( !reserve_e820_ram(e820, kdump_start, kdump_start + kdump_size) )
+    if ( rc )
     {
         printk("Kdump: DISABLED (failed to reserve %luMB (%lukB) at %#lx)"
                "\n", kdump_size >> 20, kdump_size >> 10, kdump_start);
@@ -495,8 +528,9 @@ static void __init kexec_reserve_area(struct e820map *e820)
     }
     else
     {
-        printk("Kdump: %luMB (%lukB) at %#lx\n",
-               kdump_size >> 20, kdump_size >> 10, kdump_start);
+        printk("Kdump: %luMB (%lukB) at %#lx->%#lx\n",
+               kdump_size >> 20, kdump_size >> 10, kdump_start,
+	       kdump_start + kdump_size);
     }
 }
 
> 
> ~Andrew
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Jan Beulich]

>>> On 27.11.17 at 17:58, <andrew.cooper3@citrix.com> wrote:
> On 27/11/17 15:41, Daniel Kiper wrote:
>> If it is possible we would like to have the Xen image higher than the
>> booloader put it and certainly do not overwrite the Xen code and data
>> during copy/relocation. Otherwise the Xen may crash silently at boot.
>>
>> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> 
> Actually, there is a second related bug which I've only just got to the
> bottom of, and haven't had time to fix yet.
> 
> (XEN) Bootloader: GRUB 2.02
> (XEN) Command line: hvm_fep com1=115200,8n1 console=com1,vga
> dom0_mem=2048M,max:2048M watchdog ucode=scan dom0_max_vcpus=8
> crashkernel=192M,below=4G
> (XEN) Xen image load base address: 0xaba00000
> (XEN) Video information:
> (XEN)  VGA is text mode 80x25, font 8x16
> (XEN)  VBE/DDC methods: none; EDID transfer time: 0 seconds
> (XEN)  EDID info not retrieved because no DDC retrieval method detected
> (XEN) Disc information:
> (XEN)  Found 1 MBR signatures
> (XEN)  Found 1 EDD information structures
> (XEN) Xen-e820 RAM map:
> (XEN)  0000000000000000 - 000000000009bc00 (usable)
> (XEN)  000000000009bc00 - 00000000000a0000 (reserved)
> (XEN)  00000000000e0000 - 0000000000100000 (reserved)
> (XEN)  0000000000100000 - 00000000ac209000 (usable)
> (XEN)  00000000ac209000 - 00000000aeb99000 (reserved)
> (XEN)  00000000aeb99000 - 00000000aeb9d000 (ACPI NVS)
> (XEN)  00000000aeb9d000 - 00000000aecd1000 (reserved)
> (XEN)  00000000aecd1000 - 00000000aecd4000 (ACPI NVS)
> (XEN)  00000000aecd4000 - 00000000aecf5000 (reserved)
> (XEN)  00000000aecf5000 - 00000000aecf6000 (ACPI NVS)
> (XEN)  00000000aecf6000 - 00000000aed24000 (reserved)
> (XEN)  00000000aed24000 - 00000000aef2f000 (ACPI NVS)
> (XEN)  00000000aef2f000 - 00000000aefed000 (ACPI data)
> (XEN)  00000000aefed000 - 00000000af000000 (usable)
> (XEN)  00000000af000000 - 00000000b0000000 (reserved)
> (XEN)  00000000f8000000 - 00000000fc000000 (reserved)
> (XEN)  00000000fec00000 - 00000000fec01000 (reserved)
> (XEN)  00000000fed19000 - 00000000fed1a000 (reserved)
> (XEN)  00000000fed1c000 - 00000000fed20000 (reserved)
> (XEN)  00000000fee00000 - 00000000fee01000 (reserved)
> (XEN)  00000000ff400000 - 0000000100000000 (reserved)
> (XEN)  0000000100000000 - 0000000850000000 (usable)
> (XEN) Kdump: DISABLED (failed to reserve 192MB (196608kB) at 0xa0200000)
> (XEN) ACPI: RSDP 000F0410, 0024 (r2 INTEL )
> 
> When booting with Grub2 capable of locating Xen at its preferred
> location, the calculation for the kexec region fails, because the
> current location of Xen isn't taken into account.
> 
> The end of the chosen kexec region (0xa0200000 + 0x0c000000) overlaps
> with the bottom of the Xen image.

That seems to require extending what e22e1c4795 ("x86/EFI:
avoid Xen image when looking for module/kexec position") did.

Jan


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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Jan Beulich]

>>> On 27.11.17 at 20:54, <konrad.wilk@oracle.com> wrote:
>     If the user decides to put the kexec crashkernel in the same
>     area (so at the end of the E820_RAM) the relocation routines
>     go haywire. For example with " crashkernel=512M@3327M"
>     
>     we would be usurping the end of the E820_RAM.

I'm tempted to say "just don't do that then", except I realize it may
not be fully predictable where Xen wants to put itself, or the location
may change with a Xen or firmware update.

>     This code doesn't actually fix the underlaying issue
>     with the relocation routines (See below for explanation).

But that's what a proper patch should do.

>     The code assumes that the "new" relocation physical is always
>     going to _after_ where GRUB has put the initial code.
>     
>     In other words - we always move it upwards in memory. But in this case
>     there is no space (because kexec has grabbed it all) so we must move it
>     downward (below where GRUB put us).

As Daniel's patch description says - we shouldn't move Xen at all
in such a case, rather than moving it downwards.

Jan


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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Daniel Kiper]

On Mon, Nov 27, 2017 at 04:58:52PM +0000, Andrew Cooper wrote:
> On 27/11/17 15:41, Daniel Kiper wrote:
> > If it is possible we would like to have the Xen image higher than the
> > booloader put it and certainly do not overwrite the Xen code and data
> > during copy/relocation. Otherwise the Xen may crash silently at boot.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
>
> Actually, there is a second related bug which I've only just got to the
> bottom of, and haven't had time to fix yet.
>
> (XEN) Bootloader: GRUB 2.02
> (XEN) Command line: hvm_fep com1=115200,8n1 console=com1,vga
> dom0_mem=2048M,max:2048M watchdog ucode=scan dom0_max_vcpus=8
> crashkernel=192M,below=4G
> (XEN) Xen image load base address: 0xaba00000
> (XEN) Video information:
> (XEN)  VGA is text mode 80x25, font 8x16
> (XEN)  VBE/DDC methods: none; EDID transfer time: 0 seconds
> (XEN)  EDID info not retrieved because no DDC retrieval method detected
> (XEN) Disc information:
> (XEN)  Found 1 MBR signatures
> (XEN)  Found 1 EDD information structures
> (XEN) Xen-e820 RAM map:
> (XEN)  0000000000000000 - 000000000009bc00 (usable)
> (XEN)  000000000009bc00 - 00000000000a0000 (reserved)
> (XEN)  00000000000e0000 - 0000000000100000 (reserved)
> (XEN)  0000000000100000 - 00000000ac209000 (usable)
> (XEN)  00000000ac209000 - 00000000aeb99000 (reserved)
> (XEN)  00000000aeb99000 - 00000000aeb9d000 (ACPI NVS)
> (XEN)  00000000aeb9d000 - 00000000aecd1000 (reserved)
> (XEN)  00000000aecd1000 - 00000000aecd4000 (ACPI NVS)
> (XEN)  00000000aecd4000 - 00000000aecf5000 (reserved)
> (XEN)  00000000aecf5000 - 00000000aecf6000 (ACPI NVS)
> (XEN)  00000000aecf6000 - 00000000aed24000 (reserved)
> (XEN)  00000000aed24000 - 00000000aef2f000 (ACPI NVS)
> (XEN)  00000000aef2f000 - 00000000aefed000 (ACPI data)
> (XEN)  00000000aefed000 - 00000000af000000 (usable)
> (XEN)  00000000af000000 - 00000000b0000000 (reserved)
> (XEN)  00000000f8000000 - 00000000fc000000 (reserved)
> (XEN)  00000000fec00000 - 00000000fec01000 (reserved)
> (XEN)  00000000fed19000 - 00000000fed1a000 (reserved)
> (XEN)  00000000fed1c000 - 00000000fed20000 (reserved)
> (XEN)  00000000fee00000 - 00000000fee01000 (reserved)
> (XEN)  00000000ff400000 - 0000000100000000 (reserved)
> (XEN)  0000000100000000 - 0000000850000000 (usable)
> (XEN) Kdump: DISABLED (failed to reserve 192MB (196608kB) at 0xa0200000)
> (XEN) ACPI: RSDP 000F0410, 0024 (r2 INTEL )
>
> When booting with Grub2 capable of locating Xen at its preferred
> location, the calculation for the kexec region fails, because the
> current location of Xen isn't taken into account.
>
> The end of the chosen kexec region (0xa0200000 + 0x0c000000) overlaps
> with the bottom of the Xen image.

I have a feeling that you can trigger this also when xen.efi
is launched directly from EFI. However, this may require some
fiddling with crashkernel values.

Daniel

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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Jan Beulich]

>>> On 28.11.17 at 12:32, <daniel.kiper@oracle.com> wrote:
> I have a feeling that you can trigger this also when xen.efi
> is launched directly from EFI. However, this may require some
> fiddling with crashkernel values.

I don't think so, no - that case was specifically fixed already
(I've pointed at that commit in another sub-thread).

Jan


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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Daniel Kiper]

On Mon, Nov 27, 2017 at 09:51:56AM -0700, Jan Beulich wrote:
> >>> On 27.11.17 at 16:41, <daniel.kiper@oracle.com> wrote:
> > If it is possible we would like to have the Xen image higher than the
> > booloader put it and certainly do not overwrite the Xen code and data
> > during copy/relocation. Otherwise the Xen may crash silently at boot.
>
> Is this something that you've actually observed happening? I'd be
> curious about the particular numbers if so.

We were hit by the issue in OVS Xen 4.4 with my earlier version of
EFI/Multiboot2 patches. Initially its implementation allowed relocation
of Xen even if it was relocated by the bootloader. This led to the
crashes on some new Oracle machines because copy destination partially
overlapped with the end of current/initial Xen image placement.

After some discussion here we decided to disable Xen relocation in my
EFI/Multiboot2 upstream patches if the booloader did the work for us.
Though one case is still not covered. If Xen is not relocated by the
booloader then it tries to do that by itself. If all RAM regions above
currently occupied one are unsuitable for relocation then Xen tries move
itself higher in it. And if (end - reloc_size + XEN_IMG_OFFSET) goes
below _end then copy/relocation destination overlaps, at least partially,
with its source.

I can agree that this should not happen on todays machines very often.
If at all. It is rather unusual to not have usable RAM regions above
~5 MiB nowadays. Though I think that we should at least consider putting
such safety measure here. Otherwise Xen may crash mysteriously without
any stack trace. So, as you may imagine it is very confusing and impairs
further debugging. However, due to rarity of the issue I am not going to
insist very strongly here.

> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> > ---
> >  xen/arch/x86/setup.c |    8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 32bb02e..be91d34 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -960,7 +960,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >          }
> >          else
> >              end = 0;
> > -        if ( end > s )
> > +
> > +        /*
> > +         * Is the region size greater than zero?
>
> Why "greater than zero"?

end > s?

> > +         * Does the region begins above or at the
> > +         * end of current Xen image placement?
> > +         */
> > +        if ( end > s && (end - reloc_size >= _end - _start) )
>
> And how does this added condition effect Xen only being moved
> upwards? _end - _start after all is only the Xen image size, with
> no consideration of its position. Plus I think you really mean
> __2M_rwdata_end instead of _end.

OK, we have below:

[...]

e = end - reloc_size;

[...]

move_memory(e + XEN_IMG_OFFSET, XEN_IMG_OFFSET, _end - _start, 1);

So, we have: e + XEN_IMG_OFFSET >= XEN_IMG_OFFSET + _end - _start

We can drop XEN_IMG_OFFSET, so we have: e >= _end - _start

However, we cannot use "e" in the condition, so we have:
  end - reloc_size >= _end - _start

> Also as a more general remark: While we dislike too long lines,
> too short ones aren't very nice either. Without trying it out I
> can't even be sure the whole comment wouldn't perhaps fit on
> two lines instead of the three it uses right now.

Wilco!

> Finally - please use parentheses in expressions consistently.

Wilco!

Daniel

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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Daniel Kiper]

On Tue, Nov 28, 2017 at 04:51:51AM -0700, Jan Beulich wrote:
> >>> On 28.11.17 at 12:32, <daniel.kiper@oracle.com> wrote:
> > I have a feeling that you can trigger this also when xen.efi
> > is launched directly from EFI. However, this may require some
> > fiddling with crashkernel values.
>
> I don't think so, no - that case was specifically fixed already
> (I've pointed at that commit in another sub-thread).

Ugh... Right, it looks that I misread the patch. Anyway, it seems to
me that is easy to fix. We should change line xen/arch/x86/setup.c:907

if ( efi_enabled(EFI_LOADER) )

to

if ( !xen_phys_start )

Then all cases should be covered. If you are OK with that I can
prepare a patch.

Daniel

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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Jan Beulich]

>>> On 28.11.17 at 13:47, <daniel.kiper@oracle.com> wrote:
> On Tue, Nov 28, 2017 at 04:51:51AM -0700, Jan Beulich wrote:
>> >>> On 28.11.17 at 12:32, <daniel.kiper@oracle.com> wrote:
>> > I have a feeling that you can trigger this also when xen.efi
>> > is launched directly from EFI. However, this may require some
>> > fiddling with crashkernel values.
>>
>> I don't think so, no - that case was specifically fixed already
>> (I've pointed at that commit in another sub-thread).
> 
> Ugh... Right, it looks that I misread the patch. Anyway, it seems to
> me that is easy to fix. We should change line xen/arch/x86/setup.c:907
> 
> if ( efi_enabled(EFI_LOADER) )
> 
> to
> 
> if ( !xen_phys_start )

But xen_phys_start is non-zero when efi_enabled(EFI_LOADER).

> Then all cases should be covered.

I don't think that's going to be enough: Once Xen gets moved,
the area to exclude needs to be updated too.

Jan


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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Daniel Kiper]

On Tue, Nov 28, 2017 at 06:37:17AM -0700, Jan Beulich wrote:
> >>> On 28.11.17 at 13:47, <daniel.kiper@oracle.com> wrote:
> > On Tue, Nov 28, 2017 at 04:51:51AM -0700, Jan Beulich wrote:
> >> >>> On 28.11.17 at 12:32, <daniel.kiper@oracle.com> wrote:
> >> > I have a feeling that you can trigger this also when xen.efi
> >> > is launched directly from EFI. However, this may require some
> >> > fiddling with crashkernel values.
> >>
> >> I don't think so, no - that case was specifically fixed already
> >> (I've pointed at that commit in another sub-thread).
> >
> > Ugh... Right, it looks that I misread the patch. Anyway, it seems to
> > me that is easy to fix. We should change line xen/arch/x86/setup.c:907
> >
> > if ( efi_enabled(EFI_LOADER) )
> >
> > to
> >
> > if ( !xen_phys_start )
>
> But xen_phys_start is non-zero when efi_enabled(EFI_LOADER).

Err... Copy-paste error. Of course I thought about "if ( xen_phys_start )".
Then this covers EFI_LOADER case and relocation by the bootloader.

> > Then all cases should be covered.
>
> I don't think that's going to be enough: Once Xen gets moved,
> the area to exclude needs to be updated too.

If the Xen is relocated by the booloader then the Xen does not do relocation
itself again. So, it should work.

Daniel

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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Jan Beulich]

>>> On 28.11.17 at 14:53, <daniel.kiper@oracle.com> wrote:
> On Tue, Nov 28, 2017 at 06:37:17AM -0700, Jan Beulich wrote:
>> >>> On 28.11.17 at 13:47, <daniel.kiper@oracle.com> wrote:
>> > Then all cases should be covered.
>>
>> I don't think that's going to be enough: Once Xen gets moved,
>> the area to exclude needs to be updated too.
> 
> If the Xen is relocated by the booloader then the Xen does not do relocation
> itself again. So, it should work.

Then let me reformulate this into "... the area to exclude needs to
be set".

Jan


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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Daniel Kiper]

On Tue, Nov 28, 2017 at 07:02:01AM -0700, Jan Beulich wrote:
> >>> On 28.11.17 at 14:53, <daniel.kiper@oracle.com> wrote:
> > On Tue, Nov 28, 2017 at 06:37:17AM -0700, Jan Beulich wrote:
> >> >>> On 28.11.17 at 13:47, <daniel.kiper@oracle.com> wrote:
> >> > Then all cases should be covered.
> >>
> >> I don't think that's going to be enough: Once Xen gets moved,
> >> the area to exclude needs to be updated too.
> >
> > If the Xen is relocated by the booloader then the Xen does not do relocation
> > itself again. So, it should work.
>
> Then let me reformulate this into "... the area to exclude needs to
> be set".

Oh... You mean consider_modules() with efi_enabled(EFI_LOADER) in the
same patch. Right, we can replace "efi_enabled(EFI_LOADER)" with
"!!xen_phys_start" then.

Daniel

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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Jan Beulich]

>>> On 28.11.17 at 13:41, <daniel.kiper@oracle.com> wrote:
> On Mon, Nov 27, 2017 at 09:51:56AM -0700, Jan Beulich wrote:
>> >>> On 27.11.17 at 16:41, <daniel.kiper@oracle.com> wrote:
>> > If it is possible we would like to have the Xen image higher than the
>> > booloader put it and certainly do not overwrite the Xen code and data
>> > during copy/relocation. Otherwise the Xen may crash silently at boot.
>>
>> Is this something that you've actually observed happening? I'd be
>> curious about the particular numbers if so.
> 
> We were hit by the issue in OVS Xen 4.4 with my earlier version of
> EFI/Multiboot2 patches. Initially its implementation allowed relocation
> of Xen even if it was relocated by the bootloader. This led to the
> crashes on some new Oracle machines because copy destination partially
> overlapped with the end of current/initial Xen image placement.
> 
> After some discussion here we decided to disable Xen relocation in my
> EFI/Multiboot2 upstream patches if the booloader did the work for us.
> Though one case is still not covered. If Xen is not relocated by the
> booloader then it tries to do that by itself. If all RAM regions above
> currently occupied one are unsuitable for relocation then Xen tries move
> itself higher in it. And if (end - reloc_size + XEN_IMG_OFFSET) goes
> below _end then copy/relocation destination overlaps, at least partially,
> with its source.
> 
> I can agree that this should not happen on todays machines very often.
> If at all. It is rather unusual to not have usable RAM regions above
> ~5 MiB nowadays. Though I think that we should at least consider putting
> such safety measure here. Otherwise Xen may crash mysteriously without
> any stack trace. So, as you may imagine it is very confusing and impairs
> further debugging. However, due to rarity of the issue I am not going to
> insist very strongly here.

I don't mind taking care of this case, as long as everything is
being properly described.

>> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
>> > ---
>> >  xen/arch/x86/setup.c |    8 +++++++-
>> >  1 file changed, 7 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> > index 32bb02e..be91d34 100644
>> > --- a/xen/arch/x86/setup.c
>> > +++ b/xen/arch/x86/setup.c
>> > @@ -960,7 +960,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>> >          }
>> >          else
>> >              end = 0;
>> > -        if ( end > s )
>> > +
>> > +        /*
>> > +         * Is the region size greater than zero?
>>
>> Why "greater than zero"?
> 
> end > s?

Oh, I had wrongly assumed the comment was meant to cover only
the addition you're making to the condition.

>> > +         * Does the region begins above or at the
>> > +         * end of current Xen image placement?
>> > +         */
>> > +        if ( end > s && (end - reloc_size >= _end - _start) )
>>
>> And how does this added condition effect Xen only being moved
>> upwards? _end - _start after all is only the Xen image size, with
>> no consideration of its position. Plus I think you really mean
>> __2M_rwdata_end instead of _end.
> 
> OK, we have below:
> 
> [...]
> 
> e = end - reloc_size;
> 
> [...]
> 
> move_memory(e + XEN_IMG_OFFSET, XEN_IMG_OFFSET, _end - _start, 1);
> 
> So, we have: e + XEN_IMG_OFFSET >= XEN_IMG_OFFSET + _end - _start
> 
> We can drop XEN_IMG_OFFSET, so we have: e >= _end - _start
> 
> However, we cannot use "e" in the condition, so we have:
>   end - reloc_size >= _end - _start

Okay, so this implies an original base address of zero. In that case,
however, we can't possibly move Xen downwards; looks like I've
misunderstood the title/description: You're really worried about an
overlap of the regions. From description and comment I was getting
the impression that the goal would be to cover more cases (including
ones where Xen wasn't loaded at the lowest possible address).
Could you try to make this more clear?

Furthermore I'm not convinced the condition needs to be as strict
as you make it now: As long as move_memory() can deal with
overlapping areas, a partial overlap looks to be okay. That would
allow for more cases where Xen does actually get moved even
with very little memory.

Another option is to consider not moving Xen based on other
criteria: The main goal here is to free up memory below 16Mb. If
the machine has no memory above 16Mb, moving Xen around
won't do any good in this regard.

Jan

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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Daniel Kiper]

On Tue, Nov 28, 2017 at 07:28:25AM -0700, Jan Beulich wrote:
> >>> On 28.11.17 at 13:41, <daniel.kiper@oracle.com> wrote:
> > On Mon, Nov 27, 2017 at 09:51:56AM -0700, Jan Beulich wrote:
> >> >>> On 27.11.17 at 16:41, <daniel.kiper@oracle.com> wrote:
> >> > If it is possible we would like to have the Xen image higher than the
> >> > booloader put it and certainly do not overwrite the Xen code and data
> >> > during copy/relocation. Otherwise the Xen may crash silently at boot.
> >>
> >> Is this something that you've actually observed happening? I'd be
> >> curious about the particular numbers if so.
> >
> > We were hit by the issue in OVS Xen 4.4 with my earlier version of
> > EFI/Multiboot2 patches. Initially its implementation allowed relocation
> > of Xen even if it was relocated by the bootloader. This led to the
> > crashes on some new Oracle machines because copy destination partially
> > overlapped with the end of current/initial Xen image placement.
> >
> > After some discussion here we decided to disable Xen relocation in my
> > EFI/Multiboot2 upstream patches if the booloader did the work for us.
> > Though one case is still not covered. If Xen is not relocated by the
> > booloader then it tries to do that by itself. If all RAM regions above
> > currently occupied one are unsuitable for relocation then Xen tries move
> > itself higher in it. And if (end - reloc_size + XEN_IMG_OFFSET) goes
> > below _end then copy/relocation destination overlaps, at least partially,
> > with its source.
> >
> > I can agree that this should not happen on todays machines very often.
> > If at all. It is rather unusual to not have usable RAM regions above
> > ~5 MiB nowadays. Though I think that we should at least consider putting
> > such safety measure here. Otherwise Xen may crash mysteriously without
> > any stack trace. So, as you may imagine it is very confusing and impairs
> > further debugging. However, due to rarity of the issue I am not going to
> > insist very strongly here.
>
> I don't mind taking care of this case, as long as everything is
> being properly described.

OK, great!

[...]

> > OK, we have below:
> >
> > [...]
> >
> > e = end - reloc_size;
> >
> > [...]
> >
> > move_memory(e + XEN_IMG_OFFSET, XEN_IMG_OFFSET, _end - _start, 1);
> >
> > So, we have: e + XEN_IMG_OFFSET >= XEN_IMG_OFFSET + _end - _start
> >
> > We can drop XEN_IMG_OFFSET, so we have: e >= _end - _start
> >
> > However, we cannot use "e" in the condition, so we have:
> >   end - reloc_size >= _end - _start
>
> Okay, so this implies an original base address of zero. In that case,

Yep!

> however, we can't possibly move Xen downwards; looks like I've
> misunderstood the title/description: You're really worried about an
> overlap of the regions. From description and comment I was getting
> the impression that the goal would be to cover more cases (including
> ones where Xen wasn't loaded at the lowest possible address).
> Could you try to make this more clear?

Sure thing!

> Furthermore I'm not convinced the condition needs to be as strict
> as you make it now: As long as move_memory() can deal with
> overlapping areas, a partial overlap looks to be okay. That would
> allow for more cases where Xen does actually get moved even
> with very little memory.

This made me suspicious. I spent some time looking at the code and
realized that the problem lies a few lines below.

for ( j = 0; j < L3_PAGETABLE_ENTRIES; j++, pl3e++ )
{
    /* Not present, 1GB mapping, or already relocated? */
    if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) ||
         (l3e_get_flags(*pl3e) & _PAGE_PSE) ||
         (l3e_get_pfn(*pl3e) > PFN_DOWN(xen_phys_start)) )
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Similar conditions are also below. These things mean that if xen_phys_start falls
below at least one current Xen code/data 2 MiB mapping then everything which was
originally more or less between xen_phys_start - _end is not mapped later...

So, it looks that the condition should look like this right now:

xen_phys_start >= XEN_IMG_OFFSET + _end - _start

xen_phys_start == e
e = end - reloc_size

end - reloc_size >= XEN_IMG_OFFSET + _end - _start

So, finally it should be:

if ( ( end > s ) && (end - reloc_size >= XEN_IMG_OFFSET + _end - _start) )

> Another option is to consider not moving Xen based on other
> criteria: The main goal here is to free up memory below 16Mb. If
> the machine has no memory above 16Mb, moving Xen around
> won't do any good in this regard.

This is also an option. However, I am not sure why everything below
16 MiB is so precious. I have a feeling that it is related somehow
to ancient PC days but I do not remember details.

Daniel

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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Daniel Kiper]

On Wed, Nov 29, 2017 at 07:56:24PM +0100, Daniel Kiper wrote:

[...]

> So, it looks that the condition should look like this right now:
>
> xen_phys_start >= XEN_IMG_OFFSET + _end - _start

XEN_IMG_OFFSET + _end - _start == __pa(_end)

> xen_phys_start == e
> e = end - reloc_size
>
> end - reloc_size >= XEN_IMG_OFFSET + _end - _start
>
> So, finally it should be:
>
> if ( ( end > s ) && (end - reloc_size >= XEN_IMG_OFFSET + _end - _start) )

So, we have:

if ( (end > s) && (end - reloc_size >= __pa(_end)) )

Daniel

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

Re: [PATCH] x86/setup: do not relocate below the end of current Xen image placement

[Jan Beulich]

>>> On 29.11.17 at 19:56, <daniel.kiper@oracle.com> wrote:
> On Tue, Nov 28, 2017 at 07:28:25AM -0700, Jan Beulich wrote:
>> Another option is to consider not moving Xen based on other
>> criteria: The main goal here is to free up memory below 16Mb. If
>> the machine has no memory above 16Mb, moving Xen around
>> won't do any good in this regard.
> 
> This is also an option. However, I am not sure why everything below
> 16 MiB is so precious. I have a feeling that it is related somehow
> to ancient PC days but I do not remember details.

Indeed I believe this was mainly to keep the range free for
ISA-style DMA (like what the 82077 floppy disk controllers do).
There are reportedly other (non-ISA) devices with random
restrictions on the range they can DMA to, however, but I
don't think any goes as low as 16Mb.

Jan


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