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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2024-04-02 at 14:54 +0200, Jan Beulich wrote:
> On 02.04.2024 13:43, Oleksii wrote:
> > 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?
> 
> I'm afraid I can only reply with a counter question (feels like there
> is
> some misunderstanding): Do you agree that 0x4 == 4 ==
> sizeof(*aligned_ptr)?
> It's the 0x4 that the latter part of my earlier reply was about. I'm
> pretty
> sure you have, over the various reviews I've done, noticed my dislike
> for
> the use of literal numbers, when those aren't truly "magic".
Yes, it was misunderstading. Thanks for clarifying.

~ Oleksii