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

[Juergen Gross]

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);
}


Juergen