Re: [v7][PATCH 07/16] hvmloader/e820: construct guest e820 table

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

>>> On 09.07.15 at 07:33, <tiejun.chen@intel.com> wrote:
> Now we can use that memory map to build our final
> e820 table but it may need to reorder all e820
> entries.

"it" being what? I'm afraid I can't really make sense of the second
half of the sentence...

> --- a/tools/firmware/hvmloader/e820.c
> +++ b/tools/firmware/hvmloader/e820.c
> @@ -108,7 +108,9 @@ 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;
> +    uint64_t add_high_mem = 0;
> +    uint64_t low_mem_end = hvm_info->low_mem_pgend << PAGE_SHIFT;

For the last one I don't see why uint64_t; uint32_t should be just fine
and less (binary) code.

> @@ -194,16 +189,73 @@ int build_e820_table(struct e820entry *e820,
>          nr++;
>      }
>  
> -
> -    if ( hvm_info->high_mem_pgend )
> +    /*
> +     * Construct E820 table according to recorded memory map.
> +     *
> +     * The memory map created by toolstack may include,
> +     *
> +     * #1. Low memory region
> +     *
> +     * Low RAM starts at least from 1M to make sure all standard regions
> +     * of the PC memory map, like BIOS, VGA memory-mapped I/O and vgabios,
> +     * have enough space.
> +     *
> +     * #2. Reserved regions if they exist
> +     *
> +     * #3. High memory region if it exists
> +     */
> +    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] = memory_map.map[i];
>          nr++;
>      }
>  
> +    /* Low RAM goes here. Reserve space for special pages. */
> +    BUG_ON(low_mem_end < (2u << 20));
> +
> +    /*
> +     * We may need to adjust real lowmem end since we may
> +     * populate RAM to get enough MMIO previously.

"populate"? Don't you mean "relocate"?

> +     */
> +    for ( i = 0; i < memory_map.nr_map; i++ )
> +    {
> +        uint64_t end = e820[i].addr + e820[i].size;

Either loop index/boundary or used array are wrong here: In the
earlier loop you copied memory_map[0...nr_map-1] to
e820[n...n+nr_map-1], but here you're looping looking at
e820[0...nr_map-1]

> +        if ( e820[i].type == E820_RAM &&
> +             low_mem_end > e820[i].addr && low_mem_end < end )

Assuming you mean to look at the RDM e820[] entries here, this
is not a correct check: You don't care about partly or fully
contained, all you care about is whether low_mem_end extends
beyond the start of the region.

> +        {
> +            add_high_mem = end - low_mem_end;
> +            e820[i].size = low_mem_end - e820[i].addr;
> +        }
> +    }
> +
> +    /*
> +     * And then we also need to adjust highmem.
> +     */

A single line comment should use the respective comment style.

> +    if ( add_high_mem )
> +    {
> +        for ( i = 0; i < memory_map.nr_map; i++ )
> +        {
> +            if ( e820[i].type == E820_RAM &&
> +                 e820[i].addr > (1ull << 32))
> +                e820[i].size += add_high_mem;
> +        }
> +    }

But looking at the code I think the comment should be extended to
state that we currently expect there to be exactly one such RAM
region.

> +    /* Finally we need to reorder all e820 entries. */

"reorder"? Perhaps "sort"?

But despite the many comments - the patch looks a lot better now
than earlier versions.

Jan