From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Chen, Tiejun" <tiejun.chen@intel.com>
Subject: Re: [v8][PATCH 07/16] hvmloader/e820: construct guest
 e820 table
Date: Thu, 16 Jul 2015 21:12:12 +0800
Message-ID: <55A7ADAC.1010004@intel.com>
References: <1437029582-19564-1-git-send-email-tiejun.chen@intel.com>
	<1437029582-19564-8-git-send-email-tiejun.chen@intel.com>
	<CAFLBxZZVMifw5CTLPQOtqOsWq6YGMjCS6T8BdH7-0280txV3mQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <CAFLBxZZVMifw5CTLPQOtqOsWq6YGMjCS6T8BdH7-0280txV3mQ@mail.gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Keir Fraser <keir@xen.org>, Ian Campbell <ian.campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Ian Jackson <ian.jackson@eu.citrix.com>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>, Jan Beulich <jbeulich@suse.com>, Wei Liu <wei.liu2@citrix.com>
List-Id: xen-devel@lists.xenproject.org

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