[Qemu-devel] [RFC] target-arm: fix semihosting ram base issue

* [Qemu-devel] [RFC] target-arm: fix semihosting ram base issue
@ 2016-06-17  2:37 Tsung-Han Lin
  2016-06-17  8:43 ` Peter Maydell
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Tsung-Han Lin @ 2016-06-17  2:37 UTC (permalink / raw)
  To: peter.maydell, pbonzini; +Cc: qemu-arm, qemu-devel, tsunghan.tw

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!
---
 hw/arm/xlnx-zynqmp.c      |  2 +-
 include/exec/cpu-common.h |  1 +
 include/exec/memory.h     |  6 ++++++
 memory.c                  |  8 ++++++++
 target-arm/arm-semi.c     | 37 ++++++++++++++++++++++++++-----------
 target-arm/cpu.c          |  1 +
 vl.c                      |  1 +
 7 files changed, 44 insertions(+), 12 deletions(-)

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;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread