All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [RFC] RISC-V Decoder generator
@ 2017-10-20 13:46 Bastian Koppelmann
  2017-10-20 16:57 ` Richard Henderson
  2017-10-21  6:44 ` [Qemu-devel] [sw-dev] " Richard W.M. Jones
  0 siblings, 2 replies; 6+ messages in thread
From: Bastian Koppelmann @ 2017-10-20 13:46 UTC (permalink / raw)
  To: QEMU Developers, RISC-V SW Dev

Hi QEMU and RISC-V folks,

I asked you for feedback some while ago regarding a modular RISC-V QEMU
target (see discussion [1]). I tried getting it to work with the good
old C preprocessor and quickly realized that it is too limiting. Instead
I created a data-driven decoder generator written in python (see the
code on github [2]) using YAML as a description language.

I'd like to get some feedback whether this is acceptable to be
upstreamed to QEMU or if you have any suggestions for improvements.
Right now only RV32I instruction are implemented in this scheme.

For this I will roughly sketch how the data format for RISC-V
instructions works and then how the result after processing looks like.
You can find the full tree on github [2] with all the relevant files in
'target-riscv/decode-gen/'.

The RISC-V data in YAML consists of:

1) prefixes
2) instruction formats
3) instruction specification using the data of 1) and 2)

1) prefixes have:
   - name
   - bitmask that identifies instructions belonging to it
   - length that specifies the length in bits for all instructions with
     this prefix

Example prefix description for 16 bit instructions:

16Bit: {
    bitmask: ~0b11,
    len: 16
    }

2) instruction formats have:
    - name
    - list of immediate fields
    - list of 'normal' fields (e.g. register identifier)

    The immediate fields are specified separately in the same file with:
        - name
        - data: in order list of the individual fields + possible sign i

                extension
        - fill: how is the immediate filled from the LSB (for now only
                '0' fills are allowed)

    Example for B-type immediate:
        B: {data: [31s, 7,  30..25, 11..8], fill: '0'}
        Note here that bit 31 is used for sign extension as indicated by
        the 's'.

    The 'normal' fields are similar to immediate fields but only have:
        - name
        - single field description

    Example for 'rs1' field:
	- rs1: 19..15

Example for a full instruction format 'SB':
    - SB: {
      fields: [rs1,rs2],
      immediates: [B]
      }

3) instruction specification have:
    - name
    - instruction format from 2)
    - prefix from 1)
    - keymap of opcode fields and the value they take for this
      instruction
    - func for binary translation

    The opcode fields are specified separately in the same file in the
    same manner as 'normal' opcode fields.
    Example for FUNC3:
        - FUNC3_32: 14..12

Example for the 'BEQ' instruction:
    BEQ: {format: SB, prefix: 32Bit, opcodes: {OP1_32: 99, FUNC3_32: 0},
          func: gen_beq}

    'OP1_32: 99' means that field OP1_32 defined by bits 6 until 0 have
    to have the value 99


The python script then converts these instructions into a tree of
switch-case statements depending on the specified opcodes and generates
at each case for one instruction two function calls:

1) decode_format_* depending on the instruction format used by this
   instruction.
   The decode_format_* functions are also generated by the script and
   take of decoding the 'normal' fields and the immediate fields. This
   data is stored in a data structure 'DisasFormats' which needs to be
   passed to any translation function used by instructions.

2) A call to the translation function defined by 'func' in 3)
   These need to be defined by the developer in translate.c. For example
   for LUI this looks like:

static void gen_lui(CPURISCVState *env, DisasContext *ctx)
{
    if (ctx->fmt.field_rd == 0) {
            return; /* NOP */
    }
    tcg_gen_movi_tl(cpu_gpr[ctx->fmt.field_rd], ctx->fmt.immediate_U);
}

Thanks for any comments,
Bastian

[1] https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg07735.html
[2] https://github.com/bkoppelmann/riscv-qemu/tree/decoder

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [RFC] RISC-V Decoder generator
  2017-10-20 13:46 [Qemu-devel] [RFC] RISC-V Decoder generator Bastian Koppelmann
