qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v5 2/3] pc & q35: Add new machine opt max-ram-be

From: Don Slutz
Subject: Re: [Qemu-devel] [PATCH v5 2/3] pc & q35: Add new machine opt max-ram-below-4g
Date: Mon, 09 Jun 2014 16:03:15 -0400
User-agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 
On 06/09/14 13:33, Marcel Apfelbaum wrote:

On Mon, 2014-06-09 at 17:37 +0200, Igor Mammedov wrote:

On Mon, 09 Jun 2014 18:10:27 +0300
Marcel Apfelbaum <address@hidden> wrote:


Hi,

On Mon, 2014-06-09 at 17:38 +0300, Michael S. Tsirkin wrote:

On Mon, Jun 09, 2014 at 10:20:57AM -0400, Don Slutz wrote:

On 06/08/14 11:40, Michael S. Tsirkin wrote:

On Fri, Jun 06, 2014 at 01:52:05PM -0400, Don Slutz wrote:

This is a pc & q35 only machine opt.  One use is to allow for more
ram in a 32bit guest for example:

-machine pc,max-ram-below-4g=3.75G

If you add enough PCI devices then all mmio for them will not fit
below 4G which may not be the layout the user wanted. This allows
you to increase the below 4G address space that PCI devices can use
(aka decrease ram below 4G) and therefore in more cases not have any
mmio that is above 4G.

For example using "-machine pc,max-ram-below-4g=2G" on the command
line will limit the amount of ram that is below 4G to 2G.

Signed-off-by: Don Slutz <address@hidden>
---
v5:
   Re-work based on:

   https://github.com/imammedo/qemu/commits/memory-hotplug-v11


  hw/i386/pc.c         | 38 ++++++++++++++++++++++++++++++++++++++
  hw/i386/pc_piix.c    | 15 ++++++++++++---
  hw/i386/pc_q35.c     | 15 ++++++++++++---
  include/hw/i386/pc.h |  3 +++
  vl.c                 |  4 ++++
  5 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7cdba10..bccb746 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1644,11 +1644,49 @@ pc_machine_get_hotplug_memory_region_size(Object *obj, 
Visitor *v, void *opaque,
      visit_type_int(v, &value, name, errp);
  }
+static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v,
+                                         void *opaque, const char *name,
+                                         Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+    uint64_t value = pcms->max_ram_below_4g;
+
+    visit_type_size(v, &value, name, errp);
+}
+
+static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v,
+                                         void *opaque, const char *name,
+                                         Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+    Error *error = NULL;
+    uint64_t value;
+
+    visit_type_size(v, &value, name, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+    if (value > (1ULL << 32)) {
+        error_set(&error, ERROR_CLASS_GENERIC_ERROR,
+                  "Machine option 'max-ram-below-4g=%"PRIu64
+                  "' expects size less then or equal to 4G", value);

less than

But the test is greater then.  So "not greater then" is "less then or equal".
Or did you want the test changed?

No, just correcting English: less than, not less then :)


+        error_propagate(errp, error);
+        return;
+    }
+
+    pcms->max_ram_below_4g = value;
+}
+
  static void pc_machine_initfn(Object *obj)
  {
      object_property_add(obj, PC_MACHINE_MEMHP_REGION_SIZE, "int",
                          pc_machine_get_hotplug_memory_region_size,
                          NULL, NULL, NULL, NULL);
+    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,
+                        NULL, NULL, NULL);

Maybe you can add here a sane default and avoid comparison with 0
any time you use it.

+1



The basic problem is that there is no simple default.  For pc-i440fx-1.7, 
pc-q35-1.7 and all
older versions there are 2 values:


xen_enabled() ==> 3.75G
!xen_enabled() ==> 3.5G

pc-i440fx-2.0 and pc-q35-2.0(q35 has different numbers but the same logic) have 
3 values:

xen_enabled() ==> 3.75G

!xen_enabled()  && ram_size <= 3.5G ==> 3.5G
!xen_enabled()  && ram_size > 3.5G ==> 3.0G


Gerd has more on this in:

http://lists.gnu.org/archive/html/qemu-devel/2014-02/msg05146.html



If you think you need value per machine type, you can add it to
compat props. I don't see how is related, so you may not want to do so.


I need more then 1 value per machine type.  So I do not see how one default 
would work.

   -Don Slutz


Using compat_props would be great however does compat_props work for machine
type itself already?

