| Message ID | 20220511144528.393530-8-apatel@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QEMU RISC-V nested virtualization fixes | expand |
On Wed, May 11, 2022 at 7:46 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> > --- > 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; > + } > + This will not scale in future with new extensions in the next few years. We probably need to make more intrusive changes to tie up the min_version for extensions as well. A brief look at the "Property" data structure and couldn't find an easy way to do it. For this patch, Reviewed-by: Atish Patra <atishp@rivosinc.com> > if (cpu->cfg.mmu) { > riscv_set_feature(env, RISCV_FEATURE_MMU); > } > -- > 2.34.1 >
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 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 > >
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. 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 > > > >
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 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 > > > > > >
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 > > > > > > > >
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); }
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> --- target/riscv/cpu.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)