@ 2017-10-20 16:57 ` Richard Henderson
  2017-10-22 13:18   ` Bastian Koppelmann
  2017-10-21  6:44 ` [Qemu-devel] [sw-dev] " Richard W.M. Jones
  1 sibling, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2017-10-20 16:57 UTC (permalink / raw)
  To: Bastian Koppelmann, QEMU Developers, RISC-V SW Dev

On 10/20/2017 06:46 AM, Bastian Koppelmann wrote:
> The RISC-V data in YAML consists of:
> 
> 1) prefixes
> 2) instruction formats
> 3) instruction specification using the data of 1) and 2)
> 
> 1) prefixes have:
>    - name
>    - bitmask that identifies instructions belonging to it
>    - length that specifies the length in bits for all instructions with
>      this prefix
> 
> Example prefix description for 16 bit instructions:
> 
> 16Bit: {
>     bitmask: ~0b11,
>     len: 16
>     }
> 
> 2) instruction formats have:
>     - name
>     - list of immediate fields
>     - list of 'normal' fields (e.g. register identifier)
> 
>     The immediate fields are specified separately in the same file with:
>         - name
>         - data: in order list of the individual fields + possible sign i
> 
>                 extension
>         - fill: how is the immediate filled from the LSB (for now only
>                 '0' fills are allowed)
> 
>     Example for B-type immediate:
>         B: {data: [31s, 7,  30..25, 11..8], fill: '0'}
>         Note here that bit 31 is used for sign extension as indicated by
>         the 's'.

I think "append" might be a clearer name than "fill".

Reading this for the first time, I wondered how you were specifying one bit of
fill.  That only became clear when I saw

    - U: {data: [31..20, 19..12], fill: '000000000000'}

As for the generated GET_X_IMM, please use inline functions instead of macros.
There doesn't appear to be any need for the preprocessor.

> 3) instruction specification have:
>     - name
>     - instruction format from 2)
>     - prefix from 1)
>     - keymap of opcode fields and the value they take for this
>       instruction
>     - func for binary translation

There's some redundant information here -- OP1_32 overlaps prefix.  You should
either drop the redundancy (infer prefix from opcode) or validate it (assert
that opcode matches prefix).


> The python script then converts these instructions into a tree of
> switch-case statements depending on the specified opcodes and generates
> at each case for one instruction two function calls:
> 
> 1) decode_format_* depending on the instruction format used by this
>    instruction.
>    The decode_format_* functions are also generated by the script and
>    take of decoding the 'normal' fields and the immediate fields. This
>    data is stored in a data structure 'DisasFormats' which needs to be
>    passed to any translation function used by instructions.
> 
> 2) A call to the translation function defined by 'func' in 3)
>    These need to be defined by the developer in translate.c. For example
>    for LUI this looks like:
> 
> static void gen_lui(CPURISCVState *env, DisasContext *ctx)
> {
>     if (ctx->fmt.field_rd == 0) {
>             return; /* NOP */
>     }
>     tcg_gen_movi_tl(cpu_gpr[ctx->fmt.field_rd], ctx->fmt.immediate_U);
> }

(As an aside, it's bad form to pass around ENV unless the function is going to
have to read more code from guest memory.  Which isn't true in your case.)

Unless you have a real use-case in mind, I'm not keen on all of the fields
being accessible all of the time.

You have all of the information in hand to do all of the proper checking with
the compiler.  My suggestion is to have not one DisasFormats struct with all
possible fields included, but one DisasFormatX struct per format containing
only the fields present.  Then pass that separately to the function.

E.g.

static void gen_lui(DisasContext *ctx, DisasFormatU *fmt)
{
    if (fmt.field_rd == 0) {
        return; /* NOP */
    }
    tcg_gen_movi_tl(cpu_gpr[fmt.field_rd], fmt->immediate_U);
}

Within the decode_32bit function, you could have

  union {
    DisasFormatR R;
    DisasFormatI I;
    ...
  } fmts;

  ...

  case 0:
      /* ADDI */
      decode_format_I(ctx, &fmts.I);
      gen_addi(ctx, &fmts.I);
      break;

etc.  You now have isolation on exactly the fields present in the format, and
you have type checking on the function prototype that there is agreement
between the decoder and the generation functions about the format.

Not especially important, but how much effort would it be to determine that all
valid insns below a certain point in the parse tree are the same format?  I was
just looking at e.g.

    case 35:
        /* Decode FUNC3_32 */
        switch (extract32(ctx->opcode, 12, 3)) {
        case 0:
            /* SB */
            decode_format_S(ctx);
            gen_sb(env, ctx);
            break;
        case 1:
            /* SH */
            decode_format_S(ctx);
            gen_sh(env, ctx);
            break;
        case 2:
            /* SW */
            decode_format_S(ctx);
            gen_sw(env, ctx);
            break;
        default:
            kill_unknown(ctx, RISCV_EXCP_ILLEGAL_INST);
            break;
        }
        break;

and thinking that the decode_format_S could be hoisted above the switch.  The
compiler won't do it because it's not used on the kill_unknown path.

Major OP1 == 115 is another example with multiple sub-switches, but all insns
are format I.

All that said, this looks very readable.  Thank you.


r~

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [sw-dev] [RFC] RISC-V Decoder generator
  2017-10-20 13:46 [Qemu-devel] [RFC] RISC-V Decoder generator Bastian Koppelmann
  2017-10-20 16:57 ` Richard Henderson
