Re: [PATCH v5 12/23] xen/riscv: introduce io.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Fri, 2024-03-08 at 12:52 +0100, Jan Beulich wrote:
> On 08.03.2024 12:49, Jan Beulich wrote:
> > On 08.03.2024 11:14, Oleksii wrote:
> > > On Fri, 2024-03-08 at 08:26 +0100, Jan Beulich wrote:
> > > > On 07.03.2024 21:54, Oleksii wrote:
> > > > > On Thu, 2024-03-07 at 21:49 +0100, Oleksii wrote:
> > > > > > On Thu, 2024-03-07 at 18:14 +0100, Jan Beulich wrote:
> > > > > > > For plain writes it should at least be "=Qo" then, yes.
> > > > > > Constraints Q is a machine specific constraint, and I am
> > > > > > not sure
> > > > > > that
> > > > > > it makes sense to use "=o" only and probably it is a reason
> > > > > > why
> > > > > > it is
> > > > > > enough only "r". Does it make sense?
> > > > > Probably for RISC-V can be used:
> > > > > RISC-V—config/riscv/constraints.md
> > > > >    ...
> > > > >    A
> > > > >        An address that is held in a general-purpose register.
> > > > >    ...
> > > > 
> > > > Just from the description I would have said no, but looking at
> > > > what
> > > > "A"
> > > > actually expands to it is indeed RISC-V's counterpart of Arm's
> > > > "Q".
> > > > So
> > > > yes, this looks like what amo* want to use, and then as a real
> > > > operand,
> > > > not just a fake one.
> > > I am not sure that I know how to check correctly how "A" expands,
> > > but I
> > > tried to look at code which will be generated with and without
> > > constraints and it is the same:
> > 
> > As expected.
But if it is epxected and generated code is the same, do we really need
constraints then?

> > 
> > >    // static inline void __raw_writel(uint32_t val, volatile void
> > >    __iomem *addr)
> > >    // {
> > >    //     asm volatile ( "sw %0, 0(%1)" : : "r" (val), "r"(addr)
> > > );
> > >    // }
> > >    
> > >    static inline void __raw_writel(uint32_t val, volatile void
> > > __iomem
> > >    *addr)
> > >    {
> > >        asm volatile ( "sw %0, %1" : : "r" (val), "Ao" (*(volatile
> > >    uint32_t __force *)addr) );
> > 
> > You want just "A" here though; adding an offset (as "o" permits)
> > would
> > yield an insn which the assembler would reject.
> 
> Wait - this is plain SW, so can't it even be the more generic "m"
> then?
> (As said, I'm uncertain about "o"; in general I think it's risky to
> use.)
What do you mean by "plain SW"?

Are you suggesting changing 'm' to 'o' so that the final result will be
"Am"? Based on the descriptions of 'A' and 'm', it seems to me that we
can just use 'A' alone because both constraints indicate that the
operand is in memory, and 'A' specifically denotes that an address is
held in a register.
> 
> 
> > Also just to remind
> > you: In write functions you need "=A" (and in amo ones "+A"), i.e.
> > the
> > memory operand then needs to be an output, not an input.
Could you please clarify about which one amo you are speaking? That one
who are defined by ATOMIC_OP and ATOMIC_FETCH_OP? They are already
using +A constraints:
    __asm__ __volatile__ (                                          \
        "   amo" #asm_op "." #asm_type " %1, %2, %0"                \
        : "+A" (v->counter), "=r" (ret)                             \
        : "r" (I)                                                   \
        : "memory" );                                               \

~ Oleksii