diff mbox

x86/EFI + Live Patch: avoid symbol address truncation

Message ID 57729FED02000078000F975B@prv-mh.provo.novell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jan Beulich June 28, 2016, 2:03 p.m. UTC
ld associates __init_end, placed outside of any section by the linker
script, with the following section, resulting in a huge (wrapped, as it
would be negative) section relative offset. COFF symbol tables store
section relative addresses, and hence the above leads to assembler
truncation warnings when all symbols get included in the symbol table
(for Live Patching code). To overcome this, move __init_end past both
ALIGN() directives. The consuming code (init_done()) is fine with such
an adjustment (the distinction really would only be relevant for the
loop claring the pages, and I think it's acceptable to clear a few
more on - for now - EFI). This effectively results in the
(__init_begin,__init_end) and (__2M_init_start,__2M_init_end) pairs to
become identical, with their different names only serving documentation
purposes now.

Note that moving __init_end and __2M_init_end into .init is not a good
idea, as that would significantly grow xen.efi binary size.

While inspecting symbol table and ld behavior I also noticed that
__2M_text_start gets put at address zero in the EFI case, which hasn't
caused problems solely because we don't actually reference that symbol.
Correct the setting of the initial address, and comment out said symbol
for the time being, as with the initial address correction it would in
turn cause an assembler truncation warning similar to the one mentioned
above.

While checking init_done() for correctness with the above changes I
noticed that code can easily be folded there, at once correcting the
logged amount of memory which has got freed for the 2M-alignment case
(i.e. EFI right now).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
x86/EFI + Live Patch: avoid symbol address truncation

ld associates __init_end, placed outside of any section by the linker
script, with the following section, resulting in a huge (wrapped, as it
would be negative) section relative offset. COFF symbol tables store
section relative addresses, and hence the above leads to assembler
truncation warnings when all symbols get included in the symbol table
(for Live Patching code). To overcome this, move __init_end past both
ALIGN() directives. The consuming code (init_done()) is fine with such
an adjustment (the distinction really would only be relevant for the
loop claring the pages, and I think it's acceptable to clear a few
more on - for now - EFI). This effectively results in the
(__init_begin,__init_end) and (__2M_init_start,__2M_init_end) pairs to
become identical, with their different names only serving documentation
purposes now.

Note that moving __init_end and __2M_init_end into .init is not a good
idea, as that would significantly grow xen.efi binary size.

While inspecting symbol table and ld behavior I also noticed that
__2M_text_start gets put at address zero in the EFI case, which hasn't
caused problems solely because we don't actually reference that symbol.
Correct the setting of the initial address, and comment out said symbol
for the time being, as with the initial address correction it would in
turn cause an assembler truncation warning similar to the one mentioned
above.

While checking init_done() for correctness with the above changes I
noticed that code can easily be folded there, at once correcting the
logged amount of memory which has got freed for the 2M-alignment case
(i.e. EFI right now).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -515,6 +515,7 @@ static inline bool_t using_2M_mapping(vo
 static void noinline init_done(void)
 {
     void *va;
+    unsigned long start, end;
 
     system_state = SYS_STATE_active;
 
@@ -530,18 +531,18 @@ static void noinline init_done(void)
     /* Destroy Xen's mappings, and reuse the pages. */
     if ( using_2M_mapping() )
     {
-        destroy_xen_mappings((unsigned long)&__2M_init_start,
-                             (unsigned long)&__2M_init_end);
-        init_xenheap_pages(__pa(__2M_init_start), __pa(__2M_init_end));
+        start = (unsigned long)&__2M_init_start,
+        end   = (unsigned long)&__2M_init_end;
     }
     else
     {
-        destroy_xen_mappings((unsigned long)&__init_begin,
-                             (unsigned long)&__init_end);
-        init_xenheap_pages(__pa(__init_begin), __pa(__init_end));
+        start = (unsigned long)&__init_begin;
+        end   = (unsigned long)&__init_end;
     }
 
-    printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
+    destroy_xen_mappings(start, end);
+    init_xenheap_pages(__pa(start), __pa(end));
+    printk("Freed %ldkB init memory\n", (end - start) >> 10);
 
     startup_cpu_idle_loop();
 }
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -40,9 +40,20 @@ SECTIONS
 #if !defined(EFI)
   . = __XEN_VIRT_START;
   __image_base__ = .;
+#else
+  . = __image_base__;
 #endif
 
+#if 0
+/*
+ * We don't really use this symbol anywhere, and the way it would get defined
+ * here would result in it having a negative (wrapped to huge positive)
+ * offset relative to the .text section. That, in turn, causes an assembler
+ * truncation warning when including all symbols in the symbol table for Live
+ * Patching code.
+ */
   __2M_text_start = .;         /* Start of 2M superpages, mapped RX. */
+#endif
 
   . = __XEN_VIRT_START + MB(1);
   _start = .;
@@ -194,14 +205,13 @@ SECTIONS
        *(.ctors)
        __ctors_end = .;
   } :text
