On Tue, Apr 19, 2022 at 5:04 PM Frank Chang <frank.chang@sifive.com> wrote:
>
> On Tue, Apr 19, 2022 at 2:00 PM Frank Chang <frank.chang@sifive.com> wrote:
>>
>> On Tue, Apr 19, 2022 at 1:27 PM Anup Patel <apatel@ventanamicro.com> wrote:
>>>
>>> On Tue, Apr 19, 2022 at 10:52 AM Alistair Francis <alistair23@gmail.com> wrote:
>>> >
>>> > On Fri, Apr 15, 2022 at 7:37 PM <frank.chang@sifive.com> wrote:
>>> > >
>>> > > From: Frank Chang <frank.chang@sifive.com>
>>> > >
>>> > > Allow user to set core's marchid, mvendorid, mipid CSRs through
>>> > > -cpu command line option.
>>> > >
>>> > > Signed-off-by: Frank Chang <frank.chang@sifive.com>
>>> > > Reviewed-by: Jim Shu <jim.shu@sifive.com>
>>> > > ---
>>> > > target/riscv/cpu.c | 4 ++++
>>> > > target/riscv/cpu.h | 4 ++++
>>> > > target/riscv/csr.c | 38 ++++++++++++++++++++++++++++++++++----
>>> > > 3 files changed, 42 insertions(+), 4 deletions(-)
>>> > >
>>> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>> > > index ddda4906ff..2eea0f9be7 100644
>>> > > --- a/target/riscv/cpu.c
>>> > > +++ b/target/riscv/cpu.c
>>> > > @@ -786,6 +786,10 @@ static Property riscv_cpu_properties[] = {
>>> > > DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
>>> > > DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
>>> > >
>>> > > + DEFINE_PROP_UINT32("mvendorid", RISCVCPU, cfg.mvendorid, 0),
>>> > > + DEFINE_PROP_UINT64("marchid", RISCVCPU, cfg.marchid, 0),
>>> > > + DEFINE_PROP_UINT64("mipid", RISCVCPU, cfg.mipid, 0),
>>> >
>>> > Should we have non-zero defaults here?
>>>
>>> To do that, we need mvendorid for QEMU RISC-V.
>>>
>>> The marchid and mipid can be based on the QEMU version number.
>>>
>>> Regards,
>>> Anup
>>
>>
>> The original intention for this patch is to allow users to define
>> their own $mvendorid, $marchid, and $mipid through the command line
>> when they initiate QEMU.
>>
>> If we want to provide the default values for QEMU RISC-V CPU,
>> just like what Anup said.
>> We need to define our own mvendorid, which should be a JEDEC manufacturer ID.
>> (Perhaps it's fine to just give some random legal JEDEC manufacturer ID
>> as I don't think we would really want to spend the money on that.)
This is fine as zero I think.
>>
>> For $marchid and $mipid,
>> I agree that it could base on QEMU's version from the QEMU_FULL_VERSION macro.
>> (and $marchid should have MSB set to 1 for open-source projects.)
There could be some use in setting this to the QEMU version by
default. It doesn't really get us much though, but might be useful.
I'll take this patch as is, feel free to send a new patch if you want
to add a default value
Alistair
Sure, I'll set QEMU version by default in the next version patchset.
Regards,
Frank Chang
>>
>> Regards,
>> Frank Chang
>
>
> Another simpler way is to stick with the current approach
> and leave $mvendorid, $marchid and $mipid default to 0.
> Which is still legal as RISC-V privilege spec says:
> "A value of 0 can be returned to indicate the field is not implemented."
>
> Regards,
> Frank Chang
>
>>
>>
>>>
>>>
>>> >
>>> > Alistair
>>> >
>>> > > +
>>> > > DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false),
>>> > > DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false),
>>> > > DEFINE_PROP_BOOL("svpbmt", RISCVCPU, cfg.ext_svpbmt, false),
>>> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>> > > index c069fe85fa..3ab92deb4b 100644
>>> > > --- a/target/riscv/cpu.h
>>> > > +++ b/target/riscv/cpu.h
>>> > > @@ -370,6 +370,10 @@ struct RISCVCPUConfig {
>>> > > bool ext_zve32f;
>>> > > bool ext_zve64f;
>>> > >
>>> > > + uint32_t mvendorid;
>>> > > + uint64_t marchid;
>>> > > + uint64_t mipid;
>>> > > +
>>> > > /* Vendor-specific custom extensions */
>>> > > bool ext_XVentanaCondOps;
>>> > >
>>> > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>> > > index 341c2e6f23..9a02038adb 100644
>>> > > --- a/target/riscv/csr.c
>>> > > +++ b/target/riscv/csr.c
>>> > > @@ -603,6 +603,36 @@ static RISCVException write_ignore(CPURISCVState *env, int csrno,
>>> > > return RISCV_EXCP_NONE;
>>> > > }
>>> > >
>>> > > +static RISCVException read_mvendorid(CPURISCVState *env, int csrno,
>>> > > + target_ulong *val)
>>> > > +{
>>> > > + CPUState *cs = env_cpu(env);
>>> > > + RISCVCPU *cpu = RISCV_CPU(cs);
>>> > > +
>>> > > + *val = cpu->cfg.mvendorid;
>>> > > + return RISCV_EXCP_NONE;
>>> > > +}
>>> > > +
>>> > > +static RISCVException read_marchid(CPURISCVState *env, int csrno,
>>> > > + target_ulong *val)
>>> > > +{
>>> > > + CPUState *cs = env_cpu(env);
>>> > > + RISCVCPU *cpu = RISCV_CPU(cs);
>>> > > +
>>> > > + *val = cpu->cfg.marchid;
>>> > > + return RISCV_EXCP_NONE;
>>> > > +}
>>> > > +
>>> > > +static RISCVException read_mipid(CPURISCVState *env, int csrno,
>>> > > + target_ulong *val)
>>> > > +{
>>> > > + CPUState *cs = env_cpu(env);
>>> > > + RISCVCPU *cpu = RISCV_CPU(cs);
>>> > > +
>>> > > + *val = cpu->cfg.mipid;
>>> > > + return RISCV_EXCP_NONE;
>>> > > +}
>>> > > +
>>> > > static RISCVException read_mhartid(CPURISCVState *env, int csrno,
>>> > > target_ulong *val)
>>> > > {
>>> > > @@ -3098,10 +3128,10 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>>> > > [CSR_MINSTRETH] = { "minstreth", any32, read_instreth },
>>> > >
>>> > > /* Machine Information Registers */
>>> > > - [CSR_MVENDORID] = { "mvendorid", any, read_zero },
>>> > > - [CSR_MARCHID] = { "marchid", any, read_zero },
>>> > > - [CSR_MIMPID] = { "mimpid", any, read_zero },
>>> > > - [CSR_MHARTID] = { "mhartid", any, read_mhartid },
>>> > > + [CSR_MVENDORID] = { "mvendorid", any, read_mvendorid },
>>> > > + [CSR_MARCHID] = { "marchid", any, read_marchid },
>>> > > + [CSR_MIMPID] = { "mimpid", any, read_mipid },
>>> > > + [CSR_MHARTID] = { "mhartid", any, read_mhartid },
>>> > >
>>> > > /* Machine Trap Setup */
>>> > > [CSR_MSTATUS] = { "mstatus", any, read_mstatus, write_mstatus, NULL,
>>> > > --
>>> > > 2.35.1
>>> > >
>>> > >
>>> >