| Message ID | 20231020223951.357513-4-dbarboza@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: RVA22U64 profile support | expand |
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
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
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 > > "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. Yup, sounds good. Thanks, drew
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(). >> >> >>> >>> 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. > > Yup, sounds good. > > Thanks, > drew
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 <dbarboza@ventanamicro.com> > --- > 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, &value, 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, &value, 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_liu@linux.alibaba.com> Zhiwei > for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) { > qdev_property_add_static(DEVICE(obj), prop); > }
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.
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, without the special property, error out. libvirt would default to the error out case, whichever that one is, but also provide an element to turn off erroring-out. Thanks, drew
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] 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. I understand the appeal, I just think that regular users should be prevented from stumbling into this kind of expert-level, intentionally-broken configuration by mistake. > > 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, without the > special property, error out. libvirt would default to the error out > case, whichever that one is, but also provide an element to turn off > erroring-out. I would be okay with something along these lines.
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 disabled, nothing happens. Fans of the current design are welcome to weight in the discussion, of course. If we decide in the future that stripping extensions from a CPU model is desirable we can come up with a 'bare' option, e.g. "-cpu rv64,bare=true" will strip all extensions from rv64. This is a much cleaner way of doing what profile disablement is currently doing. Thanks, Daniel [1] https://lore.kernel.org/qemu-riscv/ZRarBuEeBi7WkS6K@redhat.com/ [2] https://lore.kernel.org/qemu-riscv/20231025234459.581697-10-dbarboza@ventanamicro.com/ > >>>>>> 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. > > I understand the appeal, I just think that regular users should be > prevented from stumbling into this kind of expert-level, > intentionally-broken configuration by mistake. > >>> 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, without the >> special property, error out. libvirt would default to the error out >> case, whichever that one is, but also provide an element to turn off >> erroring-out. > > I would be okay with something along these lines. >
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 like that, if > any. If we provided a mechanism to query the state of profiles, then they should be 'true' when all their mandatory extensions are present. That means that, despite defaulting to 'false', they should become true at realize time after detecting all their mandatory extensions are present, and then, if a user set the profile false, the user would be disabling all the profile's extensions along with the profile. Without a mechanism to query profiles, then it doesn't matter if they remain false after realize, even if they should be true, but that's just a coding decision (which probably deserves a code comment). > > 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. I would disable the extensions that G represents when G is explicitly disabled, just as I suggest we do for profiles. > 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. More extension shorthands are coming[1] and, IMO, they should be addressed in the same way as G (whatever the final decision on how to address G is). Since G collects such basic extensions, then it might be hard to reason as to why it should be possible to disable it, but if, for example, you have a CPU which supports the Zvkn extensions and you want to test/debug with all of those extensions disabled, then it'd be nice to do zvkn=off and expect all of them to be disabled. [1] https://lore.kernel.org/all/20231026151828.754279-1-max.chou@sifive.com/ > > 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. I've already pointed out a couple times that disabling a profile may be useful for scripts, particularly for bare-bones CPUs, since you know that disabling something previously enabled is implementing a clean 'undo'. > > After writing all this stuff, and realizing that profile disablement creates a > lot of confusion I'm having trouble imaging this hypothetical user that knows how to construction QEMU command lines, knows what profiles are meant to enable, knows how boolean properties work, but would be surprised that 'profile=off' disabled the profile's mandatory extensions and not surprised when 'profile=off' does nothing. So, for me, 'profile=off' doing nothing looks like it'll introduce confusion, not the other way around. > 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). I presume the user disabled extensions would need to come to the right of the profiles on the command line to be respected. > If disabled, nothing happens. Fans > of the current design are welcome to weight in the discussion, of course. > > If we decide in the future that stripping extensions from a CPU model is desirable > we can come up with a 'bare' option, e.g. "-cpu rv64,bare=true" will strip all > extensions from rv64. This is a much cleaner way of doing what profile disablement > is currently doing. Doesn't this just kick the can down the road? I suspect we'd end up discussing what 'bare=false' means at that time. Thanks, drew
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, &value, 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, &value, 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); + for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) { qdev_property_add_static(DEVICE(obj), prop); }
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 <dbarboza@ventanamicro.com> --- target/riscv/tcg/tcg-cpu.c | 59 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)