diff mbox series

[v12,7/7] target/riscv: Enable updates for pointer masking variables and thus enable pointer masking extension

Message ID 20241205112304.593204-8-baturo.alexey@gmail.com (mailing list archive)
State New
Headers show
Series Pointer Masking update for Zjpm v1.0 | expand

Commit Message

Alexey Baturo Dec. 5, 2024, 11:23 a.m. UTC 
From: Alexey Baturo <baturo.alexey@gmail.com>

Signed-off-by: Alexey Baturo <baturo.alexey@gmail.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Daniel Henrique Barboza Dec. 12, 2024, 9:48 a.m. UTC | #1 
On 12/5/24 8:23 AM, baturo.alexey@gmail.com wrote:
> From: Alexey Baturo <baturo.alexey@gmail.com>
> 
> Signed-off-by: Alexey Baturo <baturo.alexey@gmail.com>
> 
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   target/riscv/cpu.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 4e80dcd2e6..fd3ea9ce76 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -186,11 +186,14 @@ const RISCVIsaExtData isa_edata_arr[] = {
>       ISA_EXT_DATA_ENTRY(smaia, PRIV_VERSION_1_12_0, ext_smaia),
>       ISA_EXT_DATA_ENTRY(smcntrpmf, PRIV_VERSION_1_12_0, ext_smcntrpmf),
>       ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
> +    ISA_EXT_DATA_ENTRY(smmpm, PRIV_VERSION_1_13_0, ext_smmpm),
> +    ISA_EXT_DATA_ENTRY(smnpm, PRIV_VERSION_1_13_0, ext_smnpm),
>       ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
>       ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
>       ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, has_priv_1_11),
>       ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
>       ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, has_priv_1_12),
> +    ISA_EXT_DATA_ENTRY(ssnpm, PRIV_VERSION_1_13_0, ext_ssnpm),

I just realized that we're not adding "ext_supm":

"Supm Pointer masking, with the execution environment providing a means
to select PMLEN=0 and PMLEN=7 at minimum."

IIUC this is always enabled in the code so this would be a flag that would
be always enabled, i.e. it would be a ISA_EXT_DATA_ENTRY that defaults to
"has_priv_1_13". "sscounterenw" is an example of this kind of extension.

If that's really the case I believe you can add "supm" in this patch or maybe
a new patch right after. We need to advertise support for "supm" for RVA23
anyway.


Thanks,

Daniel


>       ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
>       ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, has_priv_1_12),
>       ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, has_priv_1_12),
> @@ -1490,9 +1493,12 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
>       MULTI_EXT_CFG_BOOL("zvfh", ext_zvfh, false),
>       MULTI_EXT_CFG_BOOL("zvfhmin", ext_zvfhmin, false),
>       MULTI_EXT_CFG_BOOL("sstc", ext_sstc, true),
> +    MULTI_EXT_CFG_BOOL("ssnpm", ext_ssnpm, false),
>   
>       MULTI_EXT_CFG_BOOL("smaia", ext_smaia, false),
>       MULTI_EXT_CFG_BOOL("smepmp", ext_smepmp, false),
> +    MULTI_EXT_CFG_BOOL("smmpm", ext_smmpm, false),
> +    MULTI_EXT_CFG_BOOL("smnpm", ext_smnpm, false),
>       MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
>       MULTI_EXT_CFG_BOOL("ssaia", ext_ssaia, false),
>       MULTI_EXT_CFG_BOOL("svade", ext_svade, false),
Alexey Baturo Dec. 16, 2024, 12:13 p.m. UTC | #2 
Hi Daniel,

Indeed this series doesn't include the *Supm* extension, but it's mandatory
for RVA23U64.
As for the *Supm* itself, if I get it right, I don't think it should be
always-on, unless the proper profile is selected.
I think we might need to add extra flags like *ext_supm* and *ext_sspm*.
Then, to avoid adding extra checks to the existing code, I think we could
enable *Ssnpm* or *Smnpm* or both somewhere during the initialization if
*Supm* is set. And do the same for *Sspm*.
From what I see right now in the code, there's no mention of RVA23, so
maybe we could just add some fields for the missing extensions and support
them later, when RVA23 support is implemented.
Personally, I'd like to do this in a separate patch after these ones are
merged.

What do you think?

Thanks

чт, 12 дек. 2024 г. в 12:48, Daniel Henrique Barboza <
dbarboza@ventanamicro.com>:

>
>
> On 12/5/24 8:23 AM, baturo.alexey@gmail.com wrote:
> > From: Alexey Baturo <baturo.alexey@gmail.com>
> >
> > Signed-off-by: Alexey Baturo <baturo.alexey@gmail.com>
> >
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >   target/riscv/cpu.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 4e80dcd2e6..fd3ea9ce76 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -186,11 +186,14 @@ const RISCVIsaExtData isa_edata_arr[] = {
> >       ISA_EXT_DATA_ENTRY(smaia, PRIV_VERSION_1_12_0, ext_smaia),
> >       ISA_EXT_DATA_ENTRY(smcntrpmf, PRIV_VERSION_1_12_0, ext_smcntrpmf),
> >       ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
> > +    ISA_EXT_DATA_ENTRY(smmpm, PRIV_VERSION_1_13_0, ext_smmpm),
> > +    ISA_EXT_DATA_ENTRY(smnpm, PRIV_VERSION_1_13_0, ext_smnpm),
> >       ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
> >       ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
> >       ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, has_priv_1_11),
> >       ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
> >       ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0,
> has_priv_1_12),
> > +    ISA_EXT_DATA_ENTRY(ssnpm, PRIV_VERSION_1_13_0, ext_ssnpm),
>
> I just realized that we're not adding "ext_supm":
>
> "Supm Pointer masking, with the execution environment providing a means
> to select PMLEN=0 and PMLEN=7 at minimum."
>
> IIUC this is always enabled in the code so this would be a flag that would
> be always enabled, i.e. it would be a ISA_EXT_DATA_ENTRY that defaults to
> "has_priv_1_13". "sscounterenw" is an example of this kind of extension.
>
> If that's really the case I believe you can add "supm" in this patch or
> maybe
> a new patch right after. We need to advertise support for "supm" for RVA23
> anyway.
>
>
> Thanks,
>
> Daniel
>
>
> >       ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
> >       ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, has_priv_1_12),
> >       ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, has_priv_1_12),
> > @@ -1490,9 +1493,12 @@ const RISCVCPUMultiExtConfig
> riscv_cpu_extensions[] = {
> >       MULTI_EXT_CFG_BOOL("zvfh", ext_zvfh, false),
> >       MULTI_EXT_CFG_BOOL("zvfhmin", ext_zvfhmin, false),
> >       MULTI_EXT_CFG_BOOL("sstc", ext_sstc, true),
> > +    MULTI_EXT_CFG_BOOL("ssnpm", ext_ssnpm, false),
> >
> >       MULTI_EXT_CFG_BOOL("smaia", ext_smaia, false),
> >       MULTI_EXT_CFG_BOOL("smepmp", ext_smepmp, false),
> > +    MULTI_EXT_CFG_BOOL("smmpm", ext_smmpm, false),
> > +    MULTI_EXT_CFG_BOOL("smnpm", ext_smnpm, false),
> >       MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
> >       MULTI_EXT_CFG_BOOL("ssaia", ext_ssaia, false),
> >       MULTI_EXT_CFG_BOOL("svade", ext_svade, false),
>
>
Daniel Henrique Barboza Dec. 16, 2024, 12:42 p.m. UTC | #3 
On 12/16/24 9:13 AM, Alexey Baturo wrote:
> Hi Daniel,
> 
> Indeed this series doesn't include the *Supm* extension, but it's mandatory for RVA23U64.
> As for the *Supm* itself, if I get it right, I don't think it should be always-on, unless the proper profile is selected.
> I think we might need to add extra flags like *ext_supm* and *ext_sspm*. Then, to avoid adding extra checks to the existing code, I think we could enable *Ssnpm* or *Smnpm* or both somewhere during the initialization if *Supm* is set. And do the same for *Sspm*.
>  From what I see right now in the code, there's no mention of RVA23, so maybe we could just add some fields for the missing extensions and support them later, when RVA23 support is implemented.
> Personally, I'd like to do this in a separate patch after these ones are merged.


The thing is that RVA23 depends on Supm. It would be strange to drop the
RVA23 patches (which I already have) and then implement Supm after.

If Supm must be implemented as its own flag, instead of being derived from
the state of other extensions, that's fine. We have existing code to handle
both scenarios. And when RVA23S64 is selected we will enable Supm automatically.
This is how the existing profiles work w.r.t their extensions and dependencies:
selecting a profile will enable a bunch of stuff under the hood.

If you want to do Supm later on after this work is merged, this is also fine.
As soon as you send Supm to the ML I can send the RVA23 patches and make them
dependent on Supm.


Thanks,

Daniel



> 
> What do you think?
> 
> Thanks
> 
> чт, 12 дек. 2024 г. в 12:48, Daniel Henrique Barboza <dbarboza@ventanamicro.com <mailto:dbarboza@ventanamicro.com>>:
> 
> 
> 
>     On 12/5/24 8:23 AM, baturo.alexey@gmail.com <mailto:baturo.alexey@gmail.com> wrote:
>      > From: Alexey Baturo <baturo.alexey@gmail.com <mailto:baturo.alexey@gmail.com>>
>      >
>      > Signed-off-by: Alexey Baturo <baturo.alexey@gmail.com <mailto:baturo.alexey@gmail.com>>
>      >
>      > Reviewed-by: Alistair Francis <alistair.francis@wdc.com <mailto:alistair.francis@wdc.com>>
>      > ---
>      >   target/riscv/cpu.c | 6 ++++++
>      >   1 file changed, 6 insertions(+)
>      >
>      > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>      > index 4e80dcd2e6..fd3ea9ce76 100644
>      > --- a/target/riscv/cpu.c
>      > +++ b/target/riscv/cpu.c
>      > @@ -186,11 +186,14 @@ const RISCVIsaExtData isa_edata_arr[] = {
>      >       ISA_EXT_DATA_ENTRY(smaia, PRIV_VERSION_1_12_0, ext_smaia),
>      >       ISA_EXT_DATA_ENTRY(smcntrpmf, PRIV_VERSION_1_12_0, ext_smcntrpmf),
>      >       ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
>      > +    ISA_EXT_DATA_ENTRY(smmpm, PRIV_VERSION_1_13_0, ext_smmpm),
>      > +    ISA_EXT_DATA_ENTRY(smnpm, PRIV_VERSION_1_13_0, ext_smnpm),
>      >       ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
>      >       ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
>      >       ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, has_priv_1_11),
>      >       ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
>      >       ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, has_priv_1_12),
>      > +    ISA_EXT_DATA_ENTRY(ssnpm, PRIV_VERSION_1_13_0, ext_ssnpm),
> 
>     I just realized that we're not adding "ext_supm":
> 
>     "Supm Pointer masking, with the execution environment providing a means
>     to select PMLEN=0 and PMLEN=7 at minimum."
> 
>     IIUC this is always enabled in the code so this would be a flag that would
>     be always enabled, i.e. it would be a ISA_EXT_DATA_ENTRY that defaults to
>     "has_priv_1_13". "sscounterenw" is an example of this kind of extension.
> 
>     If that's really the case I believe you can add "supm" in this patch or maybe
>     a new patch right after. We need to advertise support for "supm" for RVA23
>     anyway.
> 
> 
>     Thanks,
> 
>     Daniel
> 
> 
>      >       ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
>      >       ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, has_priv_1_12),
>      >       ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, has_priv_1_12),
>      > @@ -1490,9 +1493,12 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
>      >       MULTI_EXT_CFG_BOOL("zvfh", ext_zvfh, false),
>      >       MULTI_EXT_CFG_BOOL("zvfhmin", ext_zvfhmin, false),
>      >       MULTI_EXT_CFG_BOOL("sstc", ext_sstc, true),
>      > +    MULTI_EXT_CFG_BOOL("ssnpm", ext_ssnpm, false),
>      >
>      >       MULTI_EXT_CFG_BOOL("smaia", ext_smaia, false),
>      >       MULTI_EXT_CFG_BOOL("smepmp", ext_smepmp, false),
>      > +    MULTI_EXT_CFG_BOOL("smmpm", ext_smmpm, false),
>      > +    MULTI_EXT_CFG_BOOL("smnpm", ext_smnpm, false),
>      >       MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
>      >       MULTI_EXT_CFG_BOOL("ssaia", ext_ssaia, false),
>      >       MULTI_EXT_CFG_BOOL("svade", ext_svade, false),
>
diff mbox series

