From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53706) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wu5o1-0007at-PP for qemu-devel@nongnu.org; Mon, 09 Jun 2014 16:04:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wu5nw-0006eC-Iq for qemu-devel@nongnu.org; Mon, 09 Jun 2014 16:04:33 -0400 Received: from omzsmtpe04.verizonbusiness.com ([199.249.25.207]:60871) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wu5nw-0006e0-6C for qemu-devel@nongnu.org; Mon, 09 Jun 2014 16:04:28 -0400 From: Don Slutz Message-ID: <53961303.5030502@terremark.com> Date: Mon, 09 Jun 2014 16:03:15 -0400 MIME-Version: 1.0 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> <1402335224.3754.21.camel@localhost.localdomain> In-Reply-To: <1402335224.3754.21.camel@localhost.localdomain> Content-Type: text/plain; charset=UTF-8; format=flowed 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: Marcel Apfelbaum , Igor Mammedov , Gerd Hoffmann Cc: xen-devel@lists.xensource.com, Stefano Stabellini , "Michael S. Tsirkin" , qemu-devel@nongnu.org, Don Slutz , Anthony Liguori , =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= 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 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 >> 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 >>> >>> >> > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v5 2/3] pc & q35: Add new machine opt max-ram-below-4g Date: Mon, 09 Jun 2014 16:03:15 -0400 Message-ID: <53961303.5030502@terremark.com> 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> <1402335224.3754.21.camel@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1402335224.3754.21.camel@localhost.localdomain> 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: Marcel Apfelbaum , Igor Mammedov , Gerd Hoffmann Cc: xen-devel@lists.xensource.com, Stefano Stabellini , "Michael S. Tsirkin" , qemu-devel@nongnu.org, Don Slutz , Anthony Liguori , =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= List-Id: xen-devel@lists.xenproject.org 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 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 >> 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 >>> >>> >> > >