Re: [Xen-devel] [RFC][PATCH 13/13] hvmloader/e820: construct guest e820 table

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

On 2015/5/15 14:25, Jan Beulich wrote:
> > > > On 15.05.15 at 08:11, <tiejun.chen@xxxxxxxxx> wrote:
> > On 2015/4/20 22:29, Jan Beulich wrote:
> > > > > > On 10.04.15 at 11:22, <tiejun.chen@xxxxxxxxx> wrote:
> > > > @@ -119,10 +120,6 @@ int build_e820_table(struct e820entry *e820,
> > > >
> > > >        /* Low RAM goes here. Reserve space for special pages. */
> > > >        BUG_ON((hvm_info->low_mem_pgend << PAGE_SHIFT) < (2u << 20));
> > > > -    e820[nr].addr = 0x100000;
> > > > -    e820[nr].size = (hvm_info->low_mem_pgend << PAGE_SHIFT) - e820[nr].addr;
> > > > -    e820[nr].type = E820_RAM;
> > > > -    nr++;
> > > I think the above comment needs adjustment with all this code
> > > removed. I also wonder how meaningful the BUG_ON() is with
> > > ->low_mem_pgend no longer used for E820 table construction.
> > > Perhaps this needs another BUG_ON() validating that the field
> > > matches some value from memory_map.map[]?
> > But I think hvm_info->low_mem_pgend is still correct, right?
> I think so, but as said it's becoming less used and hence less
> relevant here.
Understood.

> > And
> > additionally, there's no any obvious flag to indicate which
> > memory_map.map[x] is that last low memory map.
> I didn't imply it would be immediately obvious _how_ to do this.
> I'm merely wanting to avoid leaving meaningless BUG_ON()s in
> the code, while meaningful ones are amiss.
Maybe we should lookup all .map[] to get the lowest memory map and then 
BUG_ON?
> > Even we may separate the
> > low memory to construct memory_map.map[]...
> ???
Sorry I just mean that the low memory is not represented with only one 
memory_map.map[] in some cases. Is it impossible? Even in the future? Or 
actually we always consider the lowest memory map?
> > > > @@ -159,16 +156,37 @@ int build_e820_table(struct e820entry *e820,
> > > >            nr++;
> > > >        }
> > > >
> > > > -
> > > > -    if ( hvm_info->high_mem_pgend )
> > > > +    /* Construct the remaining according memory_map[]. */
> > > > +    for ( i = 0; i < memory_map.nr_map; i++ )
> > > >        {
> > > > -        e820[nr].addr = ((uint64_t)1 << 32);
> > > > -        e820[nr].size =
> > > > -            ((uint64_t)hvm_info->high_mem_pgend << PAGE_SHIFT) - e820[nr].addr;
> > > > -        e820[nr].type = E820_RAM;
> > > > +        e820[nr].addr = memory_map.map[i].addr;
> > > > +        e820[nr].size = memory_map.map[i].size;
> > > > +        e820[nr].type = memory_map.map[i].type;
> > > Afaict you could use structure assignment here to make this
> > > more readable.
> > Sorry, are you saying this?
> >
> > memcpy(&e820[nr], &memory_map.map[i], sizeof(struct e820entry));
> No, structure assignment (which, other than memcpy(), is type safe):
>
>      e820[nr] = memory_map.map[i];
Understood.

Thanks
Tiejun

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