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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.09.2026 15:45, Oleksii Kurochko wrote:
> 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)

Largely okay; I'd still prefer

#define INSN_IS_16BIT(insn)      (((insn) & 3) != 3)
#define INSN_IS_32BIT(insn)      (!INSN_IS_16BIT(insn) && ((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 do, yes.

>>> 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))

Reads okay, thanks.

Jan