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

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

On 2015/4/20 22:29, Jan Beulich wrote:
> > > > On 10.04.15 at 11:22, <tiejun.chen@xxxxxxxxx> wrote:
> > --- a/tools/firmware/hvmloader/e820.c
> > +++ b/tools/firmware/hvmloader/e820.c
> > @@ -73,7 +73,8 @@ int build_e820_table(struct e820entry *e820,
> >                        unsigned int lowmem_reserved_base,
> >                        unsigned int bios_image_base)
> >   {
> > -    unsigned int nr = 0;
> > +    unsigned int nr = 0, i, j;
> > +    struct e820entry tmp;
> The declaration of "tmp" belongs in the most narrow scope you need
> it in.
Right.

> > @@ -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? And 
additionally, there's no any obvious flag to indicate which 
memory_map.map[x] is that last low memory map. Even we may separate the 
low memory to construct memory_map.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));

> >           nr++;
> >       }
> >
> > +    /* May need to reorder all e820 entries. */
> > +    for ( j = 0; j < nr-1; j++ )
> > +    {
> > +        for ( i = j+1; i < nr; i++ )
> > +        {
> > +            if ( e820[j].addr > e820[i].addr )
> > +            {
> > +                tmp.addr = e820[j].addr;
> > +                tmp.size = e820[j].size;
> > +                tmp.type = e820[j].type;
> > +
> > +                e820[j].addr = e820[i].addr;
> > +                e820[j].size = e820[i].size;
> > +                e820[j].type = e820[i].type;
> > +
> > +                e820[i].addr = tmp.addr;
> > +                e820[i].size = tmp.size;
> > +                e820[i].type = tmp.type;
> Please again use structure assignments to make this more readable.
And here,

    for ( j = 0; j < nr-1; j++ )
    {
        for ( i = j+1; i < nr; i++ )
        {
            if ( e820[j].addr > e820[i].addr )
            {
                struct e820entry tmp;

                memcpy(&tmp, &e820[j], sizeof(struct e820entry));

                memcpy(&e820[j], &e820[i], sizeof(struct e820entry));

                memcpy(&e820[i], &tmp, sizeof(struct e820entry));
            }
        }
    }

If I'm wrong please correct me.

Thanks
Tiejun

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