From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58241) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bGTSe-00005o-K7 for qemu-devel@nongnu.org; Fri, 24 Jun 2016 11:56:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bGTSb-0005ii-9C for qemu-devel@nongnu.org; Fri, 24 Jun 2016 11:56:03 -0400 MIME-Version: 1.0 In-Reply-To: References: <1466131046-25516-1-git-send-email-tsunghan.tw@gmail.com> From: Tsung-Han Lin Date: Sat, 25 Jun 2016 00:55:53 +0900 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: Peter Maydell Cc: Paolo Bonzini , qemu-arm , QEMU Developers 2016-06-24 1:26 GMT+08:00 Peter Maydell : > 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. > > Hi, Peter, Thanks for the comments. Well, actually I was just trying to throw out some of my thoughts and get some feedbacks and comments. (not intend to get merged :p) since the approach I used involved some API changes and I am not sure if that's a good idea or not. But I will make sure next time I send out something that will just as the normal patch sets should be. Thank you for all those comments. > > 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? > Yes, I always use an ELF file to do some experiments. > 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 > Seems like you already have the solutions! thanks anyway :p (was just still trying to get more understanding of the code base :p) -- Tsung-Han "*Johnny*" Lin Page: http://tsunghanlin.github.com/ Email: tsunghan.tw@gmail.com