Re: [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match

From: Anup Patel <anup@brainfault.org>
To: Alistair Francis <alistair23@gmail.com>
Cc: Anup Patel <apatel@ventanamicro.com>,
	Peter Maydell <peter.maydell@linaro.org>,
	 Palmer Dabbelt <palmer@dabbelt.com>,
	Alistair Francis <Alistair.Francis@wdc.com>,
	 Sagar Karandikar <sagark@eecs.berkeley.edu>,
	Atish Patra <atishp@atishpatra.org>,
	 "open list:RISC-V" <qemu-riscv@nongnu.org>,
	 "qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>
Subject: Re: [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match
Date: Tue, 24 May 2022 17:40:24 +0530	[thread overview]
Message-ID: <CAAhSdy0iP+js5JTSXBsD-O3KennBR++_v4z7m55bztEyrgwvPQ@mail.gmail.com> (raw)
In-Reply-To: <CAKmqyKMy66XQsgmAnMEEaYrH7k1g2MC6kHqrPaRWKxvSuOz3Xw@mail.gmail.com>

On Tue, May 24, 2022 at 3:22 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Fri, May 20, 2022 at 1:07 AM Anup Patel <anup@brainfault.org> wrote:
> >
> > On Tue, May 17, 2022 at 5:46 AM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Thu, May 12, 2022 at 12:52 AM Anup Patel <apatel@ventanamicro.com> wrote:
> > > >
> > > > We should disable extensions in riscv_cpu_realize() if minimum required
> > > > priv spec version is not satisfied. This also ensures that machines with
> > > > priv spec v1.11 (or lower) cannot enable H, V, and various multi-letter
> > > > extensions.
> > > >
> > > > Fixes: a775398be2e ("target/riscv: Add isa extenstion strings to the
> > > > device tree")
> > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > >
> > > This will potentially confuse users as we just disable the extension
> > > without telling them.
> > >
> > > Could we not just leave this as is and let users specify the
> > > extensions they want? Then it's up to them to specify the correct
> > > combinations
> >
> > The ISA extensions are not independent of the Priv spec version.
> >
> > For example, we have bits for Sstc, Svpbmt, and Zicbo[m|p|z] extensions
> > in xenvcfg CSRs which are only available for Priv v1.12 spec.
> >
> > We can't allow users to enable extensions which don't meet
> > the Priv spec version requirements.
>
> Fair point. Ok we should at least report a warning if any of these are
> set though

Okay, I will update this patch accordingly.

Regards,
Anup

>
> Alistair
>
> >
> > Regards,
> > Anup
> >
> > >
> > > Alistair
> > >
> > > > ---
> > > >  target/riscv/cpu.c | 34 ++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 34 insertions(+)
> > > >
> > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > index f3b61dfd63..25a4ba3e22 100644
> > > > --- a/target/riscv/cpu.c
> > > > +++ b/target/riscv/cpu.c
> > > > @@ -541,6 +541,40 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > > >          set_priv_version(env, priv_version);
> > > >      }
> > > >
> > > > +    /* Force disable extensions if priv spec version does not match */
> > > > +    if (env->priv_ver < PRIV_VERSION_1_12_0) {
> > > > +        cpu->cfg.ext_h = false;
> > > > +        cpu->cfg.ext_v = false;
> > > > +        cpu->cfg.ext_zfh = false;
> > > > +        cpu->cfg.ext_zfhmin = false;
> > > > +        cpu->cfg.ext_zfinx = false;
> > > > +        cpu->cfg.ext_zhinx = false;
> > > > +        cpu->cfg.ext_zhinxmin = false;
> > > > +        cpu->cfg.ext_zdinx = false;
> > > > +        cpu->cfg.ext_zba = false;
> > > > +        cpu->cfg.ext_zbb = false;
> > > > +        cpu->cfg.ext_zbc = false;
> > > > +        cpu->cfg.ext_zbkb = false;
> > > > +        cpu->cfg.ext_zbkc = false;
> > > > +        cpu->cfg.ext_zbkx = false;
> > > > +        cpu->cfg.ext_zbs = false;
> > > > +        cpu->cfg.ext_zk = false;
> > > > +        cpu->cfg.ext_zkn = false;
> > > > +        cpu->cfg.ext_zknd = false;
> > > > +        cpu->cfg.ext_zkne = false;
> > > > +        cpu->cfg.ext_zknh = false;
> > > > +        cpu->cfg.ext_zkr = false;
> > > > +        cpu->cfg.ext_zks = false;
> > > > +        cpu->cfg.ext_zksed = false;
> > > > +        cpu->cfg.ext_zksh = false;
> > > > +        cpu->cfg.ext_zkt = false;
> > > > +        cpu->cfg.ext_zve32f = false;
> > > > +        cpu->cfg.ext_zve64f = false;
> > > > +        cpu->cfg.ext_svinval = false;
> > > > +        cpu->cfg.ext_svnapot = false;
> > > > +        cpu->cfg.ext_svpbmt = false;
> > > > +    }
> > > > +
> > > >      if (cpu->cfg.mmu) {
> > > >          riscv_set_feature(env, RISCV_FEATURE_MMU);
> > > >      }
> > > > --
> > > > 2.34.1
> > > >
> > > >