Re: [PATCH v5 1/7] xen/riscv: use {read,write}{b,w,l,q}_cpu() to define {read,write}_atomic()

[oleksii.kurochko@xxxxxxxxx]

On Wed, 2024-08-28 at 11:42 +0200, Jan Beulich wrote:
> On 28.08.2024 11:21, oleksii.kurochko@xxxxxxxxx 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