Re: [RFC][PATCH 13/13] hvmloader/e820: construct guest e820 table

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

On 2015/5/15 14:25, Jan Beulich wrote:
>>>> On 15.05.15 at 08:11, <tiejun.chen@intel.com> wrote:
>> On 2015/4/20 22:29, Jan Beulich wrote:
>>>>>> On 10.04.15 at 11:22, <tiejun.chen@intel.com> wrote:
>>>> @@ -119,10 +120,6 @@ int build_e820_table(struct e820entry *e820,
>>>>
>>>>        /* 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++;
>>>
>>> I think the above comment needs adjustment with all this code
>>> removed. I also wonder how meaningful the BUG_ON() is with
>>> ->low_mem_pgend no longer used for E820 table construction.
>>> Perhaps this needs another BUG_ON() validating that the field
>>> matches some value from memory_map.map[]?
>>
>> But I think hvm_info->low_mem_pgend is still correct, right?
>
> I think so, but as said it's becoming less used and hence less
> relevant here.

Understood.

>
>> And
>> additionally, there's no any obvious flag to indicate which
>> memory_map.map[x] is that last low memory map.
>
> I didn't imply it would be immediately obvious _how_ to do this.
> I'm merely wanting to avoid leaving meaningless BUG_ON()s in
> the code, while meaningful ones are amiss.

Maybe we should lookup all .map[] to get the lowest memory map and then 
BUG_ON?

>
>> Even we may separate the
>> low memory to construct memory_map.map[]...
>
> ???

Sorry I just mean that the low memory is not represented with only one 
memory_map.map[] in some cases. Is it impossible? Even in the future? Or 
actually we always consider the lowest memory map?

>
>>>> @@ -159,16 +156,37 @@ int build_e820_table(struct e820entry *e820,
>>>>            nr++;
>>>>        }
>>>>
>>>> -
>>>> -    if ( hvm_info->high_mem_pgend )
>>>> +    /* Construct the remaining according memory_map[]. */
>>>> +    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].addr = memory_map.map[i].addr;
>>>> +        e820[nr].size = memory_map.map[i].size;
>>>> +        e820[nr].type = memory_map.map[i].type;
>>>
>>> Afaict you could use structure assignment here to make this
>>> more readable.
>>
>> Sorry, are you saying this?
>>
>> memcpy(&e820[nr], &memory_map.map[i], sizeof(struct e820entry));
>
> No, structure assignment (which, other than memcpy(), is type safe):
>
>      e820[nr] = memory_map.map[i];
>

Understood.

Thanks
Tiejun