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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/21/26 4:56 PM, Baptiste Le Duc wrote:
> > [ ... 75 lines skipped ... ]
> > +/*
> > + * Read machine word from guest memory
> > + *
> > + * @guest_addr: Guest address to read
> > + * @read_insn: Flag representing whether we are reading instruction
> > + * @trap: Output pointer to trap details if something went wrong during read.
> > + *        It must be zero-initialised by the caller: it is written only when
> > + *        an access faults, so trap->scause == 0 on return is what tells the
> > + *        caller that the read succeeded.
> > + *
> > + * The hlv/hlvx instructions translate guest_addr through the live
> > + * vsatp/hgatp CSRs, so the read is only meaningful for the address
> > + * space of the currently running vCPU. It must therefore only be called
> > + * while handling a trap taken from that vCPU, never from a nested trap
> > + * taken in HS-mode, which may have interrupted Xen at a point where
> > + * hstatus, vsatp or hgatp don't belong to the vCPU.
> > + *
> > + * At most two halfwords are fetched when @read_insn is true, i.e. encodings
> > + * wider than 32 bits are not supported. Such an encoding cannot be completed
> > + * by calling this function again at @guest_addr + 4: the length check is
> > + * applied to the first halfword read, which would then be a continuation of
> > + * the instruction rather than its opcode. It is up to the caller to reject
> > + * anything that is neither a 16- nor a 32-bit encoding.
> > + */
> Shouldn't docstring belong to header file?
I don't know if we have such rule but I think that I will move this to 
header to be in sync with other functions in asm/traps.h.
But in the matter of taste it seems to me that having such explanation 
in C code is more useful as it at some point explains a code.
> > +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"
> > +            "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"
> Could you add a short comment explaining that the andi/addi/bnez sequence
> checks the low two bits of the first halfword to tell a compressed
> instruction from a 32-bit one? It is not obvious on first read.
> Something like:
> ```
>      /*
>       * bits[1:0] != 0b11: 16-bit (compressed) instruction, nothing more
>       * to read.
>       */
> ```
I thought that the last paragraph of the comment above read_guest() 
explains it but I am okay to add the suggested small comment. I will put 
it immediately after ASM_EXTABLE:
            ASM_EXTABLE_TRAP_INFO(1b, 3f, %[ti])
            /*
             * bits[1:0] != 0b11: 16-bit (compressed) instruction, nothing
             * more to read.
             */
            "   andi %[tmp], %[val], 3\n"

Thanks!

~ Oleksii