Re: [Qemu-devel] [PATCH for 4.1 v3 2/6] target/riscv: Fall back to generating a RISC-V CPU

[Alistair Francis <alistair23@gmail.com>]

On Tue, Apr 16, 2019 at 6:23 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Apr 10, 2019 at 11:10:25PM +0000, Alistair Francis wrote:
> > If a user specifies a CPU that we don't understand then we want to fall
> > back to a CPU generated from the ISA string.
> >
> > At the moment the generated CPU is assumed to be a privledge spec
> > version 1.10 CPU with an MMU. This can be changed in the future.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > v3:
> >  - Ensure a minimal length so we don't run off the end of the string.
> >  - Don't parse the rv32/rv64 in the loop
> >  target/riscv/cpu.c | 101 ++++++++++++++++++++++++++++++++++++++++++++-
> >  target/riscv/cpu.h |   2 +
> >  2 files changed, 102 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index d61bce6d55..27be9e412a 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -19,6 +19,7 @@
> >
> >  #include "qemu/osdep.h"
> >  #include "qemu/log.h"
> > +#include "qemu/error-report.h"
> >  #include "cpu.h"
> >  #include "exec/exec-all.h"
> >  #include "qapi/error.h"
> > @@ -103,6 +104,99 @@ static void set_resetvec(CPURISCVState *env, int resetvec)
> >  #endif
> >  }
> >
> > +static void riscv_generate_cpu_init(Object *obj)
> > +{
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +    CPURISCVState *env = &cpu->env;
> > +    RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> > +    const char *riscv_cpu = mcc->isa_str;
> > +    target_ulong target_misa = 0;
> > +    target_ulong rvxlen = 0;
> > +    int i;
> > +    bool valid = false;
> > +
> > +    /*
> > +     * We need at least 5 charecters for the string to be valid. Check that
> > +     * now so we can be lazier later.
> > +     */
> > +    if (strlen(riscv_cpu) < 5) {
> > +        error_report("'%s' does not appear to be a valid RISC-V ISA string",
> > +                     riscv_cpu);
> > +        exit(1);
> > +    }
> > +
> > +    if (riscv_cpu[0] == 'r' && riscv_cpu[1] == 'v') {
> > +        /* Starts with "rv" */
> > +        if (riscv_cpu[2] == '3' && riscv_cpu[3] == '2') {
> > +            valid = true;
> > +            rvxlen = RV32;
> > +        }
> > +        if (riscv_cpu[2] == '6' && riscv_cpu[3] == '4') {
> > +            valid = true;
> > +            rvxlen = RV64;
> > +        }
> > +    }
> > +
> > +    if (!valid) {
> > +        error_report("'%s' does not appear to be a valid RISC-V CPU",
> > +                     riscv_cpu);
> > +        exit(1);
> > +    }
> > +
> > +    for (i = 4; i < strlen(riscv_cpu); i++) {
> > +        switch (riscv_cpu[i]) {
> > +        case 'i':
> > +            if (target_misa & RVE) {
> > +                error_report("I and E extensions are incompatible");
> > +                exit(1);
> > +            }
> > +            target_misa |= RVI;
> > +            continue;
> > +        case 'e':
> > +            if (target_misa & RVI) {
> > +                error_report("I and E extensions are incompatible");
> > +                exit(1);
> > +            }
> > +            target_misa |= RVE;
> > +            continue;
> > +        case 'g':
> > +            target_misa |= RVI | RVM | RVA | RVF | RVD;
> > +            continue;
> > +        case 'm':
> > +            target_misa |= RVM;
> > +            continue;
> > +        case 'a':
> > +            target_misa |= RVA;
> > +            continue;
> > +        case 'f':
> > +            target_misa |= RVF;
> > +            continue;
> > +        case 'd':
> > +            target_misa |= RVD;
> > +            continue;
> > +        case 'c':
> > +            target_misa |= RVC;
> > +            continue;
> > +        case 's':
> > +            target_misa |= RVS;
> > +            continue;
> > +        case 'u':
> > +            target_misa |= RVU;
> > +            continue;
> > +        default:
> > +            warn_report("QEMU does not support the %c extension",
> > +                        riscv_cpu[i]);
> > +            continue;
> > +        }
> > +    }
> > +
> > +    set_misa(env, rvxlen | target_misa);
> > +    set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
> > +    set_resetvec(env, DEFAULT_RSTVEC);
> > +    set_feature(env, RISCV_FEATURE_MMU);
> > +    set_feature(env, RISCV_FEATURE_PMP);
> > +}
>
> This whole approach feels undesirable to me, as it is quite different to
> way CPUs are represented in the other architectures in QEMU and as a result
> does not fit in the QAPI commands we've been building in QEMU for dealing
> with CPU model representation. This will make for increased maint burden
> in both QEMU and apps managing QEMU
>
> IIUC, this code is taking an arbitrary CPU model string and looking
> at individual characters in that string & turning on individual features
> according to what characters it sees. There's several problems with this
>
>  - There's no way to enumerate valid CPU model names
>
>  - There can be many different names that all result
>    in the same CPU model. eg "fdcs", "scdf", a"ffddccss"
>    all result in the same features getting enabled
>    by this loop above.
>
>  - There's no way to check compatibility between CPUs
>
>  - There will be no way to version the CPUs if we need
>    to tie them to machine types for back compatibility.
>
> If we have a base CPU model, with a bunch of features that can optionally
> be enabled, then IMHO we should represent that the same way was we do
> on x86, whre we have a CPU model name, and then a comma sepaprated list
> of features.
>
> If on the other hand we don't want to support the combinatorial matrix
> of all possible feature flags, then we should just list all the expected
> named CPU models that are required explicitly.

Ok, I'm about to send out a new series that is based on this series.
The new series is called "RISC-V: Add properties to the CPUs". It has
dropped the ISA string part (this patch) and just adds some standard
CPU properties. If everyone is happy with that approach I'll continue
to expand it to add the extensions as CPU properties.

Alistair

>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|