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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/03/16 15:41, Jan Beulich wrote:
>>>> 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.)

Oh yes - that fact keeps on escaping me.  It is only an interim
solution, so is fine to stay.

>>> @@ -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.

Ah yes.

~Andrew

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