From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v3 3/4] pc & q35: Add new object pc-memory-layout. Date: Tue, 22 Apr 2014 19:54:32 -0400 Message-ID: <53570138.2020408@terremark.com> References: <1395705336-22528-1-git-send-email-dslutz@verizon.com> <1395705336-22528-4-git-send-email-dslutz@verizon.com> <535148AE.9030500@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <535148AE.9030500@suse.de> 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: =?ISO-8859-15?Q?Andreas_F=E4rber?= , Don Slutz Cc: xen-devel@lists.xensource.com, Eduardo Habkost , "Michael S. Tsirkin" , Stefano Stabellini , qemu-devel@nongnu.org, Anthony Liguori , Igor Mammedov List-Id: xen-devel@lists.xenproject.org On 04/18/14 11:45, Andreas Färber wrote: > Am 25.03.2014 00:55, schrieb Don Slutz: >> This new object has the property max-ram-below-4g. >> >> 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 adding "-global pc-memory-layout.max-ram-below-4g=2G" to >> the command line will limit the amount of ram that is below 4G to >> 2G. >> >> Signed-off-by: Don Slutz >> --- >> hw/i386/pc.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >> hw/i386/pc_piix.c | 23 +++++++++++++++++++++-- >> hw/i386/pc_q35.c | 23 +++++++++++++++++++++-- >> include/hw/i386/pc.h | 2 ++ >> 4 files changed, 86 insertions(+), 4 deletions(-) >> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index 14f0d91..45339f3 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -1429,3 +1429,45 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name) >> gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i); >> } >> } >> + >> +/* pc-memory-layout stuff */ >> + >> +typedef struct PcMemoryLayoutState PcMemoryLayoutState; > PCMemoryLayoutState? Fine with me. >> + >> +/** >> + * @PcMemoryLayoutState: > Please drop @. Will do. >> + */ >> +struct PcMemoryLayoutState { >> + /*< private >*/ >> + Object parent_obj; >> + /*< public >*/ >> + >> + uint64_t max_ram_below_4g; >> +}; >> + >> +static Property pc_memory_layout_props[] = { >> + DEFINE_PROP_SIZE("max-ram-below-4g", PcMemoryLayoutState, >> + max_ram_below_4g, 0), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> +static void pc_memory_layout_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + dc->props = pc_memory_layout_props; >> +} >> + >> +static const TypeInfo pc_memory_layout_info = { >> + .name = TYPE_PC_MEMORY_LAYOUT, >> + .parent = TYPE_DEVICE, >> + .instance_size = sizeof(PcMemoryLayoutState), >> + .class_init = pc_memory_layout_class_init, >> +}; >> + >> +static void pc_memory_layout_register_types(void) >> +{ >> + type_register_static(&pc_memory_layout_info); >> +} >> + >> +type_init(pc_memory_layout_register_types) >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >> index bd52f6e..81d730d 100644 >> --- a/hw/i386/pc_piix.c >> +++ b/hw/i386/pc_piix.c >> @@ -95,6 +95,23 @@ static void pc_init1(QEMUMachineInitArgs *args, >> DeviceState *icc_bridge; >> FWCfgState *fw_cfg = NULL; >> PcGuestInfo *guest_info; >> + Object *pc_memory_layout; >> + uint64_t max_ram_below_4g; >> + ram_addr_t lowmem = 0xe0000000; >> + >> + pc_memory_layout = object_new(TYPE_PC_MEMORY_LAYOUT); >> + max_ram_below_4g = object_property_get_int(pc_memory_layout, >> + "max-ram-below-4g", >> + NULL); >> + if (max_ram_below_4g) { > Isn't this always zero due to the default value? Nope. I forget if object_new() or object_property_get_int() check the global list. So if the user sets this, the value is found at this time. >> + if (max_ram_below_4g > (1ULL << 32)) { >> + fprintf(stderr, >> + "%s: max_ram_below_4g=%lld too big adjusted to %lld\n", >> + __func__, (long long)max_ram_below_4g, 1ULL << 32); > Is this forgotten debug output? For a user, __func__ is not helpful and > error_report() would be preferable. Also please just use PRIx64 for > max_ram_below_4g rather than casting. Nope. Will change to use error_report() without __func__ and PRIx64. >> + max_ram_below_4g = 1ULL << 32; >> + } >> + lowmem = 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. >> @@ -103,8 +120,10 @@ static void pc_init1(QEMUMachineInitArgs *args, >> * For old machine types, use whatever split we used historically to avoid >> * breaking migration. >> */ >> - if (args->ram_size >= 0xe0000000) { >> - ram_addr_t lowmem = gigabyte_align ? 0xc0000000 : 0xe0000000; >> + if (args->ram_size >= lowmem) { >> + if (!max_ram_below_4g && gigabyte_align) { >> + lowmem = 0xc0000000; >> + } >> above_4g_mem_size = args->ram_size - lowmem; >> below_4g_mem_size = lowmem; >> } else { > We don't have central, recursive realization of devices yet. That means > your device is instantiated but never realized, and while you're not > exposing any global state it allows to tweak the property value > throughout device lifetime. Not sure how to mark this as read only. I have not considered how this can be changed. There are a lot of other internals that are generated off of this like ram_size_below_4g. > > Also you're forgetting to add this device to the composition tree via > object_property_add_child(). Neither ACPI code nor future QOM code can > discover it that way. Ok, will add a call on object_property_add_child(). Or is the fact that this ends up in PcGuestInfoState and passed directly to acpi_setup() mean that it should not be? >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c >> index 6e34fe1..529f53d 100644 >> --- a/hw/i386/pc_q35.c >> +++ b/hw/i386/pc_q35.c >> @@ -82,6 +82,23 @@ static void pc_q35_init(QEMUMachineInitArgs *args) >> PCIDevice *ahci; >> DeviceState *icc_bridge; >> PcGuestInfo *guest_info; >> + Object *pc_memory_layout; >> + uint64_t max_ram_below_4g; >> + ram_addr_t lowmem = 0xb0000000; >> + >> + pc_memory_layout = object_new(TYPE_PC_MEMORY_LAYOUT); >> + max_ram_below_4g = object_property_get_int(pc_memory_layout, >> + "max-ram-below-4g", >> + NULL); >> + if (max_ram_below_4g) { >> + if (max_ram_below_4g > (1ULL << 32)) { >> + fprintf(stderr, >> + "%s: max_ram_below_4g=%lld too big adjusted to %lld\n", >> + __func__, (long long)max_ram_below_4g, 1ULL << 32); >> + max_ram_below_4g = 1ULL << 32; >> + } >> + lowmem = 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 >> @@ -92,8 +109,10 @@ static void pc_q35_init(QEMUMachineInitArgs *args) >> * For old machine types, use whatever split we used historically to avoid >> * breaking migration. >> */ >> - if (args->ram_size >= 0xb0000000) { >> - ram_addr_t lowmem = gigabyte_align ? 0x80000000 : 0xb0000000; >> + if (args->ram_size >= lowmem) { >> + if (!max_ram_below_4g && gigabyte_align) { >> + lowmem = 0x80000000; >> + } >> above_4g_mem_size = args->ram_size - lowmem; >> below_4g_mem_size = lowmem; >> } else { > Dito here for all pc_piix.c comments. Same as above. -Don Slutz >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h >> index 9010246..9162d61 100644 >> --- a/include/hw/i386/pc.h >> +++ b/include/hw/i386/pc.h >> @@ -163,6 +163,8 @@ void cpu_smm_register(cpu_set_smm_t callback, void *arg); >> >> void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name); >> >> +#define TYPE_PC_MEMORY_LAYOUT "pc-memory-layout" >> + >> /* acpi_piix.c */ >> >> I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base, > Regards, > Andreas >