Re: [PATCH 02/11] target/riscv: allow users to actually write the MISA CSR

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

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