On 27.08.2024 15:56, Frediano Ziglio wrote: > On Mon, Aug 26, 2024 at 9:21 AM Jan Beulich <jbeul...@suse.com> wrote: >> >> On 22.08.2024 17:29, Frediano Ziglio wrote: >>> The trampoline could have "manual" relocation entries (created >>> by assembly macros and some code to use them) and (in case of PE) >>> normal executable relocations. >>> Remove all normal executable ones. In this way we don't have to >>> worry about applying both correctly (they need proper order >>> which is hard to spot looking at the code). >>> >>> Signed-off-by: Frediano Ziglio <frediano.zig...@cloud.com> >> >> I think this wants splitting into one patch replacing sym_offs() and a >> 2nd one introducing the hand-crafted __XEN_VIRT_START additions (which >> may want macro-izing). Plus the justification for the change wants >> extending, to actually explain what the problem is - after all there's >> no issue anywhere right now. > > Should I explain the time dependency issue with source code?
Time dependency? I guess I don't understand ... > I suppose I can describe where currently is and why it would be better > to have it removed (honestly I though I did but reading the commit > message I didn't). > Maybe for search reasons it would be better to define 2 macros like > the following? > > #define phys_addr(sym) sym ## _pa > #define virt_addr(sym) sym ## _va > > This way, for instance, searching for the "__high_start" word (like > "grep -rw __high_start") would produce a result. I assume it's best to keep everything there together, so see below. >> With the sym_offs() uses gone, I think it would be best if the macro was >> #undef-ed ahead of the inclusion of trampoline.S. Since x86_64.S uses the >> macro, that'll require careful re-arrangement of #include order. >> > > I think you mean including the trampoline after including x86_64.S in > head.S. Yes. >>> --- a/xen/arch/x86/xen.lds.S >>> +++ b/xen/arch/x86/xen.lds.S >>> @@ -71,7 +71,12 @@ SECTIONS >>> __2M_text_start = .; /* Start of 2M superpages, mapped RX. */ >>> #endif >>> >>> - start_pa = ABSOLUTE(start - __XEN_VIRT_START); >>> +#define DEFINE_PA_ADDRESS(sym) sym ## _pa = ABSOLUTE(sym - >>> __XEN_VIRT_START) >>> + DEFINE_PA_ADDRESS(start); >>> + DEFINE_PA_ADDRESS(saved_magic); >>> + DEFINE_PA_ADDRESS(idle_pg_table); >>> + DEFINE_PA_ADDRESS(__high_start); >>> + DEFINE_PA_ADDRESS(s3_resume); >>> >>> . = __XEN_VIRT_START + XEN_IMG_OFFSET; >>> _start = .; >> >> For the cases where in assembly code you add __XEN_VIRT_START this is pretty >> odd: You subtract the value here just to add it back there. Did you consider >> a more straightforward approach, like introducing absolute xxx_va symbols? > > I didn't consider. Would something like > > #define DEFINE_ABS_ADDRESSES(sym) \ > sym ## _pa = ABSOLUTE(sym - __XEN_VIRT_START); \ > sym ## _va = ABSOLUTE(sym) > > make sense? Maybe the _pa and _va suffixes are too similar? Maybe > _physaddr and _virtaddr? Or use capical letters and macros (as above) > to avoid possible clashes? I'd like to ask that we don't introduce symbols we don't actually use. Hence a single macro defining both is probably not going to be overly helpful. As to capital letters: I'm struggling with the "(as above)" - I don't see any use of capital letters in symbol names being generated. But yes, I was going to suggest to consider _VA and _PA tags, precisely to reduce the risk of clashes. Jan
