[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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |