[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820



On 2014/8/8 14:42, Jan Beulich wrote:
On 08.08.14 at 04:11, <tiejun.chen@xxxxxxxxx> wrote:
On 2014/8/7 20:03, Andrew Cooper wrote:
On 07/08/14 12:02, Tiejun Chen wrote:
--- a/tools/firmware/hvmloader/e820.h
+++ b/tools/firmware/hvmloader/e820.h
@@ -15,6 +15,12 @@ struct e820entry {
       uint32_t type;
   } __attribute__((packed));

+#define E820MAX 128
+
+struct e820map {
+    int nr_map;

This value should be unsigned.

I'm not sure if we need to change this since here I just copy this from
the xen/include/asm-x86/e820.h file,

struct e820map {
      int nr_map;
      struct e820entry map[E820MAX];
};

While it be welcome for you to (in a separate patch) also change this
one, it is not considered okay to copy existing mistakes: We've been
slowly switching over to put more attention on correct signed-ness
(and const-ness) - variables/fields that can't ever be negative
shouldn't have signed types.

I'm always afraid I'm missing something magic behind this signed type but with your clarification I already send one small patch to address this, and I also will update this point in this thread.


--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -766,6 +766,19 @@ struct shared_info *get_shared_info(void)
       return shared_info;
   }

+struct e820map *get_rmrr_map_info(void)
+{
+    static struct e820map *e820_map = NULL;
+
+    if ( e820_map != NULL )
+        return e820_map;
+
+    if ( hypercall_memory_op(XENMEM_RMRR_memory_map, e820_map) != 0 )
+        BUG();

This instructs Xen to clobber the memory starting at 0, and works
because HVMLoader is in protected, non-paging mode at this point.  I
don't think this is what you meant to do, and will repeatedly make the

Sorry I can't understand this explicitly. Here I just want a way to get
RMRR mapping info under hvmloader circumstance.

Could you elaborate what you mean? Or show me a proper way I should do
as you expect.

You never allocate memory for the map, i.e. you invoke the
hypercall with a NULL second argument. This just happens to work,
but is very unlikely what you intended to do.


Looks scratch_alloc() should be used to allocate in hvmloader, so what about this?

diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 80d822f..90011fa 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -766,6 +766,16 @@ struct shared_info *get_shared_info(void)
     return shared_info;
 }

+struct e820map *get_rmrr_map_info(void)
+{
+    struct e820map *e820_map = scratch_alloc(sizeof(struct e820map), 0);
+
+    if ( hypercall_memory_op(XENMEM_RMRR_memory_map, e820_map) != 0 )
+        BUG();
+
+    return e820_map;
+}
+
 uint16_t get_cpu_mhz(void)
 {
     struct shared_info *shared_info = get_shared_info();

Thanks
Tiejun

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

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