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

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

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

I hope the following can work for you,

...
but finally we should sort them into an increasing order since
we shouldn't assume the original order is always good.

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

[snip]

>
>> +     */
>> +    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]

You're right. I should lookup all e820[] like this,

for ( i = 0; i < nr; i++ )

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

Here I'm looking at the e820 entry indicating low memory. Because

low_mem_end = hvm_info->low_mem_pgend << PAGE_SHIFT;

and when we allocate MMIO in pci.c, its possible to populate RAM so 
hvm_info->low_mem_pgend would be changed over there. So we need to 
compensate this loss with high memory. Here memory_map[] also records 
the original low/high memory, so if low_mem_end is less-than the 
original we need this compensation.

So here we have two steps to address this issue,

#1. Calculate the loss

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

#2. Compensate the loss

>> +    if ( add_high_mem )
>> +    {
>> +        for ( i = 0; i < memory_map.nr_map; i++ )

s/memory_map.nr_map/nr

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

I can add this at the beginning of #1 loop,

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.


Thanks
Tiejun