Hi, > On 8 Nov 2022, at 07:26, Michal Orzel <[email protected]> wrote: > > 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 <[email protected]> >>>>>>> >>>>>>> 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 <[email protected]> >>> Reviewed-by: Michal Orzel <[email protected]> >>> Reviewed-by: Bertrand Marquis <[email protected]> >>> >>> 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.
I will check and ack the v3 once out. Cheers Bertrand > >> Cheers, >> >> -- >> Julien Grall > > ~Michal
