Re: [PATCH v1 13/17] xen/riscv: add unprivileged guest memory read helper

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 8/18/26 12:43 PM, Jan Beulich wrote:
> On 18.08.2026 12:27, Oleksii Kurochko wrote:
> > On 8/18/26 10:17 AM, Jan Beulich wrote:
> > > On 17.08.2026 17:36, Oleksii Kurochko wrote:
> > > > On 8/12/26 5:30 PM, Jan Beulich wrote:
> > > > > On 20.07.2026 18:02, Oleksii Kurochko wrote:
> > > > > > +    if ( read_insn )
> > > > > > +    {
> > > > > > +        asm volatile ( "\n"
> > > > > > +            "1:\n"
> > > > > > +            "   hlvx.hu %[val], (%[addr])\n"
> > > > > > +            ASM_EXTABLE_TRAP_INFO(1b, 3f, %[ti])
> > > > > > +            "   andi %[tmp], %[val], 3\n"
> > > > > > +            "   addi %[tmp], %[tmp], -3\n"
> > > > > > +            "   bne %[tmp], zero, 3f\n"
> > > > > > +            "   addi %[addr], %[addr], 2\n"
> > > > > > +            "\n"
> > > > > > +            "2:\n"
> > > > > > +            "   hlvx.hu %[tmp], (%[addr])\n"
> > > > > > +            ASM_EXTABLE_TRAP_INFO(2b, 3f, %[ti])
> > > > > > +            "   sll %[tmp], %[tmp], 16\n"
> > > > > > +            "   add %[val], %[val], %[tmp]\n"
> > > > > > +            "3:\n"
> > > > > > +        : [val] "=&r" (val), [tmp] "=&r" (tmp), [addr] "+&r" (guest_addr)
> > > > > > +        : [ti] "r" (trap) : "memory" );
> > > > > You want to tell the compiler that *trap is written. Instead I don't see
> > > > > why a memory clobber would be needed: You access a different address space,
> > > > > i.e. nothing the compiler can make any assumptions about.
> > > > memory clobber tells the compiler that the assembly code performs memory
> > > > reads or writes to items other than those listed in the input and output
> > > > operands and so I don't tell here that *trap will be changed.
> > > >
> > > > Why this understanding is wrong?
> > > You can (ab)use "memory" for that purpose, but why would you when you can
> > > properly express the operand? All that achieves is the compiler possibly
> > > having to emit less efficient code.
> > Then I will use the option mentioned ...
> >
> > > > Alternative, I think, could be:
> > > >            : [val] "+r" (val), "+m" (*trap)
> > > >            : [addr] "r" (guest_addr), [ti] "r" (trap) );
> > > > And then memory clobber could be dropped.
> > ... here.
> >
> > Probably I have to return '[addr] "r" (guest_addr)' to output and use
> > +&r constraint.
> >
> > > > > You also need to take precautions for not returning an uninitialized "val".
> > > > > I think the variable wants initializing (perhaps to ~0) and "+r" wants
> > > > > using as constraint. (Afaik & isn't necessary to use together with +.)
> > > > I agree with '+' if we will initialize val with some value.
> > > >
> > > > Regarding, '&' my understanding is that I have to use it always when
> > > When what exactly? If an operand is both input and output, how could the
> > > compiler re-use the (generally) register for any further purpose? '&'
> > > indicates to the compiler that it may not use the register used for an
> > > output to hold some input's value, as that value may be lost by the time
> > > the input is actually consumed.
> > But what is written in the gcc doc:
> >
> > & - Means (in a particular alternative) that this operand is an
> > earlyclobber operand, which is written before the instruction is
> > finished using the input operands.
> >
> > What sounds like if an operand (val) in our case is written before the
> > instruction which using the input operands (and after the write
> > instuction which writes val there are instructions which are using input
> > operands) it is needed to have &.
> And that's indeed relevant, just not here. My crucial earlier question was:
> "If an operand is both input and output, how could the compiler re-use the
> (generally) register for any further purpose?" There is a case where the
> answer to this is not "it can't". In your case all inputs are distinct; in
> e.g. (using x86 assembly, sorry):
>
> int test(int i, int j) {
>         asm("nop %0; nop %1" : "+r" (i) : "r" (i));
>         asm("cmc; nop %0; nop %1" : "+&r" (j) : "r" (j));
>
>         return i + j;
> }
>
> using "+&r" indeed makes a difference.
I think it is clear when operands are equal. but what about the case 
when they are different in  first  case:
...
  asm("nop %0; nop %1" : "+r" (i) : "r" (j));
...

What guarantees that i and j will be in different registers?

We have the similar situation in RISC-V code of riscv_vcpu_unpriv_read():

        : [val] "+r" (val), [tmp] "=&r" (tmp), [addr] "+r" (guest_addr),
          "+m" (*trap)
        : [ti] "r" (trap) );

With having & for val and guest_addr & guarantees that the same 
registers won't be re-used for ti (and so ti won't be corrupted) but 
without it?
~ Oleksii

> > t1:  hlvx.hu %[val], (%[addr])   W:val R:addr + can trap -> read register ti
> >
> > t2:  andi    %[tmp], %[val], 3   W:tmp   R:val
> > t3:  addi    %[tmp], %[tmp], -3
> > t4:  bnez    %[tmp], 3f
> > t5:  addi    %[addr], %[addr], 2 W:addr  R:addr
> > t6:  hlvx.hu %[tmp], (%[addr])   W:tmp   R:addr + can trap -> read
> > register ti
> >
> > t7:  slli    %[tmp], %[tmp], 16
> > t8:  or      %[val], %[val], %[tmp] W:val
> >
> > So val is written on t1 before t6 where addr and ti still alive.
> >
> > The similar is for [addr] "+&r" (guest_addr):
> >
> > addr is written on t5 and input ti is alive till t6. So if allocator
> > will allocate the same register for addr and ti then addi %[addr],
> > %[addr], 2 will break a pointer and handler will get something wrong.
> >
> > Am I missing something?
> >
> > If I am still wrong then in both cases should be just "+r"?
> As per above, if you want to play absolutely by the rules, use "+&r",
> even if that's unnecessary here.
>
> Jan