Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Andrew Jones]

On Fri, Oct 27, 2023 at 02:52:38PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 10/26/23 14:36, Andrea Bolognani wrote:
> > On Thu, Oct 26, 2023 at 05:14:49PM +0200, Andrew Jones wrote:
> > > On Thu, Oct 26, 2023 at 07:36:21AM -0700, Andrea Bolognani wrote:
> > > > On Mon, Oct 23, 2023 at 07:35:16PM +0200, Andrew Jones wrote:
> > > > > On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza 
> > > > > wrote:
> > > > > > On 10/23/23 05:16, Andrew Jones wrote:
> > > > > > > Hmm, I'm not sure I agree with special-casing profiles like this. 
> > > > > > > I think
> > > > > > > the left-to-right processing should be consistent for all. I'm 
> > > > > > > also not
> > > > > > > sure we should always warn when disabling a profile. For example, 
> > > > > > > if a
> > > > > > > user does
> > > > > > > 
> > > > > > >-cpu rv64,rva22u64=true,rva22u64=false
> > > > > > > 
> > > > > > > then they'll get a warning, even though all they're doing is 
> > > > > > > restoring the
> > > > > > > cpu model. While that looks like an odd thing to do, a script may 
> > > > > > > be
> > > > > > > adding the rva22u64=true and the rva22u64=false is the user input 
> > > > > > > which
> > > > > > > undoes what the script did.
> > > > > > 
> > > > > > QEMU options do not work with a "the user enabled then disabled the 
> > > > > > same option,
> > > > > > thus it'll count as nothing happened" logic. The last instance of 
> > > > > > the option will
> > > > > > overwrite all previous instances. In the example you mentioned 
> > > > > > above the user would
> > > > > > disable all mandatory extensions of rva22u64 in the CPU, doesn't 
> > > > > > matter if the
> > > > > > same profile was enabled beforehand.
> > > > > 
> > > > > Yup, I'm aware, but I keep thinking that we'll only be using profiles 
> > > > > with
> > > > > a base cpu type. If you start with nothing (a base) and then add a 
> > > > > profile
> > > > > and take the same one away, you shouldn't be taking away anything 
> > > > > else. I
> > > > > agree that if you use a profile on some cpu type that already enabled 
> > > > > a
> > > > > bunch of stuff itself, then disabling a profile would potentially 
> > > > > remove
> > > > > some of those too, but mixing cpu types that have their own 
> > > > > extensions and
> > > > > profiles seems like a great way to confuse oneself as to what 
> > > > > extensions
> > > > > will be present.  IOW, we should be adding a base cpu type at the same
> > > > > time we're adding these profiles.
> > > > 
> > > > The question that keep bouncing around my head is: why would we even
> > > > allow disabling profiles?
> > > > 
> > > > It seems to me that it only makes things more complicated, and I
> > > > really can't see the use case for it.
> > > > 
> > > > Enabling additional features on top of a profile? There's obvious
> > > > value in that, so that you can model hardware that implements
> > > > optional and proprietary extensions. Enabling multiple profiles?
> > > > You've convinced me that it's useful. But disabling profiles, I just
> > > > don't see it. I believe Alistair was similarly unconvinced.
> > > 
> > > The only value I see in allowing a profile to be disabled is to undo the
> > > enabling of the profile by specifying the profile as 'off' to the right of
> > > it being specified as 'on'. That may seem pointless, but scripts take
> > > advantage of being able to do that. Besides that one possible use case,
> > > there isn't much use in disabling profiles, but treating profile
> > > properties like every other boolean property makes the UI consistent and
> > > should actually simplify the code.
> > 
> > The code might be simpler, but the result is an additional burden on
> > the user, as the interactions between the various flags become much
> > more nuanced and less intuitive. I'm not convinced the trade-off is a
> > worthwhile one.
> > 
> > For the script override scenario, fair enough, but once again I feel
> > that we're making things much worse in the general case in order to
> > cater to a much narrower one. Script authors will naturally learn to
> > avoid hardcoding profile enablement once users have reported enough
> > failures resulting from that.
> 
> I'm not thrilled about how we're able to disable profiles either. I'm
> coping with it because (1) it was a feedback from the first version of
> this work [1] and no one had strong opinions against it back then and
> (2) I believe that users won't find much use in doing "-cpu 
> rv64,profileA=false"
> in a real world/common scenario, so we can get away with this kind of
> weird functionality.
> 
> The profile flag is set to 'false' by default for all current CPUs. If
> the user manually sets it to 'false', well, it doesn't change the internal
> state of the CPU, does it? But then I need to be creative and interpret it
> as 'it's not a default false, it's an user-set false, so I need to disable
> extensions'. I can't think of many qemu options that behave 

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Daniel Henrique Barboza]

On 10/26/23 14:36, Andrea Bolognani wrote:

On Thu, Oct 26, 2023 at 05:14:49PM +0200, Andrew Jones wrote:

On Thu, Oct 26, 2023 at 07:36:21AM -0700, Andrea Bolognani wrote:

On Mon, Oct 23, 2023 at 07:35:16PM +0200, Andrew Jones wrote:

On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza wrote:

On 10/23/23 05:16, Andrew Jones wrote:

