[Qemu-devel] Change of max-ram-below-4g initial value breaks Xen

[Qemu-devel] Change of max-ram-below-4g initial value breaks Xen

[Anthony PERARD]

Hi,

following commit 8156d48 (pc: allow raising low memory via
max-ram-below-4g option), guest under Xen fail to boot. This is due to
the initial value of max-ram-below-4g been change from 4G to 3.5G.
Unfortunately, a Xen guest have a higher default split, at 3.75G, but
can be change via the -machine options max-ram-below-4g. Now, it looks
like a user ask for 3.5G where the rest of the Xen tool stack is still
at 3.75G.

In QEMU, the ram split calculation for Xen is done in xen_ram_init(). It
can be change via max-ram-below-4g.

The hvmloader firmware will start the MMIO hole at 3.75G unless told
otherwise via a xenstore entry. The xenstore entry can be set by the Xen
toolstack, which would also add 'max-ram-below-4g' to -machine if a user
request it.


How could xen_ram_init() find out if the value of max-ram-below-4g is
the default or if a user have set it? Is there another way we could fix
this?


Regards,

-- 
Anthony PERARD

Re: [Qemu-devel] Change of max-ram-below-4g initial value breaks Xen

[Gerd Hoffmann]

  Hi,

> How could xen_ram_init() find out if the value of max-ram-below-4g is
> the default or if a user have set it? Is there another way we could fix
> this?

I guess we'll need a separate variable for that then, something along
the lines of "max-ram-below-4g-default".  I'll have a look tomorrow
(unless you are faster with a patch ;)

cheers,
  Gerd

Re: [Qemu-devel] Change of max-ram-below-4g initial value breaks Xen

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 343 bytes --]

  Hi,

> How could xen_ram_init() find out if the value of max-ram-below-4g is
> the default or if a user have set it? Is there another way we could fix
> this?

Attached patch should fix it.  Patch survived a quick smoke test on kvm
so far, need to do some more testing tomorrow.  Can you give it a spin
on xen?

thanks,
  Gerd


[-- Attachment #2: 0001-xen-fix-ram-init-regression.patch --]
[-- Type: text/x-patch, Size: 3039 bytes --]

From d45a95861def18a02e1c26d3717693432517107a Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Thu, 23 Jun 2016 16:49:03 +0200
Subject: [PATCH] xen: fix ram init regression

Commit "8156d48 pc: allow raising low memory via max-ram-below-4g
option" causes a regression on xen, because it uses a different
memory split.

This patch initializes max-ram-below-4g to zero and leaves the
initialization to the memory initialization functions.  That way
they can pick different default values (max-ram-below-4g is zero
still) or use the user supplied value (max-ram-below-4g is non-zero).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/i386/pc.c      | 2 +-
 hw/i386/pc_piix.c | 3 +++
 hw/i386/pc_q35.c  | 3 +++
 xen-hvm.c         | 3 +++
 4 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7198ed5..66e1dae 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1886,7 +1886,7 @@ static void pc_machine_initfn(Object *obj)
                         pc_machine_get_hotplug_memory_region_size,
                         NULL, NULL, NULL, &error_abort);
 