@ 2017-10-21  6:44 ` Richard W.M. Jones
  2017-10-22 13:22   ` Bastian Koppelmann
  1 sibling, 1 reply; 6+ messages in thread
From: Richard W.M. Jones @ 2017-10-21  6:44 UTC (permalink / raw)
  To: Bastian Koppelmann; +Cc: QEMU Developers, RISC-V SW Dev

On Fri, Oct 20, 2017 at 03:46:54PM +0200, Bastian Koppelmann wrote:
> I asked you for feedback some while ago regarding a modular RISC-V QEMU
> target (see discussion [1]). I tried getting it to work with the good
> old C preprocessor and quickly realized that it is too limiting. Instead
> I created a data-driven decoder generator written in python (see the
> code on github [2]) using YAML as a description language.
> 
> I'd like to get some feedback whether this is acceptable to be
> upstreamed to QEMU or if you have any suggestions for improvements.
> Right now only RV32I instruction are implemented in this scheme.

My suggestion would be to reimplement (part of) the s390x decoder
using this scheme.  That would give us a direct comparison of how your
scheme is better or worse than the existing macros.

Will you be at the KVM Forum next week?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [RFC] RISC-V Decoder generator
  2017-10-20 16:57 ` Richard Henderson
@ 2017-10-22 13:18   ` Bastian Koppelmann
  0 siblings, 0 replies; 6+ messages in thread
From: Bastian Koppelmann @ 2017-10-22 13:18 UTC (permalink / raw)
  To: Richard Henderson, QEMU Developers, RISC-V SW Dev

On 10/20/2017 06:57 PM, Richard Henderson wrote:
> All that said, this looks very readable.  Thank you.
> 

Thanks for your great feedback. I think most of them are not hard to
implement. However I might need some time for that :)

