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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.07.15 at 12:22, <tiejun.chen@xxxxxxxxx> wrote:
> On 2015/7/14 17:32, Jan Beulich wrote:
>>>>> On 14.07.15 at 07:22, <tiejun.chen@xxxxxxxxx> wrote:
>>>>> +    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++ )
>>
>> Hmm, I would have thought you only care about either part of
>> the just glued together map.
>>
>>>>> +        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.
>>
>> And I'm not disputing your intentions - I'm merely pointing out that
>> afaics the code above doesn't match these intentions. In particular
>> (as said) I don't see why you need to check low_mem_end < end.
>>
> 
> Before we probably relocate RAM,
> 
> low_mem_end = hvm_info->low_mem_pgend << PAGE_SHIFT
> 
> and the e820 entry specific to low memory,
> 
> [e820[X].addr, end]
> 
> Here, end = e820[X].addr + e820[X].size;
> 
> Now low_mem_end = end.
> 
> After that, low_mem_end < end. so if
> 
> (low_mem_end > e820[X].addr && low_mem_end < end) is true, this means 
> that associated RAM entry is hitting, right? Then we need to revise this 
> entry as [e820[X].addr, low_mem_end], and compensate [end - low_mem_end] 
> to high memory. Anything I'm still wrong here?

Ah, I think I see now what I misunderstood.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel