From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v3 4/4] xen-all: Pass max_ram_below_4g to xen_hvm_init. Date: Tue, 22 Apr 2014 14:27:49 -0400 Message-ID: <5356B4A5.1020004@terremark.com> References: <1395705336-22528-1-git-send-email-dslutz@verizon.com> <1395705336-22528-5-git-send-email-dslutz@verizon.com> <535150A5.4020206@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: <535150A5.4020206@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 , Marcel Apfelbaum , "Michael S. Tsirkin" , qemu-devel@nongnu.org, Anthony Liguori , Igor Mammedov , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On 04/18/14 12:19, Andreas Färber wrote: > Am 25.03.2014 00:55, schrieb Don Slutz: >> This is the xen part of "pc & q35: Add new object pc-memory-layout." >> >> Signed-off-by: Don Slutz >> --- >> v3: Adjust for code readability. Set max_ram_below_4g always and use >> it to calculate above_4g_mem_size, below_4g_mem_size. >> >> hw/i386/pc_piix.c | 4 ++-- >> hw/i386/pc_q35.c | 4 ++-- >> include/hw/xen/xen.h | 4 ++-- >> xen-all.c | 40 +++++++++++++++++++++------------------- >> xen-stub.c | 4 ++-- >> 5 files changed, 29 insertions(+), 27 deletions(-) >> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >> index 81d730d..8a93548 100644 >> --- a/hw/i386/pc_piix.c >> +++ b/hw/i386/pc_piix.c >> @@ -131,8 +131,8 @@ static void pc_init1(QEMUMachineInitArgs *args, >> below_4g_mem_size = args->ram_size; >> } >> >> - if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size, >> - &ram_memory) != 0) { >> + if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size, >> + &above_4g_mem_size, &ram_memory) != 0) { >> fprintf(stderr, "xen hardware virtual machine initialisation failed\n"); >> exit(1); >> } >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c >> index 529f53d..09e98e6 100644 >> --- a/hw/i386/pc_q35.c >> +++ b/hw/i386/pc_q35.c >> @@ -120,8 +120,8 @@ static void pc_q35_init(QEMUMachineInitArgs *args) >> below_4g_mem_size = args->ram_size; >> } >> >> - if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size, >> - &ram_memory) != 0) { >> + if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size, >> + &above_4g_mem_size, &ram_memory) != 0) { >> fprintf(stderr, "xen hardware virtual machine initialisation failed\n"); >> exit(1); >> } >> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h >> index 0f3942e..eca39a5 100644 >> --- a/include/hw/xen/xen.h >> +++ b/include/hw/xen/xen.h >> @@ -40,8 +40,8 @@ int xen_init(QEMUMachine *machine); >> void xenstore_store_pv_console_info(int i, struct CharDriverState *chr); >> >> #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY) >> -int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size, >> - MemoryRegion **ram_memory); >> +int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size, >> + ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory); >> void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, >> struct MemoryRegion *mr); >> void xen_modified_memory(ram_addr_t start, ram_addr_t length); >> diff --git a/xen-all.c b/xen-all.c >> index c64300c..3f6f890 100644 >> --- a/xen-all.c >> +++ b/xen-all.c >> @@ -155,31 +155,32 @@ qemu_irq *xen_interrupt_controller_init(void) >> >> /* Memory Ops */ >> >> -static void xen_ram_init(ram_addr_t *below_4g_mem_size, >> +static void xen_ram_init(ram_addr_t ram_size, ram_addr_t max_ram_below_4g, >> + ram_addr_t *below_4g_mem_size, >> ram_addr_t *above_4g_mem_size, >> - ram_addr_t ram_size, MemoryRegion **ram_memory_p) >> + MemoryRegion **ram_memory_p) >> { >> MemoryRegion *sysmem = get_system_memory(); >> ram_addr_t block_len; >> >> - block_len = ram_size; >> - if (ram_size >= HVM_BELOW_4G_RAM_END) { >> - /* Xen does not allocate the memory continuously, and keep a hole at >> - * HVM_BELOW_4G_MMIO_START of HVM_BELOW_4G_MMIO_LENGTH >> - */ >> - block_len += HVM_BELOW_4G_MMIO_LENGTH; >> - } >> - memory_region_init_ram(&ram_memory, NULL, "xen.ram", block_len); >> - *ram_memory_p = &ram_memory; >> - vmstate_register_ram_global(&ram_memory); >> - >> - if (ram_size >= HVM_BELOW_4G_RAM_END) { >> - *above_4g_mem_size = ram_size - HVM_BELOW_4G_RAM_END; >> - *below_4g_mem_size = HVM_BELOW_4G_RAM_END; >> + max_ram_below_4g = max_ram_below_4g ? max_ram_below_4g : HVM_BELOW_4G_RAM_END; > Isn't max_ram_below_4g still always zero at this point? No. Patch 3/4 gets this from the global "max-ram-below-4g". So it is only 0 when the user has not set this. > Are you guys still using your own Xen machine types? Yes and no: flexarray_append(dm_args, "pc,accel=xen"); } else { flexarray_append(dm_args, "xenfv"); > Then you could set > a compat_props entry for all (not just legacy) machines to override the > actual pc-memory-layout max-ram-below-4g property value to avoid the ?: > here. Especially since I'm not seeing this modified value being written > back to the device. I think this is still possible. Will look into it. Did not see any need to write this back. > Considering that machines are objects now, might it be easier to set > this property on the machine object itself rather than having an unused > stub object hanging in limbo? That would be fine with me. > Would create a dependency on Marcel's > series though (which like this series I now need to start reviewing). I will look into using this machine object(s). What I found was: [PATCH v3 0/3] qemu-machine as a QOM object [PATCH V3 0/5] remove QEMUMachine indirection from MachineClass And if I have it correct, you are referring to the 2nd of these as a dependency. >> + if (ram_size >= max_ram_below_4g) { >> + *above_4g_mem_size = ram_size - max_ram_below_4g; >> + *below_4g_mem_size = max_ram_below_4g; >> } else { >> *above_4g_mem_size = 0; >> *below_4g_mem_size = ram_size; >> } >> + if (!*above_4g_mem_size) { >> + block_len = ram_size; >> + } else { >> + /* Xen does not allocate the memory continuously, and keep a hole of >> + * of the size computed above or passed in. */ > Please keep */ in the front for easily spotting where the comment ends. > > Possibly even /* is requested to be on its own line by Coding Style...? > But that'd be a mere code movement. > I did not find any statement about this in CODING_STYLE, but will adjust to this. -Don Slutz >> + block_len = (1ULL << 32) + *above_4g_mem_size; >> + } >> + memory_region_init_ram(&ram_memory, NULL, "xen.ram", block_len); >> + *ram_memory_p = &ram_memory; >> + vmstate_register_ram_global(&ram_memory); >> >> memory_region_init_alias(&ram_640k, NULL, "xen.ram.640k", >> &ram_memory, 0, 0xa0000); > [snip] > > Regards, > Andreas >