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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 10.04.15 at 11:22, <tiejun.chen@intel.com> 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.

> @@ -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[]?

> @@ -159,16 +156,37 @@ int build_e820_table(struct e820entry *e820,
>          nr++;
>      }
>  
> -
> -    if ( hvm_info->high_mem_pgend )
> +    /* Construct the remaings 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.

>          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.

Jan