From mboxrd@z Thu Jan  1 00:00:00 1970
From: George Dunlap <george.dunlap@eu.citrix.com>
Subject: Re: [PATCH v3 3/8] hvmloader: Set up highmem resouce
 appropriately if there is no RAM above 4G
Date: Fri, 21 Jun 2013 10:06:25 +0100
Message-ID: <51C41791.3030104@eu.citrix.com>
References: <1371746007-19073-1-git-send-email-george.dunlap@eu.citrix.com>
	<1371746007-19073-4-git-send-email-george.dunlap@eu.citrix.com>
	<51C4166402000078000DF811@nat28.tlf.novell.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: <51C4166402000078000DF811@nat28.tlf.novell.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: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>, Ian Campbell <ian.campbell@citrix.com>, Hanweidong <hanweidong@huawei.com>, xen-devel@lists.xen.org, StefanoStabellini <stefano.stabellini@citrix.com>, Ian Jackson <ian.jackson@citrix.com>
List-Id: xen-devel@lists.xenproject.org

On 21/06/13 08:01, Jan Beulich wrote:
>>>> On 20.06.13 at 18:33, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> @@ -244,7 +244,14 @@ void pci_setup(void)
>>           hvm_info->high_mem_pgend += nr_pages;
>>       }
>>   
>> -    high_mem_resource.base = ((uint64_t)hvm_info->high_mem_pgend) << PAGE_SHIFT;
>>
>> +    if ( hvm_info->high_mem_pgend )
> To be on the safe side I'd make this
>
>      if ( hvm_info->high_mem_pgend >= (1ull << 32) )

high_mem_pgend is in frames, not physical address; but I do take your point.

On the other hand, there is other code (for instance, in the memory 
relocation loop) that assumes that high_mem_pgend is either 0, or 
pointing into the high mem region.  Normally the correct way to deal 
with internal invariants like this is to add an assert() to that effect, 
but adding a new assert() at this point is not really on either.

I'd just as soon leave it the way it is, but if people felt strongly 
about it I could change it to something like this:

   high_mem_resource.base = ((uint64_t)hvm_info->high_mem_pgend) << PAGE_SHIFT;
   if ( high_mem_resource.base < 1ull << 32 )
     high_mem_resource.base = 1ull << 32;


>> +        high_mem_resource.base = ((uint64_t)hvm_info->high_mem_pgend) << PAGE_SHIFT;
>> +    else
>> +        high_mem_resource.base = 1ull << 32;
>> +    printf("%sRAM in high memory; setting high_mem resource base to %x%08x\n",
>> +           hvm_info->high_mem_pgend?"":"No ",
>> +           (uint32_t)(high_mem_resource.base>>32),
>> +           (uint32_t)high_mem_resource.base);
> Seeing the n-th incarnation of this - mind creating a macro to do the
> splitting?

I'll give it a try and see how it looks.

  -George