Re: [Xen-devel] [PATCH v1 6/9] livepatch: Initial ARM64 support.

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/08/16 16:25, Julien Grall wrote:
>
>
> On 15/08/2016 17:17, Konrad Rzeszutek Wilk wrote:
>> On Mon, Aug 15, 2016 at 04:57:26PM +0200, Julien Grall wrote:
>>> Hi Jan and Konrad,
>>>
>>> On 15/08/2016 16:23, Jan Beulich wrote:
>>>>>>> On 15.08.16 at 16:09, <konrad.wilk@xxxxxxxxxx> wrote:
>>>>> On Mon, Aug 15, 2016 at 02:21:48AM -0600, Jan Beulich wrote:
>>>>>>>>> On 15.08.16 at 01:07, <konrad.wilk@xxxxxxxxxx> wrote:
>>>>>>> @@ -711,9 +711,15 @@ static int prepare_payload(struct payload
>>>>>>> *payload,
>>>>>>>                  return -EINVAL;
>>>>>>>              }
>>>>>>>          }
>>>>>>> +#ifndef CONFIG_ARM
>>>>>>>          apply_alternatives_nocheck(start, end);
>>>>>>> +#else
>>>>>>> +        apply_alternatives(start, sec->sec->sh_size);
>>>>>>> +#endif
>>>>>>
>>>>>> Conditionals like this are ugly - can't this be properly abstracted?
>>>>>
>>>>> Yes, I can introduce an apply_alternatives_nocheck on ARM that will
>>>>> hava the same set of arguments on x86.
>>>>>
>>>>> Or I can make a new function name?
>>>>
>>>> Either way is fine with me, with a slight preference to the former
>>>> one.
>>>
>>> I am fine with the prototype of the function
>>> apply_alternatives_nocheck but
>>> I don't think the name is relevant for ARM.
>>>
>>> Is there any reason we don't want to call directly
>>> apply_alternatives in
>>> x86?
>>
>> It assumes (and has an ASSERT) that it is called with interrupts
>> disabled.
>> And we don't need to do that (as during livepatch loading we can
>> modify the
>> livepatch payload without worrying about interrupts).
>
> Oh, it makes more sense now.
>
>>
>> P.S.
>> loading != applying.
>>
>> I could do a patch where we rename 'apply_alternatives' ->
>> 'apply_alternatives_boot'
>> and 'apply_alternatives_nocheck' to 'apply_alternatives'.

The only reason apply_alternatives() is named thusly is to match Linux. 
I am not fussed if it changes.

~Andrew

>>
>> Also the x86 version instead of having:
>>
>> apply_alternatives(struct alt_instr *start, struct alt_instr *end)
>>
>> would end up with:
>>
>> apply_alternatives(void *start, size_t length)
>
> I would be fine with the prototype
> apply_alternatives(struct alt_instr *start, struct alt_instr *end)
>
> for ARM. It is up to you.
>
> Cheers,
>


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