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

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

>> +    /*
>> +     * 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."

Okay, "If there was no highmem entry, 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

No, its not true in the first if( add_high_mem ) conditional.

Before we enter hvmloader, there are two cases:

#1. hvm_info->high_mem_pgend == 0

So we wouldn't have a highmem entry in e820. But hvmloader may relocate 
RAM upward highmem (add_high_mem) to get sufficient mmio, so 
hvm_info->high_mem_pgend is expanded somewhere (4GiB + add_high_mem).

Then we would fall into the second if( add_high_mem ) conditional.

#2. hvm_info->high_mem_pgend != 0

We always walk into the first if( add_high_mem ) conditional. But here 
"add_high_mem" just represents that highmem section expanded by 
hvmloader, its really not the whole higmem:(hvm_info->high_mem_pgend << 
PAGE_SHIFT - 4GiB).

Thanks
Tiejun

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