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

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