-  . = ALIGN(PAGE_SIZE);
-  __init_end = .;
 
 #ifdef EFI
   . = ALIGN(MB(2));
 #else
   . = ALIGN(PAGE_SIZE);
 #endif
+  __init_end = .;
   __2M_init_end = .;
 
   __2M_rwdata_start = .;       /* Start of 2M superpages, mapped RW. */
@@ -296,7 +306,6 @@ ASSERT(__image_base__ > XEN_VIRT_START |
 ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too large")
 #endif
 
-ASSERT(IS_ALIGNED(__2M_text_start,   MB(2)), "__2M_text_start misaligned")
 #ifdef EFI
 ASSERT(IS_ALIGNED(__2M_text_end,     MB(2)), "__2M_text_end misaligned")
 ASSERT(IS_ALIGNED(__2M_rodata_start, MB(2)), "__2M_rodata_start misaligned")

Comments

Andrew Cooper June 28, 2016, 2:26 p.m. UTC | #1
On 28/06/16 15:03, Jan Beulich wrote:
> ld associates __init_end, placed outside of any section by the linker
> script, with the following section, resulting in a huge (wrapped, as it
> would be negative) section relative offset.

So in this case, the cause of the truncation is due to __init_end being
considered relative to .data.read_mostly?

>  COFF symbol tables store
> section relative addresses, and hence the above leads to assembler
> truncation warnings when all symbols get included in the symbol table
> (for Live Patching code). To overcome this, move __init_end past both
> ALIGN() directives. The consuming code (init_done()) is fine with such
> an adjustment (the distinction really would only be relevant for the
> loop claring the pages, and I think it's acceptable to clear a few
> more on - for now - EFI). This effectively results in the
> (__init_begin,__init_end) and (__2M_init_start,__2M_init_end) pairs to
> become identical, with their different names only serving documentation
> purposes now.
>
> Note that moving __init_end and __2M_init_end into .init is not a good
> idea, as that would significantly grow xen.efi binary size.

How about moving just __init_end ?  That shouldn't affect the size of
any binary, due to the existing page alignment between sections.

>
> While inspecting symbol table and ld behavior I also noticed that
> __2M_text_start gets put at address zero in the EFI case, which hasn't
> caused problems solely because we don't actually reference that symbol.

The reason that __2M_text_start isn't referenced is because I couldn't
get the EFI build working.  It was used in my first prototype.

> Correct the setting of the initial address, and comment out said symbol
> for the time being, as with the initial address correction it would in
> turn cause an assembler truncation warning similar to the one mentioned
> above.
>
> While checking init_done() for correctness with the above changes I
> noticed that code can easily be folded there, at once correcting the
> logged amount of memory which has got freed for the 2M-alignment case
> (i.e. EFI right now).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -515,6 +515,7 @@ static inline bool_t using_2M_mapping(vo
>  static void noinline init_done(void)
>  {
>      void *va;
> +    unsigned long start, end;
>  
>      system_state = SYS_STATE_active;
>  
> @@ -530,18 +531,18 @@ static void noinline init_done(void)
>      /* Destroy Xen's mappings, and reuse the pages. */
>      if ( using_2M_mapping() )
>      {
> -        destroy_xen_mappings((unsigned long)&__2M_init_start,
> -                             (unsigned long)&__2M_init_end);
> -        init_xenheap_pages(__pa(__2M_init_start), __pa(__2M_init_end));
> +        start = (unsigned long)&__2M_init_start,
> +        end   = (unsigned long)&__2M_init_end;
>      }
>      else
>      {
> -        destroy_xen_mappings((unsigned long)&__init_begin,
> -                             (unsigned long)&__init_end);
> -        init_xenheap_pages(__pa(__init_begin), __pa(__init_end));
> +        start = (unsigned long)&__init_begin;
> +        end   = (unsigned long)&__init_end;
>      }
>  
> -    printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
> +    destroy_xen_mappings(start, end);
> +    init_xenheap_pages(__pa(start), __pa(end));
> +    printk("Freed %ldkB init memory\n", (end - start) >> 10);

The parameter is now unsigned, so %lu.

>  
>      startup_cpu_idle_loop();
>  }
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -40,9 +40,20 @@ SECTIONS
>  #if !defined(EFI)
>    . = __XEN_VIRT_START;
>    __image_base__ = .;
> +#else
> +  . = __image_base__;
>  #endif
>  
> +#if 0
> +/*
> + * We don't really use this symbol anywhere, and the way it would get defined
> + * here would result in it having a negative (wrapped to huge positive)
> + * offset relative to the .text section. That, in turn, causes an assembler
> + * truncation warning when including all symbols in the symbol table for Live
> + * Patching code.
> + */
>    __2M_text_start = .;         /* Start of 2M superpages, mapped RX. */
> +#endif
>  
>    . = __XEN_VIRT_START + MB(1);
>    _start = .;
> @@ -194,14 +205,13 @@ SECTIONS
>         *(.ctors)
>         __ctors_end = .;
>    } :text
> -  . = ALIGN(PAGE_SIZE);
> -  __init_end = .;
>  
>  #ifdef EFI
>    . = ALIGN(MB(2));
>  #else
>    . = ALIGN(PAGE_SIZE);
>  #endif
> +  __init_end = .;
>    __2M_init_end = .;
>  
>    __2M_rwdata_start = .;       /* Start of 2M superpages, mapped RW. */
> @@ -296,7 +306,6 @@ ASSERT(__image_base__ > XEN_VIRT_START |
>  ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too large")
>  #endif
>  
> -ASSERT(IS_ALIGNED(__2M_text_start,   MB(2)), "__2M_text_start misaligned")

If we are #if 0'ing the symbol for documentation purposes, can we #if 0
this as well?

~Andrew

>  #ifdef EFI
>  ASSERT(IS_ALIGNED(__2M_text_end,     MB(2)), "__2M_text_end misaligned")
>  ASSERT(IS_ALIGNED(__2M_rodata_start, MB(2)), "__2M_rodata_start misaligned")
>
>
Jan Beulich June 28, 2016, 4:11 p.m. UTC | #2
>>> On 28.06.16 at 16:26, <andrew.cooper3@citrix.com> wrote:
> On 28/06/16 15:03, Jan Beulich wrote:
>> ld associates __init_end, placed outside of any section by the linker
>> script, with the following section, resulting in a huge (wrapped, as it
>> would be negative) section relative offset.
> 
> So in this case, the cause of the truncation is due to __init_end being
> considered relative to .data.read_mostly?

Yes.

>>  COFF symbol tables store
>> section relative addresses, and hence the above leads to assembler
>> truncation warnings when all symbols get included in the symbol table
>> (for Live Patching code). To overcome this, move __init_end past both
>> ALIGN() directives. The consuming code (init_done()) is fine with such
>> an adjustment (the distinction really would only be relevant for the
>> loop claring the pages, and I think it's acceptable to clear a few
>> more on - for now - EFI). This effectively results in the
>> (__init_begin,__init_end) and (__2M_init_start,__2M_init_end) pairs to
>> become identical, with their different names only serving documentation
>> purposes now.
>>
>> Note that moving __init_end and __2M_init_end into .init is not a good
>> idea, as that would significantly grow xen.efi binary size.
> 
> How about moving just __init_end ?  That shouldn't affect the size of
> any binary, due to the existing page alignment between sections.

There's no page alignment between sections in the disk image
representation - we build with a file alignment of 32.

>> While inspecting symbol table and ld behavior I also noticed that
>> __2M_text_start gets put at address zero in the EFI case, which hasn't
>> caused problems solely because we don't actually reference that symbol.
> 
> The reason that __2M_text_start isn't referenced is because I couldn't
> get the EFI build working.  It was used in my first prototype.

Not surprising with the symbol having ended up at zero.

>> @@ -530,18 +531,18 @@ static void noinline init_done(void)
>>      /* Destroy Xen's mappings, and reuse the pages. */
>>      if ( using_2M_mapping() )
>>      {
>> -        destroy_xen_mappings((unsigned long)&__2M_init_start,
>> -                             (unsigned long)&__2M_init_end);
>> -        init_xenheap_pages(__pa(__2M_init_start), __pa(__2M_init_end));
>> +        start = (unsigned long)&__2M_init_start,
>> +        end   = (unsigned long)&__2M_init_end;
>>      }
>>      else
>>      {
>> -        destroy_xen_mappings((unsigned long)&__init_begin,
>> -                             (unsigned long)&__init_end);
>> -        init_xenheap_pages(__pa(__init_begin), __pa(__init_end));
>> +        start = (unsigned long)&__init_begin;
>> +        end   = (unsigned long)&__init_end;
>>      }
>>  
>> -    printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
>> +    destroy_xen_mappings(start, end);
>> +    init_xenheap_pages(__pa(start), __pa(end));
>> +    printk("Freed %ldkB init memory\n", (end - start) >> 10);
> 
> The parameter is now unsigned, so %lu.

Oh, of course - fixed.

>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -40,9 +40,20 @@ SECTIONS
>>  #if !defined(EFI)
>>    . = __XEN_VIRT_START;
>>    __image_base__ = .;
>> +#else
>> +  . = __image_base__;
>>  #endif
>>  
>> +#if 0
>> +/*
>> + * We don't really use this symbol anywhere, and the way it would get defined
>> + * here would result in it having a negative (wrapped to huge positive)
>> + * offset relative to the .text section. That, in turn, causes an assembler
>> + * truncation warning when including all symbols in the symbol table for Live
>> + * Patching code.
>> + */
>>    __2M_text_start = .;         /* Start of 2M superpages, mapped RX. */
>> +#endif
>>  
>>    . = __XEN_VIRT_START + MB(1);
>>    _start = .;
>> @@ -194,14 +205,13 @@ SECTIONS
>>         *(.ctors)
>>         __ctors_end = .;
>>    } :text
>> -  . = ALIGN(PAGE_SIZE);
>> -  __init_end = .;
>>  
>>  #ifdef EFI
>>    . = ALIGN(MB(2));
>>  #else
>>    . = ALIGN(PAGE_SIZE);
>>  #endif
>> +  __init_end = .;
>>    __2M_init_end = .;
>>  
>>    __2M_rwdata_start = .;       /* Start of 2M superpages, mapped RW. */
>> @@ -296,7 +306,6 @@ ASSERT(__image_base__ > XEN_VIRT_START |
>>  ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too 
> large")
>>  #endif
>>  
>> -ASSERT(IS_ALIGNED(__2M_text_start,   MB(2)), "__2M_text_start misaligned")
> 
> If we are #if 0'ing the symbol for documentation purposes, can we #if 0
> this as well?

I considered it, but the two #if-s would end up disconnected. And
with the symbol being first thing in the image (plus the fact that so
far the assertion was there _without_ triggering despite there
being a problem - just one it couldn't detect), I think chances are
slim that it getting fully removed would be a significant problem.
I.e. I'd prefer the patch to remain as is in this regard, but if the
only way to get it acked is to do as you suggest, I would
(hesitantly) do so.

Jan
Andrew Cooper June 28, 2016, 4:39 p.m. UTC | #3
On 28/06/16 17:11, Jan Beulich wrote:
>>> --- a/xen/arch/x86/xen.lds.S
>>> +++ b/xen/arch/x86/xen.lds.S
>>> @@ -40,9 +40,20 @@ SECTIONS
>>>  #if !defined(EFI)
>>>    . = __XEN_VIRT_START;
>>>    __image_base__ = .;
>>> +#else
>>> +  . = __image_base__;
>>>  #endif
>>>  
>>> +#if 0
>>> +/*
>>> + * We don't really use this symbol anywhere, and the way it would get defined
>>> + * here would result in it having a negative (wrapped to huge positive)
>>> + * offset relative to the .text section. That, in turn, causes an assembler
>>> + * truncation warning when including all symbols in the symbol table for Live
>>> + * Patching code.
>>> + */
>>>    __2M_text_start = .;         /* Start of 2M superpages, mapped RX. */
>>> +#endif
>>>  
>>>    . = __XEN_VIRT_START + MB(1);
>>>    _start = .;
>>> @@ -194,14 +205,13 @@ SECTIONS
>>>         *(.ctors)
>>>         __ctors_end = .;
>>>    } :text
>>> -  . = ALIGN(PAGE_SIZE);
>>> -  __init_end = .;
>>>  
>>>  #ifdef EFI
>>>    . = ALIGN(MB(2));
>>>  #else
>>>    . = ALIGN(PAGE_SIZE);
>>>  #endif
>>> +  __init_end = .;
>>>    __2M_init_end = .;
>>>  
>>>    __2M_rwdata_start = .;       /* Start of 2M superpages, mapped RW. */
>>> @@ -296,7 +306,6 @@ ASSERT(__image_base__ > XEN_VIRT_START |
>>>  ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too 
>> large")
>>>  #endif
>>>  
>>> -ASSERT(IS_ALIGNED(__2M_text_start,   MB(2)), "__2M_text_start misaligned")
>> If we are #if 0'ing the symbol for documentation purposes, can we #if 0
>> this as well?
> I considered it, but the two #if-s would end up disconnected. And
> with the symbol being first thing in the image (plus the fact that so
> far the assertion was there _without_ triggering despite there
> being a problem - just one it couldn't detect), I think chances are
> slim that it getting fully removed would be a significant problem.
> I.e. I'd prefer the patch to remain as is in this regard, but if the
> only way to get it acked is to do as you suggest, I would
> (hesitantly) do so.

Ok.  I suppose it is sufficiently well documented because of the other
alignment assertions.

With the %lu fix, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

~Andrew
Konrad Rzeszutek Wilk June 28, 2016, 5:20 p.m. UTC | #4
On Tue, Jun 28, 2016 at 08:03:57AM -0600, Jan Beulich wrote:
> ld associates __init_end, placed outside of any section by the linker
> script, with the following section, resulting in a huge (wrapped, as it
> would be negative) section relative offset. COFF symbol tables store
> section relative addresses, and hence the above leads to assembler

My recollection is it was the linker? Like so:

/home/konrad/xen/xen/.xen.efi.0s.S:21: Warning: value 0x7d2f8000052e truncated to 0x8000052e
/home/konrad/xen/xen/.xen.efi.0s.S:22: Warning: value 0x7d2f80000a05 truncated to 0x80000a05
/home/konrad/xen/xen/.xen.efi.0s.S:23: Warning: value 0x7d2f80000a07 truncated to 0x80000a07

With this patch I still see those warnings?

(This is on OL7, so ldd (GNU libc) 2.17 gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-4) 

Let me try on different version.

> truncation warnings when all symbols get included in the symbol table
> (for Live Patching code). To overcome this, move __init_end past both
> ALIGN() directives. The consuming code (init_done()) is fine with such
> an adjustment (the distinction really would only be relevant for the
> loop claring the pages, and I think it's acceptable to clear a few
> more on - for now - EFI). This effectively results in the
> (__init_begin,__init_end) and (__2M_init_start,__2M_init_end) pairs to
> become identical, with their different names only serving documentation
> purposes now.
> 
> Note that moving __init_end and __2M_init_end into .init is not a good
> idea, as that would significantly grow xen.efi binary size.
> 
> While inspecting symbol table and ld behavior I also noticed that
> __2M_text_start gets put at address zero in the EFI case, which hasn't
> caused problems solely because we don't actually reference that symbol.
> Correct the setting of the initial address, and comment out said symbol
> for the time being, as with the initial address correction it would in
> turn cause an assembler truncation warning similar to the one mentioned
> above.
> 
> While checking init_done() for correctness with the above changes I
> noticed that code can easily be folded there, at once correcting the
> logged amount of memory which has got freed for the 2M-alignment case
> (i.e. EFI right now).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -515,6 +515,7 @@ static inline bool_t using_2M_mapping(vo
>  static void noinline init_done(void)
>  {
>      void *va;
> +    unsigned long start, end;
>  
>      system_state = SYS_STATE_active;
>  
> @@ -530,18 +531,18 @@ static void noinline init_done(void)
>      /* Destroy Xen's mappings, and reuse the pages. */
>      if ( using_2M_mapping() )
>      {
> -        destroy_xen_mappings((unsigned long)&__2M_init_start,
> -                             (unsigned long)&__2M_init_end);
> -        init_xenheap_pages(__pa(__2M_init_start), __pa(__2M_init_end));
> +        start = (unsigned long)&__2M_init_start,
> +        end   = (unsigned long)&__2M_init_end;
>      }
>      else
>      {
> -        destroy_xen_mappings((unsigned long)&__init_begin,
> -                             (unsigned long)&__init_end);
> -        init_xenheap_pages(__pa(__init_begin), __pa(__init_end));
> +        start = (unsigned long)&__init_begin;
> +        end   = (unsigned long)&__init_end;
>      }
>  
> -    printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
> +    destroy_xen_mappings(start, end);
> +    init_xenheap_pages(__pa(start), __pa(end));
> +    printk("Freed %ldkB init memory\n", (end - start) >> 10);
>  
>      startup_cpu_idle_loop();
>  }
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -40,9 +40,20 @@ SECTIONS
>  #if !defined(EFI)
>    . = __XEN_VIRT_START;
>    __image_base__ = .;
> +#else
> +  . = __image_base__;
>  #endif
>  
> +#if 0
> +/*
> + * We don't really use this symbol anywhere, and the way it would get defined
> + * here would result in it having a negative (wrapped to huge positive)
> + * offset relative to the .text section. That, in turn, causes an assembler
> + * truncation warning when including all symbols in the symbol table for Live
> + * Patching code.
> + */
>    __2M_text_start = .;         /* Start of 2M superpages, mapped RX. */
> +#endif
>  
>    . = __XEN_VIRT_START + MB(1);
>    _start = .;
> @@ -194,14 +205,13 @@ SECTIONS
>         *(.ctors)
>         __ctors_end = .;
>    } :text
> -  . = ALIGN(PAGE_SIZE);
> -  __init_end = .;
>  
>  #ifdef EFI
>    . = ALIGN(MB(2));
>  #else
>    . = ALIGN(PAGE_SIZE);
>  #endif
> +  __init_end = .;
>    __2M_init_end = .;
>  
>    __2M_rwdata_start = .;       /* Start of 2M superpages, mapped RW. */
> @@ -296,7 +306,6 @@ ASSERT(__image_base__ > XEN_VIRT_START |
>  ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too large")
>  #endif
>  
> -ASSERT(IS_ALIGNED(__2M_text_start,   MB(2)), "__2M_text_start misaligned")
>  #ifdef EFI
>  ASSERT(IS_ALIGNED(__2M_text_end,     MB(2)), "__2M_text_end misaligned")
>  ASSERT(IS_ALIGNED(__2M_rodata_start, MB(2)), "__2M_rodata_start misaligned")
> 
> 

> x86/EFI + Live Patch: avoid symbol address truncation
> 
> ld associates __init_end, placed outside of any section by the linker
> script, with the following section, resulting in a huge (wrapped, as it
> would be negative) section relative offset. COFF symbol tables store
> section relative addresses, and hence the above leads to assembler
> truncation warnings when all symbols get included in the symbol table
> (for Live Patching code). To overcome this, move __init_end past both
> ALIGN() directives. The consuming code (init_done()) is fine with such
> an adjustment (the distinction really would only be relevant for the
> loop claring the pages, and I think it's acceptable to clear a few
> more on - for now - EFI). This effectively results in the
> (__init_begin,__init_end) and (__2M_init_start,__2M_init_end) pairs to
> become identical, with their different names only serving documentation
> purposes now.
> 
> Note that moving __init_end and __2M_init_end into .init is not a good
> idea, as that would significantly grow xen.efi binary size.
> 
> While inspecting symbol table and ld behavior I also noticed that
> __2M_text_start gets put at address zero in the EFI case, which hasn't
> caused problems solely because we don't actually reference that symbol.
> Correct the setting of the initial address, and comment out said symbol
> for the time being, as with the initial address correction it would in
> turn cause an assembler truncation warning similar to the one mentioned
> above.
> 
> While checking init_done() for correctness with the above changes I
> noticed that code can easily be folded there, at once correcting the
> logged amount of memory which has got freed for the 2M-alignment case
> (i.e. EFI right now).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -515,6 +515,7 @@ static inline bool_t using_2M_mapping(vo
>  static void noinline init_done(void)
>  {
>      void *va;
> +    unsigned long start, end;
>  
>      system_state = SYS_STATE_active;
>  
> @@ -530,18 +531,18 @@ static void noinline init_done(void)
>      /* Destroy Xen's mappings, and reuse the pages. */
>      if ( using_2M_mapping() )
>      {
> -        destroy_xen_mappings((unsigned long)&__2M_init_start,
> -                             (unsigned long)&__2M_init_end);
> -        init_xenheap_pages(__pa(__2M_init_start), __pa(__2M_init_end));
> +        start = (unsigned long)&__2M_init_start,
> +        end   = (unsigned long)&__2M_init_end;
>      }
>      else
>      {
> -        destroy_xen_mappings((unsigned long)&__init_begin,
> -                             (unsigned long)&__init_end);
> -        init_xenheap_pages(__pa(__init_begin), __pa(__init_end));
> +        start = (unsigned long)&__init_begin;
> +        end   = (unsigned long)&__init_end;
>      }
>  
> -    printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
> +    destroy_xen_mappings(start, end);
> +    init_xenheap_pages(__pa(start), __pa(end));
> +    printk("Freed %ldkB init memory\n", (end - start) >> 10);
>  
>      startup_cpu_idle_loop();
>  }
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -40,9 +40,20 @@ SECTIONS
>  #if !defined(EFI)
>    . = __XEN_VIRT_START;
>    __image_base__ = .;
> +#else
> +  . = __image_base__;
>  #endif
>  
> +#if 0
> +/*
> + * We don't really use this symbol anywhere, and the way it would get defined
> + * here would result in it having a negative (wrapped to huge positive)
> + * offset relative to the .text section. That, in turn, causes an assembler
> + * truncation warning when including all symbols in the symbol table for Live
> + * Patching code.
> + */
>    __2M_text_start = .;         /* Start of 2M superpages, mapped RX. */
> +#endif
>  
>    . = __XEN_VIRT_START + MB(1);
>    _start = .;
> @@ -194,14 +205,13 @@ SECTIONS
>         *(.ctors)
>         __ctors_end = .;
>    } :text
> -  . = ALIGN(PAGE_SIZE);
> -  __init_end = .;
>  
>  #ifdef EFI
>    . = ALIGN(MB(2));
>  #else
>    . = ALIGN(PAGE_SIZE);
>  #endif
> +  __init_end = .;
>    __2M_init_end = .;
>  
>    __2M_rwdata_start = .;       /* Start of 2M superpages, mapped RW. */
> @@ -296,7 +306,6 @@ ASSERT(__image_base__ > XEN_VIRT_START |
>  ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too large")
>  #endif
>  
> -ASSERT(IS_ALIGNED(__2M_text_start,   MB(2)), "__2M_text_start misaligned")
>  #ifdef EFI
>  ASSERT(IS_ALIGNED(__2M_text_end,     MB(2)), "__2M_text_end misaligned")
>  ASSERT(IS_ALIGNED(__2M_rodata_start, MB(2)), "__2M_rodata_start misaligned")
Jan Beulich June 29, 2016, 9:08 a.m. UTC | #5
>>> On 28.06.16 at 19:20, <konrad.wilk@oracle.com> wrote:
> On Tue, Jun 28, 2016 at 08:03:57AM -0600, Jan Beulich wrote:
>> ld associates __init_end, placed outside of any section by the linker
>> script, with the following section, resulting in a huge (wrapped, as it
>> would be negative) section relative offset. COFF symbol tables store
>> section relative addresses, and hence the above leads to assembler
> 
> My recollection is it was the linker? Like so:
> 
> /home/konrad/xen/xen/.xen.efi.0s.S:21: Warning: value 0x7d2f8000052e 
> truncated to 0x8000052e
> /home/konrad/xen/xen/.xen.efi.0s.S:22: Warning: value 0x7d2f80000a05 
> truncated to 0x80000a05
> /home/konrad/xen/xen/.xen.efi.0s.S:23: Warning: value 0x7d2f80000a07 
> truncated to 0x80000a07
> 
> With this patch I still see those warnings?
> 
> (This is on OL7, so ldd (GNU libc) 2.17 gcc (GCC) 4.8.5 20150623 (Red Hat 
> 4.8.5-4) 

No, the ones above got fixed already in binutils. But even with that
fix in place I still observed one such truncation warning.

Jan
diff mbox

Patch

--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -515,6 +515,7 @@  static inline bool_t using_2M_mapping(vo
 static void noinline init_done(void)
 {
     void *va;
+    unsigned long start, end;
 
     system_state = SYS_STATE_active;
 
@@ -530,18 +531,18 @@  static void noinline init_done(void)
     /* Destroy Xen's mappings, and reuse the pages. */
     if ( using_2M_mapping() )
     {
-        destroy_xen_mappings((unsigned long)&__2M_init_start,
-                             (unsigned long)&__2M_init_end);
-        init_xenheap_pages(__pa(__2M_init_start), __pa(__2M_init_end));
+        start = (unsigned long)&__2M_init_start,
+        end   = (unsigned long)&__2M_init_end;
     }
     else
     {
-        destroy_xen_mappings((unsigned long)&__init_begin,
-                             (unsigned long)&__init_end);
-        init_xenheap_pages(__pa(__init_begin), __pa(__init_end));
+        start = (unsigned long)&__init_begin;
+        end   = (unsigned long)&__init_end;
     }
 
-    printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
+    destroy_xen_mappings(start, end);
+    init_xenheap_pages(__pa(start), __pa(end));
+    printk("Freed %ldkB init memory\n", (end - start) >> 10);
 
     startup_cpu_idle_loop();
 }
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -40,9 +40,20 @@  SECTIONS
 #if !defined(EFI)
   . = __XEN_VIRT_START;
   __image_base__ = .;
+#else
+  . = __image_base__;
 #endif
 
+#if 0
+/*
+ * We don't really use this symbol anywhere, and the way it would get defined
+ * here would result in it having a negative (wrapped to huge positive)
+ * offset relative to the .text section. That, in turn, causes an assembler
+ * truncation warning when including all symbols in the symbol table for Live
+ * Patching code.
+ */
   __2M_text_start = .;         /* Start of 2M superpages, mapped RX. */
+#endif
 
   . = __XEN_VIRT_START + MB(1);
   _start = .;
@@ -194,14 +205,13 @@  SECTIONS
        *(.ctors)
        __ctors_end = .;
   } :text
-  . = ALIGN(PAGE_SIZE);
-  __init_end = .;
 
 #ifdef EFI
   . = ALIGN(MB(2));
 #else
   . = ALIGN(PAGE_SIZE);
 #endif
+  __init_end = .;
   __2M_init_end = .;
 
   __2M_rwdata_start = .;       /* Start of 2M superpages, mapped RW. */
@@ -296,7 +306,6 @@  ASSERT(__image_base__ > XEN_VIRT_START |
 ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too large")
 #endif
 
-ASSERT(IS_ALIGNED(__2M_text_start,   MB(2)), "__2M_text_start misaligned")
 #ifdef EFI
 ASSERT(IS_ALIGNED(__2M_text_end,     MB(2)), "__2M_text_end misaligned")
 ASSERT(IS_ALIGNED(__2M_rodata_start, MB(2)), "__2M_rodata_start misaligned")