| Message ID | 20240109171848.32237-4-rbradford@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: Add support for 'B' extension | expand |
On 1/9/24 14:07, Rob Bradford wrote: > Signed-off-by: Rob Bradford <rbradford@rivosinc.com> > --- > target/riscv/tcg/tcg-cpu.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > index f10871d352..9705daec93 100644 > --- a/target/riscv/tcg/tcg-cpu.c > +++ b/target/riscv/tcg/tcg-cpu.c > @@ -999,7 +999,8 @@ static void riscv_init_max_cpu_extensions(Object *obj) > const RISCVCPUMultiExtConfig *prop; > > /* Enable RVG, RVJ and RVV that are disabled by default */ > - riscv_cpu_set_misa(env, env->misa_mxl, env->misa_ext | RVG | RVJ | RVV); > + riscv_cpu_set_misa(env, env->misa_mxl, > + env->misa_ext | RVG | RVJ | RVV | RVB); I'm aware that we decided a while ago the 'max' CPU could only have non-vendor and non-experimental extensions enabled. RVB is experimental, so in theory we shouldn't enable it. But RVB is an alias for zba, zbb and zbs, extensions that the 'max' CPU is already enabling. In this case I think it's sensible to enable RVB here since it would just reflect stuff that it's already happening. Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > > for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { > isa_ext_update_enabled(cpu, prop->offset, true);
Rob, Given that you'll need to resend the patches due to the conflict in patch 2, I think it would be nice to mention in this commit message that we're ok with enabling RVB in the 'max' CPU, even though RVB per se is experimental, because it's just an alias for extensions that the CPU already uses. I'm asking this because future Daniel will forget why we're adding an experimental extension in the 'max' CPU, and this info in the commit msg will spare him from searching the ML archives to discover why :) Thanks, Daniel On 1/10/24 15:32, Daniel Henrique Barboza wrote: > > > On 1/9/24 14:07, Rob Bradford wrote: >> Signed-off-by: Rob Bradford <rbradford@rivosinc.com> >> --- >> target/riscv/tcg/tcg-cpu.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c >> index f10871d352..9705daec93 100644 >> --- a/target/riscv/tcg/tcg-cpu.c >> +++ b/target/riscv/tcg/tcg-cpu.c >> @@ -999,7 +999,8 @@ static void riscv_init_max_cpu_extensions(Object *obj) >> const RISCVCPUMultiExtConfig *prop; >> /* Enable RVG, RVJ and RVV that are disabled by default */ >> - riscv_cpu_set_misa(env, env->misa_mxl, env->misa_ext | RVG | RVJ | RVV); >> + riscv_cpu_set_misa(env, env->misa_mxl, >> + env->misa_ext | RVG | RVJ | RVV | RVB); > > I'm aware that we decided a while ago the 'max' CPU could only have non-vendor and > non-experimental extensions enabled. RVB is experimental, so in theory we shouldn't > enable it. > > But RVB is an alias for zba, zbb and zbs, extensions that the 'max' CPU is already > enabling. In this case I think it's sensible to enable RVB here since it would just > reflect stuff that it's already happening. > > > Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > > > > >> for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { >> isa_ext_update_enabled(cpu, prop->offset, true);
On Wed, Jan 10, 2024 at 03:32:21PM -0300, Daniel Henrique Barboza wrote: > > > On 1/9/24 14:07, Rob Bradford wrote: > > Signed-off-by: Rob Bradford <rbradford@rivosinc.com> > > --- > > target/riscv/tcg/tcg-cpu.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > > index f10871d352..9705daec93 100644 > > --- a/target/riscv/tcg/tcg-cpu.c > > +++ b/target/riscv/tcg/tcg-cpu.c > > @@ -999,7 +999,8 @@ static void riscv_init_max_cpu_extensions(Object *obj) > > const RISCVCPUMultiExtConfig *prop; > > /* Enable RVG, RVJ and RVV that are disabled by default */ > > - riscv_cpu_set_misa(env, env->misa_mxl, env->misa_ext | RVG | RVJ | RVV); > > + riscv_cpu_set_misa(env, env->misa_mxl, > > + env->misa_ext | RVG | RVJ | RVV | RVB); > > I'm aware that we decided a while ago the 'max' CPU could only have non-vendor and > non-experimental extensions enabled. RVB is experimental, so in theory we shouldn't > enable it. > > But RVB is an alias for zba, zbb and zbs, extensions that the 'max' CPU is already > enabling. In this case I think it's sensible to enable RVB here since it would just > reflect stuff that it's already happening. It's also setting the B bit in misa, which, until this spec is at least frozen, is a reserved bit and reserved bits "must return zero when read". I don't want to stand in the way of progress and it seems 99.9% likely that the spec will be frozen and ratified, but, if we want to stick to our policies (which we should document), then even the 'max' cpu type should require x-b be added to the command line if it wants the B bit set in misa. Thanks, drew
On 1/11/24 10:02, Andrew Jones wrote: > On Wed, Jan 10, 2024 at 03:32:21PM -0300, Daniel Henrique Barboza wrote: >> >> >> On 1/9/24 14:07, Rob Bradford wrote: >>> Signed-off-by: Rob Bradford <rbradford@rivosinc.com> >>> --- >>> target/riscv/tcg/tcg-cpu.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c >>> index f10871d352..9705daec93 100644 >>> --- a/target/riscv/tcg/tcg-cpu.c >>> +++ b/target/riscv/tcg/tcg-cpu.c >>> @@ -999,7 +999,8 @@ static void riscv_init_max_cpu_extensions(Object *obj) >>> const RISCVCPUMultiExtConfig *prop; >>> /* Enable RVG, RVJ and RVV that are disabled by default */ >>> - riscv_cpu_set_misa(env, env->misa_mxl, env->misa_ext | RVG | RVJ | RVV); >>> + riscv_cpu_set_misa(env, env->misa_mxl, >>> + env->misa_ext | RVG | RVJ | RVV | RVB); >> >> I'm aware that we decided a while ago the 'max' CPU could only have non-vendor and >> non-experimental extensions enabled. RVB is experimental, so in theory we shouldn't >> enable it. >> >> But RVB is an alias for zba, zbb and zbs, extensions that the 'max' CPU is already >> enabling. In this case I think it's sensible to enable RVB here since it would just >> reflect stuff that it's already happening. > > It's also setting the B bit in misa, which, until this spec is at least > frozen, is a reserved bit and reserved bits "must return zero when read". This is a side effect that I wasn't aware of. Rob, given that the 'max' CPU already has the zb* extensions enabled, is there any other gain in enabling RVB in this CPU? If there isn't I'd rather leave this one out for now. Thanks, Daniel > > I don't want to stand in the way of progress and it seems 99.9% likely > that the spec will be frozen and ratified, but, if we want to stick to > our policies (which we should document), then even the 'max' cpu type > should require x-b be added to the command line if it wants the B bit > set in misa. > > Thanks, > drew
On Thu, 2024-01-11 at 11:53 -0300, Daniel Henrique Barboza wrote: > > > On 1/11/24 10:02, Andrew Jones wrote: > > On Wed, Jan 10, 2024 at 03:32:21PM -0300, Daniel Henrique Barboza > > wrote: > > > > > > > > > On 1/9/24 14:07, Rob Bradford wrote: > > > > Signed-off-by: Rob Bradford <rbradford@rivosinc.com> > > > > --- > > > > target/riscv/tcg/tcg-cpu.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg- > > > > cpu.c > > > > index f10871d352..9705daec93 100644 > > > > --- a/target/riscv/tcg/tcg-cpu.c > > > > +++ b/target/riscv/tcg/tcg-cpu.c > > > > @@ -999,7 +999,8 @@ static void > > > > riscv_init_max_cpu_extensions(Object *obj) > > > > const RISCVCPUMultiExtConfig *prop; > > > > /* Enable RVG, RVJ and RVV that are disabled by default > > > > */ > > > > - riscv_cpu_set_misa(env, env->misa_mxl, env->misa_ext | RVG > > > > | RVJ | RVV); > > > > + riscv_cpu_set_misa(env, env->misa_mxl, > > > > + env->misa_ext | RVG | RVJ | RVV | RVB); > > > > > > I'm aware that we decided a while ago the 'max' CPU could only > > > have non-vendor and > > > non-experimental extensions enabled. RVB is experimental, so in > > > theory we shouldn't > > > enable it. > > > > > > But RVB is an alias for zba, zbb and zbs, extensions that the > > > 'max' CPU is already > > > enabling. In this case I think it's sensible to enable RVB here > > > since it would > > > just > > > > > > reflect stuff that it's already happening. > > > > It's also setting the B bit in misa, which, until this spec is at > > least > > frozen, is a reserved bit and reserved bits "must return zero when > > read". > > This is a side effect that I wasn't aware of. > > Rob, given that the 'max' CPU already has the zb* extensions enabled, > is there any > other gain in enabling RVB in this CPU? If there isn't I'd rather > leave this one > out for now. > It seems completely reasonable to me to drop it for now. Thanks for all the feedback, Rob > > Thanks, > > Daniel > > > > > > I don't want to stand in the way of progress and it seems 99.9% > > likely > > that the spec will be frozen and ratified, but, if we want to stick > > to > > our policies (which we should document), then even the 'max' cpu > > type > > should require x-b be added to the command line if it wants the B > > bit > > set in misa. > > > > Thanks, > > drew
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c index f10871d352..9705daec93 100644 --- a/target/riscv/tcg/tcg-cpu.c +++ b/target/riscv/tcg/tcg-cpu.c @@ -999,7 +999,8 @@ static void riscv_init_max_cpu_extensions(Object *obj) const RISCVCPUMultiExtConfig *prop; /* Enable RVG, RVJ and RVV that are disabled by default */ - riscv_cpu_set_misa(env, env->misa_mxl, env->misa_ext | RVG | RVJ | RVV); + riscv_cpu_set_misa(env, env->misa_mxl, + env->misa_ext | RVG | RVJ | RVV | RVB); for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { isa_ext_update_enabled(cpu, prop->offset, true);
Signed-off-by: Rob Bradford <rbradford@rivosinc.com> --- target/riscv/tcg/tcg-cpu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)