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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

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