Re: [Xen-devel] [v9][PATCH 07/16] hvmloader/e820: construct guest e820 table

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.07.15 at 02:45, <tiejun.chen@xxxxxxxxx> wrote:
> Now use the hypervisor-supplied memory map to build our final e820 table:
> * Add regions for BIOS ranges and other special mappings not in the
>   hypervisor map
> * Add in the hypervisor regions

... hypervisor supplied regions?

> --- a/tools/firmware/hvmloader/e820.c
> +++ b/tools/firmware/hvmloader/e820.c
> @@ -105,7 +105,10 @@ 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;
> +    uint32_t low_mem_end = hvm_info->low_mem_pgend << PAGE_SHIFT;
> +    uint64_t high_mem_end = (uint64_t)hvm_info->high_mem_pgend << PAGE_SHIFT;
> +    uint64_t add_high_mem = 0;

Just like previously said for low_mem_end - why not uint32_t?

> @@ -191,16 +187,91 @@ 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));
> +
> +    /*
> +     * Its possible to relocate RAM to allocate sufficient MMIO previously

DYM "We may have relocated RAM ..."?

> +     * so low_mem_pgend would be changed over there. And here memory_map[]
> +     * records the original low/high memory, so if low_mem_end is less than
> +     * the original we need to revise low/high memory range in e820.
> +     */
> +    for ( i = 0; i < nr; i++ )
> +    {
> +        uint64_t end = e820[i].addr + e820[i].size;
> +        if ( e820[i].type == E820_RAM &&

Blank line between declarations and statements please.

> +             low_mem_end > e820[i].addr && low_mem_end < end )
> +        {
> +            add_high_mem = end - low_mem_end;
> +            e820[i].size = low_mem_end - e820[i].addr;
> +        }
> +    }

The way it's written I take it that you assume there to be exactly
one region that the adjustment needs to be done for. Iirc this is
correct with the current model, but why would you continue the
loop then afterwards? Placing a "break" in the if()'s body would
document the fact that only one such region should exist, and
would eliminate questions as to whether add_high_mem shouldn't
be updated (+=) instead of simply being assigned a new value.

And then of course there's the question of whether "nr" is really
the right upper loop bound here: Just prior to this you added
the hypervisor supplied entries - why would you need to iterate
over them here? I.e. I'd see this better be moved ahead of that
other code.

> +    /*
> +     * And then we also need to adjust highmem.
> +     */

I'm sure I gave this comment before: This is a single line comment, so
its style should be that of a single line comment.

> +    if ( add_high_mem )
> +    {
> +        /* Modify the existing highmem region if it exists. */
> +        for ( i = 0; i < nr; i++ )
> +        {
> +            if ( e820[i].type == E820_RAM &&
> +                 e820[i].addr == ((uint64_t)1 << 32))
> +            {
> +                e820[i].size += add_high_mem;
> +                break;
> +            }
> +        }
> +
> +        /* If there was no highmem region, just create one. */
> +        if ( i == nr )
> +        {
> +            e820[nr].addr = ((uint64_t)1 << 32);
> +            e820[nr].size = add_high_mem;
> +            e820[nr].type = E820_RAM;
> +            nr++;
> +        }
> +
> +        /* A sanity check if high memory is broken. */
> +        BUG_ON( high_mem_end != e820[i].addr + e820[i].size);
> +    }

Remind me again please - what prevents the highmem region from
colliding with hypervisor supplied entries?

Also, what if the resulting region exceeds the addressable range
(guest's view of CPUID[80000008].EAX[0:7])?

> +    /* Finally we need to sort all e820 entries. */
> +    for ( j = 0; j < nr-1; j++ )
> +    {
> +        for ( i = j+1; i < nr; i++ )
> +        {
> +            if ( e820[j].addr > e820[i].addr )
> +            {
> +                struct e820entry tmp;
> +                tmp = e820[j];

Please make this the initializer of tmp and add the once again missing
blank line.

Jan

> +                e820[j] = e820[i];
> +                e820[i] = tmp;
> +            }
> +        }
> +    }
> +
>      return nr;
>  }
>  
> -- 
> 1.9.1



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