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
