Re: [PATCH v3 3/8] hvmloader: Set up highmem resouce appropriately if there is no RAM above 4G

[George Dunlap <george.dunlap@eu.citrix.com>]

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