From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v2 4/4] xen-all: Pass max_ram_below_4g to xen_hvm_init. Date: Mon, 17 Mar 2014 14:22:59 -0400 Message-ID: <53273D83.7030800@terremark.com> References: <1394553546-6581-1-git-send-email-dslutz@verizon.com> <1394553546-6581-5-git-send-email-dslutz@verizon.com> <53270C42.60804@terremark.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini , Don Slutz Cc: xen-devel@lists.xensource.com, qemu-devel@nongnu.org, Anthony Liguori , "Michael S. Tsirkin" List-Id: xen-devel@lists.xenproject.org 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. -Don Slutz