Patch

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 4e80dcd2e6..fd3ea9ce76 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -186,11 +186,14 @@  const RISCVIsaExtData isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(smaia, PRIV_VERSION_1_12_0, ext_smaia),
     ISA_EXT_DATA_ENTRY(smcntrpmf, PRIV_VERSION_1_12_0, ext_smcntrpmf),
     ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
+    ISA_EXT_DATA_ENTRY(smmpm, PRIV_VERSION_1_13_0, ext_smmpm),
+    ISA_EXT_DATA_ENTRY(smnpm, PRIV_VERSION_1_13_0, ext_smnpm),
     ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
     ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
     ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, has_priv_1_11),
     ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
     ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, has_priv_1_12),
+    ISA_EXT_DATA_ENTRY(ssnpm, PRIV_VERSION_1_13_0, ext_ssnpm),
     ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
     ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, has_priv_1_12),
     ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, has_priv_1_12),
@@ -1490,9 +1493,12 @@  const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
     MULTI_EXT_CFG_BOOL("zvfh", ext_zvfh, false),
     MULTI_EXT_CFG_BOOL("zvfhmin", ext_zvfhmin, false),
     MULTI_EXT_CFG_BOOL("sstc", ext_sstc, true),
+    MULTI_EXT_CFG_BOOL("ssnpm", ext_ssnpm, false),
 
     MULTI_EXT_CFG_BOOL("smaia", ext_smaia, false),
     MULTI_EXT_CFG_BOOL("smepmp", ext_smepmp, false),
+    MULTI_EXT_CFG_BOOL("smmpm", ext_smmpm, false),
+    MULTI_EXT_CFG_BOOL("smnpm", ext_smnpm, false),
     MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
     MULTI_EXT_CFG_BOOL("ssaia", ext_ssaia, false),
     MULTI_EXT_CFG_BOOL("svade", ext_svade, false),