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

[Julien Grall <julien.grall@arm.com>]

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@lists.xen.org
http://lists.xen.org/xen-devel