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

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

> [ ... 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?
> +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.
     */
```

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>