On Wed, Nov 11, 2020 at 12:25 PM Andrew Cooper <andrew.coop...@citrix.com> wrote: > > On 11/11/2020 20:15, Josh Poimboeuf wrote: > > On Wed, Nov 11, 2020 at 09:07:30PM +0100, Peter Zijlstra wrote: > >> On Wed, Nov 11, 2020 at 01:59:00PM -0600, Josh Poimboeuf wrote: > >>> On Wed, Nov 11, 2020 at 08:42:06PM +0100, Peter Zijlstra wrote: > >>>>> Would objtool have an easier time coping if this were implemented in > >>>>> terms of a static call? > >>>> I doubt it, the big problem is that there is no visibility into the > >>>> actual alternative text. Runtime patching fragments into static call > >>>> would have the exact same problem. > >>>> > >>>> Something that _might_ maybe work is trying to morph the immediate > >>>> fragments into an alternative. That is, instead of this: > >>>> > >>>> static inline notrace unsigned long arch_local_save_flags(void) > >>>> { > >>>> return PVOP_CALLEE0(unsigned long, irq.save_fl); > >>>> } > >>>> > >>>> Write it something like: > >>>> > >>>> static inline notrace unsigned long arch_local_save_flags(void) > >>>> { > >>>> PVOP_CALL_ARGS; > >>>> PVOP_TEST_NULL(irq.save_fl); > >>>> asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL), > >>>> "PUSHF; POP _ASM_AX", > >>>> X86_FEATURE_NATIVE) > >>>> : CLBR_RET_REG, ASM_CALL_CONSTRAINT > >>>> : paravirt_type(irq.save_fl.func), > >>>> paravirt_clobber(PVOP_CALLEE_CLOBBERS) > >>>> : "memory", "cc"); > >>>> return __eax; > >>>> } > >>>> > >>>> And then we have to teach objtool how to deal with conflicting > >>>> alternatives... > >>>> > >>>> That would remove most (all, if we can figure out a form that deals with > >>>> the spinlock fragments) of paravirt_patch.c > >>>> > >>>> Hmm? > >>> I was going to suggest something similar. Though I would try to take it > >>> further and replace paravirt_patch_default() with static calls. > >> Possible, we just need to be _really_ careful to not allow changing > >> those static_call()s. So maybe we need DEFINE_STATIC_CALL_RO() which > >> does a __ro_after_init on the whole thing. > > But what if you want to live migrate to another hypervisor ;-) > > The same as what happens currently. The user gets to keep all the > resulting pieces ;) > > >>> Either way it doesn't make objtool's job much easier. But it would be > >>> nice to consolidate runtime patching mechanisms and get rid of > >>> .parainstructions. > >> I think the above (combining alternative and paravirt/static_call) does > >> make objtool's job easier, since then we at least have the actual > >> alternative instructions available to inspect, or am I mis-understanding > >> things? > > Right, it makes objtool's job a _little_ easier, since it already knows > > how to read alternatives. But it still has to learn to deal with the > > conflicting stack layouts. > > I suppose the needed abstraction is "these blocks will start and end > with the same stack layout", while allowing the internals to diverge. >
How much of this stuff is actually useful anymore? I'm wondering if we can move most or all of this crud to cpu_feature_enabled(X86_FEATURE_XENPV) and its asm equivalent. The full list, annotated, appears to be: const unsigned char irq_irq_disable[1]; This is CLI or CALL, right? const unsigned char irq_irq_enable[1]; STI or CALL. const unsigned char irq_save_fl[2]; PUSHF; POP %r/eax. I *think* I read the paravirt mess correctly and this also turns into CALL. const unsigned char mmu_read_cr2[3]; const unsigned char mmu_read_cr3[3]; const unsigned char mmu_write_cr3[3]; The write CR3 is so slow that I can't imagine us caring. Reading CR3 should already be fairly optimized because it's slow on old non-PV hypervisors, too. Reading CR2 is rare and lives in asm. These also appear to just switch between MOV and CALL, anyway. const unsigned char irq_restore_fl[2]; Ugh, this one sucks. IMO it should be, for native and PV: if (flags & X86_EFLAGS_IF) { local_irq_enable(); /* or raw? */ } else { if (some debugging option) { WARN_ON_ONCE(save_fl() & X86_EFLAGS_IF); } } POPF is slooooow. const unsigned char cpu_wbinvd[2]; This is hilariously slow no matter what. static_call() or even just a plain old indirect call should be fine. const unsigned char cpu_usergs_sysret64[6]; This is in the asm and we shouldn't be doing it at all for Xen PV. IOW we should just drop this patch site entirely. I can possibly find some time to get rid of it, and maybe someone from Xen land can help. I bet that we can gain a lot of perf on Xen PV by cleaning this up, and I bet it will simplify everything. const unsigned char cpu_swapgs[3]; This is SWAPGS or nop, unless I've missed some subtlety. const unsigned char mov64[3]; This is some PTE magic, and I haven't deciphered it yet. So I think there is at most one of these that wants anything more complicated than a plain ALTERNATIVE. Any volunteers to make it so? Juergen, if you do all of them except USERGS_SYSRET64, I hereby volunteer to do that one. BTW, if y'all want to live migrate between Xen PV and anything else, you are nuts.