Re: [XEN v2] xen/Arm: Enforce alignment check for atomic read/write

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 07/11/2022 19:06, Julien Grall wrote:
> 
> 
> Hi Ayan,
> 
> On 07/11/2022 12:49, Ayan Kumar Halder wrote:
>>
>> On 07/11/2022 10:44, Julien Grall wrote:
>>> Hi Ayan,
>> Hi Julien,
>>>
>>> On 07/11/2022 10:36, Ayan Kumar Halder wrote:
>>>>
>>>> On 06/11/2022 17:54, Julien Grall wrote:
>>>>> Hi Ayan,
>>>>
>>>> Hi Julien,
>>>>
>>>> I need some clarification.
>>>>
>>>>>
>>>>> To me the title and the explaination below suggests...
>>>>>
>>>>> On 04/11/2022 16:23, Ayan Kumar Halder wrote:
>>>>>> From: Ayan Kumar Halder <ayankuma@xxxxxxx>
>>>>>>
>>>>>> Refer ARM DDI 0487I.a ID081822, B2.2.1
>>>>>> "Requirements for single-copy atomicity
>>>>>>
>>>>>> - A read that is generated by a load instruction that loads a single
>>>>>> general-purpose register and is aligned to the size of the read in the
>>>>>> instruction is single-copy atomic.
>>>>>>
>>>>>> -A write that is generated by a store instruction that stores a single
>>>>>> general-purpose register and is aligned to the size of the write in
>>>>>> the
>>>>>> instruction is single-copy atomic"
>>>>>>
>>>>>> On AArch32, the alignment check is enabled at boot time by setting
>>>>>> HSCTLR.A bit.
>>>>>> ("HSCTLR, Hyp System Control Register").
>>>>>> However in AArch64, alignment check is not enabled at boot time.
>>>>>
>>>>> ... you want to enable the alignment check on AArch64 always.
>>>>
>>>> I want to enable alignment check *only* for atomic access.
>>>>
>>>> May be I should remove this line --> "However in AArch64, alignment
>>>> check is not enabled at boot time.".
>>>>
>>>>> However, this is not possible to do because memcpy() is using
>>>>> unaligned access.
>>>> This is a non atomic access. So the commit does not apply here.
>>>
>>> Right, but your commit message refers to the alignment check on arm32.
>>> You wrote too much for someone to wonder but not enough to explain why
>>> we can't enable the alignment check on arm64.
>>>
>>>>>
>>>>> I think the commit message/title should clarify that the check is
>>>>> *only* done during debug build. IOW, there are no enforcement in
>>>>> producation build.
>>>>
>>>> AFAICS read_atomic()/write_atomic() is enabled during non debug
>>>> builds (ie CONFIG_DEBUG=n) as well.
>>>
>>> My point was that ASSERT() is a NOP in production build. So you
>>> effectively the enforcement happens only in debug build.
>>>
>>> IOW, unless you test exhaustively with a debug build, you may never
>>> notice that the access was not atomic.
>>
>> This makes sense.
>>
>> Does the following commit message look better ?
>>
>> xen/Arm: Enforce alignment check for atomic read/write
> 
> title:
> 
> xen/arm: Enforce alignment check in debug build for {read, write}_atomic
> 
>>
>> Refer ARM DDI 0487I.a ID081822, B2.2.1
>> "Requirements for single-copy atomicity
>>
>> - A read that is generated by a load instruction that loads a single
>> general-purpose register and is aligned to the size of the read in the
>> instruction is single-copy atomic.
>>
>> -A write that is generated by a store instruction that stores a single
>> general-purpose register and is aligned to the size of the write in the
>> instruction is single-copy atomic"
>>
>> Thus, one needs to check for alignment when performing atomic operations.
>> However, as ASSERT() are disabled in production builds, so one needs to
> 
> This seems to be a bit out of context because you don't really explain
> that ASSERT() would be used. Also...
> 
>> run the debug builds to catch any unaligned access during atomic
>> operations.
>> Enforcing alignment checks during production build has quite a high
>> overhead.
>>
>> On AArch32, the alignment check is enabled at boot time by setting
>> HSCTLR.A bit.
>> ("HSCTLR, Hyp System Control Register").
>> However, on AArch64, memcpy()/memset() may be used on 64bit unaligned
>> addresses.
>> Thus, one does not wish to enable alignment check at boot time.
> 
> ... to me this paragraph should be first because this explained why we
> can't check in production. So how about the following commit message:
> 
> "
> xen/arm: Enforce alignment check in debug build for {read, write}_atomic
> 
> Xen provides helper to atomically read/write memory (see {read,
> write}_atomic()). Those helpers can only work if the address is aligned
> to the size of the access (see B2.2.1 ARM DDI 08476I.a).
> 
> On Arm32, the alignment is already enforced by the processor because
> HSCTLR.A bit is set (it enforce alignment for every access). For Arm64,
> this bit is not set because memcpy()/memset() can use unaligned access
> for performance reason (the implementation is taken from the Cortex
> library).
> 
> To avoid any overhead in production build, the alignment will only be
> checked using an ASSERT. Note that it might be possible to do it in
> production build using the acquire/exclusive version of load/store. But
> this is left to a follow-up (if wanted).
> "
This reads very well.

> 
> While trying to find a justification for the debug version. I was
> wondering whether we could actually use the acquire or exclusive
> version. I am not entirely sure about the overhead.
> 
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
>> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>>
>> I think I can keep R-b as there is no code change ?
> 
> My signed-off-by will need to be added for the commit message I proposed
> above. So I would like Bertrand/Michal to confirm they are happy with it
> (I don't usually add my reviewed-by/acked-by for patch where my
> signed-off-by is added).
> 
You can keep my Rb and Bertrand or Stefano can ack it, so that we can avoid
acking a patch by one of the authors.

> Cheers,
> 
> --
> Julien Grall

~Michal