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

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

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