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

[Josh Poimboeuf]

On Thu, Aug 10, 2017 at 04:59:36PM +0200, Juergen Gross wrote:
> On 10/08/17 16:39, Josh Poimboeuf wrote:
> > 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?
> 
> It shouldn't be too much work to let each pvops user have a file in a
> common paravirt directory containing the needed information to create:
> 
> 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);
> }
> 
> and all other needed functions at build time. It could look e.g. like
> (for xen: xen.pv):
> 
> @@feature CPU_FEATURE_XEN
> PV_IRQ_OPS_SAVE_FL "call __raw_callee_save_xen_save_fl"
> 
> and the pre-processor could be used to assemble all configured users
> (pvops.pv):
> 
> #ifdef CONFIG_XEN_PV
> #include "xen.pv"
> #endif
> #ifdef CONFIG_LGUEST
> #include "lguest.pv"
> #endif
> 
> The resulting file would the be mangled by e.g. a python or awk script
> to a header containing macro definitions like:
> 
> #define 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 can then be used in paravirt.h:
> 
> static inline notrace unsigned long arch_local_save_flags(void)
> {
>     return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl,
>                         PV_IRQ_OPS_SAVE_FL);
> }

That could work, though I'd prefer the code-based approach because I get
the feeling it would be less obtuse.  I can play around with it, though
it may be a few weeks.  Feel free to delete code in the meantime :-)

-- 
Josh