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

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Thu, Aug 10, 2017 at 04:24:58PM +0200, Juergen Gross wrote:
> >> 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.
> 
> Lets see. I have made the experience that asking whether some code can
> be removed almost never get answers. Sending a patch which actually
> removes the stuff results much more often in objections. :-)
> 
> > 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.
> 
> Well, yes. The question is here whether there is a _need_ for lguest
> or was it just out of curiosity?
> 
> In the end it is 32 bit only and you can easily test boot code via
> KVM, Xen or qemu.

Good points.  I'm all for removing code, so you have no objections from
me :-)

> > 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?
> 
> Hmm, this would modify the current approach of pvops completely: instead
> of letting each user of pvops (xen, lguest, vsmp, ...) set the functions
> it is needing, you'd have to modify the core definition of each pvops
> function for each user.

Right.  The callers (arch_local_save_flags, etc) would have to know
about the different hypervisors' functions.  But this knowledge could be
hidden in inline functions and/or macros, so I don't see it being too
much of a problem.

The upsides are that the behavior is much clearer (IMO), and we could
get rid of the .parainstructions stuff altogether.

> Or would you want to let Xen, lguest etc. opt in
> for pvops and generate above code at build time from some templates?

I'm not sure what you mean, can you clarify?

-- 
Josh