Re: [PATCH v3 21/39] xen/riscv: add guest memory read helper

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/4/26 2:28 PM, Jan Beulich wrote:
> On 04.11.2026 13:39, Oleksii Kurochko wrote:
> > On 10/21/26 5:11 PM, Baptiste Le Duc wrote:
> > > Sorry I forgot one thing.> +unsigned long read_guest(unsigned long
> > > guest_addr, bool read_insn,
> > > > +                         struct trap_info *trap)
> > > > +{
> > > > +    /*
> > > > +     * Poison the result: if the very first access faults, the fixup
> > > > skips
> > > > +     * over the loads without writing it. Callers must check trap-
> > > > > scause.
> > > > +     */
> > > > +    unsigned long val = ~0UL, tmp;
> > > > +
> > > > +    /*
> > > > +     * hlv/hlvx use hstatus.SPVP for the privilege of the access, and
> > > > the
> > > > +     * live vsatp/hgatp for the translation. Xen never installs a
> > > > value of
> > > > +     * its own in hstatus (it is only saved on trap entry and restored
> > > > +     * before sret) and it doesn't reschedule before returning to the
> > > > +     * guest, so all three still belong to the vCPU which trapped.
> > > > +     */
> > > > +    if ( read_insn )
> > > > +    {
> > > > +        asm volatile ( "\n"
> > > I think this would need asm_inline instead...> +            "1: hlvx.hu
> > > %[val], (%[addr])\n"
> > > > +            ASM_EXTABLE_TRAP_INFO(1b, 3f, %[ti])
> > > > +            "   andi %[tmp], %[val], 3\n"
> > > > +            "   addi %[tmp], %[tmp], -3\n"
> > > > +            "   bnez %[tmp], 3f\n"
> > > > +            "   addi %[addr], %[addr], 2\n"
> > > > +            "\n"
> > > > +            "2: hlvx.hu %[tmp], (%[addr])\n"
> > > > +            ASM_EXTABLE_TRAP_INFO(2b, 3f, %[ti])
> > > > +            "   slli %[tmp], %[tmp], 16\n"
> > > > +            "   or %[val], %[val], %[tmp]\n"
> > > > +            "3:\n"
> > > > +        : [val] "+&r" (val), [tmp] "=&r" (tmp), [addr] "+&r"
> > > > (guest_addr),
> > > > +          "+m" (*trap)
> > > > +        : [ti] "r" (trap) );
> > > > +
> > > > +        /*
> > > > +         * Although HLVX instructions' explicit memory accesses
> > > > require execute
> > > > +         * permissions, they still raise the same exceptions as other
> > > > load
> > > > +         * instructions, rather than raising fetch exceptions instead.
> > > > +         */
> > > > +        if ( trap->scause == CAUSE_LOAD_PAGE_FAULT )
> > > > +            trap->scause = CAUSE_FETCH_PAGE_FAULT;
> > > > +    }
> > > > +    else
> > > > +    {
> > > > +        asm volatile ( "\n"
> > > ... same for here.
> > read_guest() is an out-of-line function with its only caller in another
> > translation unit, so asm_inline wouldn't change any inlining decision here.
> >
> > volatile is still needed, as the asm may trap and reads memory which can
> > change behind the compiler's back.
> Yet that's not expressed by volatile, but by a memory clobber.
Agree, this was incorrect justification of volatile. Volatile still is 
needed I think as suppose the calling code looks like this:
  read_guest(guest_addr, false, trap);
In other words, the return value is ignored.

Theoretically, if volatile is removed, GCC may determine that:
 val is not used;
 tmp is not used;
 guest_addr is not used after the asm block;
 trap is not accessed after the function call.
If the compiler can also prove that modifications to *trap are not 
observable, it may eliminate the entire asm block. So to avoid that 
volatile will be better to keep here.
Memory clobber still seems like isn't needed here as the only Xen object 
written behind its back, by the fixup path, is *trap, which is already 
an "+m" operand.
~ Oleksii