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

[Jan Beulich <jbeulich@xxxxxxxx>]

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)

>>> 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