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

[Jan Beulich <jbeulich@xxxxxxxx>]

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".

Jan