[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Minios-devel] [UNIKRAFT PATCH] plat/*: Replace memregion hardcoded values

Hi Christian,

without checking the details of the code, the idea looks fine to me.

Acked-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>
However, Simon has been working on this code recently and run into some additional complicated cases. It sounds like there are some issues, and we might actually need a different approach that hopefully might also not require having different enums for different platforms either. 

Maybe Simon can say something about that.

Cheers,
Florian


On 4/5/19 10:54 AM, Cristian Banu wrote:

This patch replaces the hardcoded values for the memory region indexes
with enums, in order to make the process of adding or removing memory
regions less fragile.

Signed-off-by: Cristian Banu <cristb@xxxxxxxxx>
---
  plat/kvm/memory.c | 33 +++++++++++++++++++++++----------
  plat/xen/memory.c | 30 +++++++++++++++++++++---------
  2 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/plat/kvm/memory.c b/plat/kvm/memory.c
index e45b88c59495..0b6a3cee539d 100644
--- a/plat/kvm/memory.c
+++ b/plat/kvm/memory.c
@@ -31,9 +31,22 @@ extern void *_libkvmplat_heap_start;
  extern void *_libkvmplat_stack_top;
  extern void *_libkvmplat_mem_end;
+enum ukplat_memregions { 
+       UKPLAT_MEMREGION_TEXT,
+       UKPLAT_MEMREGION_EH_FRAME,
+       UKPLAT_MEMREGION_EH_FRAME_HDR,
+       UKPLAT_MEMREGION_RODATA,
+       UKPLAT_MEMREGION_CTORS,
+       UKPLAT_MEMREGION_DATA,
+       UKPLAT_MEMREGION_BSS,
+       UKPLAT_MEMREGION_HEAP,
+       UKPLAT_MEMREGION_STACK,
+       UKPLAT_MEMREGION_COUNT
+};
+
  int ukplat_memregion_count(void)
  {
-       return 9;
+       return UKPLAT_MEMREGION_COUNT;
  }
int ukplat_memregion_get(int i, struct ukplat_memregion_desc *m) 
@@ -43,7 +56,7 @@ int ukplat_memregion_get(int i, struct ukplat_memregion_desc 
*m)
        UK_ASSERT(m);
switch (i) { 
-       case 0: /* text */
+       case UKPLAT_MEMREGION_TEXT:
                m->base  = (void *) __TEXT;
                m->len   = (size_t) __ETEXT - (size_t) __TEXT;
                m->flags = (UKPLAT_MEMRF_RESERVED
@@ -53,7 +66,7 @@ int ukplat_memregion_get(int i, struct ukplat_memregion_desc 
*m)
  #endif
                ret = 0;
                break;
-       case 1: /* eh_frame */
+       case UKPLAT_MEMREGION_EH_FRAME:
                m->base  = (void *) __EH_FRAME_START;
                m->len   = (size_t) __EH_FRAME_END
                                - (size_t) __EH_FRAME_START;
@@ -64,7 +77,7 @@ int ukplat_memregion_get(int i, struct ukplat_memregion_desc 
*m)
                m->name  = "eh_frame";
  #endif
                break;
-       case 2: /* eh_frame_hdr */
+       case UKPLAT_MEMREGION_EH_FRAME_HDR:
                m->base  = (void *) __EH_FRAME_HDR_START;
                m->len   = (size_t) __EH_FRAME_HDR_END
                           - (size_t) __EH_FRAME_HDR_START;
@@ -75,7 +88,7 @@ int ukplat_memregion_get(int i, struct ukplat_memregion_desc 
*m)
                m->name  = "eh_frame_hdr";
  #endif
                break;
-       case 3: /* rodata */
+       case UKPLAT_MEMREGION_RODATA:
                m->base  = (void *) __RODATA;
                m->len   = (size_t) __ERODATA - (size_t) __RODATA;
                m->flags = (UKPLAT_MEMRF_RESERVED
@@ -85,7 +98,7 @@ int ukplat_memregion_get(int i, struct ukplat_memregion_desc 
*m)
  #endif
                ret = 0;
                break;
-       case 4: /* ctors */
+       case UKPLAT_MEMREGION_CTORS:
                m->base  = (void *) __CTORS;
                m->len   = (size_t) __ECTORS - (size_t) __CTORS;
                m->flags = (UKPLAT_MEMRF_RESERVED
@@ -95,7 +108,7 @@ int ukplat_memregion_get(int i, struct ukplat_memregion_desc 
*m)
  #endif
                ret = 0;
                break;
-       case 5: /* data */
+       case UKPLAT_MEMREGION_DATA:
                m->base  = (void *) __DATA;
                m->len   = (size_t) __EDATA - (size_t) __DATA;
                m->flags = (UKPLAT_MEMRF_RESERVED
@@ -106,7 +119,7 @@ int ukplat_memregion_get(int i, struct 
ukplat_memregion_desc *m)
  #endif
                ret = 0;
                break;
-       case 6: /* bss */
+       case UKPLAT_MEMREGION_BSS:
                m->base  = (void *) __BSS_START;
                m->len   = (size_t) __END - (size_t) __BSS_START;
                m->flags = (UKPLAT_MEMRF_RESERVED
@@ -117,7 +130,7 @@ int ukplat_memregion_get(int i, struct 
ukplat_memregion_desc *m)
  #endif
                ret = 0;
                break;
-       case 7: /* heap */
+       case UKPLAT_MEMREGION_HEAP:
                m->base  = _libkvmplat_heap_start;
                m->len   = (size_t) _libkvmplat_stack_top
                           - (size_t) _libkvmplat_heap_start;
@@ -127,7 +140,7 @@ int ukplat_memregion_get(int i, struct 
ukplat_memregion_desc *m)
  #endif
                ret = 0;
                break;
-       case 8: /* stack */
+       case UKPLAT_MEMREGION_STACK:
                m->base  = _libkvmplat_stack_top;
                m->len   = (size_t) _libkvmplat_mem_end
                           - (size_t) _libkvmplat_stack_top;
diff --git a/plat/xen/memory.c b/plat/xen/memory.c
index cb1c3552a0d9..d477544b29df 100644
--- a/plat/xen/memory.c
+++ b/plat/xen/memory.c
@@ -45,9 +45,20 @@
#include <uk/assert.h> +enum ukplat_memregions { 
+       UKPLAT_MEMREGION_TEXT,
+       UKPLAT_MEMREGION_EH_FRAME,
+       UKPLAT_MEMREGION_EH_FRAME_HDR,
+       UKPLAT_MEMREGION_RODATA,
+       UKPLAT_MEMREGION_CTORS,
+       UKPLAT_MEMREGION_DATA,
+       UKPLAT_MEMREGION_BSS,
+       UKPLAT_MEMREGION_COUNT
+};
+
  int ukplat_memregion_count(void)
  {
-       return (int) _libxenplat_mrd_num + 7;
+       return (int) _libxenplat_mrd_num + UKPLAT_MEMREGION_COUNT;
  }
int ukplat_memregion_get(int i, struct ukplat_memregion_desc *m) 
@@ -56,7 +67,7 @@ int ukplat_memregion_get(int i, struct ukplat_memregion_desc 
*m)
        UK_ASSERT(m);
switch (i) { 
-       case 0: /* text */
+       case UKPLAT_MEMREGION_TEXT:
                m->base  = (void *) __TEXT;
                m->len   = (size_t) __ETEXT - (size_t) __TEXT;
                m->flags = (UKPLAT_MEMRF_RESERVED
@@ -65,7 +76,7 @@ int ukplat_memregion_get(int i, struct ukplat_memregion_desc 
*m)
                m->name  = "text";
  #endif
                break;
-       case 1: /* eh_frame */
+       case UKPLAT_MEMREGION_EH_FRAME:
                m->base  = (void *) __EH_FRAME_START;
                m->len   = (size_t) __EH_FRAME_END
                                - (size_t) __EH_FRAME_START;
@@ -75,7 +86,7 @@ int ukplat_memregion_get(int i, struct ukplat_memregion_desc 
*m)
                m->name  = "eh_frame";
  #endif
                break;
-       case 2: /* eh_frame_hdr */
+       case UKPLAT_MEMREGION_EH_FRAME_HDR:
                m->base  = (void *) __EH_FRAME_HDR_START;
                m->len   = (size_t) __EH_FRAME_HDR_END
                                - (size_t) __EH_FRAME_HDR_START;
@@ -85,7 +96,7 @@ int ukplat_memregion_get(int i, struct ukplat_memregion_desc 
*m)
                m->name  = "eh_frame_hdr";
  #endif
                break;
-       case 3: /* ro data */
+       case UKPLAT_MEMREGION_RODATA:
                m->base  = (void *) __RODATA;
                m->len   = (size_t) __ERODATA - (size_t) __RODATA;
                m->flags = (UKPLAT_MEMRF_RESERVED
@@ -94,7 +105,7 @@ int ukplat_memregion_get(int i, struct ukplat_memregion_desc 
*m)
                m->name  = "rodata";
  #endif
                break;
-       case 4: /* ctors */
+       case UKPLAT_MEMREGION_CTORS:
                m->base  = (void *) __CTORS;
                m->len   = (size_t) __ECTORS - (size_t) __CTORS;
                m->flags = (UKPLAT_MEMRF_RESERVED
@@ -103,7 +114,7 @@ int ukplat_memregion_get(int i, struct 
ukplat_memregion_desc *m)
                m->name  = "ctors";
  #endif
                break;
-       case 5: /* data */
+       case UKPLAT_MEMREGION_DATA:
                m->base  = (void *) __DATA;
                m->len   = (size_t) __EDATA - (size_t) __DATA;
                m->flags = (UKPLAT_MEMRF_RESERVED
@@ -113,7 +124,7 @@ int ukplat_memregion_get(int i, struct 
ukplat_memregion_desc *m)
                m->name  = "data";
  #endif
                break;
-       case 6: /* bss */
+       case UKPLAT_MEMREGION_BSS:
                m->base  = (void *) __BSS_START;
                m->len   = (size_t) __END - (size_t) __BSS_START;
                m->flags = (UKPLAT_MEMRF_RESERVED
@@ -133,7 +144,8 @@ int ukplat_memregion_get(int i, struct 
ukplat_memregion_desc *m)
  #endif
                        return -1;
                } else {
-                       memcpy(m, &_libxenplat_mrd[i - 7], sizeof(*m));
+                       memcpy(m, &_libxenplat_mrd[i - UKPLAT_MEMREGION_COUNT],
+                               sizeof(*m));
                }
                break;
        }



--
Dr. Florian Schmidt
フローリアン・シュミット
Research Scientist,
Systems and Machine Learning Group
NEC Laboratories Europe
Kurfürsten-Anlage 36, D-69115 Heidelberg
Tel.     +49 (0)6221 4342-265
Fax:     +49 (0)6221 4342-155
e-mail:  florian.schmidt@xxxxxxxxx
============================================================
Registered at Amtsgericht Mannheim, Germany, HRB728558

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel

 

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.