Hmm, I'm not sure I agree with special-casing profiles like this. I think
the left-to-right processing should be consistent for all. I'm also not
sure we should always warn when disabling a profile. For example, if a
user does

   -cpu rv64,rva22u64=true,rva22u64=false

then they'll get a warning, even though all they're doing is restoring the
cpu model. While that looks like an odd thing to do, a script may be
adding the rva22u64=true and the rva22u64=false is the user input which
undoes what the script did.


QEMU options do not work with a "the user enabled then disabled the same option,
thus it'll count as nothing happened" logic. The last instance of the option 
will
overwrite all previous instances. In the example you mentioned above the user 
would
disable all mandatory extensions of rva22u64 in the CPU, doesn't matter if the
same profile was enabled beforehand.


Yup, I'm aware, but I keep thinking that we'll only be using profiles with
a base cpu type. If you start with nothing (a base) and then add a profile
and take the same one away, you shouldn't be taking away anything else. I
agree that if you use a profile on some cpu type that already enabled a
bunch of stuff itself, then disabling a profile would potentially remove
some of those too, but mixing cpu types that have their own extensions and
profiles seems like a great way to confuse oneself as to what extensions
will be present.  IOW, we should be adding a base cpu type at the same
time we're adding these profiles.


The question that keep bouncing around my head is: why would we even
allow disabling profiles?

It seems to me that it only makes things more complicated, and I
really can't see the use case for it.

Enabling additional features on top of a profile? There's obvious
value in that, so that you can model hardware that implements
optional and proprietary extensions. Enabling multiple profiles?
You've convinced me that it's useful. But disabling profiles, I just
don't see it. I believe Alistair was similarly unconvinced.


The only value I see in allowing a profile to be disabled is to undo the
enabling of the profile by specifying the profile as 'off' to the right of
it being specified as 'on'. That may seem pointless, but scripts take
advantage of being able to do that. Besides that one possible use case,
there isn't much use in disabling profiles, but treating profile
properties like every other boolean property makes the UI consistent and
should actually simplify the code.


The code might be simpler, but the result is an additional burden on
the user, as the interactions between the various flags become much
more nuanced and less intuitive. I'm not convinced the trade-off is a
worthwhile one.

For the script override scenario, fair enough, but once again I feel
that we're making things much worse in the general case in order to
cater to a much narrower one. Script authors will naturally learn to
avoid hardcoding profile enablement once users have reported enough
failures resulting from that.


I'm not thrilled about how we're able to disable profiles either. I'm
coping with it because (1) it was a feedback from the first version of
this work [1] and no one had strong opinions against it back then and
(2) I believe that users won't find much use in doing "-cpu rv64,profileA=false"
in a real world/common scenario, so we can get away with this kind of
weird functionality.

The profile flag is set to 'false' by default for all current CPUs. If
the user manually sets it to 'false', well, it doesn't change the internal
state of the CPU, does it? But then I need to be creative and interpret it
as 'it's not a default false, it's an user-set false, so I need to disable
extensions'. I can't think of many qemu options that behave like that, if
any.

We also have the example of RVG, a bit that is default set to 'false' that,
when enabled, causes IMAFD_zicsr_zifencei to be enabled. Today, if the user
set RVG to 'false', nothing happens - we're not disabling IMAFD_zicsr_zifencei.
In the latest version of this work there's a deliberate effort to make RVG
behave like a profile [2], but perhaps I should make profiles behave like RVG.

Last but not the least, I'm planning to add a couple of bare-bones CPUs (rv64i
and rv64e). Disabling profiles in these CPUs is a total waste of cycles since
the CPUs are already bare.

After writing all this stuff, and realizing that profile disablement creates a
lot of confusion and has no vocal fans, I had a change of heart. Profiles will
behave like RVG -> if set, mandatory extensions will be enabled (respecting user
choice on disabled extensions, of course). If 

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Andrea Bolognani]

On Thu, Oct 26, 2023 at 05:14:49PM +0200, Andrew Jones wrote:
> On Thu, Oct 26, 2023 at 07:36:21AM -0700, Andrea Bolognani wrote:
> > On Mon, Oct 23, 2023 at 07:35:16PM +0200, Andrew Jones wrote:
> > > On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza wrote:
> > > > On 10/23/23 05:16, Andrew Jones wrote:
> > > > > Hmm, I'm not sure I agree with special-casing profiles like this. I 
> > > > > think
> > > > > the left-to-right processing should be consistent for all. I'm also 
> > > > > not
> > > > > sure we should always warn when disabling a profile. For example, if a
> > > > > user does
> > > > >
> > > > >   -cpu rv64,rva22u64=true,rva22u64=false
> > > > >
> > > > > then they'll get a warning, even though all they're doing is 
> > > > > restoring the
> > > > > cpu model. While that looks like an odd thing to do, a script may be
> > > > > adding the rva22u64=true and the rva22u64=false is the user input 
> > > > > which
> > > > > undoes what the script did.
> > > >
> > > > QEMU options do not work with a "the user enabled then disabled the 
> > > > same option,
> > > > thus it'll count as nothing happened" logic. The last instance of the 
> > > > option will
> > > > overwrite all previous instances. In the example you mentioned above 
> > > > the user would
> > > > disable all mandatory extensions of rva22u64 in the CPU, doesn't matter 
> > > > if the
> > > > same profile was enabled beforehand.
> > >
> > > Yup, I'm aware, but I keep thinking that we'll only be using profiles with
> > > a base cpu type. If you start with nothing (a base) and then add a profile
> > > and take the same one away, you shouldn't be taking away anything else. I
> > > agree that if you use a profile on some cpu type that already enabled a
> > > bunch of stuff itself, then disabling a profile would potentially remove
> > > some of those too, but mixing cpu types that have their own extensions and
> > > profiles seems like a great way to confuse oneself as to what extensions
> > > will be present.  IOW, we should be adding a base cpu type at the same
> > > time we're adding these profiles.
> >
> > The question that keep bouncing around my head is: why would we even
> > allow disabling profiles?
> >
> > It seems to me that it only makes things more complicated, and I
> > really can't see the use case for it.
> >
> > Enabling additional features on top of a profile? There's obvious
> > value in that, so that you can model hardware that implements
> > optional and proprietary extensions. Enabling multiple profiles?
> > You've convinced me that it's useful. But disabling profiles, I just
> > don't see it. I believe Alistair was similarly unconvinced.
>
> The only value I see in allowing a profile to be disabled is to undo the
> enabling of the profile by specifying the profile as 'off' to the right of
> it being specified as 'on'. That may seem pointless, but scripts take
> advantage of being able to do that. Besides that one possible use case,
> there isn't much use in disabling profiles, but treating profile
> properties like every other boolean property makes the UI consistent and
> should actually simplify the code.

The code might be simpler, but the result is an additional burden on
the user, as the interactions between the various flags become much
more nuanced and less intuitive. I'm not convinced the trade-off is a
worthwhile one.

For the script override scenario, fair enough, but once again I feel
that we're making things much worse in the general case in order to
cater to a much narrower one. Script authors will naturally learn to
avoid hardcoding profile enablement once users have reported enough
failures resulting from that.

> > > > > As far as warnings go, it'd be nice to warn when mandatory profile
> > > > > extensions are disabled from an enabled profile. Doing that might be
> > > > > useful for debug, but users which do it without being aware they're
> > > > > "breaking" the profile may learn from that warning. Note, the warning
> > > > > should only come when the profile is actually enabled and when the
> > > > > extension would actually be disabled, i.e.
> > > > >
> > > > >   -cpu rv64,rva22u64=true,c=off
> > > > >
> > > > > should warn
> > > > >
> > > > >   -cpu rv64,c=off,rva22u64=true
> > > > >
> > > > > should not warn (rva22u64 overrides c=off since it's to the right)
> > > > >
> > > > >   -cpu rv64,rva22u64=true,rva22u64=false,c=off
> > > > >
> > > > > should not warn (rva22u64 is not enabled)
> >
> > I think these should be hard errors, not warnings.
> >
> > If you're enabling a profile and then disabling an extension that's
> > mandatory for that profile, you've invalidated the profile. You've
> > asked for a configuration that doesn't make any sense: you can't have
> > a CPU that both implements a profile and lacks one of its mandatory
> > extensions.
>
> Given a platform which implements a profile which mandates extension E and
> a need to debug E or test behavior where E is [incorrectly] 

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Andrew Jones]

On Thu, Oct 26, 2023 at 07:36:21AM -0700, Andrea Bolognani wrote:
> On Mon, Oct 23, 2023 at 07:35:16PM +0200, Andrew Jones wrote:
> > On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza wrote:
> > > On 10/23/23 05:16, Andrew Jones wrote:
> > > > Hmm, I'm not sure I agree with special-casing profiles like this. I 
> > > > think
> > > > the left-to-right processing should be consistent for all. I'm also not
> > > > sure we should always warn when disabling a profile. For example, if a
> > > > user does
> > > >
> > > >   -cpu rv64,rva22u64=true,rva22u64=false
> > > >
> > > > then they'll get a warning, even though all they're doing is restoring 
> > > > the
> > > > cpu model. While that looks like an odd thing to do, a script may be
> > > > adding the rva22u64=true and the rva22u64=false is the user input which
> > > > undoes what the script did.
> > >
> > > QEMU options do not work with a "the user enabled then disabled the same 
> > > option,
> > > thus it'll count as nothing happened" logic. The last instance of the 
> > > option will
> > > overwrite all previous instances. In the example you mentioned above the 
> > > user would
> > > disable all mandatory extensions of rva22u64 in the CPU, doesn't matter 
> > > if the
> > > same profile was enabled beforehand.
> >
> > Yup, I'm aware, but I keep thinking that we'll only be using profiles with
> > a base cpu type. If you start with nothing (a base) and then add a profile
> > and take the same one away, you shouldn't be taking away anything else. I
> > agree that if you use a profile on some cpu type that already enabled a
> > bunch of stuff itself, then disabling a profile would potentially remove
> > some of those too, but mixing cpu types that have their own extensions and
> > profiles seems like a great way to confuse oneself as to what extensions
> > will be present.  IOW, we should be adding a base cpu type at the same
> > time we're adding these profiles.
> 
> The question that keep bouncing around my head is: why would we even
> allow disabling profiles?
> 
> It seems to me that it only makes things more complicated, and I
> really can't see the use case for it.
> 
> Enabling additional features on top of a profile? There's obvious
> value in that, so that you can model hardware that implements
> optional and proprietary extensions. Enabling multiple profiles?
> You've convinced me that it's useful. But disabling profiles, I just
> don't see it. I believe Alistair was similarly unconvinced.

