On 02.09.2024 19:01, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/atomic.h
> +++ b/xen/arch/riscv/include/asm/atomic.h
> @@ -54,16 +54,16 @@ static always_inline void read_atomic_size(const volatile
> void *p,
> })
>
> static always_inline void _write_atomic(volatile void *p,
> - unsigned long x,
> + void *x,
Pointer-to-const please, to further aid in easily recognizing which
parameter is what. After all ...
> unsigned int size)
> {
> switch ( size )
> {
> - case 1: writeb_cpu(x, p); break;
> - case 2: writew_cpu(x, p); break;
> - case 4: writel_cpu(x, p); break;
... unhelpfully enough parameters are then swapped, just to confuse
things.
> + case 1: writeb_cpu(*(uint8_t *)x, p); break;
> + case 2: writew_cpu(*(uint16_t *)x, p); break;
> + case 4: writel_cpu(*(uint32_t *)x, p); break;
> #ifndef CONFIG_RISCV_32
> - case 8: writeq_cpu(x, p); break;
> + case 8: writeq_cpu(*(uint64_t *)x, p); break;
With const added to the parameter, please further make sure to then not
cast that away again.
> @@ -72,7 +72,7 @@ static always_inline void _write_atomic(volatile void *p,
> #define write_atomic(p, x) \
> ({ \
> typeof(*(p)) x_ = (x); \
> - _write_atomic(p, x_, sizeof(*(p))); \
> + _write_atomic(p, &x_, sizeof(*(p))); \
> })
>
> static always_inline void _add_sized(volatile void *p,
> @@ -82,27 +82,23 @@ static always_inline void _add_sized(volatile void *p,
> {
> case 1:
> {
> - volatile uint8_t *ptr = p;
> - write_atomic(ptr, read_atomic(ptr) + x);
> + writeb_cpu(readb_cpu(p) + x, p);
> break;
> }
> case 2:
> {
> - volatile uint16_t *ptr = p;
> - write_atomic(ptr, read_atomic(ptr) + x);
> + writew_cpu(readw_cpu(p) + x, p);
> break;
> }
> case 4:
> {
> - volatile uint32_t *ptr = p;
> - write_atomic(ptr, read_atomic(ptr) + x);
> + writel_cpu(readl_cpu(p) + x, p);
> break;
> }
> #ifndef CONFIG_RISCV_32
> case 8:
> {
> - volatile uint64_t *ptr = p;
> - write_atomic(ptr, read_atomic(ptr) + x);
> + writeq_cpu(readw_cpu(p) + x, p);
> break;
> }
> #endif
I'm afraid I don't understand this part, or more specifically the respective
part of the description. It is the first parameter of write_atomic() which is
volatile qualified. And it is the first argument that's volatile qualified
here. Isn't the problem entirely unrelated to volatile-ness, and instead a
result of the other parameter changing from scalar to pointer type, which
doesn't fit the addition expressions you pass in?
Also you surely mean readq_cpu() in the 8-byte case.
Jan