-    pcms->max_ram_below_4g = 0xe0000000; /* 3.5G */
+    pcms->max_ram_below_4g = 0; /* use default */
     object_property_add(obj, PC_MACHINE_MAX_RAM_BELOW_4G, "size",
                         pc_machine_get_max_ram_below_4g,
                         pc_machine_set_max_ram_below_4g,
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 53bc968..78e3d44 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -114,6 +114,9 @@ static void pc_init1(MachineState *machine,
      *    qemu -M pc,max-ram-below-4g=2G -m 4G     -> 2048M low, 2048M high
      *    qemu -M pc,max-ram-below-4g=4G -m 3968M  -> 3968M low (=4G-128M)
      */
+    if (!pcms->max_ram_below_4g) {
+        pcms->max_ram_below_4g = 0xe0000000; /* default: 3.5G */
+    }
     lowmem = pcms->max_ram_below_4g;
     if (machine->ram_size >= pcms->max_ram_below_4g) {
         if (pcmc->gigabyte_align) {
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index e4b541f..1b653e2 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -93,6 +93,9 @@ static void pc_q35_init(MachineState *machine)
     /* Handle the machine opt max-ram-below-4g.  It is basically doing
      * min(qemu limit, user limit).
      */
+    if (!pcms->max_ram_below_4g) {
+        pcms->max_ram_below_4g = 1ULL << 32; /* default: 4G */;
+    }
     if (lowmem > pcms->max_ram_below_4g) {
         lowmem = pcms->max_ram_below_4g;
         if (machine->ram_size - lowmem > lowmem &&
diff --git a/xen-hvm.c b/xen-hvm.c
index 98ea44f..eb57792 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -190,6 +190,9 @@ static void xen_ram_init(PCMachineState *pcms,
     /* Handle the machine opt max-ram-below-4g.  It is basically doing
      * min(xen limit, user limit).
      */
+    if (!user_lowmem) {
+        user_lowmem = HVM_BELOW_4G_RAM_END; /* default */
+    }
     if (HVM_BELOW_4G_RAM_END <= user_lowmem) {
         user_lowmem = HVM_BELOW_4G_RAM_END;
     }
-- 
1.8.3.1

Re: [Qemu-devel] Change of max-ram-below-4g initial value breaks Xen

[Anthony PERARD]

On Thu, Jun 23, 2016 at 04:57:54PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > How could xen_ram_init() find out if the value of max-ram-below-4g is
> > the default or if a user have set it? Is there another way we could fix
> > this?
> 
> Attached patch should fix it.  Patch survived a quick smoke test on kvm
> so far, need to do some more testing tomorrow.  Can you give it a spin
> on xen?

Thanks. Unfortunately, it does not work :(.

In this patch, max_ram_below_4g is set before the call to xen_ram_init()
and xen_ram_init read it back (via object_property_get_int()).  So, in
xen_ram_init, user_lowmem is not 0.

Thanks,

-- 
Anthony PERARD

Re: [Qemu-devel] Change of max-ram-below-4g initial value breaks Xen

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

On Do, 2016-06-23 at 17:18 +0100, Anthony PERARD wrote:
> On Thu, Jun 23, 2016 at 04:57:54PM +0200, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > How could xen_ram_init() find out if the value of max-ram-below-4g is
> > > the default or if a user have set it? Is there another way we could fix
> > > this?
> > 
> > Attached patch should fix it.  Patch survived a quick smoke test on kvm
> > so far, need to do some more testing tomorrow.  Can you give it a spin
> > on xen?
> 
> Thanks. Unfortunately, it does not work :(.
> 
> In this patch, max_ram_below_4g is set before the call to xen_ram_init()
> and xen_ram_init read it back (via object_property_get_int()).  So, in
> xen_ram_init, user_lowmem is not 0.

Ah, I see.  We do the split calculation twice on xen.  That is pretty
pointless.  New patch attached.

cheers,
  Gerd


[-- Attachment #2: 0001-xen-fix-ram-init-regression.patch --]
[-- Type: text/x-patch, Size: 5459 bytes --]

From a1bb0d4f7a94e97102e7ea72d0a65de2a17b1160 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Thu, 23 Jun 2016 16:49:03 +0200
Subject: [PATCH] xen: fix ram init regression

Commit "8156d48 pc: allow raising low memory via max-ram-below-4g
option" causes a regression on xen, because it uses a different
memory split.

This patch initializes max-ram-below-4g to zero and leaves the
initialization to the memory initialization functions.  That way
they can pick different default values (max-ram-below-4g is zero
still) or use the user supplied value (max-ram-below-4g is non-zero).

Also skip the whole ram split calculation on Xen.  xen_ram_init()
does its own split calculation anyway so it is superfluous, also
this way xen_ram_init can actually see whenever max-ram-below-4g
is zero or not.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/i386/pc.c      |  2 +-
 hw/i386/pc_piix.c | 52 +++++++++++++++++++++++++++++-----------------------
 hw/i386/pc_q35.c  |  3 +++
 xen-hvm.c         |  3 +++
 4 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7198ed5..66e1dae 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1886,7 +1886,7 @@ static void pc_machine_initfn(Object *obj)
                         pc_machine_get_hotplug_memory_region_size,
                         NULL, NULL, NULL, &error_abort);
 
-    pcms->max_ram_below_4g = 0xe0000000; /* 3.5G */
+    pcms->max_ram_below_4g = 0; /* use default */
     object_property_add(obj, PC_MACHINE_MAX_RAM_BELOW_4G, "size",
                         pc_machine_get_max_ram_below_4g,
                         pc_machine_set_max_ram_below_4g,
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 53bc968..f51fa77 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -108,37 +108,43 @@ static void pc_init1(MachineState *machine,
      *    so legacy non-PAE guests can get as much memory as possible in
      *    the 32bit address space below 4G.
      *
+     *  - Note that Xen has its own ram setp code in xen_ram_init(),
+     *    called via xen_hvm_init().
+     *
      * Examples:
      *    qemu -M pc-1.7 -m 4G    (old default)    -> 3584M low,  512M high
      *    qemu -M pc -m 4G        (new default)    -> 3072M low, 1024M high
      *    qemu -M pc,max-ram-below-4g=2G -m 4G     -> 2048M low, 2048M high
      *    qemu -M pc,max-ram-below-4g=4G -m 3968M  -> 3968M low (=4G-128M)
      */
-    lowmem = pcms->max_ram_below_4g;
-    if (machine->ram_size >= pcms->max_ram_below_4g) {
-        if (pcmc->gigabyte_align) {
-            if (lowmem > 0xc0000000) {
-                lowmem = 0xc0000000;
-            }
-            if (lowmem & ((1ULL << 30) - 1)) {
-                error_report("Warning: Large machine and max_ram_below_4g "
-                             "(%" PRIu64 ") not a multiple of 1G; "
-                             "possible bad performance.",
-                             pcms->max_ram_below_4g);
-            }
-        }
-    }
-
-    if (machine->ram_size >= lowmem) {
-        pcms->above_4g_mem_size = machine->ram_size - lowmem;
-        pcms->below_4g_mem_size = lowmem;
-    } else {
-        pcms->above_4g_mem_size = 0;
-        pcms->below_4g_mem_size = machine->ram_size;
-    }
-
     if (xen_enabled()) {
         xen_hvm_init(pcms, &ram_memory);
+    } else {
+        if (!pcms->max_ram_below_4g) {
+            pcms->max_ram_below_4g = 0xe0000000; /* default: 3.5G */
+        }
+        lowmem = pcms->max_ram_below_4g;
+        if (machine->ram_size >= pcms->max_ram_below_4g) {
+            if (pcmc->gigabyte_align) {
+                if (lowmem > 0xc0000000) {
+                    lowmem = 0xc0000000;
+                }
+                if (lowmem & ((1ULL << 30) - 1)) {
+                    error_report("Warning: Large machine and max_ram_below_4g "
+                                 "(%" PRIu64 ") not a multiple of 1G; "
+                                 "possible bad performance.",
+                                 pcms->max_ram_below_4g);
+                }
+            }
+        }
+
+        if (machine->ram_size >= lowmem) {
+            pcms->above_4g_mem_size = machine->ram_size - lowmem;
+            pcms->below_4g_mem_size = lowmem;
+        } else {
+            pcms->above_4g_mem_size = 0;
+            pcms->below_4g_mem_size = machine->ram_size;
+        }
     }
 
     pc_cpus_init(pcms);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index e4b541f..1b653e2 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -93,6 +93,9 @@ static void pc_q35_init(MachineState *machine)
     /* Handle the machine opt max-ram-below-4g.  It is basically doing
      * min(qemu limit, user limit).
      */
+    if (!pcms->max_ram_below_4g) {
+        pcms->max_ram_below_4g = 1ULL << 32; /* default: 4G */;
+    }
     if (lowmem > pcms->max_ram_below_4g) {
         lowmem = pcms->max_ram_below_4g;
         if (machine->ram_size - lowmem > lowmem &&
diff --git a/xen-hvm.c b/xen-hvm.c
index 98ea44f..eb57792 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -190,6 +190,9 @@ static void xen_ram_init(PCMachineState *pcms,
     /* Handle the machine opt max-ram-below-4g.  It is basically doing
      * min(xen limit, user limit).
      */
+    if (!user_lowmem) {
+        user_lowmem = HVM_BELOW_4G_RAM_END; /* default */
+    }
     if (HVM_BELOW_4G_RAM_END <= user_lowmem) {
         user_lowmem = HVM_BELOW_4G_RAM_END;
     }
-- 
1.8.3.1

Re: [Qemu-devel] Change of max-ram-below-4g initial value breaks Xen

[Anthony PERARD]

On Fri, Jun 24, 2016 at 07:46:23AM +0200, Gerd Hoffmann wrote:
> On Do, 2016-06-23 at 17:18 +0100, Anthony PERARD wrote:
> > On Thu, Jun 23, 2016 at 04:57:54PM +0200, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > How could xen_ram_init() find out if the value of max-ram-below-4g is
> > > > the default or if a user have set it? Is there another way we could fix
> > > > this?
> > > 
> > > Attached patch should fix it.  Patch survived a quick smoke test on kvm
> > > so far, need to do some more testing tomorrow.  Can you give it a spin
> > > on xen?
> > 
> > Thanks. Unfortunately, it does not work :(.
> > 
> > In this patch, max_ram_below_4g is set before the call to xen_ram_init()
> > and xen_ram_init read it back (via object_property_get_int()).  So, in
> > xen_ram_init, user_lowmem is not 0.
> 
> Ah, I see.  We do the split calculation twice on xen.  That is pretty
> pointless.  New patch attached.

I've tested on Xen, it works fine. Thanks. Also, the patch look good.

Cheers,

-- 
Anthony PERARD