The only value I see in allowing a profile to be disabled is to undo the
enabling of the profile by specifying the profile as 'off' to the right of
it being specified as 'on'. That may seem pointless, but scripts take
advantage of being able to do that. Besides that one possible use case,
there isn't much use in disabling profiles, but treating profile
properties like every other boolean property makes the UI consistent and
should actually simplify the code.

> 
> > > > As far as warnings go, it'd be nice to warn when mandatory profile
> > > > extensions are disabled from an enabled profile. Doing that might be
> > > > useful for debug, but users which do it without being aware they're
> > > > "breaking" the profile may learn from that warning. Note, the warning
> > > > should only come when the profile is actually enabled and when the
> > > > extension would actually be disabled, i.e.
> > > >
> > > >   -cpu rv64,rva22u64=true,c=off
> > > >
> > > > should warn
> > > >
> > > >   -cpu rv64,c=off,rva22u64=true
> > > >
> > > > should not warn (rva22u64 overrides c=off since it's to the right)
> > > >
> > > >   -cpu rv64,rva22u64=true,rva22u64=false,c=off
> > > >
> > > > should not warn (rva22u64 is not enabled)
> 
> I think these should be hard errors, not warnings.
> 
> If you're enabling a profile and then disabling an extension that's
> mandatory for that profile, you've invalidated the profile. You've
> asked for a configuration that doesn't make any sense: you can't have
> a CPU that both implements a profile and lacks one of its mandatory
> extensions.
> 

Given a platform which implements a profile which mandates extension E and
a need to debug E or test behavior where E is [incorrectly] absent, you'll
need to expand the profile first, listing each of the other extensions
manually. It'd be much faster to specify the profile, take away the
extension, and ignore the warning.

> QEMU users could easily miss the warning. libvirt users won't see it
> at all. It's a user error and it needs to be treated as such IMO.

I do agree with the concern that warnings will be missed/ignored. Maybe
QEMU needs something like -Werror for stuff like this, i.e.

 -cpu rv64,error-on-extension-warnings=on,profile-A=on,extension-of-A=off

would error out, but, without the special property, just warn. Or, flip
the default behavior around with

 -cpu rv64,ignore-extension-errors=on,profile-A=on,extension-of-A=off

which would either silently proceed or just warn, but, 

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Andrea Bolognani]

On Mon, Oct 23, 2023 at 07:35:16PM +0200, Andrew Jones wrote:
> On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza wrote:
> > On 10/23/23 05:16, Andrew Jones wrote:
> > > Hmm, I'm not sure I agree with special-casing profiles like this. I think
> > > the left-to-right processing should be consistent for all. I'm also not
> > > sure we should always warn when disabling a profile. For example, if a
> > > user does
> > >
> > >   -cpu rv64,rva22u64=true,rva22u64=false
> > >
> > > then they'll get a warning, even though all they're doing is restoring the
> > > cpu model. While that looks like an odd thing to do, a script may be
> > > adding the rva22u64=true and the rva22u64=false is the user input which
> > > undoes what the script did.
> >
> > QEMU options do not work with a "the user enabled then disabled the same 
> > option,
> > thus it'll count as nothing happened" logic. The last instance of the 
> > option will
> > overwrite all previous instances. In the example you mentioned above the 
> > user would
> > disable all mandatory extensions of rva22u64 in the CPU, doesn't matter if 
> > the
> > same profile was enabled beforehand.
>
> Yup, I'm aware, but I keep thinking that we'll only be using profiles with
> a base cpu type. If you start with nothing (a base) and then add a profile
> and take the same one away, you shouldn't be taking away anything else. I
> agree that if you use a profile on some cpu type that already enabled a
> bunch of stuff itself, then disabling a profile would potentially remove
> some of those too, but mixing cpu types that have their own extensions and
> profiles seems like a great way to confuse oneself as to what extensions
> will be present.  IOW, we should be adding a base cpu type at the same
> time we're adding these profiles.

The question that keep bouncing around my head is: why would we even
allow disabling profiles?

It seems to me that it only makes things more complicated, and I
really can't see the use case for it.

Enabling additional features on top of a profile? There's obvious
value in that, so that you can model hardware that implements
optional and proprietary extensions. Enabling multiple profiles?
You've convinced me that it's useful. But disabling profiles, I just
don't see it. I believe Alistair was similarly unconvinced.

> > > As far as warnings go, it'd be nice to warn when mandatory profile
> > > extensions are disabled from an enabled profile. Doing that might be
> > > useful for debug, but users which do it without being aware they're
> > > "breaking" the profile may learn from that warning. Note, the warning
> > > should only come when the profile is actually enabled and when the
> > > extension would actually be disabled, i.e.
> > >
> > >   -cpu rv64,rva22u64=true,c=off
> > >
> > > should warn
> > >
> > >   -cpu rv64,c=off,rva22u64=true
> > >
> > > should not warn (rva22u64 overrides c=off since it's to the right)
> > >
> > >   -cpu rv64,rva22u64=true,rva22u64=false,c=off
> > >
> > > should not warn (rva22u64 is not enabled)

I think these should be hard errors, not warnings.

If you're enabling a profile and then disabling an extension that's
mandatory for that profile, you've invalidated the profile. You've
asked for a configuration that doesn't make any sense: you can't have
a CPU that both implements a profile and lacks one of its mandatory
extensions.

QEMU users could easily miss the warning. libvirt users won't see it
at all. It's a user error and it needs to be treated as such IMO.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[LIU Zhiwei]

On 2023/10/21 6:39, Daniel Henrique Barboza wrote:

The TCG emulation implements all the extensions described in the
RVA22U64 profile, both mandatory and optional. The mandatory extensions
will be enabled via the profile flag. We'll leave the optional
extensions to be enabled by hand.

Given that this is the first profile we're implementing in TCG we'll
need some ground work first:

- all profiles declared in riscv_profiles[] will be exposed to users.
TCG is the main accelerator we're considering when adding profile
support in QEMU, so for now it's safe to assume that all profiles in
riscv_profiles[] will be relevant to TCG;

- we'll not support user profile settings for vendor CPUs. The flags
will still be exposed but users won't be able to change them. The idea
is that vendor CPUs in the future can enable profiles internally in
their cpu_init() functions, showing to the external world that the CPU
supports a certain profile. But users won't be able to enable/disable
it;

- Setting a profile to 'true' means 'enable all mandatory extensions of
this profile, setting it to 'false' means disabling all its mandatory
extensions. Disabling a profile is discouraged for regular use and will
issue an user warning. User choices for individual extensions will take
precedence, i.e. enabling a profile will not enable extensions that the
user set to 'false', and vice-versa. This will make us independent of
left-to-right ordering in the QEMU command line, i.e. the following QEMU
command lines:

-cpu rv64,zicbom=false,rva22u64=true,Zifencei=false
-cpu rv64,zicbom=false,Zifencei=false,rva22u64=true
-cpu rv64,rva22u64=true,zicbom=false,Zifencei=false

They mean the same thing: "enable all mandatory extensions of the
rva22u64 profile while keeping zicbom and Zifencei disabled".

For now we'll handle multi-letter extensions only. MISA extensions need
additional steps that we'll take care later.

Signed-off-by: Daniel Henrique Barboza 
---
  target/riscv/tcg/tcg-cpu.c | 59 ++
  1 file changed, 59 insertions(+)

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 7a4400e2ba..3dd4783191 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -757,6 +757,63 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
  }
  }
  
+static void cpu_set_profile(Object *obj, Visitor *v, const char *name,

+void *opaque, Error **errp)
+{
+RISCVCPUProfile *profile = opaque;
+RISCVCPU *cpu = RISCV_CPU(obj);
+bool value;
+int i, ext_offset;
+
+if (object_dynamic_cast(obj, TYPE_RISCV_DYNAMIC_CPU) == NULL) {
+error_setg(errp, "Profile %s only available for generic CPUs",
+   profile->name);
+return;
+}
+
+if (!visit_type_bool(v, name, , errp)) {
+return;
+}
+
+if (!value) {
+warn_report("Disabling the '%s' profile is a debug/development "
+"tool, not recommended for regular use",
+profile->name);
+}
+
+profile->enabled = value;
+
+for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) {
+ext_offset = profile->ext_offsets[i];
+
+if (cpu_cfg_ext_is_user_set(ext_offset)) {
+continue;
+}
+
+isa_ext_update_enabled(cpu, ext_offset, profile->enabled);
+}
+}
+
+static void cpu_get_profile(Object *obj, Visitor *v, const char *name,
+void *opaque, Error **errp)
+{
+RISCVCPUProfile *profile = opaque;
+bool value = profile->enabled;
+
+visit_type_bool(v, name, , errp);
+}
+
+static void riscv_cpu_add_profiles(Object *cpu_obj)
+{
+for (int i = 0; riscv_profiles[i] != NULL; i++) {
+const RISCVCPUProfile *profile = riscv_profiles[i];
+
+object_property_add(cpu_obj, profile->name, "bool",
+cpu_get_profile, cpu_set_profile,
+NULL, (void *)profile);
+}
+}
+
  static bool cpu_ext_is_deprecated(const char *ext_name)
  {
  return isupper(ext_name[0]);
@@ -880,6 +937,8 @@ static void riscv_cpu_add_user_properties(Object *obj)
  
  riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_deprecated_exts);
  
+riscv_cpu_add_profiles(obj);

+


Acked-by: LIU Zhiwei 

Zhiwei


  for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) {
  qdev_property_add_static(DEVICE(obj), prop);
  }

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Daniel Henrique Barboza]

On 10/23/23 14:35, Andrew Jones wrote:

On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza wrote:



On 10/23/23 05:16, Andrew Jones wrote:

On Fri, Oct 20, 2023 at 07:39:48PM -0300, Daniel Henrique Barboza wrote:

The TCG emulation implements all the extensions described in the
RVA22U64 profile, both mandatory and optional. The mandatory extensions
will be enabled via the profile flag. We'll leave the optional
extensions to be enabled by hand.

Given that this is the first profile we're implementing in TCG we'll
need some ground work first:

- all profiles declared in riscv_profiles[] will be exposed to users.
TCG is the main accelerator we're considering when adding profile
support in QEMU, so for now it's safe to assume that all profiles in
riscv_profiles[] will be relevant to TCG;

- we'll not support user profile settings for vendor CPUs. The flags
will still be exposed but users won't be able to change them. The idea
is that vendor CPUs in the future can enable profiles internally in
their cpu_init() functions, showing to the external world that the CPU
supports a certain profile. But users won't be able to enable/disable
it;

- Setting a profile to 'true' means 'enable all mandatory extensions of
this profile, setting it to 'false' means disabling all its mandatory
extensions. Disabling a profile is discouraged for regular use and will
issue an user warning. User choices for individual extensions will take
precedence, i.e. enabling a profile will not enable extensions that the
user set to 'false', and vice-versa. This will make us independent of
left-to-right ordering in the QEMU command line, i.e. the following QEMU
command lines:

-cpu rv64,zicbom=false,rva22u64=true,Zifencei=false
-cpu rv64,zicbom=false,Zifencei=false,rva22u64=true
-cpu rv64,rva22u64=true,zicbom=false,Zifencei=false

They mean the same thing: "enable all mandatory extensions of the
rva22u64 profile while keeping zicbom and Zifencei disabled".


Hmm, I'm not sure I agree with special-casing profiles like this. I think
the left-to-right processing should be consistent for all. I'm also not
sure we should always warn when disabling a profile. For example, if a
user does

   -cpu rv64,rva22u64=true,rva22u64=false

then they'll get a warning, even though all they're doing is restoring the
cpu model. While that looks like an odd thing to do, a script may be
adding the rva22u64=true and the rva22u64=false is the user input which
undoes what the script did.


QEMU options do not work with a "the user enabled then disabled the same option,
thus it'll count as nothing happened" logic. The last instance of the option 
will
overwrite all previous instances. In the example you mentioned above the user 
would
disable all mandatory extensions of rva22u64 in the CPU, doesn't matter if the
same profile was enabled beforehand.


Yup, I'm aware, but I keep thinking that we'll only be using profiles with
a base cpu type. If you start with nothing (a base) and then add a profile
and take the same one away, you shouldn't be taking away anything else. I
agree that if you use a profile on some cpu type that already enabled a
bunch of stuff itself, then disabling a profile would potentially remove
some of those too, but mixing cpu types that have their own extensions and
profiles seems like a great way to confuse oneself as to what extensions
will be present.  IOW, we should be adding a base cpu type at the same
time we're adding these profiles.


After this profile work is merged I suggest:

1a - make rv64 a base cpu type, no extensions enabled by default. Change the 
default
CPU to 'max' like linux-user mode is already doing to avoid breaking users

OR

1b - create a new rv64base|rv64bare to be used exclusively with profiles or 
when users
really want to know what they're loading in the CPU. No extensions enabled by 
default.
'rv64' remains untouched as default CPU


I can see a case for both. 1b is less work to make it happen. Thanks,


Daniel






Sure, the can put code in place to make this happen, but then this would make
profiles act different than regular extensions. "-cpu rv64,zicbom=true -cpu 
rv64,zicbom=false"
will disable zicbom, it will not preserve the original 'zicbom' rv64 default. If
we're going to keep left-to-right ordering consistent, this behavior should also
be consistent as well.


It will be consistent if we always override whatever was on the left with
what's on the right, which means with

  -cpu rv64,rva22u64=true -cpu rv64,zicbom=false

zicbom will be disabled, but with

  -cpu rv64,zicbom=false -cpu rv64,rva22u64=true

it will be enabled. The same goes if the properties are given to the same
-cpu parameter.




As for warnings, I agree that we'll throw warnings even when nothing of notice 
happened.
For example:

-cpu rv64,rva22u64=false -cpu rv64,rva22u64=true

This will throw a warning even though the user ended up enabling the extension
in the end.


We can fix it by postponing warnings to realize().

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Andrew Jones]

On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 10/23/23 05:16, Andrew Jones wrote:
> > On Fri, Oct 20, 2023 at 07:39:48PM -0300, Daniel Henrique Barboza wrote:
> > > The TCG emulation implements all the extensions described in the
> > > RVA22U64 profile, both mandatory and optional. The mandatory extensions
> > > will be enabled via the profile flag. We'll leave the optional
> > > extensions to be enabled by hand.
> > > 
> > > Given that this is the first profile we're implementing in TCG we'll
> > > need some ground work first:
> > > 
> > > - all profiles declared in riscv_profiles[] will be exposed to users.
> > > TCG is the main accelerator we're considering when adding profile
> > > support in QEMU, so for now it's safe to assume that all profiles in
> > > riscv_profiles[] will be relevant to TCG;
> > > 
> > > - we'll not support user profile settings for vendor CPUs. The flags
> > > will still be exposed but users won't be able to change them. The idea
> > > is that vendor CPUs in the future can enable profiles internally in
> > > their cpu_init() functions, showing to the external world that the CPU
> > > supports a certain profile. But users won't be able to enable/disable
> > > it;
> > > 
> > > - Setting a profile to 'true' means 'enable all mandatory extensions of
> > > this profile, setting it to 'false' means disabling all its mandatory
> > > extensions. Disabling a profile is discouraged for regular use and will
> > > issue an user warning. User choices for individual extensions will take
> > > precedence, i.e. enabling a profile will not enable extensions that the
> > > user set to 'false', and vice-versa. This will make us independent of
> > > left-to-right ordering in the QEMU command line, i.e. the following QEMU
> > > command lines:
> > > 
> > > -cpu rv64,zicbom=false,rva22u64=true,Zifencei=false
> > > -cpu rv64,zicbom=false,Zifencei=false,rva22u64=true
> > > -cpu rv64,rva22u64=true,zicbom=false,Zifencei=false
> > > 
> > > They mean the same thing: "enable all mandatory extensions of the
> > > rva22u64 profile while keeping zicbom and Zifencei disabled".
> > 
> > Hmm, I'm not sure I agree with special-casing profiles like this. I think
> > the left-to-right processing should be consistent for all. I'm also not
> > sure we should always warn when disabling a profile. For example, if a
> > user does
> > 
> >   -cpu rv64,rva22u64=true,rva22u64=false
> > 
> > then they'll get a warning, even though all they're doing is restoring the
> > cpu model. While that looks like an odd thing to do, a script may be
> > adding the rva22u64=true and the rva22u64=false is the user input which
> > undoes what the script did.
> 
> QEMU options do not work with a "the user enabled then disabled the same 
> option,
> thus it'll count as nothing happened" logic. The last instance of the option 
> will
> overwrite all previous instances. In the example you mentioned above the user 
> would
> disable all mandatory extensions of rva22u64 in the CPU, doesn't matter if the
> same profile was enabled beforehand.

Yup, I'm aware, but I keep thinking that we'll only be using profiles with
a base cpu type. If you start with nothing (a base) and then add a profile
and take the same one away, you shouldn't be taking away anything else. I
agree that if you use a profile on some cpu type that already enabled a
bunch of stuff itself, then disabling a profile would potentially remove
some of those too, but mixing cpu types that have their own extensions and
profiles seems like a great way to confuse oneself as to what extensions
will be present.  IOW, we should be adding a base cpu type at the same
time we're adding these profiles.

> 
> Sure, the can put code in place to make this happen, but then this would make
> profiles act different than regular extensions. "-cpu rv64,zicbom=true -cpu 
> rv64,zicbom=false"
> will disable zicbom, it will not preserve the original 'zicbom' rv64 default. 
> If
> we're going to keep left-to-right ordering consistent, this behavior should 
> also
> be consistent as well.

It will be consistent if we always override whatever was on the left with
what's on the right, which means with

 -cpu rv64,rva22u64=true -cpu rv64,zicbom=false

zicbom will be disabled, but with

 -cpu rv64,zicbom=false -cpu rv64,rva22u64=true

it will be enabled. The same goes if the properties are given to the same
-cpu parameter.

> 
> 
> As for warnings, I agree that we'll throw warnings even when nothing of 
> notice happened.
> For example:
> 
> -cpu rv64,rva22u64=false -cpu rv64,rva22u64=true
> 
> This will throw a warning even though the user ended up enabling the extension
> in the end.
> 
> 
> We can fix it by postponing warnings to realize().
> 
> 
> > 
> > As far as warnings go, it'd be nice to warn when mandatory profile
> > extensions are disabled from an enabled profile. Doing that might be
> > useful for debug, but users which do it without being aware they're
> > 

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Daniel Henrique Barboza]

On 10/23/23 05:16, Andrew Jones wrote:

On Fri, Oct 20, 2023 at 07:39:48PM -0300, Daniel Henrique Barboza wrote:

The TCG emulation implements all the extensions described in the
RVA22U64 profile, both mandatory and optional. The mandatory extensions
will be enabled via the profile flag. We'll leave the optional
extensions to be enabled by hand.

Given that this is the first profile we're implementing in TCG we'll
need some ground work first:

- all profiles declared in riscv_profiles[] will be exposed to users.
TCG is the main accelerator we're considering when adding profile
support in QEMU, so for now it's safe to assume that all profiles in
riscv_profiles[] will be relevant to TCG;

- we'll not support user profile settings for vendor CPUs. The flags
will still be exposed but users won't be able to change them. The idea
is that vendor CPUs in the future can enable profiles internally in
their cpu_init() functions, showing to the external world that the CPU
supports a certain profile. But users won't be able to enable/disable
it;

- Setting a profile to 'true' means 'enable all mandatory extensions of
this profile, setting it to 'false' means disabling all its mandatory
extensions. Disabling a profile is discouraged for regular use and will
issue an user warning. User choices for individual extensions will take
precedence, i.e. enabling a profile will not enable extensions that the
user set to 'false', and vice-versa. This will make us independent of
left-to-right ordering in the QEMU command line, i.e. the following QEMU
command lines:

-cpu rv64,zicbom=false,rva22u64=true,Zifencei=false
-cpu rv64,zicbom=false,Zifencei=false,rva22u64=true
-cpu rv64,rva22u64=true,zicbom=false,Zifencei=false

They mean the same thing: "enable all mandatory extensions of the
rva22u64 profile while keeping zicbom and Zifencei disabled".


Hmm, I'm not sure I agree with special-casing profiles like this. I think
the left-to-right processing should be consistent for all. I'm also not
sure we should always warn when disabling a profile. For example, if a
user does

  -cpu rv64,rva22u64=true,rva22u64=false

then they'll get a warning, even though all they're doing is restoring the
cpu model. While that looks like an odd thing to do, a script may be
adding the rva22u64=true and the rva22u64=false is the user input which
undoes what the script did.


QEMU options do not work with a "the user enabled then disabled the same option,
thus it'll count as nothing happened" logic. The last instance of the option 
will
overwrite all previous instances. In the example you mentioned above the user 
would
disable all mandatory extensions of rva22u64 in the CPU, doesn't matter if the
same profile was enabled beforehand.

Sure, the can put code in place to make this happen, but then this would make
profiles act different than regular extensions. "-cpu rv64,zicbom=true -cpu 
rv64,zicbom=false"
will disable zicbom, it will not preserve the original 'zicbom' rv64 default. If
we're going to keep left-to-right ordering consistent, this behavior should also
be consistent as well.


As for warnings, I agree that we'll throw warnings even when nothing of notice 
happened.
For example:

-cpu rv64,rva22u64=false -cpu rv64,rva22u64=true

This will throw a warning even though the user ended up enabling the extension
in the end.


We can fix it by postponing warnings to realize().




As far as warnings go, it'd be nice to warn when mandatory profile
extensions are disabled from an enabled profile. Doing that might be
useful for debug, but users which do it without being aware they're
"breaking" the profile may learn from that warning. Note, the warning
should only come when the profile is actually enabled and when the
extension would actually be disabled, i.e.

  -cpu rv64,rva22u64=true,c=off

should warn

  -cpu rv64,c=off,rva22u64=true

should not warn (rva22u64 overrides c=off since it's to the right)

  -cpu rv64,rva22u64=true,rva22u64=false,c=off

should not warn (rva22u64 is not enabled)


Ack for all the above.



And,

  -cpu rv64,rva22u64=true,rva24u64=false

should warn for each extension which is mandatory in both profiles.


The way I'm imagining this happening is to cycle through all profiles during 
realize(),
see which ones are enabled, and then warn if the user disabled their mandatory
extensions. In this example we would warn for all rva22 mandatory extensions
that were disabled because we disabled rva24, but we won't emit any warnings for
rva24 mandatory extensions given that the profile is marked as disabled.



Thanks,

Daniel




Thanks,
drew

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Andrew Jones]

On Fri, Oct 20, 2023 at 07:39:48PM -0300, Daniel Henrique Barboza wrote:
> The TCG emulation implements all the extensions described in the
> RVA22U64 profile, both mandatory and optional. The mandatory extensions
> will be enabled via the profile flag. We'll leave the optional
> extensions to be enabled by hand.
> 
> Given that this is the first profile we're implementing in TCG we'll
> need some ground work first:
> 
> - all profiles declared in riscv_profiles[] will be exposed to users.
> TCG is the main accelerator we're considering when adding profile
> support in QEMU, so for now it's safe to assume that all profiles in
> riscv_profiles[] will be relevant to TCG;
> 
> - we'll not support user profile settings for vendor CPUs. The flags
> will still be exposed but users won't be able to change them. The idea
> is that vendor CPUs in the future can enable profiles internally in
> their cpu_init() functions, showing to the external world that the CPU
> supports a certain profile. But users won't be able to enable/disable
> it;
> 
> - Setting a profile to 'true' means 'enable all mandatory extensions of
> this profile, setting it to 'false' means disabling all its mandatory
> extensions. Disabling a profile is discouraged for regular use and will
> issue an user warning. User choices for individual extensions will take
> precedence, i.e. enabling a profile will not enable extensions that the
> user set to 'false', and vice-versa. This will make us independent of
> left-to-right ordering in the QEMU command line, i.e. the following QEMU
> command lines:
> 
> -cpu rv64,zicbom=false,rva22u64=true,Zifencei=false
> -cpu rv64,zicbom=false,Zifencei=false,rva22u64=true
> -cpu rv64,rva22u64=true,zicbom=false,Zifencei=false
> 
> They mean the same thing: "enable all mandatory extensions of the
> rva22u64 profile while keeping zicbom and Zifencei disabled".

Hmm, I'm not sure I agree with special-casing profiles like this. I think
the left-to-right processing should be consistent for all. I'm also not
sure we should always warn when disabling a profile. For example, if a
user does

 -cpu rv64,rva22u64=true,rva22u64=false

then they'll get a warning, even though all they're doing is restoring the
cpu model. While that looks like an odd thing to do, a script may be
adding the rva22u64=true and the rva22u64=false is the user input which
undoes what the script did.

As far as warnings go, it'd be nice to warn when mandatory profile
extensions are disabled from an enabled profile. Doing that might be
useful for debug, but users which do it without being aware they're
"breaking" the profile may learn from that warning. Note, the warning
should only come when the profile is actually enabled and when the
extension would actually be disabled, i.e.

 -cpu rv64,rva22u64=true,c=off

should warn

 -cpu rv64,c=off,rva22u64=true

should not warn (rva22u64 overrides c=off since it's to the right)

 -cpu rv64,rva22u64=true,rva22u64=false,c=off

should not warn (rva22u64 is not enabled)

And,

 -cpu rv64,rva22u64=true,rva24u64=false

should warn for each extension which is mandatory in both profiles.

Thanks,
drew