[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

xen: linker symbol mess, and freeing errors



Hello,

Following the __ro_after_init work, I tried to complete a few pieces of
cleanup that I'd accrued, and everything has unravelled.

On x86, the __2M_* symbols haven't really been 2M aligned since their
introduction, and the utter mess that was _stext starting at 1M has long
since been cleared up.  Dropping the 2M prefix reveals that we have both
__init_{start,begin} and identifying that lead to discovering that

    /* Destroy Xen's mappings, and reuse the pages. */
    if ( using_2M_mapping() )
    {
        start = (unsigned long)&__2M_init_start,
        end   = (unsigned long)&__2M_init_end;
    }
    else
    {
        start = (unsigned long)&__init_begin;
        end   = (unsigned long)&__init_end;
    }

is a tautology that nothing is capable of optimising.

So I set about trying to simply both x86 and ARM down to a single sets
of bounding variables, with a requirement that these would be expected
to be common across all architectures.

I'm intending to use __$FOO_{start,end} because we're semi-consistent on
this already, and get rid of the ones such as _{s,e}$FOO because they're
unnecessarily obscure, and complicated to read for a compound foo.

At this point (as I haven't really started yet), I could be persuaded on
a different naming scheme if anyone has any strong views.


But that's only the start of the fun.  The is_kernel() predicate is
broken (or at least problematic) because it covers the init section. 
Reviewing its usage shows that ARM is broken when trying to handle
BUG/ASSERT in livepatches, because they don't check is_patch() on the
message target.  But for both x86 and ARM, this should be restricted to
(a new) is_active_rodata() predicate.

The x86 xen_region[] bss logic is broken, because of ebmalloc.  Its dumb
to have the ebmalloc region anywhere but at the end of bss (at least we
could avoid punching a hole in the middle of the BSS, and avoid needing
an extra region for tboot/etc), but honestly, 1M is excessive for what
it contains, and 8k is probably plenty (so call it 32/64k for headroom),
and we'll be in a far better position not freeing it at all.  Saving a
few kb of memory is absolutely not worth the breakages it introduces. 
In particular...

is_xen_fixed_mfn() is broken for it's main caller, because it includes
MFNs which are not Xen's, and the offline page logic will take the wrong
action.

The helper is also unnecessarily complicated, and unnecessarily
different between x86 and ARM.  ARM has paddr_t phys_offset, while x86
has xen_phys_start  (ARM even extern's it, looking like copy&paste from
x86), both of which relate to XEN_VIRT_START.

Details like this really need to be common, as they're invariant of
architecture.  Furthermore, making it common allows us to consolidate
helpers such as is_xen_fixed_mfn() in common logic too, which
substantially reduces the complexity for common code trying to figure
out whether predicates with the same name are the same in each arch
(because some are not).

Therefore, I'm going to make it common, along with a very clear
description of what it is and how to use it.

On x86, we further have a mess between XEN_VIRT_START and
__XEN_VIRT_START, along with XEN_IMG_OFFSET.  Now that head.S was
rewritten to be position independent for MB2, this is easy to resolve,
and doing so will massively simplify the linker file, and the setup logic.

On ARM, the embedded dtb support wants a rethink, because it was placed
after BSS, rendering any space-saving null and void in the compiled (and
loaded) image.  There is also nothing which checks the BSS is correctly
aligned for the zeroing loops (which I do have fix for).


I'm fairly sure I've forgotten some things, but lets start with these
for now.  I really am quite alarmed at the mess and errors we've got
with freeing various pieces of the compiled image.

~Andrew



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.