Re: [v3][PATCH 08/16] hvmloader/e820: construct guest e820 table

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

On 2015/6/11 17:59, Tian, Kevin wrote:
>> From: Chen, Tiejun
>> Sent: Thursday, June 11, 2015 9:15 AM
>>
>> Now we can use that memory map to build our final
>> e820 table but it may need to reorder all e820
>> entries.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   tools/firmware/hvmloader/e820.c | 62
>> +++++++++++++++++++++++++++++++----------
>>   1 file changed, 48 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
>> index 2e05e93..c39b0aa 100644
>> --- 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;
>> +    uint64_t low_mem_pgend = hvm_info->low_mem_pgend << PAGE_SHIFT;
>
> You may call it low_mem_end to differentiate from original
> low_mem_pgend since one means actual address while the
> other means pfn.

Okay.

>
>>
>>       if ( !lowmem_reserved_base )
>>               lowmem_reserved_base = 0xA0000;
>> @@ -117,13 +118,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
>> @@ -159,16 +153,56 @@ int build_e820_table(struct e820entry *e820,
>>           nr++;
>>       }
>>
>> -
>> -    if ( hvm_info->high_mem_pgend )
>> +    /*
>> +     * Construct the remaining according memory_map.
>
> "Construct E820 table according to recorded memory map"

Fixed.

>
>> +     *
>> +     * Note memory_map includes,
>
> The memory map created by toolstack may include:
>

Fixed.

>> +     *
>> +     * #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. RDM region if it exists
>
> "Reserved regions if they exist"

Fixed.

>
>> +     *
>> +     * #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_pgend < (2u << 20));
>> +    /*
>> +     * We may need to adjust real lowmem end since we may
>> +     * populate RAM to get enough MMIO previously.
>> +     */
>> +    for ( i = 0; i < memory_map.nr_map; i++ )
>
> since you already translate memory map into e820 earlier, here
> you should use 'nr' instead of memory_map.nr_map.
>

As we're saying in the code comment above, we're just handling the 
lowmem entry, so I think memory_map.nr_map is enough.

>> +    {
>> +        uint64_t end = e820[i].addr + e820[i].size;
>> +        if ( e820[i].type == E820_RAM &&
>> +             low_mem_pgend > e820[i].addr && low_mem_pgend < end )
>> +            e820[i].size = low_mem_pgend - e820[i].addr;
>> +    }
>
> Sorry I may miss the code but could you elaborate where the
> low_mem_pgend is changed after memory map is created? If
> it happens within hvmloader, suppose the amount of reduced
> memory from original E820_RAM entry should be added to
> another E820_RAM entry for highmem, right?

You're right so I really should compensate this in highmem entry,

     add_high_mem = end - low_mem_end;

     /*
      * And then we also need to adjust highmem.
      */
     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;
         }
     }


Thanks
Tiejun

>
>> +
>> +    /* Finally we 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 )
>> +            {
>> +                struct e820entry tmp;
>> +                tmp = e820[j];
>> +                e820[j] = e820[i];
>> +                e820[i] = tmp;
>> +            }
>> +        }
>> +    }
>> +
>>       return nr;
>>   }
>>
>> --
>> 1.9.1
>
>
>