Re: [PATCH v5 1/7] xen/arm: Introduce alternative runtime patching

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

On 22/07/16 16:38, Konrad Rzeszutek Wilk wrote:
>>>> +
>>>> +        origptr = ALT_ORIG_PTR(alt);
>>>> +        writeptr = origptr - (u32 *)_start + writemap;
>>>
>>> How about just using writeptr += ?
>>
>> I am not sure about your suggestion here. Regardless the Linux code, the
>> origptr will not follow a pattern at each iteration. So we have to recompute
>> it everytime.
>
> Just ignore that. I somehow equated writeptr to writemp.
>
> ..snip..
>>
>>>
>>> Ah wait. You are trying to preserve the Linux code!. Nevermind then.
>>>
>>>> +        replptr = ALT_REPL_PTR(alt);
>>>> +
>>>> +        nr_inst = alt->alt_len / sizeof(insn);
>>>> +
>>>> +        for ( i = 0; i < nr_inst; i++ )
>>>> +        {
>>>> +            insn = get_alt_insn(alt, origptr + i, replptr + i);
>>>> +            *(writeptr + i) = cpu_to_le32(insn);
>>>> +        }
>>>> +
>>>> +        /* Ensure the new instructions reached the memory and nuke */
>>>> +        clean_and_invalidate_dcache_va_range(writeptr,
>>>> +                                             (sizeof (*writeptr) * nr_inst));
>>>> +    }
>>>> +
>>>> +    /* Nuke the instruction cache */
>>>> +    invalidate_icache();
>>>> +
>>>> +    vunmap(writemap);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * 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;
>>>
>>> Shouldn't this be 'atomic_t' ?
>>
>> Does it matter? From my understanding the code will behave the same.
>
> Not at all under the hood.
>  But I see 'atomic_write' and they all operate on the 'atomic_t' .. hence
> the query.

The code is using *_atomic (and not atomic_*) which operate on any type. 
I know the naming is really confusing.

I would like to rename write_atomic and read_atomic to WRITE_ONCE and 
READ_ONCE.

>
> ..snip.
>>> This being a new file perhaps add:
>>> *
>>> * Local variables:
>>> * mode: C
>>> * c-file-style: "BSD"
>>> * c-basic-offset: 4
>>> * indent-tabs-mode: nil
>>> * End:
>>> */
>>> ?
>>
>> It is a Linux file with Linux coding style. I would need to look what should
>> be the emacs magic block here.
>
> Right. I just meant that you needed the magic block.

I don't think we ever put magic block on Linux file. I will try to find 
out and update the patch.

Cheers,

-- 
Julien Grall

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