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

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

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@oracle.com> wrote:
>>>> On Mon, Aug 15, 2016 at 02:21:48AM -0600, Jan Beulich wrote:
>>>>>>>> On 15.08.16 at 01:07, <konrad.wilk@oracle.com> 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'.
>
> 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,

-- 
Julien Grall

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