Re: [PATCH v6 08/20] xen/riscv: introduce cmpxchg.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2024-03-21 at 13:07 +0100, Jan Beulich wrote:
> > + * If resulting 4-byte access is still misalgined, it will fault
> > just as
> > + * non-emulated 4-byte access would.
> > + */
> > +#define emulate_xchg_1_2(ptr, new, lr_sfx, sc_sfx) \
> > +({ \
> > +    uint32_t *aligned_ptr = (uint32_t *)((unsigned long)ptr &
> > ~(0x4 - sizeof(*(ptr)))); \
> > +    unsigned int new_val_pos = ((unsigned long)(ptr) & (0x4 -
> > sizeof(*(ptr)))) * BITS_PER_BYTE; \
> 
> You parenthesize ptr here correctly, but not in the line above.
> 
> Instead of "_pos" in the name, maybe better "_bit"?
> 
> Finally, here and elsewhere, please limit line length to 80 chars.
> (Omitting
> the 0x here would help a little, but not quite enough. Question is
> whether
> these wouldn't better be sizeof(*aligned_ptr) anyway.)

I'm unsure if using sizeof(*aligned_ptr) is correct in this context.
The intention was to determine the position for the value we're
attempting to exchange.

Let's consider a scenario where we have 0xAABBCCDD at address 0x6. Even
though this would result in misaligned access, I believe new_val_pos
should still be calculated accurately. Let's say we want to exchange
two bytes (AABB) with FFFF.

With the current implementation, we would calculate:
0x6 & 2 = 2 * 8 = 16, which is the value on which the new value should
be shifted.

However, if this value is then ANDed with sizeof(*aligned_ptr):
0x6 & 4 = 6 * 8 = 48, which is not the expected value.

Am I overlooking something?

~ Oleksii