| Message ID | 1466131046-25516-1-git-send-email-tsunghan.tw@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 17 June 2016 at 03:37, Tsung-Han Lin <tsunghan.tw@gmail.com> 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. Could you explain what the bug is you're trying to fix, please? thanks -- PMM
2016-06-17 17:43 GMT+09:00 Peter Maydell <peter.maydell@linaro.org>: > On 17 June 2016 at 03:37, Tsung-Han Lin <tsunghan.tw@gmail.com> 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. > > Could you explain what the bug is you're trying to fix, please? > > I was trying to use semihosting on different configs. Each of them have different ram base values, which is assumed to be 0x0 in qemu. However, this may not be true for every SoC/boards. (e.g., Cortex-M's ram addr is started on 0x20000000) https://lists.gnu.org/archive/html/qemu-devel/2012-08/msg04641.html So in order to use semihosting in different configs, I have to change ram base value every time which is a bit inconvenience. Also I added semihosting support for ARMv8 devices. I am not sure my changes is legit or not and need some advices/comments from you all. Thanks. > thanks > -- PMM >
> On 17 Jun 2016, at 05:37, Tsung-Han Lin <tsunghan.tw@gmail.com> wrote: > > Hi, I made some changes to TRY TO fix the ARM semihosting issue ... > This problem has been bothering me for quite a while. semihosting was the first thing I fixed in GNU ARM Eclipse QEMU, and since then I use it constantly. I don't know if this addresses your problem, but perhaps you could also take a look there. regards, Liviu
2016-06-18 1:22 GMT+09:00 Liviu Ionescu <ilg@livius.net>: > > > On 17 Jun 2016, at 05:37, Tsung-Han Lin <tsunghan.tw@gmail.com> wrote: > > > > Hi, I made some changes to TRY TO fix the ARM semihosting issue ... > > This problem has been bothering me for quite a while. > > semihosting was the first thing I fixed in GNU ARM Eclipse QEMU, and since > then I use it constantly. > > I don't know if this addresses your problem, but perhaps you could also > take a look there. > Hi, Liviu, Thanks for the suggestion. It seems like to me that the issue is the default address assumed by qemu, which is 0x0. (since Eclipse QEMU uses the same code, I believe they have the same problem.) But it's not always the case. For example, zynqmp's ram base address is 0x80000000, and I have to tweak that address in HEAPINFO handling code every time I change to a new HW config. Regards, > > > regards, > > Liviu > >
> On 18 Jun 2016, at 01:22, Tsung-Han Lin <tsunghan.tw@gmail.com> wrote: > > ... It seems like to me that the issue is the default address assumed by qemu, which is 0x0. > (since Eclipse QEMU uses the same code, I believe they have the same problem.) it uses the same main code, but with many improvements. if I remember right, for semihosting there were several small problems requiring changes, but they were all fixed now. you can give it a try (http://gnuarmeclipse.github.io/qemu/options/). in addition to semihosting, which I use for running the CMSIS++ tests, GNU ARM Eclipse QEMU also implements graphical animated LEDs for a selection of boards, which might be very convenient for simple blinky tests. regards, Liviu
2016-06-18 14:57 GMT+09:00 Liviu Ionescu <ilg@livius.net>: > > > On 18 Jun 2016, at 01:22, Tsung-Han Lin <tsunghan.tw@gmail.com> wrote: > > > > ... It seems like to me that the issue is the default address assumed by > qemu, which is 0x0. > > (since Eclipse QEMU uses the same code, I believe they have the same > problem.) > > it uses the same main code, but with many improvements. > > if I remember right, for semihosting there were several small problems > requiring changes, but they were all fixed now. you can give it a try ( > http://gnuarmeclipse.github.io/qemu/options/). > > in addition to semihosting, which I use for running the CMSIS++ tests, GNU > ARM Eclipse QEMU also implements graphical animated LEDs for a selection of > boards, which might be very convenient for simple blinky tests. > > > Hi, Thanks again. Actually I've tried Eclipse version, but I couldn't find the option that I need, which like I wrote in the previous replies is the ram address part.(not just Cortex-M) I need to change it to some of my customized created boards's defined values every time I change to another one. Also, since I need to work on CLI environment to do some semihosting tests, I would really like to have this ram base issue solved in the mainline qemu code. Regards, > regards, > > Liviu > >
On 17 June 2016 at 03:37, Tsung-Han Lin <tsunghan.tw@gmail.com> 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
On 23 June 2016 at 18:26, Peter Maydell <peter.maydell@linaro.org> wrote: > 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. I ran into this bug myself this afternoon, so I wrote some patches which fix this part and will send them out in a minute. thanks -- PMM
2016-06-24 1:26 GMT+08:00 Peter Maydell <peter.maydell@linaro.org>: > On 17 June 2016 at 03:37, Tsung-Han Lin <tsunghan.tw@gmail.com> 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)
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); } else { diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h index aaee99563464..c345e61ede16 100644 --- a/include/exec/cpu-common.h +++ b/include/exec/cpu-common.h @@ -49,6 +49,7 @@ typedef uintptr_t ram_addr_t; #endif extern ram_addr_t ram_size; +extern ram_addr_t main_ram_base; /* memory API */ diff --git a/include/exec/memory.h b/include/exec/memory.h index 4ab680052f27..d76b0a069c98 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -972,6 +972,8 @@ void memory_region_del_eventfd(MemoryRegion *mr, * may only be added once as a subregion (unless removed with * memory_region_del_subregion()); use memory_region_init_alias() if you * want a region to be a subregion in multiple locations. + * The _main version is used to define the main working ram area, such ddr + * ram region. * * @mr: the region to contain the new subregion; must be a container * initialized with memory_region_init(). @@ -981,6 +983,10 @@ void memory_region_del_eventfd(MemoryRegion *mr, void memory_region_add_subregion(MemoryRegion *mr, hwaddr offset, MemoryRegion *subregion); + +void memory_region_add_subregion_main(MemoryRegion *mr, + hwaddr offset, + MemoryRegion *subregion); /** * memory_region_add_subregion_overlap: Add a subregion to a container * with overlap. diff --git a/memory.c b/memory.c index 8ba496dc7b2a..3221838abefe 100644 --- a/memory.c +++ b/memory.c @@ -1911,6 +1911,14 @@ void memory_region_add_subregion(MemoryRegion *mr, memory_region_add_subregion_common(mr, offset, subregion); } +void memory_region_add_subregion_main(MemoryRegion *mr, + hwaddr offset, + MemoryRegion *subregion) +{ + main_ram_base = offset; + memory_region_add_subregion(mr, offset, subregion); +} + void memory_region_add_subregion_overlap(MemoryRegion *mr, hwaddr offset, MemoryRegion *subregion, 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 return 0; } diff --git a/target-arm/cpu.c b/target-arm/cpu.c index 3fd0743cb391..fbc7d6914694 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -193,6 +193,7 @@ static void arm_cpu_reset(CPUState *s) initial_msp = ldl_phys(s->as, 0); initial_pc = ldl_phys(s->as, 4); } + main_ram_base = 0x20000000; env->regs[13] = initial_msp & 0xFFFFFFFC; env->regs[15] = initial_pc & ~1; diff --git a/vl.c b/vl.c index 0736d8430dc3..ff1eeb50329f 100644 --- a/vl.c +++ b/vl.c @@ -133,6 +133,7 @@ int request_opengl = -1; int display_opengl; const char* keyboard_layout = NULL; ram_addr_t ram_size; +ram_addr_t main_ram_base = 0x0; /* default ram base to 0 */ const char *mem_path = NULL; int mem_prealloc = 0; /* force preallocation of physical target memory */ bool enable_mlock = false;