[PATCH 1/2] riscv: zicond: make non-experimental

[PATCH 1/2] riscv: zicond: make non-experimental

[Vineet Gupta]

zicond is now codegen supported in both llvm and gcc.

This change allows seamless enabling/testing of zicond in downstream
projects. e.g. currently riscv-gnu-toolchain parses elf attributes
to create a cmdline for qemu but fails short of enabling it because of
the "x-" prefix.

Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
 target/riscv/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 6b93b04453c8..022bd9d01223 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1816,6 +1816,7 @@ static Property riscv_cpu_extensions[] = {
     DEFINE_PROP_BOOL("zcf", RISCVCPU, cfg.ext_zcf, false),
     DEFINE_PROP_BOOL("zcmp", RISCVCPU, cfg.ext_zcmp, false),
     DEFINE_PROP_BOOL("zcmt", RISCVCPU, cfg.ext_zcmt, false),
+    DEFINE_PROP_BOOL("zicond", RISCVCPU, cfg.ext_zicond, false),
 
     /* Vendor-specific custom extensions */
     DEFINE_PROP_BOOL("xtheadba", RISCVCPU, cfg.ext_xtheadba, false),
@@ -1832,7 +1833,6 @@ static Property riscv_cpu_extensions[] = {
     DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps, false),
 
     /* These are experimental so mark with 'x-' */
-    DEFINE_PROP_BOOL("x-zicond", RISCVCPU, cfg.ext_zicond, false),
 
     /* ePMP 0.9.3 */
     DEFINE_PROP_BOOL("x-epmp", RISCVCPU, cfg.epmp, false),
-- 
2.34.1

[PATCH 2/2] riscv: zicond: make default

[Vineet Gupta]

Again this helps with better testing and something qemu has been doing
with newer features anyways.

Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
 target/riscv/cpu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 022bd9d01223..e6e28414b223 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -438,6 +438,7 @@ static void rv64_thead_c906_cpu_init(Object *obj)
     cpu->cfg.ext_xtheadbs = true;
     cpu->cfg.ext_xtheadcmo = true;
     cpu->cfg.ext_xtheadcondmov = true;
+    cpu->cfg.ext_zicond = false;
     cpu->cfg.ext_xtheadfmemidx = true;
     cpu->cfg.ext_xtheadmac = true;
     cpu->cfg.ext_xtheadmemidx = true;
@@ -483,6 +484,7 @@ static void rv64_veyron_v1_cpu_init(Object *obj)
     cpu->cfg.ext_zbc = true;
     cpu->cfg.ext_zbs = true;
     cpu->cfg.ext_XVentanaCondOps = true;
+    cpu->cfg.ext_zicond = false;
 
     cpu->cfg.mvendorid = VEYRON_V1_MVENDORID;
     cpu->cfg.marchid = VEYRON_V1_MARCHID;
@@ -1816,7 +1818,7 @@ static Property riscv_cpu_extensions[] = {
     DEFINE_PROP_BOOL("zcf", RISCVCPU, cfg.ext_zcf, false),
     DEFINE_PROP_BOOL("zcmp", RISCVCPU, cfg.ext_zcmp, false),
     DEFINE_PROP_BOOL("zcmt", RISCVCPU, cfg.ext_zcmt, false),
-    DEFINE_PROP_BOOL("zicond", RISCVCPU, cfg.ext_zicond, false),
+    DEFINE_PROP_BOOL("zicond", RISCVCPU, cfg.ext_zicond, true),
 
     /* Vendor-specific custom extensions */
     DEFINE_PROP_BOOL("xtheadba", RISCVCPU, cfg.ext_xtheadba, false),
-- 
2.34.1

Re: [PATCH 1/2] riscv: zicond: make non-experimental

[Richard Henderson]

On 8/8/23 11:17, Vineet Gupta wrote:
> zicond is now codegen supported in both llvm and gcc.

It is still not in

https://wiki.riscv.org/display/HOME/Recently+Ratified+Extensions


r~

Re: [PATCH 1/2] riscv: zicond: make non-experimental

[Vineet Gupta]

On 8/8/23 11:29, Richard Henderson wrote:
> On 8/8/23 11:17, Vineet Gupta wrote:
>> zicond is now codegen supported in both llvm and gcc.
>
> It is still not in
>
> https://wiki.riscv.org/display/HOME/Recently+Ratified+Extensions

Right, its been frozen since April though and with support trickling in 
rest of tooling it becomes harder to test.
I don't know what exactly QEMU's policy is on this ?

-Vineet

Re: [PATCH 1/2] riscv: zicond: make non-experimental

[Palmer Dabbelt]

On Tue, 08 Aug 2023 11:45:49 PDT (-0700), Vineet Gupta wrote:
>
>
> On 8/8/23 11:29, Richard Henderson wrote:
>> On 8/8/23 11:17, Vineet Gupta wrote:
>>> zicond is now codegen supported in both llvm and gcc.
>>
>> It is still not in
>>
>> https://wiki.riscv.org/display/HOME/Recently+Ratified+Extensions
>
> Right, its been frozen since April though and with support trickling in
> rest of tooling it becomes harder to test.
> I don't know what exactly QEMU's policy is on this ?

IIUC we'd historically marked stuff as non-experimental when it's 
frozen, largely because ratification is such a nebulous process.  
There's obviously risk there, but there's risk to anything.  Last I can 
find is 260b594d8a ("RISC-V: Add Zawrs ISA extension support"), which 
specifically calls out Zawrs as frozen and IIUC adds support without the 
"x-" prefix.

I can't find anything written down about it, though...

Re: [PATCH 2/2] riscv: zicond: make default

[Daniel Henrique Barboza]

(CCing Alistair and other reviewers)

On 8/8/23 15:17, Vineet Gupta wrote:
> Again this helps with better testing and something qemu has been doing
> with newer features anyways.
> 
> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
> ---

Even if we can reach a consensus about removing the experimental (x- prefix) status
from an extension that is Frozen instead of ratified, enabling stuff in the default
CPUs because it's easier to test is something we would like to avoid. The rv64
CPU has a random set of extensions enabled for the most different and undocumented
reasons, and users don't know what they'll get because we keep beefing up the
generic CPUs arbitrarily.

Starting on QEMU 8.2 we'll have a 'max' CPU type that will enable all non-experimental
and non-vendor extensions by default, making it easier for tooling to test new
features/extensions. All tooling should consider changing their scripts to use the
'max' CPU when it's available.

For now, I fear that gcc and friends will still need to enable 'zicond' in the command
line via 'zicond=true'.  Thanks,


Daniel


>   target/riscv/cpu.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 022bd9d01223..e6e28414b223 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -438,6 +438,7 @@ static void rv64_thead_c906_cpu_init(Object *obj)
>       cpu->cfg.ext_xtheadbs = true;
>       cpu->cfg.ext_xtheadcmo = true;
>       cpu->cfg.ext_xtheadcondmov = true;
> +    cpu->cfg.ext_zicond = false;
>       cpu->cfg.ext_xtheadfmemidx = true;
>       cpu->cfg.ext_xtheadmac = true;
>       cpu->cfg.ext_xtheadmemidx = true;
> @@ -483,6 +484,7 @@ static void rv64_veyron_v1_cpu_init(Object *obj)
>       cpu->cfg.ext_zbc = true;
>       cpu->cfg.ext_zbs = true;
>       cpu->cfg.ext_XVentanaCondOps = true;
> +    cpu->cfg.ext_zicond = false;
>   
>       cpu->cfg.mvendorid = VEYRON_V1_MVENDORID;
>       cpu->cfg.marchid = VEYRON_V1_MARCHID;
> @@ -1816,7 +1818,7 @@ static Property riscv_cpu_extensions[] = {
>       DEFINE_PROP_BOOL("zcf", RISCVCPU, cfg.ext_zcf, false),
>       DEFINE_PROP_BOOL("zcmp", RISCVCPU, cfg.ext_zcmp, false),
>       DEFINE_PROP_BOOL("zcmt", RISCVCPU, cfg.ext_zcmt, false),
> -    DEFINE_PROP_BOOL("zicond", RISCVCPU, cfg.ext_zicond, false),
> +    DEFINE_PROP_BOOL("zicond", RISCVCPU, cfg.ext_zicond, true),
>   
>       /* Vendor-specific custom extensions */
>       DEFINE_PROP_BOOL("xtheadba", RISCVCPU, cfg.ext_xtheadba, false),

Re: [PATCH 1/2] riscv: zicond: make non-experimental

[Daniel Henrique Barboza]

On 8/8/23 17:52, Palmer Dabbelt wrote:
> On Tue, 08 Aug 2023 11:45:49 PDT (-0700), Vineet Gupta wrote:
>>
>>
>> On 8/8/23 11:29, Richard Henderson wrote:
>>> On 8/8/23 11:17, Vineet Gupta wrote:
>>>> zicond is now codegen supported in both llvm and gcc.
>>>
>>> It is still not in
>>>
>>> https://wiki.riscv.org/display/HOME/Recently+Ratified+Extensions
>>
>> Right, its been frozen since April though and with support trickling in
>> rest of tooling it becomes harder to test.
>> I don't know what exactly QEMU's policy is on this ?
> 
> IIUC we'd historically marked stuff as non-experimental when it's frozen, largely because ratification is such a nebulous process. There's obviously risk there, but there's risk to anything.  Last I can find is 260b594d8a ("RISC-V: Add Zawrs ISA extension support"), which specifically calls out Zawrs as frozen and IIUC adds support without the "x-" prefix.

If that's the case then I think it's sensible to remove the 'experimental' status
of zicond as well.

> 
> I can't find anything written down about it, though...

As soon as we agree on an official policy I'll do a doc update. Thanks,


Daniel

> 

Re: [PATCH 1/2] riscv: zicond: make non-experimental

[Palmer Dabbelt]

On Tue, 08 Aug 2023 14:10:54 PDT (-0700), dbarboza@ventanamicro.com wrote:
>
>
> On 8/8/23 17:52, Palmer Dabbelt wrote:
>> On Tue, 08 Aug 2023 11:45:49 PDT (-0700), Vineet Gupta wrote:
>>>
>>>
>>> On 8/8/23 11:29, Richard Henderson wrote:
>>>> On 8/8/23 11:17, Vineet Gupta wrote:
>>>>> zicond is now codegen supported in both llvm and gcc.
>>>>
>>>> It is still not in
>>>>
>>>> https://wiki.riscv.org/display/HOME/Recently+Ratified+Extensions
>>>
>>> Right, its been frozen since April though and with support trickling in
>>> rest of tooling it becomes harder to test.
>>> I don't know what exactly QEMU's policy is on this ?
>>
>> IIUC we'd historically marked stuff as non-experimental when it's frozen, largely because ratification is such a nebulous process. There's obviously risk there, but there's risk to anything.  Last I can find is 260b594d8a ("RISC-V: Add Zawrs ISA extension support"), which specifically calls out Zawrs as frozen and IIUC adds support without the "x-" prefix.
>
> If that's the case then I think it's sensible to remove the 'experimental' status
> of zicond as well.
>
>>
>> I can't find anything written down about it, though...
>
> As soon as we agree on an official policy I'll do a doc update. Thanks,

Thanks.  We should probably give Alistair some time to chime in, it's 
still pretty early there.

>
>
> Daniel
>
>>

Re: [PATCH 2/2] riscv: zicond: make default

[Vineet Gupta]

On 8/8/23 14:06, Daniel Henrique Barboza wrote:
> (CCing Alistair and other reviewers)
>
> On 8/8/23 15:17, Vineet Gupta wrote:
>> Again this helps with better testing and something qemu has been doing
>> with newer features anyways.
>>
>> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
>> ---
>
> Even if we can reach a consensus about removing the experimental (x- 
> prefix) status
> from an extension that is Frozen instead of ratified, enabling stuff 
> in the default
> CPUs because it's easier to test is something we would like to avoid. 
> The rv64
> CPU has a random set of extensions enabled for the most different and 
> undocumented
> reasons, and users don't know what they'll get because we keep beefing 
> up the
> generic CPUs arbitrarily.

I understand this position given the arbitrary nature of gazillion 
extensions. However pragmatically things like bitmanip and zicond are so 
fundamental it would be strange for designs to not have them, in a few 
years. Besides these don't compete or conflict with other extensions.
But on face value it is indeed possible for vendors to drop them for 
various reasons or no-reasons.

But having the x- dropped is good enough for our needs as there's 
already mechanisms to enable the toggles from elf attributes.

>
> Starting on QEMU 8.2 we'll have a 'max' CPU type that will enable all 
> non-experimental
> and non-vendor extensions by default, making it easier for tooling to 
> test new
> features/extensions. All tooling should consider changing their 
> scripts to use the
> 'max' CPU when it's available.

That would be great.

>
> For now, I fear that gcc and friends will still need to enable 
> 'zicond' in the command
> line via 'zicond=true'.  Thanks,

Thx,
-Vineet

Re: [PATCH 1/2] riscv: zicond: make non-experimental

[Alistair Francis]

On Tue, Aug 8, 2023 at 5:16 PM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> On Tue, 08 Aug 2023 14:10:54 PDT (-0700), dbarboza@ventanamicro.com wrote:
> >
> >
> > On 8/8/23 17:52, Palmer Dabbelt wrote:
> >> On Tue, 08 Aug 2023 11:45:49 PDT (-0700), Vineet Gupta wrote:
> >>>
> >>>
> >>> On 8/8/23 11:29, Richard Henderson wrote:
> >>>> On 8/8/23 11:17, Vineet Gupta wrote:
> >>>>> zicond is now codegen supported in both llvm and gcc.
> >>>>
> >>>> It is still not in
> >>>>
> >>>> https://wiki.riscv.org/display/HOME/Recently+Ratified+Extensions
> >>>
> >>> Right, its been frozen since April though and with support trickling in
> >>> rest of tooling it becomes harder to test.
> >>> I don't know what exactly QEMU's policy is on this ?
> >>
> >> IIUC we'd historically marked stuff as non-experimental when it's frozen, largely because ratification is such a nebulous process. There's obviously risk there, but there's risk to anything.  Last I can find is 260b594d8a ("RISC-V: Add Zawrs ISA extension support"), which specifically calls out Zawrs as frozen and IIUC adds support without the "x-" prefix.
> >
> > If that's the case then I think it's sensible to remove the 'experimental' status
> > of zicond as well.
> >
> >>
> >> I can't find anything written down about it, though...
> >
> > As soon as we agree on an official policy I'll do a doc update. Thanks,
>
> Thanks.  We should probably give Alistair some time to chime in, it's
> still pretty early there.

Frozen should be enough to remove the `x-`. We do have it written down
at: https://wiki.qemu.org/Documentation/Platforms/RISCV#RISC-V_Foundation_Extensions

Alistair

>
> >
> >
> > Daniel
> >
> >>
>

Re: [PATCH 1/2] riscv: zicond: make non-experimental

[Alistair Francis]

On Tue, Aug 8, 2023 at 2:18 PM Vineet Gupta <vineetg@rivosinc.com> wrote:
>
> zicond is now codegen supported in both llvm and gcc.
>
> This change allows seamless enabling/testing of zicond in downstream
> projects. e.g. currently riscv-gnu-toolchain parses elf attributes
> to create a cmdline for qemu but fails short of enabling it because of
> the "x-" prefix.
>
> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 6b93b04453c8..022bd9d01223 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1816,6 +1816,7 @@ static Property riscv_cpu_extensions[] = {
>      DEFINE_PROP_BOOL("zcf", RISCVCPU, cfg.ext_zcf, false),
>      DEFINE_PROP_BOOL("zcmp", RISCVCPU, cfg.ext_zcmp, false),
>      DEFINE_PROP_BOOL("zcmt", RISCVCPU, cfg.ext_zcmt, false),
> +    DEFINE_PROP_BOOL("zicond", RISCVCPU, cfg.ext_zicond, false),
>
>      /* Vendor-specific custom extensions */
>      DEFINE_PROP_BOOL("xtheadba", RISCVCPU, cfg.ext_xtheadba, false),
> @@ -1832,7 +1833,6 @@ static Property riscv_cpu_extensions[] = {
>      DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps, false),
>
>      /* These are experimental so mark with 'x-' */
> -    DEFINE_PROP_BOOL("x-zicond", RISCVCPU, cfg.ext_zicond, false),
>
>      /* ePMP 0.9.3 */
>      DEFINE_PROP_BOOL("x-epmp", RISCVCPU, cfg.epmp, false),
> --
> 2.34.1
>
>

Re: [PATCH 2/2] riscv: zicond: make default

[Alistair Francis]

On Tue, Aug 8, 2023 at 6:10 PM Vineet Gupta <vineetg@rivosinc.com> wrote:
>
>
>
> On 8/8/23 14:06, Daniel Henrique Barboza wrote:
> > (CCing Alistair and other reviewers)
> >
> > On 8/8/23 15:17, Vineet Gupta wrote:
> >> Again this helps with better testing and something qemu has been doing
> >> with newer features anyways.
> >>
> >> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
> >> ---
> >
> > Even if we can reach a consensus about removing the experimental (x-
> > prefix) status
> > from an extension that is Frozen instead of ratified, enabling stuff
> > in the default
> > CPUs because it's easier to test is something we would like to avoid.
> > The rv64
> > CPU has a random set of extensions enabled for the most different and
> > undocumented
> > reasons, and users don't know what they'll get because we keep beefing
> > up the
> > generic CPUs arbitrarily.

The idea was to enable "most" extensions for the virt machine. It's a
bit wishy-washy, but the idea was to enable as much as possible by
default on the virt machine, as long as it doesn't conflict. The goal
being to allow users to get the "best" experience as all their
favourite extensions are enabled.

It's harder to do in practice, so we are in a weird state where users
don't know what is and isn't enabled.

We probably want to revisit this. We should try to enable what is
useful for users and make it clear what is and isn't enabled. I'm not
clear on how best to do that though.

Again, I think this comes back to we need to version the virt machine.
I might do that as a starting point, that allows us to make changes in
a clear way.

>
> I understand this position given the arbitrary nature of gazillion
> extensions. However pragmatically things like bitmanip and zicond are so
> fundamental it would be strange for designs to not have them, in a few
> years. Besides these don't compete or conflict with other extensions.
> But on face value it is indeed possible for vendors to drop them for
> various reasons or no-reasons.
>
> But having the x- dropped is good enough for our needs as there's
> already mechanisms to enable the toggles from elf attributes.
>
> >
> > Starting on QEMU 8.2 we'll have a 'max' CPU type that will enable all
> > non-experimental
> > and non-vendor extensions by default, making it easier for tooling to
> > test new
> > features/extensions. All tooling should consider changing their
> > scripts to use the
> > 'max' CPU when it's available.
>
> That would be great.

The max CPU helps, but I do feel that the default should allow users
to experience as many RISC-V extensions/features as practical.

Alistair

>
> >
> > For now, I fear that gcc and friends will still need to enable
> > 'zicond' in the command
> > line via 'zicond=true'.  Thanks,
>
> Thx,
> -Vineet
>

Re: [PATCH 2/2] riscv: zicond: make default

[Andrew Jones]

On Thu, Aug 10, 2023 at 02:07:17PM -0400, Alistair Francis wrote:
> On Tue, Aug 8, 2023 at 6:10 PM Vineet Gupta <vineetg@rivosinc.com> wrote:
> >
> >
> >
> > On 8/8/23 14:06, Daniel Henrique Barboza wrote:
> > > (CCing Alistair and other reviewers)
> > >
> > > On 8/8/23 15:17, Vineet Gupta wrote:
> > >> Again this helps with better testing and something qemu has been doing
> > >> with newer features anyways.
> > >>
> > >> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
> > >> ---
> > >
> > > Even if we can reach a consensus about removing the experimental (x-
> > > prefix) status
> > > from an extension that is Frozen instead of ratified, enabling stuff
> > > in the default
> > > CPUs because it's easier to test is something we would like to avoid.
> > > The rv64
> > > CPU has a random set of extensions enabled for the most different and
> > > undocumented
> > > reasons, and users don't know what they'll get because we keep beefing
> > > up the
> > > generic CPUs arbitrarily.
> 
> The idea was to enable "most" extensions for the virt machine. It's a
> bit wishy-washy, but the idea was to enable as much as possible by
> default on the virt machine, as long as it doesn't conflict. The goal
> being to allow users to get the "best" experience as all their
> favourite extensions are enabled.
> 
> It's harder to do in practice, so we are in a weird state where users
> don't know what is and isn't enabled.
> 
> We probably want to revisit this. We should try to enable what is
> useful for users and make it clear what is and isn't enabled. I'm not
> clear on how best to do that though.
> 
> Again, I think this comes back to we need to version the virt machine.
> I might do that as a starting point, that allows us to make changes in
> a clear way.

While some extensions will impact the machine model, as well as cpu
models, versioning the machine model won't help much with ambiguity in
cpu model extension support. Daniel's proposal of having a base cpu mode,
which, on top, users can explicitly enable what they want (including with
profile support which work like a shorthand to enable many extensions at
once), is, IMO, the best way for users to know what they get. Also, the
'max' cpu model is the best way to "quickly get as much as possible" for
testing. To know what's in 'max', or named cpu models, we need to
implement qmp_query_cpu_model_expansion(). Something that could be used
from the command line would also be nice, but neither x86 nor arm provide
that (they have '-cpu help', but arm doesn't output anything for cpu
features and x86 dumps all features out without saying what's enabled for
any particular cpu model...)

I know x86 people have in the past discussed versioning cpu models, but
I don't think that should be necessary for riscv with the base+profile
approach. A profile would effectively be a versioned cpu model in that
case.

Finally, I'd discourage versioning the virt machine type until we need
to worry about users creating riscv guest images that they are unwilling
to modify, despite wanting to update their QEMU versions. And, even then,
we should only consider versioning when we're aware of problems for those
static guest images, i.e. we introduce a change to the virt machine model
which breaks that supported, old guest image. (NB: It was me that
advocated to start versioning Arm's virt machine type. In hindsight, I
think I may have advocated prematurely. I'm trying not to make that
mistake twice!)

> 
> >
> > I understand this position given the arbitrary nature of gazillion
> > extensions. However pragmatically things like bitmanip and zicond are so
> > fundamental it would be strange for designs to not have them, in a few
> > years. Besides these don't compete or conflict with other extensions.
> > But on face value it is indeed possible for vendors to drop them for
> > various reasons or no-reasons.
> >
> > But having the x- dropped is good enough for our needs as there's
> > already mechanisms to enable the toggles from elf attributes.
> >
> > >
> > > Starting on QEMU 8.2 we'll have a 'max' CPU type that will enable all
> > > non-experimental
> > > and non-vendor extensions by default, making it easier for tooling to
> > > test new
> > > features/extensions. All tooling should consider changing their
> > > scripts to use the
> > > 'max' CPU when it's available.
> >
> > That would be great.
> 
> The max CPU helps, but I do feel that the default should allow users
> to experience as many RISC-V extensions/features as practical.
> 

If the virt machine type has a default cpu type, then why not set it to
'max'?

Thanks,
drew

Re: [PATCH 2/2] riscv: zicond: make default

[Daniel Henrique Barboza]

On 8/10/23 15:07, Alistair Francis wrote:
> On Tue, Aug 8, 2023 at 6:10 PM Vineet Gupta <vineetg@rivosinc.com> wrote:
>>
>>
>>
>> On 8/8/23 14:06, Daniel Henrique Barboza wrote:
>>> (CCing Alistair and other reviewers)
>>>
>>> On 8/8/23 15:17, Vineet Gupta wrote:
>>>> Again this helps with better testing and something qemu has been doing
>>>> with newer features anyways.
>>>>
>>>> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
>>>> ---
>>>
>>> Even if we can reach a consensus about removing the experimental (x-
>>> prefix) status
>>> from an extension that is Frozen instead of ratified, enabling stuff
>>> in the default
>>> CPUs because it's easier to test is something we would like to avoid.
>>> The rv64
>>> CPU has a random set of extensions enabled for the most different and
>>> undocumented
>>> reasons, and users don't know what they'll get because we keep beefing
>>> up the
>>> generic CPUs arbitrarily.
> 
> The idea was to enable "most" extensions for the virt machine. It's a
> bit wishy-washy, but the idea was to enable as much as possible by
> default on the virt machine, as long as it doesn't conflict. The goal
> being to allow users to get the "best" experience as all their
> favourite extensions are enabled.
> 
> It's harder to do in practice, so we are in a weird state where users
> don't know what is and isn't enabled.
> 
> We probably want to revisit this. We should try to enable what is
> useful for users and make it clear what is and isn't enabled. I'm not
> clear on how best to do that though.

Yeah, thing is how we define 'useful for users'. If you take a look at this
thread where we discussed the 'max' CPU design:

https://lore.kernel.org/qemu-riscv/35a847a1-2720-14ab-61b0-c72d77d5f43b@ventanamicro.com/

The 'max' CPU design is rather straightforward, the profile support is also
straightforward (I'll work on that soon), but the role of the rv64 CPU is
confusing.

IMO we should freeze the current rv64 extensions, document it, and the leave
it alone unless we have a very good reason to enable more stuff. We'll have the
max CPU for the use case where users want the maximum amount of stable features
and, with profile support, we can make rv64 operate with a predictable set of
extensions. Seems like a good coverage.


Thanks,

Daniel

> 
> Again, I think this comes back to we need to version the virt machine.
> I might do that as a starting point, that allows us to make changes in
> a clear way.
> 
>>
>> I understand this position given the arbitrary nature of gazillion
>> extensions. However pragmatically things like bitmanip and zicond are so
>> fundamental it would be strange for designs to not have them, in a few
>> years. Besides these don't compete or conflict with other extensions.
>> But on face value it is indeed possible for vendors to drop them for
>> various reasons or no-reasons.
>>
>> But having the x- dropped is good enough for our needs as there's
>> already mechanisms to enable the toggles from elf attributes.
>>
>>>
>>> Starting on QEMU 8.2 we'll have a 'max' CPU type that will enable all
>>> non-experimental
>>> and non-vendor extensions by default, making it easier for tooling to
>>> test new
>>> features/extensions. All tooling should consider changing their
>>> scripts to use the
>>> 'max' CPU when it's available.
>>
>> That would be great.
> 
> The max CPU helps, but I do feel that the default should allow users
> to experience as many RISC-V extensions/features as practical.
> 
> Alistair
> 
>>
>>>
>>> For now, I fear that gcc and friends will still need to enable
>>> 'zicond' in the command
>>> line via 'zicond=true'.  Thanks,
>>
>> Thx,
>> -Vineet
>>

Re: [PATCH 1/2] riscv: zicond: make non-experimental

[Vineet Gupta]

On 8/10/23 10:14, Alistair Francis wrote:
> On Tue, Aug 8, 2023 at 2:18 PM Vineet Gupta <vineetg@rivosinc.com> wrote:
>> zicond is now codegen supported in both llvm and gcc.
>>
>> This change allows seamless enabling/testing of zicond in downstream
>> projects. e.g. currently riscv-gnu-toolchain parses elf attributes
>> to create a cmdline for qemu but fails short of enabling it because of
>> the "x-" prefix.
>>
>> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Alistair

Gentle ping to remind that this lands in some -next tree and not forgotten !

Thx,
-Vineet

>
>> ---
>>   target/riscv/cpu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 6b93b04453c8..022bd9d01223 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1816,6 +1816,7 @@ static Property riscv_cpu_extensions[] = {
>>       DEFINE_PROP_BOOL("zcf", RISCVCPU, cfg.ext_zcf, false),
>>       DEFINE_PROP_BOOL("zcmp", RISCVCPU, cfg.ext_zcmp, false),
>>       DEFINE_PROP_BOOL("zcmt", RISCVCPU, cfg.ext_zcmt, false),
>> +    DEFINE_PROP_BOOL("zicond", RISCVCPU, cfg.ext_zicond, false),
>>
>>       /* Vendor-specific custom extensions */
>>       DEFINE_PROP_BOOL("xtheadba", RISCVCPU, cfg.ext_xtheadba, false),
>> @@ -1832,7 +1833,6 @@ static Property riscv_cpu_extensions[] = {
>>       DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps, false),
>>
>>       /* These are experimental so mark with 'x-' */
>> -    DEFINE_PROP_BOOL("x-zicond", RISCVCPU, cfg.ext_zicond, false),
>>
>>       /* ePMP 0.9.3 */
>>       DEFINE_PROP_BOOL("x-epmp", RISCVCPU, cfg.epmp, false),
>> --
>> 2.34.1
>>
>>

Re: [PATCH 1/2] riscv: zicond: make non-experimental

[Alistair Francis]

On Wed, Aug 9, 2023 at 4:18 AM Vineet Gupta <vineetg@rivosinc.com> wrote:
>
> zicond is now codegen supported in both llvm and gcc.
>
> This change allows seamless enabling/testing of zicond in downstream
> projects. e.g. currently riscv-gnu-toolchain parses elf attributes
> to create a cmdline for qemu but fails short of enabling it because of
> the "x-" prefix.
>
> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 6b93b04453c8..022bd9d01223 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1816,6 +1816,7 @@ static Property riscv_cpu_extensions[] = {
>      DEFINE_PROP_BOOL("zcf", RISCVCPU, cfg.ext_zcf, false),
>      DEFINE_PROP_BOOL("zcmp", RISCVCPU, cfg.ext_zcmp, false),
>      DEFINE_PROP_BOOL("zcmt", RISCVCPU, cfg.ext_zcmt, false),
> +    DEFINE_PROP_BOOL("zicond", RISCVCPU, cfg.ext_zicond, false),
>
>      /* Vendor-specific custom extensions */
>      DEFINE_PROP_BOOL("xtheadba", RISCVCPU, cfg.ext_xtheadba, false),
> @@ -1832,7 +1833,6 @@ static Property riscv_cpu_extensions[] = {
>      DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps, false),
>
>      /* These are experimental so mark with 'x-' */
> -    DEFINE_PROP_BOOL("x-zicond", RISCVCPU, cfg.ext_zicond, false),
>
>      /* ePMP 0.9.3 */
>      DEFINE_PROP_BOOL("x-epmp", RISCVCPU, cfg.epmp, false),
> --
> 2.34.1
>
>

Re: [PATCH 2/2] riscv: zicond: make default

[Alistair Francis]

On Fri, Aug 11, 2023 at 5:01 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Thu, Aug 10, 2023 at 02:07:17PM -0400, Alistair Francis wrote:
> > On Tue, Aug 8, 2023 at 6:10 PM Vineet Gupta <vineetg@rivosinc.com> wrote:
> > >
> > >
> > >
> > > On 8/8/23 14:06, Daniel Henrique Barboza wrote:
> > > > (CCing Alistair and other reviewers)
> > > >
> > > > On 8/8/23 15:17, Vineet Gupta wrote:
> > > >> Again this helps with better testing and something qemu has been doing
> > > >> with newer features anyways.
> > > >>
> > > >> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
> > > >> ---
> > > >
> > > > Even if we can reach a consensus about removing the experimental (x-
> > > > prefix) status
> > > > from an extension that is Frozen instead of ratified, enabling stuff
> > > > in the default
> > > > CPUs because it's easier to test is something we would like to avoid.
> > > > The rv64
> > > > CPU has a random set of extensions enabled for the most different and
> > > > undocumented
> > > > reasons, and users don't know what they'll get because we keep beefing
> > > > up the
> > > > generic CPUs arbitrarily.
> >
> > The idea was to enable "most" extensions for the virt machine. It's a
> > bit wishy-washy, but the idea was to enable as much as possible by
> > default on the virt machine, as long as it doesn't conflict. The goal
> > being to allow users to get the "best" experience as all their
> > favourite extensions are enabled.
> >
> > It's harder to do in practice, so we are in a weird state where users
> > don't know what is and isn't enabled.
> >
> > We probably want to revisit this. We should try to enable what is
> > useful for users and make it clear what is and isn't enabled. I'm not
> > clear on how best to do that though.
> >
> > Again, I think this comes back to we need to version the virt machine.
> > I might do that as a starting point, that allows us to make changes in
> > a clear way.
>
> While some extensions will impact the machine model, as well as cpu
> models, versioning the machine model won't help much with ambiguity in
> cpu model extension support. Daniel's proposal of having a base cpu mode,
> which, on top, users can explicitly enable what they want (including with
> profile support which work like a shorthand to enable many extensions at
> once), is, IMO, the best way for users to know what they get. Also, the
> 'max' cpu model is the best way to "quickly get as much as possible" for
> testing. To know what's in 'max', or named cpu models, we need to
> implement qmp_query_cpu_model_expansion(). Something that could be used
> from the command line would also be nice, but neither x86 nor arm provide
> that (they have '-cpu help', but arm doesn't output anything for cpu
> features and x86 dumps all features out without saying what's enabled for
> any particular cpu model...)
>
> I know x86 people have in the past discussed versioning cpu models, but
> I don't think that should be necessary for riscv with the base+profile
> approach. A profile would effectively be a versioned cpu model in that
> case.
>
> Finally, I'd discourage versioning the virt machine type until we need
> to worry about users creating riscv guest images that they are unwilling
> to modify, despite wanting to update their QEMU versions. And, even then,

What's the problem with versioning the virt machine though?

I'm thinking that in the future we would want to switch from PLIC to
AIA; change the memory map; or change the default extensions (maybe to
a profile). All of those would require a versioned virt machine.

I was thinking about doing this now, so we have a base for a few
releases. So when we do need a change we are ready to go

Alistair

> we should only consider versioning when we're aware of problems for those
> static guest images, i.e. we introduce a change to the virt machine model
> which breaks that supported, old guest image. (NB: It was me that
> advocated to start versioning Arm's virt machine type. In hindsight, I
> think I may have advocated prematurely. I'm trying not to make that
> mistake twice!)
>
> >
> > >
> > > I understand this position given the arbitrary nature of gazillion
> > > extensions. However pragmatically things like bitmanip and zicond are so
> > > fundamental it would be strange for designs to not have them, in a few
> > > years. Besides these don't compete or conflict with other extensions.
> > > But on face value it is indeed possible for vendors to drop them for
> > > various reasons or no-reasons.
> > >
> > > But having the x- dropped is good enough for our needs as there's
> > > already mechanisms to enable the toggles from elf attributes.
> > >
> > > >
> > > > Starting on QEMU 8.2 we'll have a 'max' CPU type that will enable all
> > > > non-experimental
> > > > and non-vendor extensions by default, making it easier for tooling to
> > > > test new
> > > > features/extensions. All tooling should consider changing their
> > > > scripts to use the
> > > > 'max' CPU when it's available.
> > >
> > > That would be great.
> >
> > The max CPU helps, but I do feel that the default should allow users
> > to experience as many RISC-V extensions/features as practical.
> >
>
> If the virt machine type has a default cpu type, then why not set it to
> 'max'?
>
> Thanks,
> drew

Re: [PATCH 2/2] riscv: zicond: make default

[Andrew Jones]

On Mon, Oct 16, 2023 at 03:39:40PM +1000, Alistair Francis wrote:
> On Fri, Aug 11, 2023 at 5:01 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Thu, Aug 10, 2023 at 02:07:17PM -0400, Alistair Francis wrote:
> > > On Tue, Aug 8, 2023 at 6:10 PM Vineet Gupta <vineetg@rivosinc.com> wrote:
> > > >
> > > >
> > > >
> > > > On 8/8/23 14:06, Daniel Henrique Barboza wrote:
> > > > > (CCing Alistair and other reviewers)
> > > > >
> > > > > On 8/8/23 15:17, Vineet Gupta wrote:
> > > > >> Again this helps with better testing and something qemu has been doing
> > > > >> with newer features anyways.
> > > > >>
> > > > >> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
> > > > >> ---
> > > > >
> > > > > Even if we can reach a consensus about removing the experimental (x-
> > > > > prefix) status
> > > > > from an extension that is Frozen instead of ratified, enabling stuff
> > > > > in the default
> > > > > CPUs because it's easier to test is something we would like to avoid.
> > > > > The rv64
> > > > > CPU has a random set of extensions enabled for the most different and
> > > > > undocumented
> > > > > reasons, and users don't know what they'll get because we keep beefing
> > > > > up the
> > > > > generic CPUs arbitrarily.
> > >
> > > The idea was to enable "most" extensions for the virt machine. It's a
> > > bit wishy-washy, but the idea was to enable as much as possible by
> > > default on the virt machine, as long as it doesn't conflict. The goal
> > > being to allow users to get the "best" experience as all their
> > > favourite extensions are enabled.
> > >
> > > It's harder to do in practice, so we are in a weird state where users
> > > don't know what is and isn't enabled.
> > >
> > > We probably want to revisit this. We should try to enable what is
> > > useful for users and make it clear what is and isn't enabled. I'm not
> > > clear on how best to do that though.
> > >
> > > Again, I think this comes back to we need to version the virt machine.
> > > I might do that as a starting point, that allows us to make changes in
> > > a clear way.
> >
> > While some extensions will impact the machine model, as well as cpu
> > models, versioning the machine model won't help much with ambiguity in
> > cpu model extension support. Daniel's proposal of having a base cpu mode,
> > which, on top, users can explicitly enable what they want (including with
> > profile support which work like a shorthand to enable many extensions at
> > once), is, IMO, the best way for users to know what they get. Also, the
> > 'max' cpu model is the best way to "quickly get as much as possible" for
> > testing. To know what's in 'max', or named cpu models, we need to
> > implement qmp_query_cpu_model_expansion(). Something that could be used
> > from the command line would also be nice, but neither x86 nor arm provide
> > that (they have '-cpu help', but arm doesn't output anything for cpu
> > features and x86 dumps all features out without saying what's enabled for
> > any particular cpu model...)
> >
> > I know x86 people have in the past discussed versioning cpu models, but
> > I don't think that should be necessary for riscv with the base+profile
> > approach. A profile would effectively be a versioned cpu model in that
> > case.
> >
> > Finally, I'd discourage versioning the virt machine type until we need
> > to worry about users creating riscv guest images that they are unwilling
> > to modify, despite wanting to update their QEMU versions. And, even then,
> 
> What's the problem with versioning the virt machine though?

The initial versioning support is no big deal, just a couple new functions
and macros. And, new versions which don't change anything or just change
the current state of preexisting properties and attributes also have very
little developer work. However, when changes require adding compat
variables, which scatter around if's to manage things in one way for
one machine version and another way for others, then code starts to get
more difficult to maintain. And, since adding compat variables is known to
cause a maintenance burden, then just about every change the machine model
gets will lead to time spent discussing whether or not a compat variable
is necessary. But, none of that is the worst part of versioned machines.
The worst part is that the test matrix explodes. Typically all test cases
will get run N times where N is the number of supported machine types,
even if for most test cases the machine type doesn't make a difference. So
that's a waste of time and energy. And, if nobody really cares about the
old machine types, then running test cases which do depend on the machine
type is still a waste of time and energy. And, of course, machine types
which are just a number bump, definitely lead to waste.

> 
> I'm thinking that in the future we would want to switch from PLIC to
> AIA; change the memory map; or change the default extensions (maybe to
> a profile). All of those would require a versioned virt machine.

Having never versioned the machine type before, we're going break command
lines whether we put these types of changes behind machine versions or
into machine properties. Currently a command line only specifies 'virt',
so unless we leave 'virt' pointing at the current, which will be the
oldest, machine type forever, then users would need to change their
command lines to point at virt-1.0 or whatever in order to preserve their
configurations anyway. I think, particularly considering the current state
of RISC-V in production environments (not much), that we could instead
require users to change their command lines to be more explicit about what
they want, by using machine properties.

Regarding the 'virt' name, Arm also has 'virt' which is always an alias to
the latest virt-x.y machine. This works best for users who use libvirt,
since libvirt will convert the alias to the versioned name when storing it
in the XML. So, IMO, RISC-V should focus on libvirt support before QEMU
versioning, which also gets RISC-V closer to being used in production,
where machine versions are more important.

As for extensions and profiles, IMO, they shouldn't be coupled with the
machine type. Either the max CPU type should be used by default or users
should be required to select the CPU type, i.e. no default.

Thanks,
drew