Cheers,
Bastian

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [sw-dev] [RFC] RISC-V Decoder generator
  2017-10-21  6:44 ` [Qemu-devel] [sw-dev] " Richard W.M. Jones
@ 2017-10-22 13:22   ` Bastian Koppelmann
  2017-10-24 21:25     ` Richard Henderson
  0 siblings, 1 reply; 6+ messages in thread
From: Bastian Koppelmann @ 2017-10-22 13:22 UTC (permalink / raw)
  To: Richard W.M. Jones; +Cc: RISC-V SW Dev, QEMU Developers

Hi Richard,

On 10/21/2017 08:44 AM, Richard W.M. Jones wrote:
> On Fri, Oct 20, 2017 at 03:46:54PM +0200, Bastian Koppelmann wrote:
>> I asked you for feedback some while ago regarding a modular RISC-V QEMU
>> target (see discussion [1]). I tried getting it to work with the good
>> old C preprocessor and quickly realized that it is too limiting. Instead
>> I created a data-driven decoder generator written in python (see the
>> code on github [2]) using YAML as a description language.
>>
>> I'd like to get some feedback whether this is acceptable to be
>> upstreamed to QEMU or if you have any suggestions for improvements.
>> Right now only RV32I instruction are implemented in this scheme.
> 
> My suggestion would be to reimplement (part of) the s390x decoder
> using this scheme.  That would give us a direct comparison of how your
> scheme is better or worse than the existing macros.

Yeah that would be a great test. However I'm not sure if it's worth the
effort. AFAIK s390x will not be extended with new instructions, so there
is no need for a new scheme unless it helps making the code better
maintainable. But that's up to the s390x maintainers.

> 
> Will you be at the KVM Forum next week?

No, sorry. I'm at the verge of graduating from University, so my time is
very limited right now :(

Cheers,
Bastian

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [sw-dev] [RFC] RISC-V Decoder generator
  2017-10-22 13:22   ` Bastian Koppelmann
@ 2017-10-24 21:25     ` Richard Henderson
  0 siblings, 0 replies; 6+ messages in thread
From: Richard Henderson @ 2017-10-24 21:25 UTC (permalink / raw)
  To: Bastian Koppelmann, Richard W.M. Jones; +Cc: RISC-V SW Dev, QEMU Developers

On 10/22/2017 03:22 PM, Bastian Koppelmann wrote:
> Hi Richard,
> 
> On 10/21/2017 08:44 AM, Richard W.M. Jones wrote:
>> On Fri, Oct 20, 2017 at 03:46:54PM +0200, Bastian Koppelmann wrote:
>>> I asked you for feedback some while ago regarding a modular RISC-V QEMU
>>> target (see discussion [1]). I tried getting it to work with the good
>>> old C preprocessor and quickly realized that it is too limiting. Instead
>>> I created a data-driven decoder generator written in python (see the
>>> code on github [2]) using YAML as a description language.
>>>
>>> I'd like to get some feedback whether this is acceptable to be
>>> upstreamed to QEMU or if you have any suggestions for improvements.
>>> Right now only RV32I instruction are implemented in this scheme.
>>
>> My suggestion would be to reimplement (part of) the s390x decoder
>> using this scheme.  That would give us a direct comparison of how your
>> scheme is better or worse than the existing macros.
> 
> Yeah that would be a great test. However I'm not sure if it's worth the
> effort. AFAIK s390x will not be extended with new instructions, so there
> is no need for a new scheme unless it helps making the code better
> maintainable. But that's up to the s390x maintainers.

s390x has been getting regular updates and new instructions.

However, s390x is much more regular in its opcode placement wrt the various
insn formats, so it's slightly easier to decode than riscv.

In addition, the s390x translator has more sophisticated handling of operands
and writeback than you have for riscv at present.  That is, where we indicate
that particular register or memory arguments should be loaded into TCG
temporaries; storing the result; computing the PSW flags; etc.  This is because
there are so many CISC instruction variants where arguments come from whereever.

So, really, I think it would be a large amount of work to extend this riscv
translator to handle s390x, and would largely be a waste.

It would perhaps more interesting to apply it to other risc targets.  But that
is surely out of scope for one patch set.


r~

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-10-24 21:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-20 13:46 [Qemu-devel] [RFC] RISC-V Decoder generator Bastian Koppelmann
2017-10-20 16:57 ` Richard Henderson
2017-10-22 13:18   ` Bastian Koppelmann
2017-10-21  6:44 ` [Qemu-devel] [sw-dev] " Richard W.M. Jones
2017-10-22 13:22   ` Bastian Koppelmann
2017-10-24 21:25     ` Richard Henderson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.