| Message ID | 20230210133635.589647-3-dbarboza@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | enable write_misa() and RISCV_FEATURE_* cleanups | expand |
On 2023/2/10 21:36, Daniel Henrique Barboza wrote: > At this moment, and apparently since ever, we have no way of enabling > RISCV_FEATURE_MISA. This means that all the code from write_misa(), all > the nuts and bolts that handles how to properly write this CSR, has > always been a no-op as well because write_misa() will always exit > earlier. > > This seems to be benign in the majority of cases. Booting an Ubuntu > 'virt' guest and logging all the calls to 'write_misa' shows that no > writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling > RISC-V extensions after the machine is powered on, seems to be a niche > use. > > There is a good chance that the code in write_misa() hasn't been > properly tested. Allowing users to write MISA can open the floodgates of > new breeds of bugs. We could instead remove most (if not all) of > write_misa() since it's never used. Well, as a hardware emulator, > dealing with crashes because a register write went wrong is what we're > here for. > > Create a 'misa-w' CPU property to allow users to choose whether writes > to MISA should be allowed. The default is set to 'false' for all RISC-V > machines to keep compatibility with what we´ve been doing so far. > > Read cpu->cfg.misa_w directly in write_misa(), instead of executing > riscv_set_feature(RISCV_FEATURE_MISA) in riscv_cpu_realize(), that would > simply reflect the cpu->cfg.misa_w bool value in 'env->features' and > require a riscv_feature() call to read it back. > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/cpu.c | 1 + > target/riscv/cpu.h | 1 + > target/riscv/csr.c | 4 +++- > 3 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 93b52b826c..69fb9e123f 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -1197,6 +1197,7 @@ static void register_cpu_props(DeviceState *dev) > > static Property riscv_cpu_properties[] = { > DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true), > + DEFINE_PROP_BOOL("misa-w", RISCVCPU, cfg.misa_w, false), > > DEFINE_PROP_UINT32("mvendorid", RISCVCPU, cfg.mvendorid, 0), > DEFINE_PROP_UINT64("marchid", RISCVCPU, cfg.marchid, RISCV_CPU_MARCHID), > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 7128438d8e..103963b386 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -498,6 +498,7 @@ struct RISCVCPUConfig { > bool pmp; > bool epmp; > bool debug; > + bool misa_w; > > bool short_isa_string; > }; > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index e149b453da..4f9cc501b2 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -1329,7 +1329,9 @@ static RISCVException read_misa(CPURISCVState *env, int csrno, > static RISCVException write_misa(CPURISCVState *env, int csrno, > target_ulong val) > { > - if (!riscv_feature(env, RISCV_FEATURE_MISA)) { > + RISCVCPU *cpu = env_archcpu(env); > + > + if (!cpu->cfg.misa_w) { It's Ok to get it directly from cfg. However, personally, I prefer to keep the non-isa features in a separate list. Regards, Weiwei Li > /* drop write to misa */ > return RISCV_EXCP_NONE; > }
On 2/10/23 23:43, weiwei wrote: > > On 2023/2/10 21:36, Daniel Henrique Barboza wrote: >> At this moment, and apparently since ever, we have no way of enabling >> RISCV_FEATURE_MISA. This means that all the code from write_misa(), all >> the nuts and bolts that handles how to properly write this CSR, has >> always been a no-op as well because write_misa() will always exit >> earlier. >> >> This seems to be benign in the majority of cases. Booting an Ubuntu >> 'virt' guest and logging all the calls to 'write_misa' shows that no >> writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling >> RISC-V extensions after the machine is powered on, seems to be a niche >> use. >> >> There is a good chance that the code in write_misa() hasn't been >> properly tested. Allowing users to write MISA can open the floodgates of >> new breeds of bugs. We could instead remove most (if not all) of >> write_misa() since it's never used. Well, as a hardware emulator, >> dealing with crashes because a register write went wrong is what we're >> here for. >> >> Create a 'misa-w' CPU property to allow users to choose whether writes >> to MISA should be allowed. The default is set to 'false' for all RISC-V >> machines to keep compatibility with what we´ve been doing so far. >> >> Read cpu->cfg.misa_w directly in write_misa(), instead of executing >> riscv_set_feature(RISCV_FEATURE_MISA) in riscv_cpu_realize(), that would >> simply reflect the cpu->cfg.misa_w bool value in 'env->features' and >> require a riscv_feature() call to read it back. >> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >> --- >> target/riscv/cpu.c | 1 + >> target/riscv/cpu.h | 1 + >> target/riscv/csr.c | 4 +++- >> 3 files changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index 93b52b826c..69fb9e123f 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -1197,6 +1197,7 @@ static void register_cpu_props(DeviceState *dev) >> static Property riscv_cpu_properties[] = { >> DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true), >> + DEFINE_PROP_BOOL("misa-w", RISCVCPU, cfg.misa_w, false), >> DEFINE_PROP_UINT32("mvendorid", RISCVCPU, cfg.mvendorid, 0), >> DEFINE_PROP_UINT64("marchid", RISCVCPU, cfg.marchid, RISCV_CPU_MARCHID), >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >> index 7128438d8e..103963b386 100644 >> --- a/target/riscv/cpu.h >> +++ b/target/riscv/cpu.h >> @@ -498,6 +498,7 @@ struct RISCVCPUConfig { >> bool pmp; >> bool epmp; >> bool debug; >> + bool misa_w; >> bool short_isa_string; >> }; >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >> index e149b453da..4f9cc501b2 100644 >> --- a/target/riscv/csr.c >> +++ b/target/riscv/csr.c >> @@ -1329,7 +1329,9 @@ static RISCVException read_misa(CPURISCVState *env, int csrno, >> static RISCVException write_misa(CPURISCVState *env, int csrno, >> target_ulong val) >> { >> - if (!riscv_feature(env, RISCV_FEATURE_MISA)) { >> + RISCVCPU *cpu = env_archcpu(env); >> + >> + if (!cpu->cfg.misa_w) { > > It's Ok to get it directly from cfg. However, personally, I prefer to keep the non-isa features in a separate list. I don't mind a separated non-isa list. cpu->cfg has everything contained in it though, ISA and non-ISA (e.g. vendor extensions that weren't ratified yet), and the current RISCV_FEATURES_* list is just a duplicate of it that we need to update it during riscv_cpu_realize(). In my opinion we can spare the extra effort of keeping a separated, up-to-date non-ISA extension/features list, by just reading everything from cfg. Thanks, Daniel > > Regards, > > Weiwei Li > >> /* drop write to misa */ >> return RISCV_EXCP_NONE; >> } >
On 2023/2/11 19:50, Daniel Henrique Barboza wrote: > > > On 2/10/23 23:43, weiwei wrote: >> >> On 2023/2/10 21:36, Daniel Henrique Barboza wrote: >>> At this moment, and apparently since ever, we have no way of enabling >>> RISCV_FEATURE_MISA. This means that all the code from write_misa(), all >>> the nuts and bolts that handles how to properly write this CSR, has >>> always been a no-op as well because write_misa() will always exit >>> earlier. >>> >>> This seems to be benign in the majority of cases. Booting an Ubuntu >>> 'virt' guest and logging all the calls to 'write_misa' shows that no >>> writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling >>> RISC-V extensions after the machine is powered on, seems to be a niche >>> use. >>> >>> There is a good chance that the code in write_misa() hasn't been >>> properly tested. Allowing users to write MISA can open the >>> floodgates of >>> new breeds of bugs. We could instead remove most (if not all) of >>> write_misa() since it's never used. Well, as a hardware emulator, >>> dealing with crashes because a register write went wrong is what we're >>> here for. >>> >>> Create a 'misa-w' CPU property to allow users to choose whether writes >>> to MISA should be allowed. The default is set to 'false' for all RISC-V >>> machines to keep compatibility with what we´ve been doing so far. >>> >>> Read cpu->cfg.misa_w directly in write_misa(), instead of executing >>> riscv_set_feature(RISCV_FEATURE_MISA) in riscv_cpu_realize(), that >>> would >>> simply reflect the cpu->cfg.misa_w bool value in 'env->features' and >>> require a riscv_feature() call to read it back. >>> >>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >>> --- >>> target/riscv/cpu.c | 1 + >>> target/riscv/cpu.h | 1 + >>> target/riscv/csr.c | 4 +++- >>> 3 files changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >>> index 93b52b826c..69fb9e123f 100644 >>> --- a/target/riscv/cpu.c >>> +++ b/target/riscv/cpu.c >>> @@ -1197,6 +1197,7 @@ static void register_cpu_props(DeviceState *dev) >>> static Property riscv_cpu_properties[] = { >>> DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true), >>> + DEFINE_PROP_BOOL("misa-w", RISCVCPU, cfg.misa_w, false), >>> DEFINE_PROP_UINT32("mvendorid", RISCVCPU, cfg.mvendorid, 0), >>> DEFINE_PROP_UINT64("marchid", RISCVCPU, cfg.marchid, >>> RISCV_CPU_MARCHID), >>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >>> index 7128438d8e..103963b386 100644 >>> --- a/target/riscv/cpu.h >>> +++ b/target/riscv/cpu.h >>> @@ -498,6 +498,7 @@ struct RISCVCPUConfig { >>> bool pmp; >>> bool epmp; >>> bool debug; >>> + bool misa_w; >>> bool short_isa_string; >>> }; >>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >>> index e149b453da..4f9cc501b2 100644 >>> --- a/target/riscv/csr.c >>> +++ b/target/riscv/csr.c >>> @@ -1329,7 +1329,9 @@ static RISCVException read_misa(CPURISCVState >>> *env, int csrno, >>> static RISCVException write_misa(CPURISCVState *env, int csrno, >>> target_ulong val) >>> { >>> - if (!riscv_feature(env, RISCV_FEATURE_MISA)) { >>> + RISCVCPU *cpu = env_archcpu(env); >>> + >>> + if (!cpu->cfg.misa_w) { >> >> It's Ok to get it directly from cfg. However, personally, I prefer to >> keep the non-isa features in a separate list. > > I don't mind a separated non-isa list. cpu->cfg has everything > contained in it > though, ISA and non-ISA (e.g. vendor extensions that weren't ratified > yet), and > the current RISCV_FEATURES_* list is just a duplicate of it that we > need to > update it during riscv_cpu_realize(). > > In my opinion we can spare the extra effort of keeping a separated, > up-to-date > non-ISA extension/features list, by just reading everything from cfg. > > > Thanks, > > > Daniel OK. It's acceptable to me. Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn> By the way, the riscv_cpu_cfg() in patch 4 can be used here. Regards, Weiwei Li > >> >> Regards, >> >> Weiwei Li >> >>> /* drop write to misa */ >>> return RISCV_EXCP_NONE; >>> } >>
On 2/14/23 12:12, weiwei wrote: > > On 2023/2/11 19:50, Daniel Henrique Barboza wrote: >> >> >> On 2/10/23 23:43, weiwei wrote: >>> >>> On 2023/2/10 21:36, Daniel Henrique Barboza wrote: >>>> At this moment, and apparently since ever, we have no way of enabling >>>> RISCV_FEATURE_MISA. This means that all the code from write_misa(), all >>>> the nuts and bolts that handles how to properly write this CSR, has >>>> always been a no-op as well because write_misa() will always exit >>>> earlier. >>>> >>>> This seems to be benign in the majority of cases. Booting an Ubuntu >>>> 'virt' guest and logging all the calls to 'write_misa' shows that no >>>> writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling >>>> RISC-V extensions after the machine is powered on, seems to be a niche >>>> use. >>>> >>>> There is a good chance that the code in write_misa() hasn't been >>>> properly tested. Allowing users to write MISA can open the floodgates of >>>> new breeds of bugs. We could instead remove most (if not all) of >>>> write_misa() since it's never used. Well, as a hardware emulator, >>>> dealing with crashes because a register write went wrong is what we're >>>> here for. >>>> >>>> Create a 'misa-w' CPU property to allow users to choose whether writes >>>> to MISA should be allowed. The default is set to 'false' for all RISC-V >>>> machines to keep compatibility with what we´ve been doing so far. >>>> >>>> Read cpu->cfg.misa_w directly in write_misa(), instead of executing >>>> riscv_set_feature(RISCV_FEATURE_MISA) in riscv_cpu_realize(), that would >>>> simply reflect the cpu->cfg.misa_w bool value in 'env->features' and >>>> require a riscv_feature() call to read it back. >>>> >>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >>>> --- >>>> target/riscv/cpu.c | 1 + >>>> target/riscv/cpu.h | 1 + >>>> target/riscv/csr.c | 4 +++- >>>> 3 files changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >>>> index 93b52b826c..69fb9e123f 100644 >>>> --- a/target/riscv/cpu.c >>>> +++ b/target/riscv/cpu.c >>>> @@ -1197,6 +1197,7 @@ static void register_cpu_props(DeviceState *dev) >>>> static Property riscv_cpu_properties[] = { >>>> DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true), >>>> + DEFINE_PROP_BOOL("misa-w", RISCVCPU, cfg.misa_w, false), >>>> DEFINE_PROP_UINT32("mvendorid", RISCVCPU, cfg.mvendorid, 0), >>>> DEFINE_PROP_UINT64("marchid", RISCVCPU, cfg.marchid, RISCV_CPU_MARCHID), >>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >>>> index 7128438d8e..103963b386 100644 >>>> --- a/target/riscv/cpu.h >>>> +++ b/target/riscv/cpu.h >>>> @@ -498,6 +498,7 @@ struct RISCVCPUConfig { >>>> bool pmp; >>>> bool epmp; >>>> bool debug; >>>> + bool misa_w; >>>> bool short_isa_string; >>>> }; >>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >>>> index e149b453da..4f9cc501b2 100644 >>>> --- a/target/riscv/csr.c >>>> +++ b/target/riscv/csr.c >>>> @@ -1329,7 +1329,9 @@ static RISCVException read_misa(CPURISCVState *env, int csrno, >>>> static RISCVException write_misa(CPURISCVState *env, int csrno, >>>> target_ulong val) >>>> { >>>> - if (!riscv_feature(env, RISCV_FEATURE_MISA)) { >>>> + RISCVCPU *cpu = env_archcpu(env); >>>> + >>>> + if (!cpu->cfg.misa_w) { >>> >>> It's Ok to get it directly from cfg. However, personally, I prefer to keep the non-isa features in a separate list. >> >> I don't mind a separated non-isa list. cpu->cfg has everything contained in it >> though, ISA and non-ISA (e.g. vendor extensions that weren't ratified yet), and >> the current RISCV_FEATURES_* list is just a duplicate of it that we need to >> update it during riscv_cpu_realize(). >> >> In my opinion we can spare the extra effort of keeping a separated, up-to-date >> non-ISA extension/features list, by just reading everything from cfg. >> >> >> Thanks, >> >> >> Daniel > > OK. It's acceptable to me. > > Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn> > > By the way, the riscv_cpu_cfg() in patch 4 can be used here. Good point. I'll move patch 4 up so I can use that function here. Daniel > > Regards, > > Weiwei Li > >> >>> >>> Regards, >>> >>> Weiwei Li >>> >>>> /* drop write to misa */ >>>> return RISCV_EXCP_NONE; >>>> } >>> >
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 93b52b826c..69fb9e123f 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -1197,6 +1197,7 @@ static void register_cpu_props(DeviceState *dev) static Property riscv_cpu_properties[] = { DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true), + DEFINE_PROP_BOOL("misa-w", RISCVCPU, cfg.misa_w, false), DEFINE_PROP_UINT32("mvendorid", RISCVCPU, cfg.mvendorid, 0), DEFINE_PROP_UINT64("marchid", RISCVCPU, cfg.marchid, RISCV_CPU_MARCHID), diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index 7128438d8e..103963b386 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -498,6 +498,7 @@ struct RISCVCPUConfig { bool pmp; bool epmp; bool debug; + bool misa_w; bool short_isa_string; }; diff --git a/target/riscv/csr.c b/target/riscv/csr.c index e149b453da..4f9cc501b2 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -1329,7 +1329,9 @@ static RISCVException read_misa(CPURISCVState *env, int csrno, static RISCVException write_misa(CPURISCVState *env, int csrno, target_ulong val) { - if (!riscv_feature(env, RISCV_FEATURE_MISA)) { + RISCVCPU *cpu = env_archcpu(env); + + if (!cpu->cfg.misa_w) { /* drop write to misa */ return RISCV_EXCP_NONE; }
At this moment, and apparently since ever, we have no way of enabling RISCV_FEATURE_MISA. This means that all the code from write_misa(), all the nuts and bolts that handles how to properly write this CSR, has always been a no-op as well because write_misa() will always exit earlier. This seems to be benign in the majority of cases. Booting an Ubuntu 'virt' guest and logging all the calls to 'write_misa' shows that no writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling RISC-V extensions after the machine is powered on, seems to be a niche use. There is a good chance that the code in write_misa() hasn't been properly tested. Allowing users to write MISA can open the floodgates of new breeds of bugs. We could instead remove most (if not all) of write_misa() since it's never used. Well, as a hardware emulator, dealing with crashes because a register write went wrong is what we're here for. Create a 'misa-w' CPU property to allow users to choose whether writes to MISA should be allowed. The default is set to 'false' for all RISC-V machines to keep compatibility with what we´ve been doing so far. Read cpu->cfg.misa_w directly in write_misa(), instead of executing riscv_set_feature(RISCV_FEATURE_MISA) in riscv_cpu_realize(), that would simply reflect the cpu->cfg.misa_w bool value in 'env->features' and require a riscv_feature() call to read it back. Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- target/riscv/cpu.c | 1 + target/riscv/cpu.h | 1 + target/riscv/csr.c | 4 +++- 3 files changed, 5 insertions(+), 1 deletion(-)