Re: [Xen-devel] [PATCH v6 01/15] x86: properly calculate ELF end of image address

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.09.16 at 15:56, <daniel.kiper@xxxxxxxxxx> wrote:
> On Mon, Sep 19, 2016 at 05:14:07AM -0600, Jan Beulich wrote:
>> >>> On 16.09.16 at 22:43, <konrad.wilk@xxxxxxxxxx> wrote:
>> > On Mon, Sep 12, 2016 at 10:18:16PM +0200, Daniel Kiper wrote:
>> >> Currently ELF end of image address is calculated using first line from
>> >> "nm -nr xen/xen-syms" output. However, today usually it contains random
>> >> symbol address not related to end of image in any way. So, it looks
>> >
>> > Weird. The -n says:
>> >
>> > "  -n
>> >        -v
>> >        --numeric-sort
>> >            Sort symbols numerically by their addresses, rather than
>> > alphabetically by their names.
>> > "
>> >
>> > And you are right. It is ignoring it:
>> >
>> > [konrad@char xen]$ nm -nr xen/xen-syms| sort | head -1
>> > ffff82d080000000 T __image_base__
>> > [konrad@char xen]$ nm -nr xen/xen-syms | head -1
>> > ffff82d08033d000 B efi
>>
>> So what is it ignoring, you think? The -n? Surely not. Are you perhaps
>> overlooking that -r means "reverse" (and hence that piping through
>> "sort" inverts all the sorting done by nm)?
>>
>> > [konrad@char xen]$ nm -nr xen/xen-syms| sort | tail -5
>> > ffff82d08033cb00 B _end
>> > ffff82d08033cb00 B __per_cpu_data_end
>> > ffff82d08033d000 B __2M_rwdata_end
>> > ffff82d08033d000 B efi
>> >                  U _GLOBAL_OFFSET_TABLE_
>> >
>> >> that for years that stuff have been working just by lucky coincidence.
>> >> Hence, it have to be changed to something more reliable. So, let's take
>> >> ELF end of image address by reading _end symbol address from nm output.
>>
>> So before taking this patch I'd really like to see proof that what gets
>> done currently does actually go wrong in at least one case. So far I
> 
> During initial work on this patch series I discovered that p_memsz in xen
> ELF file is set to unreasonably huge value, IIRC, ~1 GiB. That is why at
> first I enforced 16 MiB here (just temporary workaround) and later proposed
> this patch. Sadly, right now I am not able to find commit which introduced
> this issue. However, it seems that it was "fixed" later by another commit.
> 
> Anyway, I am not sure why are you against, IMO, more reliable solution.
> This way we would avoid in the future similar issues which I described 
> above.

I'm not against anything if it gets properly explained. Here,
however, you present some vague statements which even you
can't verify right now as it looks.

And then I'm not sure going from one way of using nm to another
is all that much of an improvement. A true improvement might be
to do away with the nm use and e.g. also consider the section
table.

Jan

>> can't see things working as "just by lucky coincidence". In particular
>> I can't see why __2M_rwdata_end (aliased to efi) does not relate to
>> the intended image end. Once we switch back to using large pages
>> even when not using xen.efi I'd even question whether preferring
>> _end over __2M_rwdata_end is actually correct.
> 
> This is good question. I was thinking about that after posting v6. It seems
> that from image POV _end is correct. However, taking into account that Xen
> image mapping covers 16 MiB then I think we should use __2M_rwdata_end (or
> define __end_of_image__ symbol equal to __2M_rwdata_end). This way boot
> loader will allocate memory region for Xen image later covered properly by
> mapping, nothing will be put in memory immediately after the Xen image and
> later incorrectly mapped as Xen image.
> 
> Daniel




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