Re: [PATCH for-8.1 v4 18/25] target/riscv: error out on priv failure for RVH

[Daniel Henrique Barboza]

On 3/24/23 11:56, liweiwei wrote:
> On 2023/3/23 06:19, Daniel Henrique Barboza wrote:
> > riscv_cpu_disable_priv_spec_isa_exts(), at the end of
> > riscv_cpu_validate_set_extensions(), will disable cpu->cfg.ext_h and
> > cpu->cfg.ext_v if priv_ver check fails.
> >
> > This check can be done in riscv_cpu_validate_misa_ext(). The difference
> > here is that we're not silently disable it: we'll error out. Silently
> > disabling a MISA extension after all the validation is completed can can
> > cause inconsistencies that we don't have to deal with. Verify ealier and
> > fail faster.
> >
> > Note that we're ignoring RVV priv_ver validation since its minimal priv
> > is also the minimal value we support. RVH will error out if enabled
> > under priv_ver under 1_12_0.
> >
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >   target/riscv/cpu.c | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 399f63b42f..d2eb2b3ba1 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -1055,6 +1055,20 @@ static void riscv_cpu_validate_misa_ext(RISCVCPU *cpu, 
> > Error **errp)
> >           return;
> >       }
> > +    /*
> > +     * Check for priv spec version. RVH is 1_12_0, RVV is 1_10_0.
> > +     * We don't support anything under 1_10_0 so skip checking
> > +     * priv for RVV.
> > +     *
> > +     * We're hardcoding it here to avoid looping into the
> > +     * 50+ entries of isa_edata_arr[] just to check the RVH
> > +     * entry.
> > +     */
> > +    if (cpu->cfg.ext_h && env->priv_ver < PRIV_VERSION_1_12_0) {
> > +        error_setg(errp, "H extension requires priv spec 1.12.0");
> > +        return;
> > +    }
> The other multi-letter extensions are directly disabled for lower priv version 
> with warning message.
>
> Whether we should do the similar action here?

I'd rather error out in all cases, to be honest. cpu_init() functions should be
responsible for choosing an adequate priv spec level for the extensions it wants
to use.


Thanks,


Daniel

> Regards,
>
> Weiwei Li
>
> > +
> >       if (cpu->cfg.ext_v) {
> >           riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
> >           if (local_err != NULL) {