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

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

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.
Cheers,

--
Julien Grall