Re: [PATCH v4 12/30] xen/riscv: introduce cmpxchg.h

[Julien Grall <julien@xxxxxxx>]

Hi Oleksii,

On 19/02/2024 14:00, Oleksii wrote:
> On Sun, 2024-02-18 at 19:00 +0000, Julien Grall wrote:
> > On 05/02/2024 15:32, Oleksii Kurochko wrote:
> > > The header was taken from Linux kernl 6.4.0-rc1.
> > >
> > > Addionally, were updated:
> > > * add emulation of {cmp}xchg for 1/2 byte types
> > This explaination is a little bit light. IIUC, you are implementing
> > them
> > using 32-bit atomic access. Is that correct? If so, please spell it
> > out.
> Sure, I'll update commit message.
>
> > Also, I wonder whether it would be better to try to get rid of the
> > 1/2
> > bytes access. Do you know where they are used?
> Right now, the issue is with test_and_clear_bool() which is used in
> common/sched/core.c:840
> [https://gitlab.com/xen-project/xen/-/blob/staging/xen/common/sched/core.c?ref_type=heads#L840
> ]
>
> I don't remember details, but in xen-devel chat someone told me that
> grant table requires 1/2 bytes access.
Ok :/. This would be part of the ABI then and therefore can't be easily 
changed.
> > > * replace tabs with spaces
> > Does this mean you are not planning to backport any Linux fixes?
> If it will be any fixes for sure I'll back port them, but it looks like
> this code is stable enough and not to many fixes will be done there, so
> it shouldn't be hard to backport them and switch to spaces.
Fair enough.

> > > +        "   and  %1, %0, %z4\n" \
> > > +        "   or   %1, %1, %z3\n" \
> > > +        "   sc.d %1, %1, %2\n" \
> > ... respectively sc.w. The same applies for cmpxchg.
> I agree that it would be better, and my initial attempt was to handle
> 4-byte or 8-byte boundary crossing during 2-byte access:
>
> 0 1 2 3 4 5 6 7 8
> X X X 1 1 X X X X
>
> In this case, if I align address 3 to address 0 and then read 4 bytes
> instead of 8 bytes, I will not process the byte at address 4. This was
> the reason why I started to read 8 bytes.
At least on Arm, the architecture doesn't support atomic operations if 
the access is not aligned to its size (this will send a data abort). On 
some architecture, this is supported but potentially very slow.
So all the common code should already use properly aligned address. 
Therefore, I don't really see the reason to add support for unaligned 
access.
Cheers,

--
Julien Grall