Re: [PATCH] xen/x86: Use 2M superpages for text/data/bss mappings

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 19/02/16 14:58, Jan Beulich wrote:
>>>> On 18.02.16 at 19:03, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -921,13 +921,51 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>              /* The only data mappings to be relocated are in the Xen area. */
>>              pl2e = __va(__pa(l2_xenmap));
>>              *pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT,
>> -                                   PAGE_HYPERVISOR_RWX | _PAGE_PSE);
>> +                                   PAGE_HYPERVISOR_RX | _PAGE_PSE);
>>              for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
>>              {
>> +                unsigned int flags;
>> +
>>                  if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )
>>                      continue;
>> -                *pl2e = l2e_from_intpte(l2e_get_intpte(*pl2e) +
>> -                                        xen_phys_start);
>> +
>> +                if ( /*
>> +                      * Should be:
>> +                      *
>> +                      * i >= l2_table_offset((unsigned long)&__2M_text_start) &&
>> +                      *
>> +                      * but the EFI build can't manage the relocation.  It
>> +                      * evaluates to 0, so just use the upper bound.
>> +                      */
>> +                     i < l2_table_offset((unsigned long)&__2M_text_end) )
> I'll need some more detail about this, not the least because
> excusing what looks like a hack with EFI, under which we won't
> ever get here, is suspicious.

Specifically, the EFI uses i386pep and it objects to a 64bit relocation
of 0xfffffffffffffffc.

I can't explain why this symbol ends up with that relocation.

>
>> +                {
>> +                    flags = PAGE_HYPERVISOR_RX | _PAGE_PSE;
>> +                }
>> +                else if ( i >= l2_table_offset((unsigned long)&__2M_rodata_start) &&
>> +                          i <  l2_table_offset((unsigned long)&__2M_rodata_end) )
>> +                {
>> +                    flags = PAGE_HYPERVISOR_RO | _PAGE_PSE;
>> +                }
>> +                else if ( (i >= l2_table_offset((unsigned long)&__2M_data_start) &&
>> +                           i <  l2_table_offset((unsigned long)&__2M_data_end)) ||
>> +                          (i >= l2_table_offset((unsigned long)&__2M_bss_start) &&
>> +                           i <  l2_table_offset((unsigned long)&__2M_bss_end)) )
> This is odd - why can't .data and .bss share a (multiple of) 2M
> region, at once presumably getting the whole image down to 10M
> again?

.init is between .data and .bss, to allow .bss to be the final section
and not included in the result of mkelf32

>
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -38,6 +38,9 @@ SECTIONS
>>    . = __XEN_VIRT_START;
>>    __image_base__ = .;
>>  #endif
>> +
>> +  __2M_text_start = .;         /* Start of 2M superpages, mapped RX. */
> Is the reason for aforementioned build problem perhaps the fact
> that this label (and the others too) lives outside of any section?

I am not sure.  It is only this symbol which is a problem.  All others
are fine.

I actually intended this to be an RFC patch, to see if anyone had
suggestions.

>
>> --- a/xen/include/xen/kernel.h
>> +++ b/xen/include/xen/kernel.h
>> @@ -65,6 +65,12 @@
>>  	1;                                      \
>>  })
>>  
>> +extern unsigned long __2M_text_start[], __2M_text_end[];
>> +extern unsigned long __2M_rodata_start[], __2M_rodata_end[];
>> +extern unsigned long __2M_data_start[], __2M_data_end[];
>> +extern unsigned long __2M_init_start[], __2M_init_end[];
>> +extern unsigned long __2M_bss_start[], __2M_bss_end[];
> I'd really like to see at least the ones which are reference to r/o
> sections marked const.

Ok.  I should probably also make them char rather than unsigned long, so
pointer arithmetic works sensibly for the length.

~Andrew