Re: [PATCH v2] x86: debugging code for testing 16Tb support on smaller memory systems

["Jan Beulich" <JBeulich@suse.com>]

>>> On 24.01.13 at 12:36, Tim Deegan <tim@xen.org> wrote:
> At 14:26 +0000 on 23 Jan (1358951188), Jan Beulich wrote:
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -82,6 +82,11 @@ boolean_param("noapic", skip_ioapic_setu
>>  s8 __read_mostly xen_cpuidle = -1;
>>  boolean_param("cpuidle", xen_cpuidle);
>>  
>> +#ifndef NDEBUG
>> +unsigned long __initdata highmem_start;
>> +size_param("highmem-start", highmem_start);
>> +#endif
>> +
>>  cpumask_t __read_mostly cpu_present_map;
>>  
>>  unsigned long __read_mostly xen_phys_start;
>> @@ -787,6 +792,14 @@ void __init __start_xen(unsigned long mb
>>      modules_headroom = bzimage_headroom(bootstrap_map(mod), mod->mod_end);
>>      bootstrap_map(NULL);
>>  
>> +#ifndef highmem_start
>> +    /* Don't allow split below 4Gb. */
>> +    if ( highmem_start < GB(4) )
>> +        highmem_start = 0;
>> +    else /* align to L3 entry boundary */
>> +        highmem_start &= ~((1UL << L3_PAGETABLE_SHIFT) - 1);
>> +#endif
> 
> DYM #ifndef NDEBUG ?  I can see that checking for highmem_start being a
> macro is strictly correct

I intended it to be that way, because there could be other uses
for having the symbol #define-d/real.

> but it seems more vulnerable to later changes,
> esp. since this:
> 
>> --- a/xen/include/asm-x86/setup.h
>> +++ b/xen/include/asm-x86/setup.h
>> @@ -43,4 +43,10 @@ void microcode_grab_module(
>>  
>>  extern uint8_t kbd_shift_flags;
>>  
>> +#ifdef NDEBUG
>> +# define highmem_start 0
>> +#else
>> +extern unsigned long highmem_start;
>> +#endif
> 
> happens so far away. 

I realize that, but these getting out of sync is no problem the
way it is coded now. The distance of the two would really be
more of a problem imo if the condition here got changed (which
would then require to also change it up there).

Jan