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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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.kurochko@xxxxxxxxx>
> As I'm happy to see the duplication go away:
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
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