Re: [Xen-devel] [PATCH] x86: partially revert use of 2M mappings for hypervisor image

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.03.16 at 16:23, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 14/03/16 15:12, Jan Beulich wrote:
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -497,6 +497,17 @@ static void __init kexec_reserve_area(st
>>  #endif
>>  }
>>  
>> +static inline bool_t using_2M_mapping(void)
>> +{
>> +    return !l1_table_offset((unsigned long)__2M_text_end) &&
>> +           !l1_table_offset((unsigned long)__2M_rodata_start) &&
>> +           !l1_table_offset((unsigned long)__2M_rodata_end) &&
>> +           !l1_table_offset((unsigned long)__2M_init_start) &&
>> +           !l1_table_offset((unsigned long)__2M_init_end) &&
>> +           !l1_table_offset((unsigned long)__2M_rwdata_start) &&
>> +           !l1_table_offset((unsigned long)__2M_rwdata_end);
> 
> I would recommend
> 
> #ifdef EFI
> return 1;
> #else
> return 0;
> #endif
> 
> The compiler is unable to collapse that expression into a constant,
> because it can only be evaluated at link time.

But that wouldn't work, since setup.c gets compiled just once. The
only usable difference in building is the linking stage. (And
performance here is of no issue - this gets executed exactly twice.)

>> @@ -509,10 +520,19 @@ static void noinline init_done(void)
>>      for ( va = __init_begin; va < _p(__init_end); va += PAGE_SIZE )
>>          clear_page(va);
>>  
>> -    /* Destroy Xen's mappings, and reuse the pages. */
>> -    destroy_xen_mappings((unsigned long)&__2M_init_start,
>> -                         (unsigned long)&__2M_init_end);
>> -    init_xenheap_pages(__pa(__2M_init_start), __pa(__2M_init_end));
>> +    if ( using_2M_mapping() )
>> +    {
>> +        /* Destroy Xen's mappings, and reuse the pages. */
> 
> I would be tempted to leave this comment back outside using_2M_mapping()
> which reduces the size of this hunk.

Oh, of course.

>> @@ -922,6 +942,8 @@ void __init noreturn __start_xen(unsigne
>>               * Undo the temporary-hooking of the l1_identmap.  
>> __2M_text_start
>>               * is contained in this PTE.
>>               */
>> +            BUG_ON(l2_table_offset((unsigned long)_erodata) ==
>> +                   l2_table_offset((unsigned long)_stext));
> 
> Is this intentional to stay, or the remnants of debugging?

This is intentional, as it documents the validity of the immediately
following page table write: The change is not undoing the RWX ->
RX change that your original patch did, and using RX past
_erodata would obviously be wrong.

> Irrespective of some of these minor tweaks, Reviewed-by: Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>

Thanks.

Jan


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