| Message ID | 44810c0c3faa4fd2d36a8be9a87c5022088c0e62.1725295716.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | RISCV device tree mapping | expand |
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
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. > > > + 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? 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 *'} > > Also you surely mean readq_cpu() in the 8-byte case. Yes, thanks for finding that. ~ Oleksii
On 10.09.2024 17:28, oleksii.kurochko@gmail.com 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. >>> @@ -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. Jan
On Tue, 2024-09-10 at 18:05 +0200, Jan Beulich wrote: > On 10.09.2024 17:28, oleksii.kurochko@gmail.com 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
On 11.09.2024 13:34, oleksii.kurochko@gmail.com wrote: > On Tue, 2024-09-10 at 18:05 +0200, Jan Beulich wrote: >> On 10.09.2024 17:28, oleksii.kurochko@gmail.com wrote: >>> On Tue, 2024-09-10 at 11:53 +0200, Jan Beulich wrote: >>>> On 02.09.2024 19:01, Oleksii Kurochko wrote: >>>>> @@ -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 > (?). What C version it requires doesn't matter much for our purposes. The question is as of which gcc / clang version (if any) this is supported as an extension. > __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))); \ > }) For our purposes __auto_type doesn't provide advantages over typeof(). Plus, more importantly, the use above is wrong, just like typeof(x) would also be wrong. It needs to be p that the type is derived from. Otherwise consider what happens when ptr is unsigned long * or unsigned short * and you write write_atomic(ptr, 0); > And another option could be just drop volatile by casting: > #define write_atomic(p, x) \ > ... > _write_atomic(p, (const void *)&x_, sizeof(*(p))); See what I said earlier about casts: You shall not cast away qualifiers. Besides doing so being bad practice, you'll notice the latest when RISC-V code also becomes subject to Misra compliance. Jan
On Wed, 2024-09-11 at 13:49 +0200, Jan Beulich wrote: > On 11.09.2024 13:34, oleksii.kurochko@gmail.com wrote: > > On Tue, 2024-09-10 at 18:05 +0200, Jan Beulich wrote: > > > On 10.09.2024 17:28, oleksii.kurochko@gmail.com wrote: > > > > On Tue, 2024-09-10 at 11:53 +0200, Jan Beulich wrote: > > > > > On 02.09.2024 19:01, Oleksii Kurochko wrote: > > > > > > @@ -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 > > (?). > > What C version it requires doesn't matter much for our purposes. The > question is as of which gcc / clang version (if any) this is > supported > as an extension. > > > __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))); \ > > }) > > For our purposes __auto_type doesn't provide advantages over > typeof(). > Plus, more importantly, the use above is wrong, just like typeof(x) > would also be wrong. It needs to be p that the type is derived from. > Otherwise consider what happens when ptr is unsigned long * or > unsigned short * and you write > > write_atomic(ptr, 0); > > > And another option could be just drop volatile by casting: > > #define write_atomic(p, x) \ > > ... > > _write_atomic(p, (const void *)&x_, > > sizeof(*(p))); > > See what I said earlier about casts: You shall not cast away > qualifiers. Besides doing so being bad practice, you'll notice the > latest when RISC-V code also becomes subject to Misra compliance. Then probably the best one option will be to use union: #define write_atomic(p, x) \ ({ \ union { typeof(*(p)) val; char c[sizeof(*(p))]; } x_ = { .val = (x) }; \ _write_atomic(p, x_.c, sizeof(*(p))); \ }) ~ Oleksii
On Thu, 2024-09-12 at 13:15 +0200, oleksii.kurochko@gmail.com wrote: > On Wed, 2024-09-11 at 13:49 +0200, Jan Beulich wrote: > > On 11.09.2024 13:34, oleksii.kurochko@gmail.com wrote: > > > On Tue, 2024-09-10 at 18:05 +0200, Jan Beulich wrote: > > > > On 10.09.2024 17:28, oleksii.kurochko@gmail.com wrote: > > > > > On Tue, 2024-09-10 at 11:53 +0200, Jan Beulich wrote: > > > > > > On 02.09.2024 19:01, Oleksii Kurochko wrote: > > > > > > > @@ -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 > > > (?). > > > > What C version it requires doesn't matter much for our purposes. > > The > > question is as of which gcc / clang version (if any) this is > > supported > > as an extension. > > > > > __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))); \ > > > }) > > > > For our purposes __auto_type doesn't provide advantages over > > typeof(). > > Plus, more importantly, the use above is wrong, just like typeof(x) > > would also be wrong. It needs to be p that the type is derived > > from. > > Otherwise consider what happens when ptr is unsigned long * or > > unsigned short * and you write > > > > write_atomic(ptr, 0); > > > > > And another option could be just drop volatile by casting: > > > #define write_atomic(p, x) \ > > > ... > > > _write_atomic(p, (const void *)&x_, > > > sizeof(*(p))); > > > > See what I said earlier about casts: You shall not cast away > > qualifiers. Besides doing so being bad practice, you'll notice the > > latest when RISC-V code also becomes subject to Misra compliance. > > Then probably the best one option will be to use union: > #define write_atomic(p, x) > \ > ({ > \ > union { typeof(*(p)) val; char c[sizeof(*(p))]; } x_ = { .val > = > (x) }; \ > _write_atomic(p, x_.c, sizeof(*(p))); > \ > }) Or maybe we can use 'unsigned long' instead of char c[] and then the casts inside _write_atomic() could be dropped as we can start to use _write_atomic(..., const unsigned long x, ...). But then probably it will be good to init: x_.c = 0UL to be sure that when type of val is uint8_t for example then the significant bytes of 'union {...; unsigned long c}' are 0. ~ Oleksii
diff --git a/xen/arch/riscv/include/asm/atomic.h b/xen/arch/riscv/include/asm/atomic.h index 3c6bd86406..92b92fb4d4 100644 --- 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, 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; + 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; #endif default: __bad_atomic_size(); break; } @@ -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
Update the 2nd argument of _write_atomic() from 'unsigned long x' to 'void *x' to allow write_atomic() to handle non-scalar types, aligning it with read_atomic(), which can work with non-scalar types. Additionally, update the implementation of _add_sized() to use "writeX_cpu(readX_cpu(p) + x, p)" instead of "write_atomic(ptr, read_atomic(ptr) + x)" because 'ptr' is defined as 'volatile uintX_t *'. This avoids a compilation error that occurs when passing the 2nd argument to _write_atomic() (i.e., "passing argument 2 of '_write_atomic' discards 'volatile' qualifier from pointer target type") since the 2nd argument of _write_atomic() is now 'void *' instead of 'unsigned long'. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in v6: - new patch. --- xen/arch/riscv/include/asm/atomic.h | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)