Re: [PATCH v2 02/39] xen/riscv: drop bug.h's duplicate instruction length helpers

[Oleksii Kurochko]

On 9/1/26 9:01 AM, Jan Beulich wrote:
On 27.08.2026 17:20, Oleksii Kurochko wrote:
asm/riscv_encoding.h already provides INSN_16BIT_MASK and INSN_LEN(), and
emulate.c uses them, so the tree carried two spellings of the same thing
which could drift apart. COMPRESSED_INSN_MASK never had a user.

No functional change.

Signed-off-by: Oleksii Kurochko <oleksii.kuroc...@gmail.com>

As I'm happy to see the duplication go away:
Acked-by: Jan Beulich <jbeul...@suse.com>

Thanks.

However, ...

--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -214,7 +214,7 @@ void do_trap(struct cpu_user_regs *cpu_regs)
                  die();
              }
  
-            cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
+            cpu_regs->sepc += INSN_LEN(*(uint16_t *)pc);

... for one I'd prefer if we took the opportunity and add "const" to the
pointer target here.

Thanks for adding 'const' during commit.


And then

#define INSN_16BIT_MASK                 0x3
#define INSN_32BIT_MASK                 0x1c

are really named backwards, seeing e.g. their use in

It was derived from OpenSBI project and I haven't paid enough attention 
for these defines.

Now looking at them again I fully agree that names here not really 
correct. It should be according to the spec:
   #define INSN_32BIT_MASK          0x3
   #define INSN_48BIT_MASK          0x1c

Then ...


#define INSN_IS_16BIT(insn)             \
        (((insn) & INSN_16BIT_MASK) != INSN_16BIT_MASK)
#define INSN_IS_32BIT(insn)             \
        (((insn) & INSN_16BIT_MASK) == INSN_16BIT_MASK && \
         ((insn) & INSN_32BIT_MASK) != INSN_32BIT_MASK)

...

#define INSN_IS_16BIT(insn)      \
    (((insn) & INSN_32BIT_MASK) != INSN_32BIT_MASK)

#define INSN_IS_32BIT(insn)      \
    ((((insn) & INSN_32BIT_MASK) == INSN_32BIT_MASK) && \
     (((insn) & INSN_48BIT_MASK) != INSN_48BIT_MASK))


Furthermore,

#define INSN_LEN(insn)                  (INSN_IS_16BIT(insn) ? 2 : 4)

fails to use INSN_32BIT_MASK / INSN_IS_32BIT() altogether.

Then INSN_LEN will be redefined as:

/*
 * Length in bytes of the instruction whose first parcel is @insn.  Callers
 * must have established that the encoding is 16- or 32-bit wide; the ISA
 * defines no instruction wider than that, and the wider encodings are
 * reserved.
 */
#define INSN_LEN(insn) \
    (INSN_IS_16BIT(insn) ? 2 : (INSN_IS_32BIT(insn) ? 4 : 0))

I am thinking if it makes sense to have here the generic way to 
calculate INSN_LEN. Something like:

/*
 * Length in bytes of the instruction whose first 16-bit parcel is 
@insn, or
 * zero for the encoding reserved for instructions of 192 bits or more, 
whose
 * length the ISA leaves unspecified.  See "Expanded Instruction-Length
 * Encoding" in the unprivileged spec; the first parcel holds all of the
 * information needed.
 */
static inline unsigned int INSN_LEN(uint16_t insn)
{
    unsigned int nnn;

    if ( INSN_IS_16BIT(insn) )
        return 2;

    if ( INSN_IS_32BIT(insn) )
        return 4;

    if ( INSN_IS_48BIT(insn) )
        return 6;

    if ( INSN_IS_64BIT(insn) )
        return 8;

    /* xnnnxxxxx1111111 with nnn != 111 is (80 + 16 * nnn) bits wide. */
    nnn = MASK_EXTR(insn, INSN_NNN_MASK);

    return (nnn == 7) ? 0 : 10 + 2 * nnn;
}

I think that for now it is enough to go without common implmenntation of 
INSN_LEN and just return 0 as suggested above.

If you are okay with suggested changes I will send a separate patch for 
them.

~ Oleksii