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

[Juergen Gross]

On 10/08/17 16:09, Josh Poimboeuf wrote:
> 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.

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.

> 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. Or would you want to let Xen, lguest etc. opt in
for pvops and generate above code at build time from some templates?


Juergen