[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 3/9] xen/riscv: allow write_atomic() to work with non-scalar types
On Tue, 2024-09-10 at 18:05 +0200, Jan Beulich wrote: > On 10.09.2024 17:28, oleksii.kurochko@xxxxxxxxx wrote: > > On Tue, 2024-09-10 at 11:53 +0200, Jan Beulich wrote: > > > 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. > > If it would be better to keep 'unsigned long' as the type of x, > > then, > > if I am not mistaken, write_atomic() should be updated in the > > following > > way: > > #define write_atomic(p, x) \ > > ({ \ > > typeof(*(p)) x_ = (x); \ > > _write_atomic(p, *(unsigned long *)&x_, > > sizeof(*(p))); > > \ > > }) > > However, I am not sure if it is safe when x is a 2-byte value (for > > example) that it will read more than 2 bytes before passing the > > value > > to the _write_atomic() function. > > No, that's definitely unsafe. Then, at the moment, I don't see a better option than having const void *x as an argument for the _write_atomic() function and then performing casts when writeX_cpu(*(const uintX *)x, p) is called. > > > > > @@ -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? > > if _add_sized() is defined as it was before: > > static always_inline void _add_sized(volatile void *p, > > unsigned long x, unsigned > > int > > size) > > { > > switch ( size ) > > { > > case 1: > > { > > volatile uint8_t *ptr = p; > > write_atomic(ptr, read_atomic(ptr) + x); > > break; > > } > > ... > > Then write_atomic(ptr, read_atomic(ptr) + x) will be be changed to: > > volatile uint8_t x_ = (x); > > > > And that will cause a compiler error: > > ./arch/riscv/include/asm/atomic.h:75:22: error: passing argument > > 2 > > of '_write_atomic' discards 'volatile' qualifier from pointer > > target > > type [-Werror=discarded-qualifiers] > > 75 | _write_atomic(p, &x_, sizeof(*(p))); > > Because it can't cast 'volatile uint8_t *' to 'void *': > > expected 'void *' but argument is of type 'volatile uint8_t *' > > {aka > > 'volatile unsigned char *'} > > Oh, I think I see now. What we'd like write_atomic() to derive is the > bare > (unqualified) type of *ptr, yet iirc only recent compilers have a way > to > obtain that. I assume that you are speaking about typeof_unqual which requires C23 (?). __auto_type seems to me can also drop volatile quilifier but in the docs I don't see that it should (or not) discard qualifier. Could it be an option: #define write_atomic(p, x) \ ({ \ __auto_type x_ = (x); \ _write_atomic(p, &x_, sizeof(*(p))); \ }) And another option could be just drop volatile by casting: #define write_atomic(p, x) \ ... _write_atomic(p, (const void *)&x_, sizeof(*(p))); ~ Oleksii
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |