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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.02.16 at 16:51, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 19/02/16 14:58, Jan Beulich wrote:
>>>>> On 18.02.16 at 19:03, <andrew.cooper3@xxxxxxxxxx> 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.

Interesting.

>>> +                {
>>> +                    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

But I don't think it needs to remain there? Should be possible to be
put between .text and .rodata, or between .rodata and .data ...

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

Since you now imply this to be at the image base, I don't see
why using e.g. __XEN_VIRT_START (in its place, or via #define)
wouldn't be as good an option.

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

Good idea indeed.

Jan

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