[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC 10/16] xen/arm: Introduce alternative runtime patching



On Tue, May 31, 2016 at 11:24:10AM +0100, Julien Grall wrote:
> Hi Stefano,
> 
> On 31/05/16 10:21, Stefano Stabellini wrote:
> >On Mon, 30 May 2016, Julien Grall wrote:
> >>Hi Stefano,
> >>
> >>On 30/05/2016 15:45, Stefano Stabellini wrote:
> >>>On Mon, 23 May 2016, Julien Grall wrote:
> >>>>Hi Stefano,
> >>>>
> >>>>On 21/05/16 16:09, Stefano Stabellini wrote:
> >>>>>On Thu, 5 May 2016, Julien Grall wrote:
> >>>>>>+void __init apply_alternatives_all(void)
> >>>>>>+{
> >>>>>>+    int ret;
> >>>>>>+
> >>>>>>+       /* better not try code patching on a live SMP system */
> >>>>>>+    ret = stop_machine_run(__apply_alternatives_multi_stop, NULL,
> >>>>>>NR_CPUS);
> >>>>>
> >>>>>Why not just call stop_machine_run, passing 0 as the cpu to run
> >>>>>__apply_alternatives_multi_stop on? Given that you already have
> >>>>>secondary cpus spinning in __apply_alternatives_multi_stop.  What am I
> >>>>>missing?
> >>>>
> >>>>Someone as to call __apply_alternatives_multi_stop on secondary CPUs.
> >>>
> >>>Why? Secondary cpus would be just left spinning at the beginning of the
> >>>function. You might as well leave them spinning in
> >>>stopmachine_wait_state.
> >>>
> >>
> >>Because, we may need to patch the stop_machine state machine (spinlock,...).
> >>So the safest place whilst the code is patched is
> >>__apply_alternatives_multi_stop.
> >>
> >>Note that there is a comment on top of __apply_alternatives_multi_stop to
> >>explain the reason.
> >
> >Have you tried patching stop_machine? What if you need to patch
> >__apply_alternatives_multi_stop? :-)
> 
> We have to define a safe place where the CPUs could spin. I.e the
> instructions will not be patched.
> Whilst stop_machine may not be patched today, it is common code and we don't
> know how this will evolve in the future.

Right, which is why livepatching persued a different path as it may
be patched. And instead choose a simpler mechanism to trigger IPIs and
either patched it from idle_loop() or from the VMX assembler code.


> 
> By spinning in __apply_alternatives_multi_stop we protect us against
> modification in the common code and tricky bug (yes it might be difficult to
> hit and debug).

I feel that you are moving down the stack to try to make the
impact of doing modifications have no impact (or as low as possible).

And it would work now, but I am concerned that in the future it may be
not soo future proof.

Would it perhaps make sense to make some of the livepatching mechanism
be exposed as general code? And use it for alternative asm as well?


> 
> Regards,
> 
> -- 
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.