Now that you are mentioning it, I was hoping that compat_props are converted
into QemuOpts or something and then mapped into machine properties.
I'll look into it.


  }
  static void pc_machine_class_init(ObjectClass *oc, void *data)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 40f6eaf..25f4727 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -98,6 +98,13 @@ static void pc_init1(MachineState *machine,
      DeviceState *icc_bridge;
      FWCfgState *fw_cfg = NULL;
      PcGuestInfo *guest_info;
+    Object *mo = qdev_get_machine();
+    PCMachineState *pcms = PC_MACHINE(mo);
+    ram_addr_t lowmem = 0xe0000000;
+
+    if (pcms && pcms->max_ram_below_4g) {

 From my QOM understanding, max_ram_below_4g is a private field,
so it not should be used directly.
You can use QOMs object_property_get or add a pc_machine wrapper
for getting/setting the field.

pc_init1() is sort of private function of PCMachine, so there is no much
point to use verbose getters/setters internally unless there is checks behind
setter.

I was just being QOM's advocate :). That being said, I'll not argue here,
as it is not a major issue.


Thanks,
Marcel


Is pcms ever NULL? If yes why?

Not that I know of.  I would be happy to convert this to an assert(pcms).

In fact, PC_MACHINE already includes an assert doesn't it?
So no need to check it everywhere.

+1. No need for assert here. Is already done by OBJECT_CHECK.

Hope I helped,
Marcel


+        lowmem = pcms->max_ram_below_4g;
+    }
      /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
       * If it doesn't, we need to split it in chunks below and above 4G.
@@ -106,8 +113,10 @@ static void pc_init1(MachineState *machine,
       * For old machine types, use whatever split we used historically to avoid
       * breaking migration.
       */
-    if (machine->ram_size >= 0xe0000000) {
-        ram_addr_t lowmem = gigabyte_align ? 0xc0000000 : 0xe0000000;
+    if (machine->ram_size >= lowmem) {
+        if (!(pcms && pcms->max_ram_below_4g) && gigabyte_align) {
+            lowmem = 0xc0000000;
+        }
          above_4g_mem_size = machine->ram_size - lowmem;
          below_4g_mem_size = lowmem;
      } else {

So why do we need gigabyte_align anymore?

Because of qemu 2.0 and the user is not required to specify this option.


Can't we set property to 0xc0000000 by default, and
override for old machine types?

There is a strange compatibility part here.  Since this code includes ram_size 
(see:

http://lists.gnu.org/archive/html/qemu-devel/2014-02/msg05146.html

) and xen has a different default.


So instead of default 0, it would be preferable to set the default to the
actual value, and let user override it.

Or if that's too hard, set max_ram_below_4g instead of setting
gigabyte_align. gigabyte_align switches everywhere is messy
enough, adding max_ram_below_4g into mix is just too messy.




Also, a value that isn't a multiple of 1G will lead to bad
performance for large machines which do have above_4g_mem_size.
Let's detect and print a warning.

Will Do.

    -Don Slutz




@@ -122,7 +131,7 @@ static void pc_init1(MachineState *machine,
      }
      icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
-    object_property_add_child(qdev_get_machine(), "icc-bridge",
+    object_property_add_child(mo, "icc-bridge",
                                OBJECT(icc_bridge), NULL);
      pc_cpus_init(machine->cpu_model, icc_bridge);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index e28ce40..155cdf1 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -85,6 +85,13 @@ static void pc_q35_init(MachineState *machine)
      PCIDevice *ahci;
      DeviceState *icc_bridge;
      PcGuestInfo *guest_info;
+    Object *mo = qdev_get_machine();
+    PCMachineState *pcms = PC_MACHINE(mo);
+    ram_addr_t lowmem = 0xb0000000;
+
+    if (pcms && pcms->max_ram_below_4g) {
+        lowmem = pcms->max_ram_below_4g;
+    }
      /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
       * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
@@ -95,8 +102,10 @@ static void pc_q35_init(MachineState *machine)
       * For old machine types, use whatever split we used historically to avoid
       * breaking migration.
       */
-    if (machine->ram_size >= 0xb0000000) {
-        ram_addr_t lowmem = gigabyte_align ? 0x80000000 : 0xb0000000;
+    if (machine->ram_size >= lowmem) {
+        if (!(pcms && pcms->max_ram_below_4g) && gigabyte_align) {
+            lowmem = 0x800000000;
+        }
          above_4g_mem_size = machine->ram_size - lowmem;
          below_4g_mem_size = lowmem;
      } else {
@@ -111,7 +120,7 @@ static void pc_q35_init(MachineState *machine)
      }
      icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
-    object_property_add_child(qdev_get_machine(), "icc-bridge",
+    object_property_add_child(mo, "icc-bridge",
                                OBJECT(icc_bridge), NULL);
      pc_cpus_init(machine->cpu_model, icc_bridge);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 19530bd..2d8b562 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -32,10 +32,13 @@ struct PCMachineState {
      MemoryRegion hotplug_memory;
      HotplugHandler *acpi_dev;
+
+    uint64_t max_ram_below_4g;
  };
  #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
  #define PC_MACHINE_MEMHP_REGION_SIZE "hotplug-memory-region-size"
+#define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
  /**
   * PCMachineClass:
diff --git a/vl.c b/vl.c
index 5e77a27..cffb9c5 100644
--- a/vl.c
+++ b/vl.c
@@ -382,6 +382,10 @@ static QemuOptsList qemu_machine_opts = {
              .name = "kvm-type",
              .type = QEMU_OPT_STRING,
              .help = "Specifies the KVM virtualization mode (HV, PR)",
+        },{
+            .name = PC_MACHINE_MAX_RAM_BELOW_4G,
+            .type = QEMU_OPT_SIZE,
+            .help = "maximum ram below the 4G boundary (32bit boundary)",
          },
          { /* End of list */ }
      },
--
1.8.4
reply via email to
[Prev in Thread] Current Thread [Next in Thread]