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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

> +
> +/*
> + * 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..)'?

> +        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 ?

> + * into the idle_loop.
> + */
> +void __init apply_alternatives_all(void)

s/apply_alternatives_all/apply_alternatives_init/ ?

> +{
> +    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.

> +    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.
> +    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.

>    } :text
>    . = ALIGN(PAGE_SIZE);
>    .init.data : {
> @@ -167,6 +170,13 @@ SECTIONS
>         *(.xsm_initcall.init)
>         __xsm_initcall_end = .;
>    } :text
> +#ifdef CONFIG_ALTERNATIVE
> +  .init.alternatives : {
> +      __alt_instructions = .;
> +      *(.altinstructions)
> +      __alt_instructions_end = .;
> +  }
> +#endif
>    __init_end_efi = .;
>    . = ALIGN(STACK_SIZE);
>    __init_end = .;

Otherwise it looks OK to me.

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