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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Thu, Jul 16, 2015 at 2:12 PM, Chen, Tiejun <tiejun.chen@xxxxxxxxx> wrote:
>>> +    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).

Yes, sorry, add_high_mem will be the size of memory *relocated*, not
the actual end of it (unless, as you say, the original highmem region
didn't exist).

What I really meant was that either way, after adjusting the highmem
region in the e820, the end of that region should correspond to
hvm_info->high_mem_pgend.

What about something like this?
---
    /*
     * And then we also need to adjust highmem.
     */
    if ( add_high_mem )
    {
        /*
         * Modify the existing highmem region if it exists
         */
        for ( i = 0; i < nr; i++ )
        {
            if ( e820[i].type == E820_RAM &&
                 e820[i].addr == (1ull << 32))
            {
                e820[i].size += add_high_mem;
                break;
            }
        }

        /*
         * If we didn't find a highmem region, make one
         */
        if ( i == nr )
        {
            e820[nr].addr = ((uint64_t)1 << 32);
            e820[nr].size = e820[nr].addr + add_high_mem;
            e820[nr].type = E820_RAM;
            nr++;
        }

        /*
         * Either way, at this point i points to the entry containing
         * highmem.  Compare it to what's in hvm_info as a sanity
         * check.
         */
        BUG_ON(e820[i].addr+e820[i].size !=
               ((uint64_t)hvm_info->high_mem_pgend << PAGE_SHIFT));
    }

--

 -George

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