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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.06.16 at 16:26, <andrew.cooper3@xxxxxxxxxx> 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

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