Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder

[Josh Poimboeuf]

On Thu, Aug 10, 2017 at 09:05:19AM +0200, Juergen Gross wrote:
> > I'm wondering why xen_patch() even exists.  The main difference between
> > xen_patch() and native_patch() seems to be that xen_patch() does some
> > relocs when doing an inline patch after calling paravirt_patch_insns().
> > 
> > But I can't see how that code path would ever run, because the
> > replacement functions are all larger than the size of the call
> > instruction to be replaced (7 bytes).  So they would never fit, and
> > instead the paravirt_patch_default() case would always run.  Or am I
> > missing something?
> 
> Hmm, interesting. Just checked it and it seems you are right.
> 
> > If we could get rid of the hypervisor-specific patching functions
> > (pv_init_ops) -- including possibly removing the lguest and vsmp code,
> > if nobody cares about them anymore -- that might make it easier to
> > consolidate all the patching things into a single place.
> 
> I'll send some patches to:
> 
> - remove xen_patch()
> - remove lguest
> - remove vsmp
> 
> In case nobody objects to apply those patches we can possibly simplify
> some more code.
> 
> I'd love that. :-)

Well, I might have spoken too soon about getting rid of vsmp.  The
scalemp.com domain still exists.  The code hasn't changed much in three
years, but maybe it's simple enough that it hasn't needed to change.

Also, looking at the lguest mailing list, there seem to have been at
least a few people trying lguest out in the past year or so.

Even if we couldn't get rid of vsmp or lguest, I wonder if the PVOP_CALL
stuff could be reworked to something like the following:

static inline notrace unsigned long arch_local_save_flags(void)
{
	return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl,
			    "pushfq; popq %rax", CPU_FEATURE_NATIVE,
			    "call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN,
			    "call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP,
			    "call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST);
} 

Which would eventually translate to something like:

asm volatile(ALTERNATIVE_4("call *pv_irq_ops.save_fl",
			   "pushfq; popq %rax", CPU_FEATURE_NATIVE,
			   "call __raw_callee_save_xen_save_fl", CPU_FEATURE_XEN,
			   "call __raw_callee_save_vsmp_save_fl", CPU_FEATURE_VSMP,
			   "call __raw_callee_save_lguest_save_fl", CPU_FEATURE_LGUEST
			   : ... pvop clobber stuff ... );

where ALTERNATIVE_4 is a logical extension of ALTERNATIVE_2 and
CPU_FEATURE_NATIVE would always be set.

It might need some more macro magic, but if it worked I think it would
be a lot clearer than the current voodoo.

Thoughts?

-- 
Josh