Re: [Qemu-devel] [PATCH v5 00/35] target/riscv: Convert to decodetree

[Alistair Francis]

On Tue, Jan 29, 2019 at 11:31 AM Palmer Dabbelt <address@hidden> wrote:
>
> On Sat, 26 Jan 2019 00:51:23 PST (-0800), Bastian Koppelmann wrote:
> > On 1/26/19 12:54 AM, Palmer Dabbelt wrote:
> >> On Tue, 22 Jan 2019 13:38:52 PST (-0800), address@hidden wrote:
> >>> On 1/22/19 1:28 AM, Bastian Koppelmann wrote:
> >>>> Hi,
> >>>>
> >>>> this patchset converts the RISC-V decoder to decodetree in four major 
> >>>> steps:
> >>>>
> >>>> 1) Convert 32-bit instructions to decodetree [Patch 1-16]:
> >>>>     Many of the gen_* functions are called by the decode functions for 
> >>>> 16-bit
> >>>>     and 32-bit functions. If we move translation code from the gen_*
> >>>>     functions to the generated trans_* functions of decode-tree, we get 
> >>>> a lot of
> >>>>     duplication. Therefore, we mostly generate calls to the old gen_* 
> >>>> function
> >>>>     which are properly replaced after step 2).
> >>>>
> >>>>     Each of the trans_ functions are grouped into files corresponding to 
> >>>> their
> >>>>     ISA extension, e.g. addi which is in RV32I is translated in the file
> >>>>     'trans_rvi.inc.c'.
> >>>>
> >>>> 2) Convert 16-bit instructions to decodetree [Patch 17-19]:
> >>>>     All 16 bit instructions have a direct mapping to a 32 bit 
> >>>> instruction. Thus,
> >>>>     we convert the arguments in the 16 bit trans_ function to the 
> >>>> arguments of
> >>>>     the corresponding 32 bit instruction and call the 32 bit trans_ 
> >>>> function.
> >>>>
> >>>> 3) Remove old manual decoding in gen_* function [Patch 20-30]:
> >>>>     this move all manual translation code into the trans_* instructions 
> >>>> of
> >>>>     decode tree, such that we can remove the old decode_* functions.
> >>>>
> >>>> 4) Simplify RVC by reusing as much as possible from the RVG decoder as 
> >>>> suggested
> >>>>    by Richard. [Patch 31-35]
> >>>>
> >>>> full tree available at
> >>>> https://github.com/bkoppelmann/qemu/tree/riscv-dt-v5
> >>>>
> >>>> Cheers,
> >>>> Bastian
> >>>>
> >>>> v4 -> v5:
> >>>>     - fixed rebase error
> >>>>     - moved TARGET_LONG_BITS check of shift instructions before rd == 0 
> >>>> check
> >>>>     - removed extra sign extension of sraiw
> >>>>     - removed rs2 == 0 special cases in sraw/srlw
> >>>
> >>> All looks good to me now.  Thanks for persevering.
> >>
> >> Thanks for reviewing this.
> >>
> >> Bastian: what sort of testing have you done here?  This is a pretty big 
> >> change,
> >> and while it's been fairly extensively reviewed I'm just generally 
> >> paranoid.
> >>
> >
> > I ran riscv-test as well as booting the newest fedora.
>
> Well, that pretty much exhausts my testing scheme :).  I think Alistair might
> have a setup for booting a 32-bit Yocto-based distro in QEMU.  Assuming he has
> the flow set up and it boots then I don't see any reason not to merge it.

I just tested it and 32-bit images boot the same before and after this
patchset. So the 4.19 (with patches ontop) kernel boots, but the 5.0
(with patches ontop) crashes at init.

>
> Thanks for pulling this all together, it's really great to have a cleaner
> decoder!
>
> Alistair: if that's true, can you give this a shot?

Yep, I have been using this in my daily use and haven't seen any
problems. That isn't exhaustive testing but should cover a bit.

Tested-by: Alistair Francis <address@hidden>

Alistair

>
> Bastian: do you mind collecting the relevant tags (Reviwed-by, etc) and 
> putting
> them in a v6?
>