From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58540) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bG8P4-0007M2-HQ for qemu-devel@nongnu.org; Thu, 23 Jun 2016 13:26:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bG8Oz-0007Rk-2o for qemu-devel@nongnu.org; Thu, 23 Jun 2016 13:26:57 -0400 Received: from mail-vk0-x234.google.com ([2607:f8b0:400c:c05::234]:32927) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bG8Oy-0007RV-H2 for qemu-devel@nongnu.org; Thu, 23 Jun 2016 13:26:52 -0400 Received: by mail-vk0-x234.google.com with SMTP id d185so116200817vkg.0 for ; Thu, 23 Jun 2016 10:26:52 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1466131046-25516-1-git-send-email-tsunghan.tw@gmail.com> References: <1466131046-25516-1-git-send-email-tsunghan.tw@gmail.com> From: Peter Maydell Date: Thu, 23 Jun 2016 18:26:32 +0100 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [RFC] target-arm: fix semihosting ram base issue List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Tsung-Han Lin Cc: Paolo Bonzini , qemu-arm , QEMU Developers On 17 June 2016 at 03:37, Tsung-Han Lin wrote: > Hi, I made some changes to TRY TO fix the ARM semihosting issue in > SYS_HEAPINFO handling. > This problem has been bothering me for quite a while. > > A new global variable 'main_ram_base' is added while a new memory > API, memory_region_add_subregion_main, is also provided to let > SoC/board creator to initialize this variable. > I am not sure if this is a good idea (to add a new API) > or maybe we just let SoC/board creator to simply > set 'main_ram_base' in their 'xxx_realize' functions? > > As for Cortex-M series, 'main_ram_base' is set during cpu initialization. > A64 semihosting handling is also added and use zynqmp as an example. > > Any comments/reviews are big welcome! > Thanks in advance! Hi. First of all, unfortunately we can't accept any patch from you unless you provide a signed-off-by: line (which is basically saying you have the legal right to provide it to us under QEMU's license terms; see http://wiki.qemu.org/Contribute/SubmitAPatch#Patch_emails_must_include_a_Signed-off-by:_line for more detail). We can fix up most other stuff, but this one is a hard requirement. > diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c > index 23c719986715..8124f71992b4 100644 > --- a/hw/arm/xlnx-zynqmp.c > +++ b/hw/arm/xlnx-zynqmp.c > @@ -206,7 +206,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp) > memory_region_init_alias(&s->ddr_ram_high, NULL, > "ddr-ram-high", s->ddr_ram, > ddr_low_size, ddr_high_size); > - memory_region_add_subregion(get_system_memory(), > + memory_region_add_subregion_main(get_system_memory(), > XLNX_ZYNQMP_HIGH_RAM_START, > &s->ddr_ram_high); This isn't necessarily the main RAM for this board -- if you don't pass more than XLNX_ZYNQMP_MAX_LOW_RAM_SIZE as the RAM size then the only RAM is the low ram at address 0. In any case, even if you do have enough RAM to go through this code path, the executable being loaded might be linked so it goes into the low RAM alias at 0, in which case using this address as the heap start/limit would be wrong. > } else { If we can avoid having to change every board to specify this that would be nice. (Most of them already specify the RAM base in vbi->bootinfo.loader_start.) Is your use case passing an ELF file to QEMU to run? I suspect what we actually need to do for boards like the Xilinx with more than one RAM area is address the /* TODO: Make this use the limit of the loaded application. */ and actually use the values from the loaded executable, rather than guessing them. This would also address problems with the Cortex-M cores, where the application being loaded might be linked to be in RAM (non-zero start) or to be in flash (zero start). We should also be able to do a better job of guessing for simple boards with one RAM area at a non-zero offset, but if we look at the ELF files we're loading we might not need to bother... > diff --git a/target-arm/arm-semi.c b/target-arm/arm-semi.c > index 8be0645eb08b..d30469688b01 100644 > --- a/target-arm/arm-semi.c > +++ b/target-arm/arm-semi.c > @@ -599,17 +599,32 @@ target_ulong do_arm_semihosting(CPUARMState *env) > unlock_user(ptr, arg0, 16); > #else > limit = ram_size; > - ptr = lock_user(VERIFY_WRITE, arg0, 16, 0); > - if (!ptr) { > - /* FIXME - should this error code be -TARGET_EFAULT ? */ > - return (uint32_t)-1; > - } > - /* TODO: Make this use the limit of the loaded application. */ > - ptr[0] = tswap32(limit / 2); > - ptr[1] = tswap32(limit); > - ptr[2] = tswap32(limit); /* Stack base */ > - ptr[3] = tswap32(0); /* Stack limit. */ > - unlock_user(ptr, arg0, 16); > + if (is_a64(env)) { > + uint64_t *ptrx; > + ptrx = lock_user(VERIFY_WRITE, arg0, 32, 0); > + if (!ptrx) { > + /* FIXME - should this error code be -TARGET_EFAULT ? */ > + return (uint32_t)-1; > + } > + /* TODO: Make this use the limit of the loaded application. */ > + ptrx[0] = tswap64(main_ram_base + ram_size / 2); /* Heap base */ > + ptrx[1] = tswap64(main_ram_base + ram_size); /* limit */ > + ptrx[2] = tswap64(main_ram_base + ram_size); /* Stack base */ > + ptrx[3] = tswap64(main_ram_base + ram_size / 2); /* limit */ > + unlock_user(ptrx, arg0, 32); > + } else { > + ptr = lock_user(VERIFY_WRITE, arg0, 16, 0); > + if (!ptr) { > + /* FIXME - should this error code be -TARGET_EFAULT ? */ > + return (uint32_t)-1; > + } > + /* TODO: Make this use the limit of the loaded application. */ > + ptr[0] = tswap32(main_ram_base + limit / 2); > + ptr[1] = tswap32(main_ram_base + limit); > + ptr[2] = tswap32(main_ram_base + limit); /* Stack base */ > + ptr[3] = tswap32(main_ram_base); /* Stack limit. */ > + unlock_user(ptr, arg0, 16); > + } > #endif This is making two bug fixes at once. The part of this which is fixing the 64-bit code path to write 64-bit values into the data block is a simple non-controversial bugfix, and it should be in its own patch. Making better guesses at limit values for system emulation is trickier (see remarks above). You've also got some problems with your code indent, which should be four-space. scripts/checkpatch.pl can tell you about some style issues with patches. I suggest you start by sending a patch which just fixes the 64-bit case to write 64-bit values, since that's the easy bit. thanks -- PMM