From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36235) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPcDL-0004t7-1B for qemu-devel@nongnu.org; Mon, 17 Mar 2014 14:24:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WPcDE-0005PI-TR for qemu-devel@nongnu.org; Mon, 17 Mar 2014 14:24:42 -0400 Received: from omzsmtpe04.verizonbusiness.com ([199.249.25.207]:11227) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPcDE-0005Oa-MI for qemu-devel@nongnu.org; Mon, 17 Mar 2014 14:24:36 -0400 From: Don Slutz Message-ID: <53273DE1.9000501@terremark.com> Date: Mon, 17 Mar 2014 14:24:33 -0400 MIME-Version: 1.0 References: <1394553546-6581-1-git-send-email-dslutz@verizon.com> <1394553546-6581-5-git-send-email-dslutz@verizon.com> <53270C42.60804@terremark.com> <53273D83.7030800@terremark.com> In-Reply-To: <53273D83.7030800@terremark.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 4/4] xen-all: Pass max_ram_below_4g to xen_hvm_init. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Don Slutz , Stefano Stabellini Cc: xen-devel@lists.xensource.com, qemu-devel@nongnu.org, Anthony Liguori , "Michael S. Tsirkin" On 03/17/14 14:22, Don Slutz wrote: > On 03/17/14 13:55, Stefano Stabellini wrote: >> On Mon, 17 Mar 2014, Don Slutz wrote: >>> On 03/16/14 12:10, Stefano Stabellini wrote: >>>> On Tue, 11 Mar 2014, Don Slutz wrote: >>>>> This is the xen part of "pc & q35: Add new object pc-memory-layout." >>>>> >>>>> Signed-off-by: Don Slutz >>>>> --- >>>>> hw/i386/pc_piix.c | 4 ++-- >>>>> hw/i386/pc_q35.c | 4 ++-- >>>>> include/hw/xen/xen.h | 4 ++-- >>>>> xen-all.c | 41 ++++++++++++++++++++++------------------- >>>>> xen-stub.c | 4 ++-- >>>>> 5 files changed, 30 insertions(+), 27 deletions(-) >>>>> >>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >>>>> index 964ea33..691fc5d 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 c95e6e2..8e1c417 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 0769db2..fda559a 100644 >>>>> --- a/include/hw/xen/xen.h >>>>> +++ b/include/hw/xen/xen.h >>>>> @@ -41,8 +41,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..48ba335 100644 >>>>> --- a/xen-all.c >>>>> +++ b/xen-all.c >>>>> @@ -155,32 +155,34 @@ 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; >>>>> + if (!max_ram_below_4g) { >>>>> + 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; >>>>> + } else { >>>>> + *above_4g_mem_size = 0; >>>>> + *below_4g_mem_size = ram_size; >>>>> + } >>>>> + } >>>> Instead of treating max_ram_below_4g as a special case, couldn't >>>> initialize it to HVM_BELOW_4G_RAM_END? >>> Since max_ram_below_4g is used and initialize in normal QEMU code >>> where HVM_BELOW_4G_RAM_END is not defined, I do not see how to >>> do this outside of this routine without adding a new routine for this. >> You can simply do this at the beginning of xen_ram_init: >> >> max_ram_below_4g = max_ram_below_4g ? max_ram_below_4g : HVM_BELOW_4G_RAM_END; >> >> my comment was about code readability, I didn't mean change any >> functionalities. > > So the partial code delta: > > 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; > + 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. */ > + 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); > > > Which does redo the calculation of above and below when max_ram_below_4g > is set, is better for code readability. > > I am happy to send a v2 with this, just do not want to send too many versions. > That would be v3. :( > -Don Slutz > > >