From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40672) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wu3SD-0006nr-V5 for qemu-devel@nongnu.org; Mon, 09 Jun 2014 13:33:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wu3S9-0002t8-VA for qemu-devel@nongnu.org; Mon, 09 Jun 2014 13:33:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:13198) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wu3S9-0002t1-Lk for qemu-devel@nongnu.org; Mon, 09 Jun 2014 13:33:49 -0400 Message-ID: <1402335224.3754.21.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Mon, 09 Jun 2014 20:33:44 +0300 In-Reply-To: <20140609173719.111a6b2f@thinkpad> References: <1402077126-17799-1-git-send-email-dslutz@verizon.com> <1402077126-17799-3-git-send-email-dslutz@verizon.com> <20140608154014.GE8464@redhat.com> <5395C2C9.7070200@terremark.com> <20140609143844.GA6797@redhat.com> <1402326627.3754.12.camel@localhost.localdomain> <20140609173719.111a6b2f@thinkpad> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 2/3] pc & q35: Add new machine opt max-ram-below-4g List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: xen-devel@lists.xensource.com, Stefano Stabellini , "Michael S. Tsirkin" , qemu-devel@nongnu.org, Don Slutz , Anthony Liguori , Andreas =?ISO-8859-1?Q?F=E4rber?= On Mon, 2014-06-09 at 17:37 +0200, Igor Mammedov wrote: > On Mon, 09 Jun 2014 18:10:27 +0300 > Marcel Apfelbaum 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 > > > > >>--- > > > > >>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 > > > 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. > 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 > > > > > > > > > > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcel Apfelbaum Subject: Re: [PATCH v5 2/3] pc & q35: Add new machine opt max-ram-below-4g Date: Mon, 09 Jun 2014 20:33:44 +0300 Message-ID: <1402335224.3754.21.camel@localhost.localdomain> References: <1402077126-17799-1-git-send-email-dslutz@verizon.com> <1402077126-17799-3-git-send-email-dslutz@verizon.com> <20140608154014.GE8464@redhat.com> <5395C2C9.7070200@terremark.com> <20140609143844.GA6797@redhat.com> <1402326627.3754.12.camel@localhost.localdomain> <20140609173719.111a6b2f@thinkpad> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140609173719.111a6b2f@thinkpad> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org To: Igor Mammedov Cc: xen-devel@lists.xensource.com, Stefano Stabellini , "Michael S. Tsirkin" , qemu-devel@nongnu.org, Don Slutz , Anthony Liguori , Andreas =?ISO-8859-1?Q?F=E4rber?= List-Id: xen-devel@lists.xenproject.org On Mon, 2014-06-09 at 17:37 +0200, Igor Mammedov wrote: > On Mon, 09 Jun 2014 18:10:27 +0300 > Marcel Apfelbaum 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 > > > > >>--- > > > > >>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 > > > 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. > 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 > > > > > > > > > > >