| Message ID | 5140f9eb3d1cb0b69e3b1cbbcce6167ff8d59e4c.1724256026.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISCV device tree mapping | expand |
On 21.08.2024 18:06, Oleksii Kurochko wrote: > In Xen, memory-ordered atomic operations are not necessary, This is an interesting statement. I'd like to suggest that you at least limit it to the two constructs in question, rather than stating this globally for everything. > based on {read,write}_atomic() implementations for other architectures. > Therefore, {read,write}{b,w,l,q}_cpu() can be used instead of > {read,write}{b,w,l,q}(), allowing the caller to decide if additional > fences should be applied before or after {read,write}_atomic(). > > Change the declaration of _write_atomic() to accept a 'volatile void *' > type for the 'x' argument instead of 'unsigned long'. > This prevents compilation errors such as: > 1."discards 'volatile' qualifier from pointer target type," which occurs > due to the initialization of a volatile pointer, > e.g., `volatile uint8_t *ptr = p;` in _add_sized(). I don't follow you here. It's the other argument of write_atomic() that has ptr passed there. > 2."incompatible type for argument 2 of '_write_atomic'," which can occur > when calling write_pte(), where 'x' is of type pte_t rather than > unsigned long. How's this related to the change at hand? That isn't different ahead of this change, is it? > --- a/xen/arch/riscv/include/asm/atomic.h > +++ b/xen/arch/riscv/include/asm/atomic.h > @@ -31,21 +31,17 @@ > > void __bad_atomic_size(void); > > -/* > - * Legacy from Linux kernel. For some reason they wanted to have ordered > - * read/write access. Thereby read* is used instead of read*_cpu() > - */ > static always_inline void read_atomic_size(const volatile void *p, > void *res, > unsigned int size) > { > switch ( size ) > { > - case 1: *(uint8_t *)res = readb(p); break; > - case 2: *(uint16_t *)res = readw(p); break; > - case 4: *(uint32_t *)res = readl(p); break; > + case 1: *(uint8_t *)res = readb_cpu(p); break; > + case 2: *(uint16_t *)res = readw_cpu(p); break; > + case 4: *(uint32_t *)res = readl_cpu(p); break; > #ifndef CONFIG_RISCV_32 > - case 8: *(uint32_t *)res = readq(p); break; > + case 8: *(uint32_t *)res = readq_cpu(p); break; > #endif > default: __bad_atomic_size(); break; > } > @@ -58,15 +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, unsigned int size) > + volatile void *x, If this really needs to become a pointer, it ought to also be pointer- to-const. Otherwise it is yet more confusing which operand is which. > + unsigned int size) > { > switch ( size ) > { > - case 1: writeb(x, p); break; > - case 2: writew(x, p); break; > - case 4: writel(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(x, p); break; > + case 8: writeq_cpu(*(uint64_t *)x, p); break; Of course you may not cast away const-ness then. You also be casting away volatile-ness, but (as per above) I question the need for volatile on x. Jan
On Tue, 2024-08-27 at 12:06 +0200, Jan Beulich wrote: > On 21.08.2024 18:06, Oleksii Kurochko wrote: > > In Xen, memory-ordered atomic operations are not necessary, > > This is an interesting statement. I looked at the definition of build_atomic_{write,read}() for other architectures and didn't find any additional memory-ordered primitives such as fences. > I'd like to suggest that you at least > limit it to the two constructs in question, rather than stating this > globally for everything. I am not sure that I understand what is "the two constructs". Could you please clarify? > > > based on {read,write}_atomic() implementations for other > > architectures. > > Therefore, {read,write}{b,w,l,q}_cpu() can be used instead of > > {read,write}{b,w,l,q}(), allowing the caller to decide if > > additional > > fences should be applied before or after {read,write}_atomic(). > > > > Change the declaration of _write_atomic() to accept a 'volatile > > void *' > > type for the 'x' argument instead of 'unsigned long'. > > This prevents compilation errors such as: > > 1."discards 'volatile' qualifier from pointer target type," which > > occurs > > due to the initialization of a volatile pointer, > > e.g., `volatile uint8_t *ptr = p;` in _add_sized(). > > I don't follow you here. This issue started occurring after the change mentioned in point 2 below. I initially provided an incorrect explanation for the compilation error mentioned above. Let me correct that now and update the commit message in the next patch version. The reason for this error is that after the _write_atomic() prototype was updated from _write_atomic(..., unsigned long, ...) to _write_atomic(..., void *x, ...), the write_atomic() macro contains x_, which is of type 'volatile uintX_t' because ptr has the type 'volatile uintX_t *'. Therefore, _write_atomic() should have its second argument declared as volatile const void *. Alternatively, we can consider updating write_atomic() to: #define write_atomic(p, x) \ ({ \ typeof(*(p)) x_ = (x); \ _write_atomic(p, (const void *)&x_, sizeof(*(p))); \ }) Would this be a better approach?Would it be better? > It's the other argument of write_atomic() that > has ptr passed there. Probably the thing mentioned above it is what you mean here. I wasn't sure that I understand this sentence correctly. > > > 2."incompatible type for argument 2 of '_write_atomic'," which can > > occur > > when calling write_pte(), where 'x' is of type pte_t rather than > > unsigned long. > > How's this related to the change at hand? That isn't different ahead > of > this change, is it? This is not directly related to the current change, which is why I decided to add a sentence about write_pte(). Since write_pte(pte_t *p, pte_t pte) uses write_atomic(), and the argument types are pte_t * and pte respectively, we encounter a compilation issue in write_atomic() because: _write_atomic() expects the second argument to be of type unsigned long, leading to a compilation error like "incompatible type for argument 2 of '_write_atomic'." I considered defining write_pte() as write_atomic(p, pte.pte), but this would fail at 'typeof(*(p)) x_ = (x);' and result in a compilation error 'invalid initializer' or something like that. It might be better to update write_pte() to: /* Write a pagetable entry. */ static inline void write_pte(pte_t *p, pte_t pte) { write_atomic((unsigned long *)p, pte.pte); } Then, we wouldn't need to modify the definition of write_atomic() or change the type of the second argument of _write_atomic(). Would it be better? > > > --- a/xen/arch/riscv/include/asm/atomic.h > > +++ b/xen/arch/riscv/include/asm/atomic.h > > @@ -31,21 +31,17 @@ > > > > void __bad_atomic_size(void); > > > > -/* > > - * Legacy from Linux kernel. For some reason they wanted to have > > ordered > > - * read/write access. Thereby read* is used instead of read*_cpu() > > - */ > > static always_inline void read_atomic_size(const volatile void *p, > > void *res, > > unsigned int size) > > { > > switch ( size ) > > { > > - case 1: *(uint8_t *)res = readb(p); break; > > - case 2: *(uint16_t *)res = readw(p); break; > > - case 4: *(uint32_t *)res = readl(p); break; > > + case 1: *(uint8_t *)res = readb_cpu(p); break; > > + case 2: *(uint16_t *)res = readw_cpu(p); break; > > + case 4: *(uint32_t *)res = readl_cpu(p); break; > > #ifndef CONFIG_RISCV_32 > > - case 8: *(uint32_t *)res = readq(p); break; > > + case 8: *(uint32_t *)res = readq_cpu(p); break; > > #endif > > default: __bad_atomic_size(); break; > > } > > @@ -58,15 +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, unsigned > > int size) > > + volatile void *x, > > If this really needs to become a pointer, it ought to also be > pointer- > to-const. Otherwise it is yet more confusing which operand is which. Sure. I will add 'const' if the prototype of _write_atomic() won't use 'unsigned long' for x argument. > > > + unsigned int size) > > { > > switch ( size ) > > { > > - case 1: writeb(x, p); break; > > - case 2: writew(x, p); break; > > - case 4: writel(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(x, p); break; > > + case 8: writeq_cpu(*(uint64_t *)x, p); break; > > Of course you may not cast away const-ness then. You also be casting > away volatile-ness, but (as per above) I question the need for > volatile > on x. I added an explanation about this earlier in the message. Let's discuss whether volatile is needed there or not. If I should not cast away the const and volatile qualifiers, then I need to update the prototypes of writeX_cpu()? ~ Oleksii
On 28.08.2024 11:21, oleksii.kurochko@gmail.com wrote: > On Tue, 2024-08-27 at 12:06 +0200, Jan Beulich wrote: >> On 21.08.2024 18:06, Oleksii Kurochko wrote: >>> In Xen, memory-ordered atomic operations are not necessary, >> >> This is an interesting statement. > I looked at the definition of build_atomic_{write,read}() for other > architectures and didn't find any additional memory-ordered primitives > such as fences. > >> I'd like to suggest that you at least >> limit it to the two constructs in question, rather than stating this >> globally for everything. > I am not sure that I understand what is "the two constructs". Could you > please clarify? {read,write}_atomic() (the statement in your description is, after all, not obviously limited to just those two, yet I understand you mean to say what you say only for them) >>> based on {read,write}_atomic() implementations for other >>> architectures. >>> Therefore, {read,write}{b,w,l,q}_cpu() can be used instead of >>> {read,write}{b,w,l,q}(), allowing the caller to decide if >>> additional >>> fences should be applied before or after {read,write}_atomic(). >>> >>> Change the declaration of _write_atomic() to accept a 'volatile >>> void *' >>> type for the 'x' argument instead of 'unsigned long'. >>> This prevents compilation errors such as: >>> 1."discards 'volatile' qualifier from pointer target type," which >>> occurs >>> due to the initialization of a volatile pointer, >>> e.g., `volatile uint8_t *ptr = p;` in _add_sized(). >> >> I don't follow you here. > This issue started occurring after the change mentioned in point 2 > below. > > I initially provided an incorrect explanation for the compilation error > mentioned above. Let me correct that now and update the commit message > in the next patch version. The reason for this error is that after the > _write_atomic() prototype was updated from _write_atomic(..., unsigned > long, ...) to _write_atomic(..., void *x, ...), the write_atomic() > macro contains x_, which is of type 'volatile uintX_t' because ptr has > the type 'volatile uintX_t *'. While there's no "ptr" in write_atomic(), I think I see what you mean. Yet at the same time Arm - having a similar construct - gets away without volatile. Perhaps this wants modelling after read_atomic() then, using a union? > Therefore, _write_atomic() should have its second argument declared as > volatile const void *. Alternatively, we can consider updating > write_atomic() to: > #define write_atomic(p, x) \ > ({ \ > typeof(*(p)) x_ = (x); \ > _write_atomic(p, (const void *)&x_, sizeof(*(p))); \ > }) > Would this be a better approach?Would it be better? Like const you also should avoid to cast away volatile, whenever possible. >>> 2."incompatible type for argument 2 of '_write_atomic'," which can >>> occur >>> when calling write_pte(), where 'x' is of type pte_t rather than >>> unsigned long. >> >> How's this related to the change at hand? That isn't different ahead >> of >> this change, is it? > This is not directly related to the current change, which is why I > decided to add a sentence about write_pte(). > > Since write_pte(pte_t *p, pte_t pte) uses write_atomic(), and the > argument types are pte_t * and pte respectively, we encounter a > compilation issue in write_atomic() because: > > _write_atomic() expects the second argument to be of type unsigned > long, leading to a compilation error like "incompatible type for > argument 2 of '_write_atomic'." > I considered defining write_pte() as write_atomic(p, pte.pte), but this > would fail at 'typeof(*(p)) x_ = (x);' and result in a compilation > error 'invalid initializer' or something like that. > > It might be better to update write_pte() to: > /* Write a pagetable entry. */ > static inline void write_pte(pte_t *p, pte_t pte) > { > write_atomic((unsigned long *)p, pte.pte); > } > Then, we wouldn't need to modify the definition of write_atomic() or > change the type of the second argument of _write_atomic(). > Would it be better? As said numerous times before: Whenever you can get away without a cast, you should avoid the cast. Here: static inline void write_pte(pte_t *p, pte_t pte) { write_atomic(&p->pte, pte.pte); } That's one of the possible options, yes. Yet, like Arm has it, you may actually want the capability to read/write non-scalar types. If so, adjustments to write_atomic() are necessary, yet as indicated before: Please keep such entirely independent changes separate. Jan
On Wed, 2024-08-28 at 11:42 +0200, Jan Beulich wrote: > On 28.08.2024 11:21, oleksii.kurochko@gmail.com wrote: > > On Tue, 2024-08-27 at 12:06 +0200, Jan Beulich wrote: > > > On 21.08.2024 18:06, Oleksii Kurochko wrote: > > > > In Xen, memory-ordered atomic operations are not necessary, > > > > > > This is an interesting statement. > > I looked at the definition of build_atomic_{write,read}() for other > > architectures and didn't find any additional memory-ordered > > primitives > > such as fences. > > > > > I'd like to suggest that you at least > > > limit it to the two constructs in question, rather than stating > > > this > > > globally for everything. > > I am not sure that I understand what is "the two constructs". Could > > you > > please clarify? > > {read,write}_atomic() (the statement in your description is, after > all, > not obviously limited to just those two, yet I understand you mean to > say what you say only for them) Yeah, I re-read commit message after your reply and now I can see that is not really clear. > > > > > based on {read,write}_atomic() implementations for other > > > > architectures. > > > > Therefore, {read,write}{b,w,l,q}_cpu() can be used instead of > > > > {read,write}{b,w,l,q}(), allowing the caller to decide if > > > > additional > > > > fences should be applied before or after {read,write}_atomic(). > > > > > > > > Change the declaration of _write_atomic() to accept a 'volatile > > > > void *' > > > > type for the 'x' argument instead of 'unsigned long'. > > > > This prevents compilation errors such as: > > > > 1."discards 'volatile' qualifier from pointer target type," > > > > which > > > > occurs > > > > due to the initialization of a volatile pointer, > > > > e.g., `volatile uint8_t *ptr = p;` in _add_sized(). > > > > > > I don't follow you here. > > This issue started occurring after the change mentioned in point 2 > > below. > > > > I initially provided an incorrect explanation for the compilation > > error > > mentioned above. Let me correct that now and update the commit > > message > > in the next patch version. The reason for this error is that after > > the > > _write_atomic() prototype was updated from _write_atomic(..., > > unsigned > > long, ...) to _write_atomic(..., void *x, ...), the write_atomic() > > macro contains x_, which is of type 'volatile uintX_t' because ptr > > has > > the type 'volatile uintX_t *'. > > While there's no "ptr" in write_atomic(), I think I see what you > mean. Yet > at the same time Arm - having a similar construct - gets away without > volatile. Perhaps this wants modelling after read_atomic() then, > using a > union? The use of a union could be considered as a solution. For now, I think I will just update write_pte() to avoid this issue and and minimize changes in this patch. > > > > > 2."incompatible type for argument 2 of '_write_atomic'," which > > > > can > > > > occur > > > > when calling write_pte(), where 'x' is of type pte_t rather > > > > than > > > > unsigned long. > > > > > > How's this related to the change at hand? That isn't different > > > ahead > > > of > > > this change, is it? > > This is not directly related to the current change, which is why I > > decided to add a sentence about write_pte(). > > > > Since write_pte(pte_t *p, pte_t pte) uses write_atomic(), and the > > argument types are pte_t * and pte respectively, we encounter a > > compilation issue in write_atomic() because: > > > > _write_atomic() expects the second argument to be of type unsigned > > long, leading to a compilation error like "incompatible type for > > argument 2 of '_write_atomic'." > > I considered defining write_pte() as write_atomic(p, pte.pte), but > > this > > would fail at 'typeof(*(p)) x_ = (x);' and result in a compilation > > error 'invalid initializer' or something like that. > > > > It might be better to update write_pte() to: > > /* Write a pagetable entry. */ > > static inline void write_pte(pte_t *p, pte_t pte) > > { > > write_atomic((unsigned long *)p, pte.pte); > > } > > Then, we wouldn't need to modify the definition of write_atomic() > > or > > change the type of the second argument of _write_atomic(). > > Would it be better? > > As said numerous times before: Whenever you can get away without a > cast, > you should avoid the cast. Here: > > static inline void write_pte(pte_t *p, pte_t pte) > { > write_atomic(&p->pte, pte.pte); > } > > That's one of the possible options, yes. Yet, like Arm has it, you > may > actually want the capability to read/write non-scalar types. If so, > adjustments to write_atomic() are necessary, yet as indicated before: > Please keep such entirely independent changes separate. I quickly checked that there is only one instance where write_atomic() is used for a scalar type in the Arm code. I think it would be better to update RISC-V's write_pte() and not modify write_atomic(), at least for now. Thanks. ~ Oleksii
diff --git a/xen/arch/riscv/include/asm/atomic.h b/xen/arch/riscv/include/asm/atomic.h index 31b91a79c8..446c8c7928 100644 --- a/xen/arch/riscv/include/asm/atomic.h +++ b/xen/arch/riscv/include/asm/atomic.h @@ -31,21 +31,17 @@ void __bad_atomic_size(void); -/* - * Legacy from Linux kernel. For some reason they wanted to have ordered - * read/write access. Thereby read* is used instead of read*_cpu() - */ static always_inline void read_atomic_size(const volatile void *p, void *res, unsigned int size) { switch ( size ) { - case 1: *(uint8_t *)res = readb(p); break; - case 2: *(uint16_t *)res = readw(p); break; - case 4: *(uint32_t *)res = readl(p); break; + case 1: *(uint8_t *)res = readb_cpu(p); break; + case 2: *(uint16_t *)res = readw_cpu(p); break; + case 4: *(uint32_t *)res = readl_cpu(p); break; #ifndef CONFIG_RISCV_32 - case 8: *(uint32_t *)res = readq(p); break; + case 8: *(uint32_t *)res = readq_cpu(p); break; #endif default: __bad_atomic_size(); break; } @@ -58,15 +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, unsigned int size) + volatile void *x, + unsigned int size) { switch ( size ) { - case 1: writeb(x, p); break; - case 2: writew(x, p); break; - case 4: writel(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(x, p); break; + case 8: writeq_cpu(*(uint64_t *)x, p); break; #endif default: __bad_atomic_size(); break; } @@ -75,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,
In Xen, memory-ordered atomic operations are not necessary, based on {read,write}_atomic() implementations for other architectures. Therefore, {read,write}{b,w,l,q}_cpu() can be used instead of {read,write}{b,w,l,q}(), allowing the caller to decide if additional fences should be applied before or after {read,write}_atomic(). Change the declaration of _write_atomic() to accept a 'volatile void *' type for the 'x' argument instead of 'unsigned long'. This prevents compilation errors such as: 1."discards 'volatile' qualifier from pointer target type," which occurs due to the initialization of a volatile pointer, e.g., `volatile uint8_t *ptr = p;` in _add_sized(). 2."incompatible type for argument 2 of '_write_atomic'," which can occur when calling write_pte(), where 'x' is of type pte_t rather than unsigned long. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in v5: - new patch. --- xen/arch/riscv/include/asm/atomic.h | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)