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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Thu, Jul 16, 2015 at 7:52 AM, Tiejun Chen <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
> * Adjust the lowmem and highmem regions if we've had to relocate
>   memory (adding a highmem region if necessary)
> * Sort all the ranges so that they appear in memory order.
>
> CC: Keir Fraser <keir@xxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> CC: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> CC: Ian Campbell <ian.campbell@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
> ---
> v8:
>
> * define low_mem_end as uint32_t
>
> * Correct those two wrong loops, memory_map.nr_map -> nr
>   when we're trying to revise low/high memory e820 entries.
>
> * Improve code comments and the patch head description
>
> * Add one check if highmem is just populated by hvmloader itself
>
> v5 ~ v7:
>
> * Nothing is changed.
>
> v4:
>
> * Rename local variable, low_mem_pgend, to low_mem_end.
>
> * Improve some code comments
>
> * Adjust highmem after lowmem is changed.
>
>
>  tools/firmware/hvmloader/e820.c | 92 
> +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 83 insertions(+), 9 deletions(-)
>
> diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
> index b72baa5..aa678a7 100644
> --- 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;
> +    uint32_t low_mem_end = hvm_info->low_mem_pgend << PAGE_SHIFT;
>
>      if ( !lowmem_reserved_base )
>              lowmem_reserved_base = 0xA0000;
> @@ -152,13 +154,6 @@ int build_e820_table(struct e820entry *e820,
>      e820[nr].type = E820_RESERVED;
>      nr++;
>
> -    /* 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++;
> -
>      /*
>       * Explicitly reserve space for special pages.
>       * This space starts at RESERVED_MEMBASE an extends to cover various
> @@ -194,9 +189,73 @@ int build_e820_table(struct e820entry *e820,
>          nr++;
>      }
>
> +    /*
> +     * 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] = memory_map.map[i];
> +        nr++;
> +    }
> +
> +    /* Low RAM goes here. Reserve space for special pages. */
> +    BUG_ON(low_mem_end < (2u << 20));
>
> -    if ( hvm_info->high_mem_pgend )
> +    /*
> +     * Its possible to relocate RAM to allocate sufficient MMIO previously
> +     * 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 &&
> +             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;
> +        }
> +    }
> +
> +    /*
> +     * And then we also need to adjust highmem.
> +     */
> +    if ( add_high_mem )
> +    {
> +        for ( i = 0; i < nr; i++ )
> +        {
> +            if ( e820[i].type == E820_RAM &&
> +                 e820[i].addr == (1ull << 32))
> +            {
> +                e820[i].size += add_high_mem;
> +                add_high_mem = 0;
> +                break;
> +            }
> +        }
> +    }
> +
> +    /* Or this is just populated by hvmloader itself. */

This should probably say something like:

"If there was no highmem region, we need to create one."

> +    if ( add_high_mem )
> +    {
> +        /*
> +         * hvmloader should always update hvm_info->high_mem_pgend
> +         * when it relocates RAM anywhere.
> +         */
> +        BUG_ON( !hvm_info->high_mem_pgend );
> +
>          e820[nr].addr = ((uint64_t)1 << 32);
>          e820[nr].size =
>              ((uint64_t)hvm_info->high_mem_pgend << PAGE_SHIFT) - 
> e820[nr].addr;

In theory add_high_mem and hvm_info->high_mem_pgend << PAGE_SHIFT -
4GiB is the same, but it seems like asking for trouble to assume so
without checking.

Perhaps in the first if( add_high_mem ) conditional, you can
BUG_ON(add_high_mem != ((hvm_info->high_mem_pgend << PAGE_SHIFT) -
(ull1 << 32))) ?

Other than that, this looks good, thanks.

 -George

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