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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/2/26 3:02 PM, Jan Beulich wrote:
> On 02.09.2026 12:48, Oleksii Kurochko wrote:
> > On 9/1/26 9:01 AM, Jan Beulich wrote:
> > > 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
> Especially the latter would then still be misnamed, as the mask merely
> identifies insns >= 48 bits. Even for the former I'd still question the
> name to some degree ("mask" doesn't mean all of the bits need to be set).
>
> In how many places are you going to need these constants? If, by suitably
> using helpers, it's just one - maybe better to get away without any named
> constants in this case (i.e. when suitable names are apparently difficult
> to come up with)?
INSN_16BIT_MASK - will be used only once (except INSN_IS_32BIT() and 
INSN_IS_16BIT()) in insn_fetch_faulted() introduced later in this patch 
series:
        di->insn = htinst | INSN_16BIT_MASK;

INSN_32BIT_MASK - is used only inside INSN_IS_32BIT().

And INSN_IS_32BIT() and INSN_IS_16BIT() are using in several places.

I am okay to drop these masks and then just have a comment:

/*
 * Instruction length encoding helpers (see RISC-V Unprivileged ISA,
 * Section "Base Instruction-Length Encoding").
 *
 * - Instructions with bits [1:0] != 11 are 16-bit (compressed).
 * - Instructions with bits [1:0] == 11 and bits [4:2] != 111 are 32-bit.
 */
#define INSN_IS_16BIT(insn)      (((insn) & 0x3) != 0x3)
#define INSN_IS_32BIT(insn)      (((insn) & 0x3) == 0x3 && ((insn) & 
0x1c) != 0x1c)
As an option we could come up with the following names:

/* Bitmasks for instruction length encoding fields */
#define INSN_LEN_1_0_MASK        0x3   /* Bits [1:0] for 16-bit vs 
>=32-bit */
#define INSN_LEN_4_2_MASK        0x1c  /* Bits [4:2] for 32-bit vs 
>=48-bit */
#define INSN_IS_16BIT(insn)      (((insn) & INSN_LEN_1_0_MASK) != 
INSN_LEN_1_0_MASK)
#define INSN_IS_32BIT(insn)      \
    (((insn) & INSN_LEN_1_0_MASK) == INSN_LEN_1_0_MASK && \
     ((insn) & INSN_LEN_4_2_MASK) != INSN_LEN_4_2_MASK)

But it seems the first option still looks better.

Would you also prefer Option 1?

> > I think that for now it is enough to go without common implmenntation of
> > INSN_LEN and just return 0 as suggested above.
> Suitably commented upon that would certainly be okay with me.
The the following comment looks good enough for me:

/*
 * Length in bytes of the instruction whose first parcel is @insn: 2 or 
4, or
 * 0 when the encoding is 48 bits or wider.  No ratified extension 
defines an
 * instruction of such a length, so rather than open-coding a decoder for
 * encodings which cannot legitimately occur, leave it to the caller to 
treat
 * the 0 as an illegal instruction.
 */
#define INSN_LEN(insn) \
    (INSN_IS_16BIT(insn) ? 2 : (INSN_IS_32BIT(insn) ? 4 : 0))

~ Oleksii