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

["Jan Beulich" <JBeulich@suse.com>]

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

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

> Even we may separate the 
> low memory to construct memory_map.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];

Jan