Re: [Xen-devel] [PATCH v3 09/16] xen/arm: Introduce alternative runtime patching

[Julien Grall <julien.grall@xxxxxxx>]

Hi Konrad,

On 07/06/2016 18:24, Konrad Rzeszutek Wilk wrote:
> > +
> > +/*
> > + * We might be patching the stop_machine state machine, so implement a
> > + * really simple polling protocol here.
> > + */
> > +static int __apply_alternatives_multi_stop(void *unused)
> > +{
> > +    static int patched = 0;
> > +    struct alt_region region = {
> > +        .begin = __alt_instructions,
> > +        .end = __alt_instructions_end,
> > +    };
> > +
> > +    /* We always have a CPU 0 at this point (__init) */
> > +    if ( smp_processor_id() )
> > +    {
> > +        while ( !read_atomic(&patched) )
> > +            cpu_relax();
> > +        isb();
> > +    }
> > +    else
> > +    {
> > +        int ret;
> > +
> > +        BUG_ON(patched);
> > +        ret = __apply_alternatives(&region);
> > +        /* The patching is not expected to fail during boot. */
> > +        BUG_ON(ret != 0);
> > +
> > +        /* Barriers provided by the cache flushing */
> Could you add the stop at the end here (and above in the 'We always have..)'?
Will do.

> > +        write_atomic(&patched, 1);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +/*
> > + * This function should only be called during boot and before CPU0 jump
> Would it make sense to then have an ASSERT or BUG on:
>
> SYS_STATE_early_boot ?
No need, this function is part of __init. Anyone calling this function 
after boot will crash the hypervisor :).
> > + * into the idle_loop.
> > + */
> > +void __init apply_alternatives_all(void)
> s/apply_alternatives_all/apply_alternatives_init/ ?
I think _all makes more sense because we apply all the alternatives to 
the hypervisor.
> > +{
> > +    int ret;
> > +
> > +       /* better not try code patching on a live SMP system */
> Something is off with this comment (wrong column). And while
> at it you could also add the '.' at the end.
It contains an hard tab. I will fix it in the next version.

> > +    ret = stop_machine_run(__apply_alternatives_multi_stop, NULL, NR_CPUS);
> > +
> > +    /* stop_machine_run should never fail at this stage of the boot */
> Missing stpo.
ok.

> > +    BUG_ON(ret);
> > +}
> > +
> ..snip..
> > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> > index 1f010bd..495f9d8 100644
> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -129,6 +129,9 @@ SECTIONS
> >         _sinittext = .;
> >         *(.init.text)
> >         _einittext = .;
> > +#ifdef CONFIG_ALTERNATIVE
> > +       *(.altinstr_replacement)
> > +#endif
> This is outside the _einittext? x86 looks to have .altinstr_replacement
> inside the _einittext.
Yes, I looked at the x86 code when I did the implement and I did not 
find any good reason to keep .altinstr_replace inside the inittext.
altinstr_replacement contains replacement instructions. Anything inside 
the inittext region will be mark executable, which is not what we want here.
Regards,

--
Julien Grall

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