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) {
         /* drop write to misa */
         return RISCV_EXCP_NONE;
     }
-- 
2.39.1

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;
>>>>       }
>>>
>