Re: [Xen-devel] [PATCH] x86/EFI + Live Patch: avoid symbol address truncation

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>
>
> --- 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")
>
>


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