riscv: 'max' CPU, detect user choice in TCG
v8 cover riscv: 'max' CPU, detect user choice in TCG
Hi,
This is a resend of these two patch sets because they no longer apply into Alistair's riscv-to-apply.next:
[PATCH v8 00/12] riscv: add 'max' CPU, deprecate 'any'
https://lore.kernel.org/qemu-riscv/[email protected]/
[PATCH v3 0/8] riscv: detecting user choice in TCG extensions
https://lore.kernel.org/qemu-riscv/[email protected]/
They're being sent in a single package for convenience. No other changes made from their old versions aside from the rebase.
Patches missing acks: 4,7,8
Changes from v7:
- patch 7:
- add riscv_cpu_add_qdev_prop_array() function instead of a macro
- patch 8:
- add riscv_cpu_add_kvm_unavail_prop_array() function instead of a
macro
- v7 link: https://lore.kernel.org/qemu-riscv/[email protected]/
Daniel Henrique Barboza (20):
target/riscv/cpu.c: split CPU options from riscv_cpu_extensions[] target/riscv/cpu.c: skip 'bool' check when filtering KVM props target/riscv/cpu.c: split kvm prop handling to its own helper target/riscv: add DEFINE_PROP_END_OF_LIST() to riscv_cpu_options[] target/riscv/cpu.c: split non-ratified exts from
riscv_cpu_extensions[]
target/riscv/cpu.c: split vendor exts from riscv_cpu_extensions[] target/riscv/cpu.c: add riscv_cpu_add_qdev_prop_array() target/riscv/cpu.c: add riscv_cpu_add_kvm_unavail_prop_array() target/riscv/cpu.c: limit cfg->vext_spec log message target/riscv: add 'max' CPU type
avocado, risc-v: add opensbi tests for 'max' CPU target/riscv: deprecate the 'any' CPU type target/riscv/cpu.c: use offset in isa_ext_is_enabled/update_enabled target/riscv: make CPUCFG() macro public target/riscv/cpu.c: introduce cpu_cfg_ext_auto_update() target/riscv/cpu.c: use cpu_cfg_ext_auto_update() during realize() target/riscv/cpu.c: introduce RISCVCPUMultiExtConfig target/riscv: use isa_ext_update_enabled() in
init_max_cpu_extensions()
target/riscv/cpu.c: honor user choice in cpu_cfg_ext_auto_update() target/riscv/cpu.c: consider user option with RVG
docs/about/deprecated.rst | 12 + target/riscv/cpu-qom.h | 1 + target/riscv/cpu.c | 555 +++++++++++++++++++++++---------- target/riscv/cpu.h | 2 + target/riscv/kvm.c | 8 +- tests/avocado/riscv_opensbi.py | 16 + 6 files changed, 425 insertions(+), 169 deletions(-)
Signature · 1 line
2.41.0
PATCH RESEND v8 1/20 target/riscv/cpu.c: split CPU options from riscv_cpu_extensions[]
We'll add a new CPU type that will enable a considerable amount of extensions. To make it easier for us we'll do a few cleanups in our existing riscv_cpu_extensions[] array.
Start by splitting all CPU non-boolean options from it. Create a new riscv_cpu_options[] array for them. Add all these properties in riscv_cpu_add_user_properties() as it is already being done today.
'mmu' and 'pmp' aren't really extensions in the usual way we think about RISC-V extensions. These are closer to CPU features/options, so move both to riscv_cpu_options[] too. In the near future we'll need to match all extensions with all entries in isa_edata_arr[], and so it happens that both 'mmu' and 'pmp' do not have a riscv,isa string (thus, no priv spec version restriction). This further emphasizes the point that these are more a CPU option than an extension.
No functional changes made.
--- target/riscv/cpu.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-)
target/riscv/cpu.c +23 -10
@@ -1804,7 +1804,6 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)static Property riscv_cpu_extensions[] = {/* Defaults for standard extensions */ -DEFINE_PROP_UINT8 ("pmu-num" ,RISCVCPU , cfg.pmu_num,16 ),DEFINE_PROP_BOOL ("sscofpmf" ,RISCVCPU , cfg.ext_sscofpmf, false),DEFINE_PROP_BOOL ("Zifencei" ,RISCVCPU , cfg.ext_ifencei, true),DEFINE_PROP_BOOL ("Zicsr" ,RISCVCPU , cfg.ext_icsr, true),
@@ -1817,15 +1816,8 @@ static Property riscv_cpu_extensions[] = {DEFINE_PROP_BOOL ("Zve32f" ,RISCVCPU , cfg.ext_zve32f, false),DEFINE_PROP_BOOL ("Zve64f" ,RISCVCPU , cfg.ext_zve64f, false),DEFINE_PROP_BOOL ("Zve64d" ,RISCVCPU , cfg.ext_zve64d, false), -DEFINE_PROP_BOOL ("mmu" ,RISCVCPU , cfg.mmu, true), -DEFINE_PROP_BOOL ("pmp" ,RISCVCPU , cfg.pmp, true),DEFINE_PROP_BOOL ("sstc" ,RISCVCPU , cfg.ext_sstc, true), -DEFINE_PROP_STRING ("priv_spec" ,RISCVCPU , cfg.priv_spec), -DEFINE_PROP_STRING ("vext_spec" ,RISCVCPU , cfg.vext_spec), -DEFINE_PROP_UINT16 ("vlen" ,RISCVCPU , cfg.vlen,128 ), -DEFINE_PROP_UINT16 ("elen" ,RISCVCPU , cfg.elen,64 ), -DEFINE_PROP_BOOL ("smstateen" ,RISCVCPU , cfg.ext_smstateen, false),DEFINE_PROP_BOOL ("svadu" ,RISCVCPU , cfg.ext_svadu, true), DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false),
@@ -1856,9 +1848,7 @@ static Property riscv_cpu_extensions[] = {DEFINE_PROP_BOOL ("zhinxmin" ,RISCVCPU , cfg.ext_zhinxmin, false),DEFINE_PROP_BOOL ("zicbom" ,RISCVCPU , cfg.ext_icbom, true), -DEFINE_PROP_UINT16 ("cbom_blocksize" ,RISCVCPU , cfg.cbom_blocksize,64 ),DEFINE_PROP_BOOL ("zicboz" ,RISCVCPU , cfg.ext_icboz, true), -DEFINE_PROP_UINT16 ("cboz_blocksize" ,RISCVCPU , cfg.cboz_blocksize,64 ), DEFINE_PROP_BOOL("zmmul", RISCVCPU, cfg.ext_zmmul, false),
@@ -1912,6 +1902,21 @@ static Property riscv_cpu_extensions[] = { DEFINE_PROP_END_OF_LIST(), }; +static Property riscv_cpu_options[] = { +DEFINE_PROP_UINT8 ("pmu-num" ,RISCVCPU , cfg.pmu_num,16 ), + +DEFINE_PROP_BOOL ("mmu" ,RISCVCPU , cfg.mmu, true), +DEFINE_PROP_BOOL ("pmp" ,RISCVCPU , cfg.pmp, true), + +DEFINE_PROP_STRING ("priv_spec" ,RISCVCPU , cfg.priv_spec), +DEFINE_PROP_STRING ("vext_spec" ,RISCVCPU , cfg.vext_spec), + +DEFINE_PROP_UINT16 ("vlen" ,RISCVCPU , cfg.vlen,128 ), +DEFINE_PROP_UINT16 ("elen" ,RISCVCPU , cfg.elen,64 ), + +DEFINE_PROP_UINT16 ("cbom_blocksize" ,RISCVCPU , cfg.cbom_blocksize,64 ), +DEFINE_PROP_UINT16 ("cboz_blocksize" ,RISCVCPU , cfg.cboz_blocksize,64 ), +};#ifndef CONFIG_USER_ONLY static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
@@ -1980,6 +1985,14 @@ static void riscv_cpu_add_user_properties(Object *obj)#endif qdev_property_add_static (dev, prop); } + +for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_options); i++) { +/* Check if KVM created the property already */ +if (object_property_find (obj, riscv_cpu_options[i].name)) { +continue ; + } +qdev_property_add_static (dev, &riscv_cpu_options[i]); + } }static Property riscv_cpu_properties[] = {
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 01/20] target/riscv/cpu.c: split CPU options from riscv_cpu_extensions[]
Quoted 16 lines · Daniel Henrique Barboza
We'll add a new CPU type that will enable a considerable amount of extensions. To make it easier for us we'll do a few cleanups in our existing riscv_cpu_extensions[] array.
Start by splitting all CPU non-boolean options from it. Create a new riscv_cpu_options[] array for them. Add all these properties in
riscv_cpu_add_user_properties()as it is already being done today.'mmu' and 'pmp' aren't really extensions in the usual way we think about RISC-V extensions. These are closer to CPU features/options, so move both to riscv_cpu_options[] too. In the near future we'll need to match all extensions with all entries in isa_edata_arr[], and so it happens that both 'mmu' and 'pmp' do not have a riscv,isa string (thus, no priv spec version restriction). This further emphasizes the point that these are more a CPU option than an extension.
Quoted 9 lines
No functional changes made.
Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Weiwei Li <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- target/riscv/cpu.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-)
PATCH RESEND v8 2/20 target/riscv/cpu.c: skip 'bool' check when filtering KVM props
In reply to Daniel Henrique BarbozaAfter the introduction of riscv_cpu_options[] all properties in riscv_cpu_extensions[] are booleans. This check is now obsolete.
--- target/riscv/cpu.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
target/riscv/cpu.c +4 -10
@@ -1970,17 +1970,11 @@ static void riscv_cpu_add_user_properties(Object *obj)* Set the default to disabled for every extension * unknown to KVM and error out if the user attempts * to enable any of them. -* -* We're giving a pass for non-bool properties since they're -* not related to the availability of extensions and can be -* safely ignored as is. */ -if (prop->info == &qdev_prop_bool) { -object_property_add (obj, prop->name,"bool" , -NULL , cpu_set_cfg_unavailable, -NULL , (void *)prop->name); -continue ; - } +object_property_add (obj, prop->name,"bool" , +NULL , cpu_set_cfg_unavailable, +NULL , (void *)prop->name); + continue; }#endif qdev_property_add_static (dev, prop);
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 02/20] target/riscv/cpu.c: skip 'bool' check when filtering KVM props
Quoted 39 lines · Daniel Henrique Barboza
After the introduction of riscv_cpu_options[] all properties in riscv_cpu_extensions[] are booleans. This check is now obsolete.
Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- target/riscv/cpu.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)target/riscv/cpu.c +4
-10@@ -1970,17 +1970,11 @@ static void riscv_cpu_add_user_properties(Object *obj)* Set the default to disabled for every extension * unknown to KVM and error out if the user attempts * to enable any of them. -* -* We're giving a pass for non-bool properties since they're -* not related to the availability of extensions and can be -* safely ignored as is. */ -if (prop->info == &qdev_prop_bool) { -object_property_add (obj, prop->name,"bool" , -NULL , cpu_set_cfg_unavailable, -NULL , (void *)prop->name); -continue ; - } +object_property_add (obj, prop->name,"bool" , +NULL , cpu_set_cfg_unavailable, +NULL , (void *)prop->name); + continue; }#endif qdev_property_add_static (dev, prop);Signature · 1 line
2.41.0
PATCH RESEND v8 3/20 target/riscv/cpu.c: split kvm prop handling to its own helper
In reply to Daniel Henrique BarbozaFuture patches will split the existing Property arrays even further, and the existing code in riscv_cpu_add_user_properties() will start to scale bad with it because it's dealing with KVM constraints mixed in with TCG constraints. We're going to pay a high price to share a couple of common lines of code between the two.
Create a new riscv_cpu_add_kvm_properties() that will be forked from riscv_cpu_add_user_properties() if we're running KVM. The helper includes all properties that a KVM CPU will add. The rest of riscv_cpu_add_user_properties() body will then be relieved from having to deal with KVM constraints.
--- target/riscv/cpu.c | 65 ++++++++++++++++++++++++++++++---------------- 1 file changed, 42 insertions(+), 23 deletions(-)
target/riscv/cpu.c +42 -23
@@ -1937,6 +1937,46 @@ static void cpu_set_cfg_unavailable(Object *obj, Visitor *v, }#endif +#ifndef CONFIG_USER_ONLY +static void riscv_cpu_add_kvm_unavail_prop (Object *obj,const char *prop_name) +{ +/* Check if KVM created the property already */ +if (object_property_find (obj, prop_name)) { +return ; + } + +/* +* Set the default to disabled for every extension +* unknown to KVM and error out if the user attempts +* to enable any of them. +*/ +object_property_add (obj, prop_name,"bool" , +NULL , cpu_set_cfg_unavailable, +NULL , (void *)prop_name); +} + +static void riscv_cpu_add_kvm_properties (Object *obj) +{ +Property *prop; +DeviceState *dev =DEVICE (obj); + +kvm_riscv_init_user_properties (obj); +riscv_cpu_add_misa_properties (obj); + +for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { +riscv_cpu_add_kvm_unavail_prop (obj, prop->name); + } + +for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_options); i++) { +/* Check if KVM created the property already */ +if (object_property_find (obj, riscv_cpu_options[i].name)) { +continue ; + } +qdev_property_add_static (dev, &riscv_cpu_options[i]); + } +} +#endif +/* * Add CPU properties with user-facing flags. *
@@ -1952,39 +1992,18 @@ static void riscv_cpu_add_user_properties(Object *obj)riscv_add_satp_mode_properties (obj);if (kvm_enabled ()) { -kvm_riscv_init_user_properties (obj); +riscv_cpu_add_kvm_properties (obj); +return ; }#endif riscv_cpu_add_misa_properties (obj);for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { -#ifndef CONFIG_USER_ONLY -if (kvm_enabled ()) { -/* Check if KVM created the property already */ -if (object_property_find (obj, prop->name)) { -continue ; - } - -/* -* Set the default to disabled for every extension -* unknown to KVM and error out if the user attempts -* to enable any of them. -*/ -object_property_add (obj, prop->name,"bool" , -NULL , cpu_set_cfg_unavailable, -NULL , (void *)prop->name); -continue ; - } -#endif qdev_property_add_static (dev, prop); }for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_options); i++) { -/* Check if KVM created the property already */ -if (object_property_find (obj, riscv_cpu_options[i].name)) { -continue ; - }qdev_property_add_static (dev, &riscv_cpu_options[i]); } }
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 03/20] target/riscv/cpu.c: split kvm prop handling to its own helper
Quoted 12 lines · Daniel Henrique Barboza
Future patches will split the existing Property arrays even further, and the existing code in
riscv_cpu_add_user_properties()will start to scale bad with it because it's dealing with KVM constraints mixed in with TCG constraints. We're going to pay a high price to share a couple of common lines of code between the two.Create a new
riscv_cpu_add_kvm_properties()that will be forked fromriscv_cpu_add_user_properties()if we're running KVM. The helper includes all properties that a KVM CPU will add. The rest ofriscv_cpu_add_user_properties()body will then be relieved from having to deal with KVM constraints.
Quoted 7 lines
Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Weiwei Li <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- target/riscv/cpu.c | 65 ++++++++++++++++++++++++++++++---------------- 1 file changed, 42 insertions(+), 23 deletions(-)
PATCH RESEND v8 4/20 target/riscv: add DEFINE_PROP_END_OF_LIST() to riscv_cpu_options[]
In reply to Daniel Henrique BarbozaAdd DEFINE_PROP_END_OF_LIST() and eliminate the ARRAY_SIZE() usage when iterating in the riscv_cpu_options[] array, making it similar to what we already do when working with riscv_cpu_extensions[].
We also have a more sophisticated motivation behind this change. In the future we might need to export riscv_cpu_options[] to other files, and ARRAY_LIST() doesn't work properly in that case because the array size isn't exposed to the header file. Here's a future sight of what we would deal with:
./target/riscv/kvm.c:1057:5: error: nested extern declaration of 'riscv_cpu_add_misa_properties' [-Werror=nested-externs] n file included from ../target/riscv/kvm.c:19:home/danielhb/work/qemu/include/qemu/osdep.h:473:31: error: invalid application of 'sizeof' to incomplete type 'const RISCVCPUMultiExtConfig[]'
473 | #define ARRAY_SIZE(x) ((sizeof(x) / sizeof((x)[0])) + \
| ^./target/riscv/kvm.c:1047:29: note: in expansion of macro 'ARRAY_SIZE'
1047 | for (int i = 0; i < ARRAY_SIZE(_array); i++) { \
| ^~~~~~~~~~
./target/riscv/kvm.c:1059:5: note: in expansion of macro 'ADD_UNAVAIL_KVM_PROP_ARRAY'
1059 | ADD_UNAVAIL_KVM_PROP_ARRAY(obj, riscv_cpu_extensions);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~home/danielhb/work/qemu/include/qemu/osdep.h:473:31: error: invalid application of 'sizeof' to incomplete type 'const RISCVCPUMultiExtConfig[]'
473 | #define ARRAY_SIZE(x) ((sizeof(x) / sizeof((x)[0])) + \
| ^./target/riscv/kvm.c:1047:29: note: in expansion of macro 'ARRAY_SIZE'
1047 | for (int i = 0; i < ARRAY_SIZE(_array); i++) { \
Homogenize the present and change the future by using DEFINE_PROP_END_OF_LIST() in riscv_cpu_options[].
--- target/riscv/cpu.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
target/riscv/cpu.c +7 -5
@@ -1916,6 +1916,8 @@ static Property riscv_cpu_options[] = {DEFINE_PROP_UINT16 ("cbom_blocksize" ,RISCVCPU , cfg.cbom_blocksize,64 ),DEFINE_PROP_UINT16 ("cboz_blocksize" ,RISCVCPU , cfg.cboz_blocksize,64 ), + + DEFINE_PROP_END_OF_LIST(), };#ifndef CONFIG_USER_ONLY
@@ -1967,12 +1969,12 @@ static void riscv_cpu_add_kvm_properties(Object *obj) riscv_cpu_add_kvm_unavail_prop(obj, prop->name); } -for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_options); i++) { +for (prop = riscv_cpu_options; prop && prop->name; prop++) {/* Check if KVM created the property already */ -if (object_property_find (obj, riscv_cpu_options[i].name)) { +if (object_property_find (obj, prop->name)) {continue ; } -qdev_property_add_static (dev, &riscv_cpu_options[i]); +qdev_property_add_static (dev, prop); } }#endif
@@ -2003,8 +2005,8 @@ static void riscv_cpu_add_user_properties(Object *obj)qdev_property_add_static (dev, prop); } -for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_options); i++) { -qdev_property_add_static (dev, &riscv_cpu_options[i]); +for (prop = riscv_cpu_options; prop && prop->name; prop++) { +qdev_property_add_static (dev, prop); } }
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 04/20] target/riscv: add DEFINE_PROP_END_OF_LIST() to riscv_cpu_options[]
Quoted 49 lines · Daniel Henrique Barboza
Add
DEFINE_PROP_END_OF_LIST()and eliminate theARRAY_SIZE()usage when iterating in the riscv_cpu_options[] array, making it similar to what we already do when working with riscv_cpu_extensions[].We also have a more sophisticated motivation behind this change. In the future we might need to export riscv_cpu_options[] to other files, and
ARRAY_LIST()doesn't work properly in that case because the array size isn't exposed to the header file. Here's a future sight of what we would deal with:
./target/riscv/kvm.c:1057:5: error: nested extern declaration of 'riscv_cpu_add_misa_properties' [-Werror=nested-externs] n file included from../target/riscv/kvm.c:19:home/danielhb/work/qemu/include/qemu/osdep.h:473:31: error: invalid application of 'sizeof' to incomplete type 'const RISCVCPUMultiExtConfig[]'
473 | #defineARRAY_SIZE(x)((sizeof(x) / sizeof((x)[0])) + \
| ^./target/riscv/kvm.c:1047:29: note: in expansion of macro 'ARRAY_SIZE'1047 | for (int i = 0; i < ARRAY_SIZE(_array); i++) { \ | ^~~~~~~~~~ ./target/riscv/kvm.c:1059:5: note: in expansion of macro 'ADD_UNAVAIL_KVM_PROP_ARRAY' 1059 | ADD_UNAVAIL_KVM_PROP_ARRAY(obj, riscv_cpu_extensions); | ^~~~~~~~~~~~~~~~~~~~~~~~~~
home/danielhb/work/qemu/include/qemu/osdep.h:473:31: error: invalid application of 'sizeof' to incomplete type 'const RISCVCPUMultiExtConfig[]'
473 | #defineARRAY_SIZE(x)((sizeof(x) / sizeof((x)[0])) + \
| ^./target/riscv/kvm.c:1047:29: note: in expansion of macro 'ARRAY_SIZE'
1047 | for (int i = 0; i <ARRAY_SIZE(_array); i++) { \Homogenize the present and change the future by using
DEFINE_PROP_END_OF_LIST()in riscv_cpu_options[].Signed-off-by Daniel Henrique Barboza <[email protected]>--- target/riscv/cpu.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)target/riscv/cpu.c +2
-0@@ -1916,6 +1916,8 @@ static Property riscv_cpu_options[] = {DEFINE_PROP_UINT16 ("cbom_blocksize" ,RISCVCPU , cfg.cbom_blocksize,64 ),DEFINE_PROP_UINT16 ("cboz_blocksize" ,RISCVCPU , cfg.cboz_blocksize,64 ), + + DEFINE_PROP_END_OF_LIST(), };#ifndef CONFIG_USER_ONLY
target/riscv/cpu.c static void riscv_cpu_add_kvm_properties(Object *obj) · line 1972
Quoted 5 lines
target/riscv/cpu.c +1
-1@@ -1967,12 +1969,12 @@ static void riscv_cpu_add_kvm_properties(Object *obj)riscv_cpu_add_kvm_unavail_prop (obj, prop->name); } -for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_options); i++) { +for (prop = riscv_cpu_options; prop && prop->name; prop++) {
Checking prop->name is sufficient.
Quoted 25 lines
target/riscv/cpu.c +4
-4/* Check if KVM created the property already */ -if (object_property_find (obj, riscv_cpu_options[i].name)) { +if (object_property_find (obj, prop->name)) {continue ; } - qdev_property_add_static(dev, &riscv_cpu_options[i]); + qdev_property_add_static(dev, prop); } }#endif @@ -2003,8 +2005,8 @@ static void riscv_cpu_add_user_properties(Object *obj)qdev_property_add_static (dev, prop); } -for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_options); i++) { -qdev_property_add_static (dev, &riscv_cpu_options[i]); +for (prop = riscv_cpu_options; prop && prop->name; prop++) { +qdev_property_add_static (dev, prop); } }Signature · 1 line
2.41.0
PATCH RESEND v8 5/20 target/riscv/cpu.c: split non-ratified exts from riscv_cpu_extensions[]
In reply to Daniel Henrique BarbozaCreate a new riscv_cpu_experimental_exts[] to store the non-ratified extensions properties. Once they are ratified we'll move them back to riscv_cpu_extensions[].
riscv_cpu_add_user_properties() and riscv_cpu_add_kvm_properties() are changed to keep adding non-ratified properties to users.
--- target/riscv/cpu.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
target/riscv/cpu.c +13 -1
@@ -1874,7 +1874,11 @@ static Property riscv_cpu_extensions[] = {DEFINE_PROP_BOOL ("xtheadsync" ,RISCVCPU , cfg.ext_xtheadsync, false),DEFINE_PROP_BOOL ("xventanacondops" ,RISCVCPU , cfg.ext_XVentanaCondOps, false), -/* These are experimental so mark with 'x-' */ + DEFINE_PROP_END_OF_LIST(), +}; + +/* These are experimental so mark with 'x-' */ +static Property riscv_cpu_experimental_exts[] = {DEFINE_PROP_BOOL ("x-zicond" ,RISCVCPU , cfg.ext_zicond, false), /* ePMP 0.9.3 */
@@ -1969,6 +1973,10 @@ static void riscv_cpu_add_kvm_properties(Object *obj)riscv_cpu_add_kvm_unavail_prop (obj, prop->name); } +for (prop = riscv_cpu_experimental_exts; prop && prop->name; prop++) { +riscv_cpu_add_kvm_unavail_prop (obj, prop->name); + } +for (prop = riscv_cpu_options; prop && prop->name; prop++) {/* Check if KVM created the property already */ if (object_property_find (obj, prop->name)) {
@@ -2008,6 +2016,10 @@ static void riscv_cpu_add_user_properties(Object *obj)for (prop = riscv_cpu_options; prop && prop->name; prop++) {qdev_property_add_static (dev, prop); } + +for (prop = riscv_cpu_experimental_exts; prop && prop->name; prop++) { +qdev_property_add_static (dev, prop); + } }static Property riscv_cpu_properties[] = {
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 05/20] target/riscv/cpu.c: split non-ratified exts from riscv_cpu_extensions[]
Quoted 56 lines · Daniel Henrique Barboza
Create a new riscv_cpu_experimental_exts[] to store the non-ratified extensions properties. Once they are ratified we'll move them back to riscv_cpu_extensions[].
riscv_cpu_add_user_properties()andriscv_cpu_add_kvm_properties()are changed to keep adding non-ratified properties to users.Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- target/riscv/cpu.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)target/riscv/cpu.c +13
-1@@ -1874,7 +1874,11 @@ static Property riscv_cpu_extensions[] = {DEFINE_PROP_BOOL ("xtheadsync" ,RISCVCPU , cfg.ext_xtheadsync, false),DEFINE_PROP_BOOL ("xventanacondops" ,RISCVCPU , cfg.ext_XVentanaCondOps, false), -/* These are experimental so mark with 'x-' */ + DEFINE_PROP_END_OF_LIST(), +}; + +/* These are experimental so mark with 'x-' */ +static Property riscv_cpu_experimental_exts[] = {DEFINE_PROP_BOOL ("x-zicond" ,RISCVCPU , cfg.ext_zicond, false), /* ePMP 0.9.3 */@@ -1969,6 +1973,10 @@ static void riscv_cpu_add_kvm_properties(Object *obj)riscv_cpu_add_kvm_unavail_prop (obj, prop->name); } +for (prop = riscv_cpu_experimental_exts; prop && prop->name; prop++) { +riscv_cpu_add_kvm_unavail_prop (obj, prop->name); + } +for (prop = riscv_cpu_options; prop && prop->name; prop++) {/* Check if KVM created the property already */ if (object_property_find (obj, prop->name)) {@@ -2008,6 +2016,10 @@ static void riscv_cpu_add_user_properties(Object *obj)for (prop = riscv_cpu_options; prop && prop->name; prop++) {qdev_property_add_static (dev, prop); } + +for (prop = riscv_cpu_experimental_exts; prop && prop->name; prop++) { +qdev_property_add_static (dev, prop); + } }static Property riscv_cpu_properties[] = {Signature · 1 line
2.41.0
PATCH RESEND v8 6/20 target/riscv/cpu.c: split vendor exts from riscv_cpu_extensions[]
In reply to Daniel Henrique BarbozaOur goal is to make riscv_cpu_extensions[] hold only ratified, non-vendor extensions.
Create a new riscv_cpu_vendor_exts[] array for them, changing riscv_cpu_add_user_properties() and riscv_cpu_add_kvm_properties() accordingly.
--- target/riscv/cpu.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
target/riscv/cpu.c +12 -1
@@ -1860,7 +1860,10 @@ static Property riscv_cpu_extensions[] = {DEFINE_PROP_BOOL ("zcmp" ,RISCVCPU , cfg.ext_zcmp, false),DEFINE_PROP_BOOL ("zcmt" ,RISCVCPU , cfg.ext_zcmt, false), -/* Vendor-specific custom extensions */ + DEFINE_PROP_END_OF_LIST(), +}; + +static Property riscv_cpu_vendor_exts[] = {DEFINE_PROP_BOOL ("xtheadba" ,RISCVCPU , cfg.ext_xtheadba, false),DEFINE_PROP_BOOL ("xtheadbb" ,RISCVCPU , cfg.ext_xtheadbb, false), DEFINE_PROP_BOOL("xtheadbs", RISCVCPU, cfg.ext_xtheadbs, false),
@@ -1973,6 +1976,10 @@ static void riscv_cpu_add_kvm_properties(Object *obj)riscv_cpu_add_kvm_unavail_prop (obj, prop->name); } +for (prop = riscv_cpu_vendor_exts; prop && prop->name; prop++) { +riscv_cpu_add_kvm_unavail_prop (obj, prop->name); + } +for (prop = riscv_cpu_experimental_exts; prop && prop->name; prop++) {riscv_cpu_add_kvm_unavail_prop (obj, prop->name); }
@@ -2017,6 +2024,10 @@ static void riscv_cpu_add_user_properties(Object *obj)qdev_property_add_static (dev, prop); } +for (prop = riscv_cpu_vendor_exts; prop && prop->name; prop++) { +qdev_property_add_static (dev, prop); + } +for (prop = riscv_cpu_experimental_exts; prop && prop->name; prop++) {qdev_property_add_static (dev, prop); }
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 06/20] target/riscv/cpu.c: split vendor exts from riscv_cpu_extensions[]
Quoted 55 lines · Daniel Henrique Barboza
Our goal is to make riscv_cpu_extensions[] hold only ratified, non-vendor extensions.
Create a new riscv_cpu_vendor_exts[] array for them, changing
riscv_cpu_add_user_properties()andriscv_cpu_add_kvm_properties()accordingly.Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- target/riscv/cpu.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)target/riscv/cpu.c +12
-1@@ -1860,7 +1860,10 @@ static Property riscv_cpu_extensions[] = {DEFINE_PROP_BOOL ("zcmp" ,RISCVCPU , cfg.ext_zcmp, false),DEFINE_PROP_BOOL ("zcmt" ,RISCVCPU , cfg.ext_zcmt, false), -/* Vendor-specific custom extensions */ + DEFINE_PROP_END_OF_LIST(), +}; + +static Property riscv_cpu_vendor_exts[] = {DEFINE_PROP_BOOL ("xtheadba" ,RISCVCPU , cfg.ext_xtheadba, false),DEFINE_PROP_BOOL ("xtheadbb" ,RISCVCPU , cfg.ext_xtheadbb, false), DEFINE_PROP_BOOL("xtheadbs", RISCVCPU, cfg.ext_xtheadbs, false),@@ -1973,6 +1976,10 @@ static void riscv_cpu_add_kvm_properties(Object *obj)riscv_cpu_add_kvm_unavail_prop (obj, prop->name); } +for (prop = riscv_cpu_vendor_exts; prop && prop->name; prop++) { +riscv_cpu_add_kvm_unavail_prop (obj, prop->name); + } +for (prop = riscv_cpu_experimental_exts; prop && prop->name; prop++) {riscv_cpu_add_kvm_unavail_prop (obj, prop->name); }@@ -2017,6 +2024,10 @@ static void riscv_cpu_add_user_properties(Object *obj)qdev_property_add_static (dev, prop); } +for (prop = riscv_cpu_vendor_exts; prop && prop->name; prop++) { +qdev_property_add_static (dev, prop); + } +for (prop = riscv_cpu_experimental_exts; prop && prop->name; prop++) {qdev_property_add_static (dev, prop); }Signature · 1 line
2.41.0
PATCH RESEND v8 7/20 target/riscv/cpu.c: add riscv_cpu_add_qdev_prop_array()
In reply to Daniel Henrique BarbozaThe code inside riscv_cpu_add_user_properties() became quite repetitive after recent changes. Add a helper to hide the repetition away.
--- target/riscv/cpu.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-)
target/riscv/cpu.c +11 -16
@@ -1946,6 +1946,13 @@ static void cpu_set_cfg_unavailable(Object *obj, Visitor *v, }#endif +static void riscv_cpu_add_qdev_prop_array (DeviceState *dev,Property *array) +{ +for (Property *prop = array; prop && prop->name; prop++) { +qdev_property_add_static (dev, prop); + } +} +#ifndef CONFIG_USER_ONLY static void riscv_cpu_add_kvm_unavail_prop (Object *obj,const char *prop_name) {
@@ -2002,7 +2009,6 @@ static void riscv_cpu_add_kvm_properties(Object *obj)*/ static void riscv_cpu_add_user_properties (Object *obj) { -Property *prop;DeviceState *dev =DEVICE (obj);#ifndef CONFIG_USER_ONLY
@@ -2016,21 +2022,10 @@ static void riscv_cpu_add_user_properties(Object *obj)riscv_cpu_add_misa_properties (obj); -for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { -qdev_property_add_static (dev, prop); - } - -for (prop = riscv_cpu_options; prop && prop->name; prop++) { -qdev_property_add_static (dev, prop); - } - -for (prop = riscv_cpu_vendor_exts; prop && prop->name; prop++) { -qdev_property_add_static (dev, prop); - } - -for (prop = riscv_cpu_experimental_exts; prop && prop->name; prop++) { -qdev_property_add_static (dev, prop); - } +riscv_cpu_add_qdev_prop_array (dev, riscv_cpu_extensions); +riscv_cpu_add_qdev_prop_array (dev, riscv_cpu_options); +riscv_cpu_add_qdev_prop_array (dev, riscv_cpu_vendor_exts); +riscv_cpu_add_qdev_prop_array (dev, riscv_cpu_experimental_exts); }static Property riscv_cpu_properties[] = {
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 07/20] target/riscv/cpu.c: add riscv_cpu_add_qdev_prop_array()
Quoted 13 lines · Daniel Henrique Barboza
The code inside
riscv_cpu_add_user_properties()became quite repetitive after recent changes. Add a helper to hide the repetition away.Signed-off-by Daniel Henrique Barboza <[email protected]>--- target/riscv/cpu.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-)target/riscv/cpu.c +0
-0
target/riscv/cpu.c static void cpu_set_cfg_unavailable(Object *obj, Visitor *v, · line 1951
Quoted 6 lines
target/riscv/cpu.c +3
-0@@ -1946,6 +1946,13 @@ static void cpu_set_cfg_unavailable(Object *obj, Visitor *v, }#endif +static void riscv_cpu_add_qdev_prop_array (DeviceState *dev,Property *array) +{ +for (Property *prop = array; prop && prop->name; prop++) {
Here checking prop first is a good idea, as a caller could pass NULL for 'array'. But, if we don't want callers to ever pass NULL for array, then dropping the prop check will ensure we get a segfault to spot the bug (or, a bit cleaner, we could add an explicit assert(array)).
Quoted 45 lines
target/riscv/cpu.c +8
-16+qdev_property_add_static (dev, prop); + } +} +#ifndef CONFIG_USER_ONLY static void riscv_cpu_add_kvm_unavail_prop (Object *obj,const char *prop_name) {@@ -2002,7 +2009,6 @@ static void riscv_cpu_add_kvm_properties(Object *obj)*/ static void riscv_cpu_add_user_properties (Object *obj) { -Property *prop;DeviceState *dev =DEVICE (obj);#ifndef CONFIG_USER_ONLY @@ -2016,21 +2022,10 @@ static void riscv_cpu_add_user_properties(Object *obj)riscv_cpu_add_misa_properties (obj); -for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { -qdev_property_add_static (dev, prop); - } - -for (prop = riscv_cpu_options; prop && prop->name; prop++) { -qdev_property_add_static (dev, prop); - } - -for (prop = riscv_cpu_vendor_exts; prop && prop->name; prop++) { -qdev_property_add_static (dev, prop); - } - -for (prop = riscv_cpu_experimental_exts; prop && prop->name; prop++) { -qdev_property_add_static (dev, prop); - } +riscv_cpu_add_qdev_prop_array (dev, riscv_cpu_extensions); +riscv_cpu_add_qdev_prop_array (dev, riscv_cpu_options); +riscv_cpu_add_qdev_prop_array (dev, riscv_cpu_vendor_exts); +riscv_cpu_add_qdev_prop_array (dev, riscv_cpu_experimental_exts); }static Property riscv_cpu_properties[] = {Signature · 1 line
2.41.0
PATCH RESEND v8 8/20 target/riscv/cpu.c: add riscv_cpu_add_kvm_unavail_prop_array()
In reply to Daniel Henrique BarbozaUse a helper in riscv_cpu_add_kvm_properties() to eliminate some of its code repetition.
--- target/riscv/cpu.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
target/riscv/cpu.c +11 -11
@@ -1971,6 +1971,14 @@ static void riscv_cpu_add_kvm_unavail_prop(Object *obj, const char *prop_name) NULL, (void *)prop_name); } +static void riscv_cpu_add_kvm_unavail_prop_array (Object *obj, +Property *array) +{ +for (Property *prop = array; prop && prop->name; prop++) { +riscv_cpu_add_kvm_unavail_prop (obj, prop->name); + } +} +static void riscv_cpu_add_kvm_properties (Object *obj) {Property *prop;
@@ -1979,17 +1987,9 @@ static void riscv_cpu_add_kvm_properties(Object *obj)kvm_riscv_init_user_properties (obj);riscv_cpu_add_misa_properties (obj); -for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { -riscv_cpu_add_kvm_unavail_prop (obj, prop->name); - } - -for (prop = riscv_cpu_vendor_exts; prop && prop->name; prop++) { -riscv_cpu_add_kvm_unavail_prop (obj, prop->name); - } - -for (prop = riscv_cpu_experimental_exts; prop && prop->name; prop++) { -riscv_cpu_add_kvm_unavail_prop (obj, prop->name); - } +riscv_cpu_add_kvm_unavail_prop_array (obj, riscv_cpu_extensions); +riscv_cpu_add_kvm_unavail_prop_array (obj, riscv_cpu_vendor_exts); +riscv_cpu_add_kvm_unavail_prop_array (obj, riscv_cpu_experimental_exts);for (prop = riscv_cpu_options; prop && prop->name; prop++) {/* Check if KVM created the property already */
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 08/20] target/riscv/cpu.c: add riscv_cpu_add_kvm_unavail_prop_array()
Quoted 52 lines · Daniel Henrique Barboza
Use a helper in
riscv_cpu_add_kvm_properties()to eliminate some of its code repetition.Signed-off-by Daniel Henrique Barboza <[email protected]>--- target/riscv/cpu.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)target/riscv/cpu.c +11
-11@@ -1971,6 +1971,14 @@ static void riscv_cpu_add_kvm_unavail_prop(Object *obj, const char *prop_name) NULL, (void *)prop_name); } +static void riscv_cpu_add_kvm_unavail_prop_array (Object *obj, +Property *array) +{ +for (Property *prop = array; prop && prop->name; prop++) { +riscv_cpu_add_kvm_unavail_prop (obj, prop->name); + } +} +static void riscv_cpu_add_kvm_properties (Object *obj) {Property *prop;@@ -1979,17 +1987,9 @@ static void riscv_cpu_add_kvm_properties(Object *obj)kvm_riscv_init_user_properties (obj);riscv_cpu_add_misa_properties (obj); -for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { -riscv_cpu_add_kvm_unavail_prop (obj, prop->name); - } - -for (prop = riscv_cpu_vendor_exts; prop && prop->name; prop++) { -riscv_cpu_add_kvm_unavail_prop (obj, prop->name); - } - -for (prop = riscv_cpu_experimental_exts; prop && prop->name; prop++) { -riscv_cpu_add_kvm_unavail_prop (obj, prop->name); - } +riscv_cpu_add_kvm_unavail_prop_array (obj, riscv_cpu_extensions); +riscv_cpu_add_kvm_unavail_prop_array (obj, riscv_cpu_vendor_exts); +riscv_cpu_add_kvm_unavail_prop_array (obj, riscv_cpu_experimental_exts);for (prop = riscv_cpu_options; prop && prop->name; prop++) {/* Check if KVM created the property already */ Signature · 1 line
2.41.0
PATCH RESEND v8 9/20 target/riscv/cpu.c: limit cfg->vext_spec log message
In reply to Daniel Henrique BarbozaInside riscv_cpu_validate_v() we're always throwing a log message if the user didn't set a vector version via 'vext_spec'.
We're going to include one case with the 'max' CPU where env->vext_ver will be set in the cpu_init(). But that alone will not stop the "vector version is not specified" message from appearing. The usefulness of this log message is debatable for the generic CPUs, but for a 'max' CPU type, where we are supposed to deliver a CPU model with all features possible, it's strange to force users to set 'vext_spec' to get rid of this message.
Change riscv_cpu_validate_v() to not throw this log message if env->vext_ver is already set.
--- target/riscv/cpu.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
target/riscv/cpu.c +4 -5
@@ -959,8 +959,6 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)static void riscv_cpu_validate_v (CPURISCVState *env,RISCVCPUConfig *cfg,Error **errp) { -int vext_version =VEXT_VERSION_1_00_0 ; -if (!is_power_of_2 (cfg->vlen)) {error_setg (errp,"Vector extension VLEN must be power of 2" );return ;
@@ -983,17 +981,18 @@ static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg, }if (cfg->vext_spec) {if (!g_strcmp0 (cfg->vext_spec,"v1.0" )) { - vext_version =VEXT_VERSION_1_00_0 ; + env->vext_ver =VEXT_VERSION_1_00_0 ; }else {error_setg (errp,"Unsupported vector spec version '%s'" , cfg->vext_spec);return ; } - }else { + }else if (env->vext_ver ==0 ) {qemu_log ("vector version is not specified, " "use the default value v1.0\n" ); + + env->vext_ver =VEXT_VERSION_1_00_0 ; } - env->vext_ver = vext_version; }static void riscv_cpu_validate_priv_spec (RISCVCPU *cpu,Error **errp)
Signature · 1 line
2.41.0
PATCH RESEND v8 10/20 target/riscv: add 'max' CPU type
In reply to Daniel Henrique BarbozaThe 'max' CPU type is used by tooling to determine what's the most capable CPU a current QEMU version implements. Other archs such as ARM implements this type. Let's add it to RISC-V.
What we consider "most capable CPU" in this context are related to ratified, non-vendor extensions. This means that we want the 'max' CPU to enable all (possible) ratified extensions by default. The reasoning behind this design is (1) vendor extensions can conflict with each other and we won't play favorities deciding which one is default or not and
(2) non-ratified extensions are always prone to changes, not being stable enough to be enabled by default.
All this said, we're still not able to enable all ratified extensions due to conflicts between them. Zfinx and all its dependencies aren't enabled because of a conflict with RVF. zce, zcmp and zcmt are also disabled due to RVD conflicts. When running with 64 bits we're also disabling zcf.
MISA bits RVG, RVJ and RVV are also being set manually since they're default disabled.
This is the resulting 'riscv,isa' DT for this new CPU:
rv64imafdcvh_zicbom_zicboz_zicsr_zifencei_zihintpause_zawrs_zfa_ zfh_zfhmin_zca_zcb_zcd_zba_zbb_zbc_zbkb_zbkc_zbkx_zbs_zk_zkn_zknd_ zkne_zknh_zkr_zks_zksed_zksh_zkt_zve32f_zve64f_zve64d_ smstateen_sscofpmf_sstc_svadu_svinval_svnapot_svpbmt
--- target/riscv/cpu-qom.h | 1 + target/riscv/cpu.c | 56 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+)
target/riscv/cpu-qom.h +1 -0
@@ -30,6 +30,7 @@#define CPU_RESOLVING_TYPE TYPE_RISCV_CPU#define TYPE_RISCV_CPU_ANY RISCV_CPU_TYPE_NAME("any") +#define TYPE_RISCV_CPU_MAX RISCV_CPU_TYPE_NAME("max")#define TYPE_RISCV_CPU_BASE32 RISCV_CPU_TYPE_NAME("rv32")#define TYPE_RISCV_CPU_BASE64 RISCV_CPU_TYPE_NAME("rv64")#define TYPE_RISCV_CPU_BASE128 RISCV_CPU_TYPE_NAME("x-rv128")
target/riscv/cpu.c +56 -0
80 lines not on this page · raw message
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 10/20] target/riscv: add 'max' CPU type
Quoted 28 lines · Daniel Henrique Barboza
The 'max' CPU type is used by tooling to determine what's the most capable CPU a current QEMU version implements. Other archs such as ARM implements this type. Let's add it to RISC-V.
What we consider "most capable CPU" in this context are related to ratified, non-vendor extensions. This means that we want the 'max' CPU to enable all (possible) ratified extensions by default. The reasoning behind this design is (1) vendor extensions can conflict with each other and we won't play favorities deciding which one is default or not and
(2) non-ratified extensions are always prone to changes, not being stable enough to be enabled by default.All this said, we're still not able to enable all ratified extensions due to conflicts between them. Zfinx and all its dependencies aren't enabled because of a conflict with RVF. zce, zcmp and zcmt are also disabled due to RVD conflicts. When running with 64 bits we're also disabling zcf.
MISA bits RVG, RVJ and RVV are also being set manually since they're default disabled.
This is the resulting 'riscv,isa' DT for this new CPU:
rv64imafdcvh_zicbom_zicboz_zicsr_zifencei_zihintpause_zawrs_zfa_zfh_zfhmin_zca_zcb_zcd_zba_zbb_zbc_zbkb_zbkc_zbkx_zbs_zk_zkn_zknd_zkne_zknh_zkr_zks_zksed_zksh_zkt_zve32f_zve64f_zve64d_smstateen_sscofpmf_sstc_svadu_svinval_svnapot_svpbmt
Quoted 8 lines
Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Weiwei Li <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- target/riscv/cpu-qom.h | 1 + target/riscv/cpu.c | 56 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+)
PATCH RESEND v8 11/20 avocado, risc-v: add opensbi tests for 'max' CPU
In reply to Daniel Henrique BarbozaAdd smoke tests to ensure that we'll not break the 'max' CPU type when adding new ratified extensions to be enabled.
--- tests/avocado/riscv_opensbi.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
tests/avocado/riscv_opensbi.py +16 -0
@@ -61,3 +61,19 @@ def test_riscv64_virt(self): :avocado: tags=machine:virt """ self.boot_opensbi() + + def test_riscv32_virt_maxcpu(self): + """ + :avocado: tags=arch:riscv32 + :avocado: tags=machine:virt + :avocado: tags=cpu:max + """ + self.boot_opensbi() + + def test_riscv64_virt_maxcpu(self): + """ + :avocado: tags=arch:riscv64 + :avocado: tags=machine:virt + :avocado: tags=cpu:max + """ + self.boot_opensbi()
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 11/20] avocado, risc-v: add opensbi tests for 'max' CPU
Quoted 2 lines · Daniel Henrique Barboza
Add smoke tests to ensure that we'll not break the 'max' CPU type when adding new ratified extensions to be enabled.
I'm not really sure what this test proves other than we didn't remove the minimally supported set of extensions that opensbi requires to boot. The other opensbi tests appear to be ensuring boards can boot, rather than cpus.
Thanks,
drew
Quoted 35 lines
Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- tests/avocado/riscv_opensbi.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)tests/avocado/riscv_opensbi.py +16
-0@@ -61,3 +61,19 @@ def test_riscv64_virt(self): :avocado: tags=machine:virt """ self.boot_opensbi() + + def test_riscv32_virt_maxcpu(self): + """ + :avocado: tags=arch:riscv32 + :avocado: tags=machine:virt + :avocado: tags=cpu:max + """ + self.boot_opensbi() + + def test_riscv64_virt_maxcpu(self): + """ + :avocado: tags=arch:riscv64 + :avocado: tags=machine:virt + :avocado: tags=cpu:max + """ + self.boot_opensbi()Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 11/20] avocado, risc-v: add opensbi tests for 'max' CPU
Quoted 8 lines · Andrew Jones
Quoted 2 lines · Daniel Henrique Barboza
Add smoke tests to ensure that we'll not break the 'max' CPU type when adding new ratified extensions to be enabled.
I'm not really sure what this test proves other than we didn't remove the minimally supported set of extensions that opensbi requires to boot. The other opensbi tests appear to be ensuring boards can boot, rather than cpus.
I added this test because the 'max' CPU takes extra steps to init (e.g. enabling a bunch of extensions) in comparison with the other CPUs. I agree that it would be better to add a Tux test like we already have for the rv32/rv64 CPUs to ensure
that the CPU is able to boot up to prompt.
I'll see if I can get that working and, in that case, we can ditch this test and use a TuxBoot test instead.
Thanks,
Daniel
Quoted 39 lines
Thanks,
drewQuoted 35 lines
Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- tests/avocado/riscv_opensbi.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)tests/avocado/riscv_opensbi.py +16
-0@@ -61,3 +61,19 @@ def test_riscv64_virt(self): :avocado: tags=machine:virt """ self.boot_opensbi() + + def test_riscv32_virt_maxcpu(self): + """ + :avocado: tags=arch:riscv32 + :avocado: tags=machine:virt + :avocado: tags=cpu:max + """ + self.boot_opensbi() + + def test_riscv64_virt_maxcpu(self): + """ + :avocado: tags=arch:riscv64 + :avocado: tags=machine:virt + :avocado: tags=cpu:max + """ + self.boot_opensbi()Signature · 1 line
2.41.0
PATCH RESEND v8 12/20 target/riscv: deprecate the 'any' CPU type
In reply to Daniel Henrique BarbozaThe 'any' CPU type was introduced in commit dc5bd18fa5725 ("RISC-V CPU Core Definition"), being around since the beginning. It's not an easy CPU to use: it's undocumented and its name doesn't tell users much about what the CPU is supposed to bring. 'git log' doesn't help us either in knowing what was the original design of this CPU type.
The closest we have is a comment from Alistair [1] where he recalls from memory that the 'any' CPU is supposed to behave like the newly added 'max' CPU. He also suggested that the 'any' CPU should be removed.
The default CPUs are rv32 and rv64, so removing the 'any' CPU will have impact only on users that might have a script that uses '-cpu any'. And those users are better off using the default CPUs or the new 'max' CPU.
We would love to just remove the code and be done with it, but one does not simply remove a feature in QEMU. We'll put the CPU in quarantine first, letting users know that we have the intent of removing it in the future.
[1] https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg02891.html
--- docs/about/deprecated.rst | 12 ++++++++++++ target/riscv/cpu.c | 5 +++++ 2 files changed, 17 insertions(+)
docs/about/deprecated.rst +12 -0
@@ -371,6 +371,18 @@ QEMU's ``vhost`` feature, which would eliminate the high latency costs under which the 9p ``proxy`` backend currently suffers. However as of to date nobody has indicated plans for such kind of reimplementation unfortunately. +RISC-V 'any' CPU type ``-cpu any`` (since 8.2) +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The 'any' CPU type was introduced back in 2018 and has been around since the +initial RISC-V QEMU port. Its usage has always been unclear: users don't know +what to expect from a CPU called 'any', and in fact the CPU does not do anything +special that aren't already done by the default CPUs rv32/rv64. + +After the introduction of the 'max' CPU type RISC-V now has a good coverage +of generic CPUs: rv32 and rv64 as default CPUs and 'max' as a feature complete +CPU for both 32 and 64 bit builds. Users are then discouraged to use the 'any' +CPU type starting in 8.2. Block device options ''''''''''''''''''''
target/riscv/cpu.c +5 -0
@@ -1522,6 +1522,11 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)RISCVCPUClass *mcc =RISCV_CPU_GET_CLASS (dev);Error *local_err =NULL ; +if (object_dynamic_cast (OBJECT (dev),TYPE_RISCV_CPU_ANY ) !=NULL ) { +warn_report ("The 'any' CPU is deprecated and will be " +"removed in the future." ); + } +cpu_exec_realizefn (cs, &local_err);if (local_err !=NULL ) {error_propagate (errp, local_err);
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 12/20] target/riscv: deprecate the 'any' CPU type
Quoted 34 lines · Daniel Henrique Barboza
The 'any' CPU type was introduced in commit
dc5bd18fa5725("RISC-V CPU Core Definition"), being around since the beginning. It's not an easy CPU to use: it's undocumented and its name doesn't tell users much about what the CPU is supposed to bring. 'git log' doesn't help us either in knowing what was the original design of this CPU type.The closest we have is a comment from Alistair [1] where he recalls from memory that the 'any' CPU is supposed to behave like the newly added 'max' CPU. He also suggested that the 'any' CPU should be removed.
The default CPUs are rv32 and rv64, so removing the 'any' CPU will have impact only on users that might have a script that uses '-cpu any'. And those users are better off using the default CPUs or the new 'max' CPU.
We would love to just remove the code and be done with it, but one does not simply remove a feature in QEMU. We'll put the CPU in quarantine first, letting users know that we have the intent of removing it in the future.
[1] https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg02891.html
Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- docs/about/deprecated.rst | 12 ++++++++++++ target/riscv/cpu.c | 5 +++++ 2 files changed, 17 insertions(+)docs/about/deprecated.rst +0
-0
docs/about/deprecated.rst QEMU's ``vhost`` feature, which would eliminate the high latency costs under · line 380
Quoted 10 lines
docs/about/deprecated.rst +7
-0@@ -371,6 +371,18 @@ QEMU's ``vhost`` feature, which would eliminate the high latency costs under which the 9p ``proxy`` backend currently suffers. However as of to date nobody has indicated plans for such kind of reimplementation unfortunately. +RISC-V 'any' CPU type ``-cpu any`` (since 8.2) +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The 'any' CPU type was introduced back in 2018 and has been around since the +initial RISC-V QEMU port. Its usage has always been unclear: users don't know +what to expect from a CPU called 'any', and in fact the CPU does not do anything +special that aren't already done by the default CPUs rv32/rv64.
^ isn't
docs/about/deprecated.rst QEMU's ``vhost`` feature, which would eliminate the high latency costs under · line 382
Quoted 2 lines
docs/about/deprecated.rst +2
-0+ +After the introduction of the 'max' CPU type RISC-V now has a good coverage
^ ,
Quoted 26 lines
docs/about/deprecated.rst +3
-0+of generic CPUs: rv32 and rv64 as default CPUs and 'max' as a feature complete +CPU for both 32 and 64 bit builds. Users are then discouraged to use the 'any' +CPU type starting in 8.2. Block device options ''''''''''''''''''''target/riscv/cpu.c +5
-0@@ -1522,6 +1522,11 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)RISCVCPUClass *mcc =RISCV_CPU_GET_CLASS (dev);Error *local_err =NULL ; +if (object_dynamic_cast (OBJECT (dev),TYPE_RISCV_CPU_ANY ) !=NULL ) { +warn_report ("The 'any' CPU is deprecated and will be " +"removed in the future." ); + } +cpu_exec_realizefn (cs, &local_err);if (local_err !=NULL ) {error_propagate (errp, local_err);Signature · 1 line
2.41.0
Besides the text edits,
PATCH RESEND v8 13/20 target/riscv/cpu.c: use offset in isa_ext_is_enabled/update_enabled
In reply to Daniel Henrique BarbozaWe'll have future usage for a function where, given an offset of the struct RISCVCPUConfig, the flag is updated to a certain val.
Change all existing callers to use edata->ext_enable_offset instead of 'edata'.
--- target/riscv/cpu.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
target/riscv/cpu.c +9 -9
@@ -162,18 +162,17 @@ static const struct isa_ext_data isa_edata_arr[] = { ISA_EXT_DATA_ENTRY(xventanacondops, PRIV_VERSION_1_12_0, ext_XVentanaCondOps), }; -static bool isa_ext_is_enabled (RISCVCPU *cpu, -const struct isa_ext_data *edata) +static bool isa_ext_is_enabled (RISCVCPU *cpu,uint32_t ext_offset) { -bool *ext_enabled = (void *)&cpu->cfg + edata->ext_enable_offset; +bool *ext_enabled = (void *)&cpu->cfg + ext_offset;return *ext_enabled; } -static void isa_ext_update_enabled (RISCVCPU *cpu, -const struct isa_ext_data *edata,bool en) +static void isa_ext_update_enabled (RISCVCPU *cpu,uint32_t ext_offset, +bool en) { -bool *ext_enabled = (void *)&cpu->cfg + edata->ext_enable_offset; +bool *ext_enabled = (void *)&cpu->cfg + ext_offset; *ext_enabled = en; }
@@ -1045,9 +1044,10 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)/* Force disable extensions if priv spec version does not match */ for (i =0 ; i <ARRAY_SIZE (isa_edata_arr); i++) { -if (isa_ext_is_enabled (cpu, &isa_edata_arr[i]) && +if (isa_ext_is_enabled (cpu, isa_edata_arr[i].ext_enable_offset) && (env->priv_ver < isa_edata_arr[i].min_version)) { -isa_ext_update_enabled (cpu, &isa_edata_arr[i], false); +isa_ext_update_enabled (cpu, isa_edata_arr[i].ext_enable_offset, + false);#ifndef CONFIG_USER_ONLY warn_report("disabling %s extension for hart 0x" TARGET_FMT_lx " because privilege spec version does not match",
@@ -2337,7 +2337,7 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,int i;for (i =0 ; i <ARRAY_SIZE (isa_edata_arr); i++) { -if (isa_ext_is_enabled (cpu, &isa_edata_arr[i])) { +if (isa_ext_is_enabled (cpu, isa_edata_arr[i].ext_enable_offset)) { new =g_strconcat (old,"_" , isa_edata_arr[i].name,NULL );g_free (old); old = new;
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 13/20] target/riscv/cpu.c: use offset in isa_ext_is_enabled/update_enabled
Quoted 12 lines · Daniel Henrique Barboza
We'll have future usage for a function where, given an offset of the struct RISCVCPUConfig, the flag is updated to a certain val.
Change all existing callers to use
edata->ext_enable_offsetinstead of 'edata'.Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- target/riscv/cpu.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
PATCH RESEND v8 14/20 target/riscv: make CPUCFG() macro public
In reply to Daniel Henrique BarbozaThe RISC-V KVM driver uses a CPUCFG() macro that calculates the offset of a certain field in the struct RISCVCPUConfig. We're going to use this macro in target/riscv/cpu.c as well in the next patches. Make it public.
Rename it to CPU_CFG_OFFSET() for more clarity while we're at it.
--- target/riscv/cpu.c | 2 +- target/riscv/cpu.h | 2 ++ target/riscv/kvm.c | 8 +++----- 3 files changed, 6 insertions(+), 6 deletions(-)
target/riscv/cpu.c +1 -1
@@ -48,7 +48,7 @@ struct isa_ext_data { };#define ISA_EXT_DATA_ENTRY (_name, _min_ver, _prop) \ - {#_name, _min_ver, offsetof(struct RISCVCPUConfig, _prop)} + {#_name, _min_ver, CPU_CFG_OFFSET(_prop)}/* * From vector_helper.c
target/riscv/cpu.h +2 -0
@@ -62,6 +62,8 @@const char *riscv_get_misa_ext_name (uint32_t bit);const char *riscv_get_misa_ext_description (uint32_t bit); +#define CPU_CFG_OFFSET (_prop) offsetof(struct RISCVCPUConfig, _prop) +/* Privileged specification version */ enum {PRIV_VERSION_1_10_0 =0 ,
target/riscv/kvm.c +3 -5
@@ -200,10 +200,8 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs) } } -#define CPUCFG (_prop) offsetof(struct RISCVCPUConfig, _prop) -#define KVM_EXT_CFG (_name, _prop, _reg_id) \ - {.name = _name, .offset = CPUCFG(_prop), \ + {.name = _name, .offset = CPU_CFG_OFFSET(_prop), \ .kvm_reg_id = _reg_id}static KVMCPUConfig kvm_multi_ext_cfgs[] = {
@@ -280,13 +278,13 @@ static void kvm_cpu_set_multi_ext_cfg(Object *obj, Visitor *v,static KVMCPUConfig kvm_cbom_blocksize = { .name ="cbom_blocksize" , - .offset =CPUCFG (cbom_blocksize), + .offset =CPU_CFG_OFFSET (cbom_blocksize), .kvm_reg_id =KVM_REG_RISCV_CONFIG_REG (zicbom_block_size) };static KVMCPUConfig kvm_cboz_blocksize = { .name ="cboz_blocksize" , - .offset =CPUCFG (cboz_blocksize), + .offset =CPU_CFG_OFFSET (cboz_blocksize), .kvm_reg_id =KVM_REG_RISCV_CONFIG_REG (zicboz_block_size) };
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 14/20] target/riscv: make CPUCFG() macro public
Quoted 6 lines · Daniel Henrique Barboza
The RISC-V KVM driver uses a
CPUCFG()macro that calculates the offset of a certain field in the struct RISCVCPUConfig. We're going to use this macro intarget/riscv/cpu.cas well in the next patches. Make it public.Rename it to
CPU_CFG_OFFSET()for more clarity while we're at it.
Quoted 8 lines
Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- target/riscv/cpu.c | 2 +- target/riscv/cpu.h | 2 ++ target/riscv/kvm.c | 8 +++----- 3 files changed, 6 insertions(+), 6 deletions(-)
PATCH RESEND v8 15/20 target/riscv/cpu.c: introduce cpu_cfg_ext_auto_update()
In reply to Daniel Henrique BarbozaDuring realize() time we're activating a lot of extensions based on some criteria, e.g.:
if (cpu->cfg.ext_zk) {
cpu->cfg.ext_zkn = true;
cpu->cfg.ext_zkr = true;
cpu->cfg.ext_zkt = true;
}This practice resulted in at least one case where we ended up enabling something we shouldn't: RVC enabling zca/zcd/zcf when using a CPU that has priv_spec older than 1.12.0.
We're also not considering user choice. There's no way of doing it now but this is about to change in the next few patches.
cpu_cfg_ext_auto_update() will check for priv version mismatches before enabling extensions. If we have a mismatch between the current priv version and the extension we want to enable, do not enable it. In the near future, this same function will also consider user choice when deciding if we're going to enable/disable an extension or not.
For now let's use it to handle zca/zcd/zcf enablement if RVC is enabled.
--- target/riscv/cpu.c | 44 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 3 deletions(-)
target/riscv/cpu.c +41 -3
@@ -177,6 +177,44 @@ static void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset, *ext_enabled = en; } +static int cpu_cfg_ext_get_min_version (uint32_t ext_offset) +{ +int i; + +for (i =0 ; i <ARRAY_SIZE (isa_edata_arr); i++) { +if (isa_edata_arr[i].ext_enable_offset != ext_offset) { +continue ; + } + +return isa_edata_arr[i].min_version; + } + +/* Default to oldest priv spec if no match found */ +return PRIV_VERSION_1_10_0 ; +} + +static void cpu_cfg_ext_auto_update (RISCVCPU *cpu,uint32_t ext_offset, +bool value) +{ +CPURISCVState *env = &cpu->env; +bool prev_val =isa_ext_is_enabled (cpu, ext_offset); +int min_version; + +if (prev_val == value) { +return ; + } + +if (value && env->priv_ver !=PRIV_VERSION_LATEST ) { +/* Do not enable it if priv_ver is older than min_version */ + min_version =cpu_cfg_ext_get_min_version (ext_offset); +if (env->priv_ver < min_version) { +return ; + } + } + +isa_ext_update_enabled (cpu, ext_offset, value); +} +const char *const riscv_int_regnames[] = {"x0/zero" ,"x1/ra" ,"x2/sp" ,"x3/gp" ,"x4/tp" ,"x5/t0" ,"x6/t1" ,"x7/t2" ,"x8/s0" ,"x9/s1" ,"x10/a0" ,"x11/a1" ,"x12/a2" ,"x13/a3" ,
@@ -1268,12 +1306,12 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)/* zca, zcd and zcf has a PRIV 1.12.0 restriction */ if (riscv_has_ext (env,RVC ) && env->priv_ver >=PRIV_VERSION_1_12_0 ) { - cpu->cfg.ext_zca = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zca), true);if (riscv_has_ext (env,RVF ) && env->misa_mxl_max ==MXL_RV32 ) { - cpu->cfg.ext_zcf = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zcf), true); }if (riscv_has_ext (env,RVD )) { - cpu->cfg.ext_zcd = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zcd), true); } }
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 15/20] target/riscv/cpu.c: introduce cpu_cfg_ext_auto_update()
Quoted 42 lines · Daniel Henrique Barboza
During
realize()time we're activating a lot of extensions based on some criteria, e.g.:if (cpu->cfg.ext_zk) { cpu->cfg.ext_zkn = true; cpu->cfg.ext_zkr = true; cpu->cfg.ext_zkt = true; }This practice resulted in at least one case where we ended up enabling something we shouldn't: RVC enabling zca/zcd/zcf when using a CPU that has
priv_specolder than 1.12.0.We're also not considering user choice. There's no way of doing it now but this is about to change in the next few patches.
cpu_cfg_ext_auto_update()will check for priv version mismatches before enabling extensions. If we have a mismatch between the current priv version and the extension we want to enable, do not enable it. In the near future, this same function will also consider user choice when deciding if we're going to enable/disable an extension or not.For now let's use it to handle zca/zcd/zcf enablement if RVC is enabled.
Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- target/riscv/cpu.c | 44 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 3 deletions(-)target/riscv/cpu.c +4
-0@@ -177,6 +177,44 @@ static void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset, *ext_enabled = en; } +static int cpu_cfg_ext_get_min_version (uint32_t ext_offset) +{ +int i; +
target/riscv/cpu.c static void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset, · line 193
Quoted 10 lines
target/riscv/cpu.c +10
-0+for (i =0 ; i <ARRAY_SIZE (isa_edata_arr); i++) { +if (isa_edata_arr[i].ext_enable_offset != ext_offset) { +continue ; + } + +return isa_edata_arr[i].min_version; + } + +/* Default to oldest priv spec if no match found */ +return PRIV_VERSION_1_10_0 ;
This seems backwards to me. If we don't know for how long an extension has been ratified, then it seems like the default should be "Well, I guess it's new". Or, we could assert here to ensure we don't have any extensions having their minimum version checked which are missing version information. An assert also makes sense for the case that an invalid ext_offset was passed to this function.
Quoted 47 lines
target/riscv/cpu.c +27
-3+} + +static void cpu_cfg_ext_auto_update (RISCVCPU *cpu,uint32_t ext_offset, +bool value) +{ +CPURISCVState *env = &cpu->env; +bool prev_val =isa_ext_is_enabled (cpu, ext_offset); +int min_version; + +if (prev_val == value) { +return ; + } + +if (value && env->priv_ver !=PRIV_VERSION_LATEST ) { +/* Do not enable it if priv_ver is older than min_version */ + min_version =cpu_cfg_ext_get_min_version (ext_offset); +if (env->priv_ver < min_version) { +return ; + } + } + +isa_ext_update_enabled (cpu, ext_offset, value); +} +const char *const riscv_int_regnames[] = {"x0/zero" ,"x1/ra" ,"x2/sp" ,"x3/gp" ,"x4/tp" ,"x5/t0" ,"x6/t1" ,"x7/t2" ,"x8/s0" ,"x9/s1" ,"x10/a0" ,"x11/a1" ,"x12/a2" ,"x13/a3" ,@@ -1268,12 +1306,12 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)/* zca, zcd and zcf has a PRIV 1.12.0 restriction */ if (riscv_has_ext (env,RVC ) && env->priv_ver >=PRIV_VERSION_1_12_0 ) { - cpu->cfg.ext_zca = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zca), true);if (riscv_has_ext (env,RVF ) && env->misa_mxl_max ==MXL_RV32 ) { - cpu->cfg.ext_zcf = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zcf), true); }if (riscv_has_ext (env,RVD )) { - cpu->cfg.ext_zcd = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zcd), true); } }Signature · 1 line
2.41.0
Thanks,
drew
Re: [PATCH RESEND v8 15/20] target/riscv/cpu.c: introduce cpu_cfg_ext_auto_update()
Quoted 52 lines · Andrew Jones
Quoted 51 lines · Daniel Henrique Barboza
During
realize()time we're activating a lot of extensions based on some criteria, e.g.:if (cpu->cfg.ext_zk) { cpu->cfg.ext_zkn = true; cpu->cfg.ext_zkr = true; cpu->cfg.ext_zkt = true; }This practice resulted in at least one case where we ended up enabling something we shouldn't: RVC enabling zca/zcd/zcf when using a CPU that has
priv_specolder than 1.12.0.We're also not considering user choice. There's no way of doing it now but this is about to change in the next few patches.
cpu_cfg_ext_auto_update()will check for priv version mismatches before enabling extensions. If we have a mismatch between the current priv version and the extension we want to enable, do not enable it. In the near future, this same function will also consider user choice when deciding if we're going to enable/disable an extension or not.For now let's use it to handle zca/zcd/zcf enablement if RVC is enabled.
Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- target/riscv/cpu.c | 44 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 3 deletions(-)target/riscv/cpu.c +13
-0@@ -177,6 +177,44 @@ static void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset, *ext_enabled = en; } +static int cpu_cfg_ext_get_min_version (uint32_t ext_offset) +{ +int i; + +for (i =0 ; i <ARRAY_SIZE (isa_edata_arr); i++) { +if (isa_edata_arr[i].ext_enable_offset != ext_offset) { +continue ; + } + +return isa_edata_arr[i].min_version; + } + +/* Default to oldest priv spec if no match found */
Quoted 8 lines
target/riscv/cpu.c static void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset, · line 193
Quoted 1 line
target/riscv/cpu.c +1
-0+return PRIV_VERSION_1_10_0 ;This seems backwards to me. If we don't know for how long an extension has been ratified, then it seems like the default should be "Well, I guess it's new". Or, we could assert here to ensure we don't have any extensions having their minimum version checked which are missing version information. An assert also makes sense for the case that an invalid
ext_offsetwas passed to this function.
IIRC I decided to return a default instead of asserting due to some extensions not having priv_spec versions, but that was in an older codebase (a few months ago).
I'll see if I can change this to assert out instead of setting a default value.
Thanks,
Daniel
Quoted 51 lines
Quoted 48 lines
target/riscv/cpu.c +27
-3+} + +static void cpu_cfg_ext_auto_update (RISCVCPU *cpu,uint32_t ext_offset, +bool value) +{ +CPURISCVState *env = &cpu->env; +bool prev_val =isa_ext_is_enabled (cpu, ext_offset); +int min_version; + +if (prev_val == value) { +return ; + } + +if (value && env->priv_ver !=PRIV_VERSION_LATEST ) { +/* Do not enable it if priv_ver is older than min_version */ + min_version =cpu_cfg_ext_get_min_version (ext_offset); +if (env->priv_ver < min_version) { +return ; + } + } + +isa_ext_update_enabled (cpu, ext_offset, value); +} +const char *const riscv_int_regnames[] = {"x0/zero" ,"x1/ra" ,"x2/sp" ,"x3/gp" ,"x4/tp" ,"x5/t0" ,"x6/t1" ,"x7/t2" ,"x8/s0" ,"x9/s1" ,"x10/a0" ,"x11/a1" ,"x12/a2" ,"x13/a3" ,@@ -1268,12 +1306,12 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)/* zca, zcd and zcf has a PRIV 1.12.0 restriction */ if (riscv_has_ext (env,RVC ) && env->priv_ver >=PRIV_VERSION_1_12_0 ) { - cpu->cfg.ext_zca = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zca), true);if (riscv_has_ext (env,RVF ) && env->misa_mxl_max ==MXL_RV32 ) { - cpu->cfg.ext_zcf = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zcf), true); }if (riscv_has_ext (env,RVD )) { - cpu->cfg.ext_zcd = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zcd), true); } }Signature · 1 line
2.41.0Thanks,
drew
PATCH RESEND v8 16/20 target/riscv/cpu.c: use cpu_cfg_ext_auto_update() during realize()
In reply to Daniel Henrique BarbozaLet's change the other instances in realize() where we're enabling an extension based on a certain criteria (e.g. it's a dependency of another extension).
We're leaving icsr and ifencei being enabled during RVG for later - we'll want to error out in that case. Every other extension enablement during realize is now done via cpu_cfg_ext_auto_update().
The end goal is that only cpu init() functions will handle extension flags directly via "cpu->cfg.ext_N = true|false".
--- target/riscv/cpu.c | 50 +++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 25 deletions(-)
target/riscv/cpu.c +25 -25
@@ -1194,7 +1194,7 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) }if (cpu->cfg.ext_zfh) { - cpu->cfg.ext_zfhmin = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zfhmin), true); }if (cpu->cfg.ext_zfhmin && !riscv_has_ext (env,RVF )) {
@@ -1220,17 +1220,17 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) }/* The V vector extension depends on the Zve64d extension */ - cpu->cfg.ext_zve64d = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zve64d), true); }/* The Zve64d extension depends on the Zve64f extension */ if (cpu->cfg.ext_zve64d) { - cpu->cfg.ext_zve64f = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zve64f), true); }/* The Zve64f extension depends on the Zve32f extension */ if (cpu->cfg.ext_zve64f) { - cpu->cfg.ext_zve32f = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zve32f), true); }if (cpu->cfg.ext_zve64d && !riscv_has_ext (env,RVD )) {
@@ -1244,7 +1244,7 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) }if (cpu->cfg.ext_zvfh) { - cpu->cfg.ext_zvfhmin = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zvfhmin), true); }if (cpu->cfg.ext_zvfhmin && !cpu->cfg.ext_zve32f) {
@@ -1274,7 +1274,7 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)/* Set the ISA extensions, checks should have happened above */ if (cpu->cfg.ext_zhinx) { - cpu->cfg.ext_zhinxmin = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zca), true); }if ((cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinxmin) && !cpu->cfg.ext_zfinx) {
@@ -1295,12 +1295,12 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) }if (cpu->cfg.ext_zce) { - cpu->cfg.ext_zca = true; - cpu->cfg.ext_zcb = true; - cpu->cfg.ext_zcmp = true; - cpu->cfg.ext_zcmt = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zca), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zcb), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zcmp), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zcmt), true);if (riscv_has_ext (env,RVF ) && env->misa_mxl_max ==MXL_RV32 ) { - cpu->cfg.ext_zcf = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zcf), true); } }
@@ -1368,26 +1368,26 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) }if (cpu->cfg.ext_zk) { - cpu->cfg.ext_zkn = true; - cpu->cfg.ext_zkr = true; - cpu->cfg.ext_zkt = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zkn), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zkr), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zkt), true); }if (cpu->cfg.ext_zkn) { - cpu->cfg.ext_zbkb = true; - cpu->cfg.ext_zbkc = true; - cpu->cfg.ext_zbkx = true; - cpu->cfg.ext_zkne = true; - cpu->cfg.ext_zknd = true; - cpu->cfg.ext_zknh = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zbkb), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zbkc), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zbkx), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zkne), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zknd), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zknh), true); }if (cpu->cfg.ext_zks) { - cpu->cfg.ext_zbkb = true; - cpu->cfg.ext_zbkc = true; - cpu->cfg.ext_zbkx = true; - cpu->cfg.ext_zksed = true; - cpu->cfg.ext_zksh = true; +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zbkb), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zbkc), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zbkx), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zksed), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_zksh), true); }/*
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 16/20] target/riscv/cpu.c: use cpu_cfg_ext_auto_update() during realize()
Quoted 8 lines · Daniel Henrique Barboza
Let's change the other instances in
realize()where we're enabling an extension based on a certain criteria (e.g. it's a dependency of another extension).We're leaving icsr and ifencei being enabled during RVG for later - we'll want to error out in that case. Every other extension enablement during realize is now done via
cpu_cfg_ext_auto_update().
Quoted 9 lines
The end goal is that only cpu
init()functions will handle extension flags directly via "cpu->cfg.ext_N= true|false".Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- target/riscv/cpu.c | 50 +++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 25 deletions(-)
PATCH RESEND v8 17/20 target/riscv/cpu.c: introduce RISCVCPUMultiExtConfig
In reply to Daniel Henrique BarbozaIf we want to make better decisions when auto-enabling extensions during realize() we need a way to tell if an user set an extension manually. The RISC-V KVM driver has its own solution via a KVMCPUConfig struct that has an 'user_set' flag that is set during the Property set() callback. The set() callback also does init() time validations based on the current KVM driver capabilities.
For TCG we would want a 'user_set' mechanic too, but we would look ad-hoc via cpu_cfg_ext_auto_update() if a certain extension was user set or not. If we copy what was made in the KVM side we would look for 'user_set' for one into 60+ extension structs spreaded in 3 arrays (riscv_cpu_extensions, riscv_cpu_experimental_exts, riscv_cpu_vendor_exts).
We'll still need an extension struct but we won't be using the 'user_set' flag:
- 'RISCVCPUMultiExtConfig' will be our specialized structure, similar to what we're already doing with the MISA extensions in 'RISCVCPUMisaExtConfig'. DEFINE_PROP_BOOL() for all 3 extensions arrays were replaced by MULTI_EXT_CFG_BOOL(), a macro that will init our specialized struct;
- the 'multi_ext_user_opts' hash will be used to store the offset of each extension that the user set via the set() callback, cpu_set_multi_ext_cfg(). For now we're just initializing and populating it - next patch will use it to determine if a certain extension was user set;
- cpu_add_multi_ext_prop() is a new helper that will replace the qdev_property_add_static() calls that our macros are doing to populate user properties. The macro was renamed to ADD_CPU_MULTIEXT_PROPS_ARRAY() for clarity. Note that the non-extension properties in riscv_cpu_options[] still need to be declared via qdev().
--- target/riscv/cpu.c | 257 ++++++++++++++++++++++++++++----------------- 1 file changed, 158 insertions(+), 99 deletions(-)
target/riscv/cpu.c +158 -99
@@ -162,6 +162,9 @@ static const struct isa_ext_data isa_edata_arr[] = { ISA_EXT_DATA_ENTRY(xventanacondops, PRIV_VERSION_1_12_0, ext_XVentanaCondOps), }; +/* Hash that stores user set extensions */ +static GHashTable *multi_ext_user_opts; +static bool isa_ext_is_enabled (RISCVCPU *cpu,uint32_t ext_offset) {bool *ext_enabled = (void *)&cpu->cfg + ext_offset;
@@ -1713,6 +1716,8 @@ static void riscv_cpu_init(Object *obj)qdev_init_gpio_in (DEVICE (cpu), riscv_cpu_set_irq,IRQ_LOCAL_MAX +IRQ_LOCAL_GUEST_MAX );#endif /* CONFIG_USER_ONLY */ + + multi_ext_user_opts =g_hash_table_new (NULL , g_direct_equal); } typedef struct RISCVCPUMisaExtConfig {
@@ -1864,109 +1869,118 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj) } } -static Property riscv_cpu_extensions[] = { +typedef struct RISCVCPUMultiExtConfig { +const char *name; +uint32_t offset; +bool enabled; +}RISCVCPUMultiExtConfig ; + +#define MULTI_EXT_CFG_BOOL (_name, _prop, _defval) \ + {.name = _name, .offset = CPU_CFG_OFFSET(_prop), \ + .enabled = _defval} + +static RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {/* Defaults for standard extensions */ -DEFINE_PROP_BOOL ("sscofpmf" ,RISCVCPU , cfg.ext_sscofpmf, false), -DEFINE_PROP_BOOL ("Zifencei" ,RISCVCPU , cfg.ext_ifencei, true), -DEFINE_PROP_BOOL ("Zicsr" ,RISCVCPU , cfg.ext_icsr, true), -DEFINE_PROP_BOOL ("Zihintntl" ,RISCVCPU , cfg.ext_zihintntl, true), -DEFINE_PROP_BOOL ("Zihintpause" ,RISCVCPU , cfg.ext_zihintpause, true), -DEFINE_PROP_BOOL ("Zawrs" ,RISCVCPU , cfg.ext_zawrs, true), -DEFINE_PROP_BOOL ("Zfa" ,RISCVCPU , cfg.ext_zfa, true), -DEFINE_PROP_BOOL ("Zfh" ,RISCVCPU , cfg.ext_zfh, false), -DEFINE_PROP_BOOL ("Zfhmin" ,RISCVCPU , cfg.ext_zfhmin, false), -DEFINE_PROP_BOOL ("Zve32f" ,RISCVCPU , cfg.ext_zve32f, false), -DEFINE_PROP_BOOL ("Zve64f" ,RISCVCPU , cfg.ext_zve64f, false), -DEFINE_PROP_BOOL ("Zve64d" ,RISCVCPU , cfg.ext_zve64d, false), -DEFINE_PROP_BOOL ("sstc" ,RISCVCPU , cfg.ext_sstc, true), - -DEFINE_PROP_BOOL ("smstateen" ,RISCVCPU , cfg.ext_smstateen, false), -DEFINE_PROP_BOOL ("svadu" ,RISCVCPU , cfg.ext_svadu, true), -DEFINE_PROP_BOOL ("svinval" ,RISCVCPU , cfg.ext_svinval, false), -DEFINE_PROP_BOOL ("svnapot" ,RISCVCPU , cfg.ext_svnapot, false), -DEFINE_PROP_BOOL ("svpbmt" ,RISCVCPU , cfg.ext_svpbmt, false), - -DEFINE_PROP_BOOL ("zba" ,RISCVCPU , cfg.ext_zba, true), -DEFINE_PROP_BOOL ("zbb" ,RISCVCPU , cfg.ext_zbb, true), -DEFINE_PROP_BOOL ("zbc" ,RISCVCPU , cfg.ext_zbc, true), -DEFINE_PROP_BOOL ("zbkb" ,RISCVCPU , cfg.ext_zbkb, false), -DEFINE_PROP_BOOL ("zbkc" ,RISCVCPU , cfg.ext_zbkc, false), -DEFINE_PROP_BOOL ("zbkx" ,RISCVCPU , cfg.ext_zbkx, false), -DEFINE_PROP_BOOL ("zbs" ,RISCVCPU , cfg.ext_zbs, true), -DEFINE_PROP_BOOL ("zk" ,RISCVCPU , cfg.ext_zk, false), -DEFINE_PROP_BOOL ("zkn" ,RISCVCPU , cfg.ext_zkn, false), -DEFINE_PROP_BOOL ("zknd" ,RISCVCPU , cfg.ext_zknd, false), -DEFINE_PROP_BOOL ("zkne" ,RISCVCPU , cfg.ext_zkne, false), -DEFINE_PROP_BOOL ("zknh" ,RISCVCPU , cfg.ext_zknh, false), -DEFINE_PROP_BOOL ("zkr" ,RISCVCPU , cfg.ext_zkr, false), -DEFINE_PROP_BOOL ("zks" ,RISCVCPU , cfg.ext_zks, false), -DEFINE_PROP_BOOL ("zksed" ,RISCVCPU , cfg.ext_zksed, false), -DEFINE_PROP_BOOL ("zksh" ,RISCVCPU , cfg.ext_zksh, false), -DEFINE_PROP_BOOL ("zkt" ,RISCVCPU , cfg.ext_zkt, false), - -DEFINE_PROP_BOOL ("zdinx" ,RISCVCPU , cfg.ext_zdinx, false), -DEFINE_PROP_BOOL ("zfinx" ,RISCVCPU , cfg.ext_zfinx, false), -DEFINE_PROP_BOOL ("zhinx" ,RISCVCPU , cfg.ext_zhinx, false), -DEFINE_PROP_BOOL ("zhinxmin" ,RISCVCPU , cfg.ext_zhinxmin, false), - -DEFINE_PROP_BOOL ("zicbom" ,RISCVCPU , cfg.ext_icbom, true), -DEFINE_PROP_BOOL ("zicboz" ,RISCVCPU , cfg.ext_icboz, true), - -DEFINE_PROP_BOOL ("zmmul" ,RISCVCPU , cfg.ext_zmmul, false), - -DEFINE_PROP_BOOL ("zca" ,RISCVCPU , cfg.ext_zca, false), -DEFINE_PROP_BOOL ("zcb" ,RISCVCPU , cfg.ext_zcb, false), -DEFINE_PROP_BOOL ("zcd" ,RISCVCPU , cfg.ext_zcd, false), -DEFINE_PROP_BOOL ("zce" ,RISCVCPU , cfg.ext_zce, false), -DEFINE_PROP_BOOL ("zcf" ,RISCVCPU , cfg.ext_zcf, false), -DEFINE_PROP_BOOL ("zcmp" ,RISCVCPU , cfg.ext_zcmp, false), -DEFINE_PROP_BOOL ("zcmt" ,RISCVCPU , cfg.ext_zcmt, false), +MULTI_EXT_CFG_BOOL ("sscofpmf" , ext_sscofpmf, false), +MULTI_EXT_CFG_BOOL ("Zifencei" , ext_ifencei, true), +MULTI_EXT_CFG_BOOL ("Zicsr" , ext_icsr, true), +MULTI_EXT_CFG_BOOL ("Zihintpause" , ext_zihintpause, true), +MULTI_EXT_CFG_BOOL ("Zawrs" , ext_zawrs, true), +MULTI_EXT_CFG_BOOL ("Zfa" , ext_zfa, true), +MULTI_EXT_CFG_BOOL ("Zfh" , ext_zfh, false), +MULTI_EXT_CFG_BOOL ("Zfhmin" , ext_zfhmin, false), +MULTI_EXT_CFG_BOOL ("Zve32f" , ext_zve32f, false), +MULTI_EXT_CFG_BOOL ("Zve64f" , ext_zve64f, false), +MULTI_EXT_CFG_BOOL ("Zve64d" , ext_zve64d, false), +MULTI_EXT_CFG_BOOL ("sstc" , ext_sstc, true), + +MULTI_EXT_CFG_BOOL ("smstateen" , ext_smstateen, false), +MULTI_EXT_CFG_BOOL ("svadu" , ext_svadu, true), +MULTI_EXT_CFG_BOOL ("svinval" , ext_svinval, false), +MULTI_EXT_CFG_BOOL ("svnapot" , ext_svnapot, false), +MULTI_EXT_CFG_BOOL ("svpbmt" , ext_svpbmt, false), + +MULTI_EXT_CFG_BOOL ("zba" , ext_zba, true), +MULTI_EXT_CFG_BOOL ("zbb" , ext_zbb, true), +MULTI_EXT_CFG_BOOL ("zbc" , ext_zbc, true), +MULTI_EXT_CFG_BOOL ("zbkb" , ext_zbkb, false), +MULTI_EXT_CFG_BOOL ("zbkc" , ext_zbkc, false), +MULTI_EXT_CFG_BOOL ("zbkx" , ext_zbkx, false), +MULTI_EXT_CFG_BOOL ("zbs" , ext_zbs, true), +MULTI_EXT_CFG_BOOL ("zk" , ext_zk, false), +MULTI_EXT_CFG_BOOL ("zkn" , ext_zkn, false), +MULTI_EXT_CFG_BOOL ("zknd" , ext_zknd, false), +MULTI_EXT_CFG_BOOL ("zkne" , ext_zkne, false), +MULTI_EXT_CFG_BOOL ("zknh" , ext_zknh, false), +MULTI_EXT_CFG_BOOL ("zkr" , ext_zkr, false), +MULTI_EXT_CFG_BOOL ("zks" , ext_zks, false), +MULTI_EXT_CFG_BOOL ("zksed" , ext_zksed, false), +MULTI_EXT_CFG_BOOL ("zksh" , ext_zksh, false), +MULTI_EXT_CFG_BOOL ("zkt" , ext_zkt, false), + +MULTI_EXT_CFG_BOOL ("zdinx" , ext_zdinx, false), +MULTI_EXT_CFG_BOOL ("zfinx" , ext_zfinx, false), +MULTI_EXT_CFG_BOOL ("zhinx" , ext_zhinx, false), +MULTI_EXT_CFG_BOOL ("zhinxmin" , ext_zhinxmin, false), + +MULTI_EXT_CFG_BOOL ("zicbom" , ext_icbom, true), +MULTI_EXT_CFG_BOOL ("zicboz" , ext_icboz, true), + +MULTI_EXT_CFG_BOOL ("zmmul" , ext_zmmul, false), + +MULTI_EXT_CFG_BOOL ("zca" , ext_zca, false), +MULTI_EXT_CFG_BOOL ("zcb" , ext_zcb, false), +MULTI_EXT_CFG_BOOL ("zcd" , ext_zcd, false), +MULTI_EXT_CFG_BOOL ("zce" , ext_zce, false), +MULTI_EXT_CFG_BOOL ("zcf" , ext_zcf, false), +MULTI_EXT_CFG_BOOL ("zcmp" , ext_zcmp, false), +MULTI_EXT_CFG_BOOL ("zcmt" , ext_zcmt, false),DEFINE_PROP_END_OF_LIST (), }; -static Property riscv_cpu_vendor_exts[] = { -DEFINE_PROP_BOOL ("xtheadba" ,RISCVCPU , cfg.ext_xtheadba, false), -DEFINE_PROP_BOOL ("xtheadbb" ,RISCVCPU , cfg.ext_xtheadbb, false), -DEFINE_PROP_BOOL ("xtheadbs" ,RISCVCPU , cfg.ext_xtheadbs, false), -DEFINE_PROP_BOOL ("xtheadcmo" ,RISCVCPU , cfg.ext_xtheadcmo, false), -DEFINE_PROP_BOOL ("xtheadcondmov" ,RISCVCPU , cfg.ext_xtheadcondmov, false), -DEFINE_PROP_BOOL ("xtheadfmemidx" ,RISCVCPU , cfg.ext_xtheadfmemidx, false), -DEFINE_PROP_BOOL ("xtheadfmv" ,RISCVCPU , cfg.ext_xtheadfmv, false), -DEFINE_PROP_BOOL ("xtheadmac" ,RISCVCPU , cfg.ext_xtheadmac, false), -DEFINE_PROP_BOOL ("xtheadmemidx" ,RISCVCPU , cfg.ext_xtheadmemidx, false), -DEFINE_PROP_BOOL ("xtheadmempair" ,RISCVCPU , cfg.ext_xtheadmempair, false), -DEFINE_PROP_BOOL ("xtheadsync" ,RISCVCPU , cfg.ext_xtheadsync, false), -DEFINE_PROP_BOOL ("xventanacondops" ,RISCVCPU , cfg.ext_XVentanaCondOps, false), +static RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[] = { +MULTI_EXT_CFG_BOOL ("xtheadba" , ext_xtheadba, false), +MULTI_EXT_CFG_BOOL ("xtheadbb" , ext_xtheadbb, false), +MULTI_EXT_CFG_BOOL ("xtheadbs" , ext_xtheadbs, false), +MULTI_EXT_CFG_BOOL ("xtheadcmo" , ext_xtheadcmo, false), +MULTI_EXT_CFG_BOOL ("xtheadcondmov" , ext_xtheadcondmov, false), +MULTI_EXT_CFG_BOOL ("xtheadfmemidx" , ext_xtheadfmemidx, false), +MULTI_EXT_CFG_BOOL ("xtheadfmv" , ext_xtheadfmv, false), +MULTI_EXT_CFG_BOOL ("xtheadmac" , ext_xtheadmac, false), +MULTI_EXT_CFG_BOOL ("xtheadmemidx" , ext_xtheadmemidx, false), +MULTI_EXT_CFG_BOOL ("xtheadmempair" , ext_xtheadmempair, false), +MULTI_EXT_CFG_BOOL ("xtheadsync" , ext_xtheadsync, false), +MULTI_EXT_CFG_BOOL ("xventanacondops" , ext_XVentanaCondOps, false),DEFINE_PROP_END_OF_LIST (), };/* These are experimental so mark with 'x-' */ -static Property riscv_cpu_experimental_exts[] = { -DEFINE_PROP_BOOL ("x-zicond" ,RISCVCPU , cfg.ext_zicond, false), +static RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = { +MULTI_EXT_CFG_BOOL ("x-zicond" , ext_zicond, false),/* ePMP 0.9.3 */ -DEFINE_PROP_BOOL ("x-epmp" ,RISCVCPU , cfg.epmp, false), -DEFINE_PROP_BOOL ("x-smaia" ,RISCVCPU , cfg.ext_smaia, false), -DEFINE_PROP_BOOL ("x-ssaia" ,RISCVCPU , cfg.ext_ssaia, false), +MULTI_EXT_CFG_BOOL ("x-epmp" , epmp, false), +MULTI_EXT_CFG_BOOL ("x-smaia" , ext_smaia, false), +MULTI_EXT_CFG_BOOL ("x-ssaia" , ext_ssaia, false), -DEFINE_PROP_BOOL ("x-zvfh" ,RISCVCPU , cfg.ext_zvfh, false), -DEFINE_PROP_BOOL ("x-zvfhmin" ,RISCVCPU , cfg.ext_zvfhmin, false), +MULTI_EXT_CFG_BOOL ("x-zvfh" , ext_zvfh, false), +MULTI_EXT_CFG_BOOL ("x-zvfhmin" , ext_zvfhmin, false), -DEFINE_PROP_BOOL ("x-zfbfmin" ,RISCVCPU , cfg.ext_zfbfmin, false), -DEFINE_PROP_BOOL ("x-zvfbfmin" ,RISCVCPU , cfg.ext_zvfbfmin, false), -DEFINE_PROP_BOOL ("x-zvfbfwma" ,RISCVCPU , cfg.ext_zvfbfwma, false), +MULTI_EXT_CFG_BOOL ("x-zfbfmin" , ext_zfbfmin, false), +MULTI_EXT_CFG_BOOL ("x-zvfbfmin" , ext_zvfbfmin, false), +MULTI_EXT_CFG_BOOL ("x-zvfbfwma" , ext_zvfbfwma, false),/* Vector cryptography extensions */ -DEFINE_PROP_BOOL ("x-zvbb" ,RISCVCPU , cfg.ext_zvbb, false), -DEFINE_PROP_BOOL ("x-zvbc" ,RISCVCPU , cfg.ext_zvbc, false), -DEFINE_PROP_BOOL ("x-zvkg" ,RISCVCPU , cfg.ext_zvkg, false), -DEFINE_PROP_BOOL ("x-zvkned" ,RISCVCPU , cfg.ext_zvkned, false), -DEFINE_PROP_BOOL ("x-zvknha" ,RISCVCPU , cfg.ext_zvknha, false), -DEFINE_PROP_BOOL ("x-zvknhb" ,RISCVCPU , cfg.ext_zvknhb, false), -DEFINE_PROP_BOOL ("x-zvksed" ,RISCVCPU , cfg.ext_zvksed, false), -DEFINE_PROP_BOOL ("x-zvksh" ,RISCVCPU , cfg.ext_zvksh, false), +MULTI_EXT_CFG_BOOL ("x-zvbb" , ext_zvbb, false), +MULTI_EXT_CFG_BOOL ("x-zvbc" , ext_zvbc, false), +MULTI_EXT_CFG_BOOL ("x-zvkg" , ext_zvkg, false), +MULTI_EXT_CFG_BOOL ("x-zvkned" , ext_zvkned, false), +MULTI_EXT_CFG_BOOL ("x-zvknha" , ext_zvknha, false), +MULTI_EXT_CFG_BOOL ("x-zvknhb" , ext_zvknhb, false), +MULTI_EXT_CFG_BOOL ("x-zvksed" , ext_zvksed, false), +MULTI_EXT_CFG_BOOL ("x-zvksh" , ext_zvksh, false),DEFINE_PROP_END_OF_LIST (), };
@@ -1989,6 +2003,49 @@ static Property riscv_cpu_options[] = { DEFINE_PROP_END_OF_LIST(), }; +static void cpu_set_multi_ext_cfg (Object *obj,Visitor *v,const char *name, +void *opaque,Error **errp) +{ +const RISCVCPUMultiExtConfig *multi_ext_cfg = opaque; +bool value; + +if (!visit_type_bool (v, name, &value, errp)) { +return ; + } + +isa_ext_update_enabled (RISCV_CPU (obj), multi_ext_cfg->offset, value); + +g_hash_table_insert (multi_ext_user_opts, +GUINT_TO_POINTER (multi_ext_cfg->offset), + (gpointer )value); +} + +static void cpu_get_multi_ext_cfg (Object *obj,Visitor *v,const char *name, +void *opaque,Error **errp) +{ +const RISCVCPUMultiExtConfig *multi_ext_cfg = opaque; +bool value =isa_ext_is_enabled (RISCV_CPU (obj), multi_ext_cfg->offset); + +visit_type_bool (v, name, &value, errp); +} + +static void cpu_add_multi_ext_prop (Object *cpu_obj, +RISCVCPUMultiExtConfig *multi_cfg) +{ +object_property_add (cpu_obj, multi_cfg->name,"bool" , + cpu_get_multi_ext_cfg, + cpu_set_multi_ext_cfg, +NULL , (void *)multi_cfg); + +/* +* Set def val directly instead of using +* object_property_set_bool() to save the set() +* callback hash for user inputs. +*/ +isa_ext_update_enabled (RISCV_CPU (cpu_obj), multi_cfg->offset, + multi_cfg->enabled); +} +#ifndef CONFIG_USER_ONLY static void cpu_set_cfg_unavailable(Object *obj, Visitor *v, const char *name,
@@ -2008,10 +2065,11 @@ static void cpu_set_cfg_unavailable(Object *obj, Visitor *v, }#endif -static void riscv_cpu_add_qdev_prop_array (DeviceState *dev,Property *array) +static void riscv_cpu_add_multiext_prop_array (Object *obj, +RISCVCPUMultiExtConfig *array) { -for (Property *prop = array; prop && prop->name; prop++) { -qdev_property_add_static (dev, prop); +for (RISCVCPUMultiExtConfig *prop = array; prop && prop->name; prop++) { +cpu_add_multi_ext_prop (obj, prop); } }
@@ -2034,9 +2092,9 @@ static void riscv_cpu_add_kvm_unavail_prop(Object *obj, const char *prop_name) }static void riscv_cpu_add_kvm_unavail_prop_array (Object *obj, -Property *array) +RISCVCPUMultiExtConfig *array) { -for (Property *prop = array; prop && prop->name; prop++) { +for (RISCVCPUMultiExtConfig *prop = array; prop && prop->name; prop++) {riscv_cpu_add_kvm_unavail_prop (obj, prop->name); } }
@@ -2071,8 +2129,6 @@ static void riscv_cpu_add_kvm_properties(Object *obj)*/ static void riscv_cpu_add_user_properties (Object *obj) { -DeviceState *dev =DEVICE (obj); -#ifndef CONFIG_USER_ONLY riscv_add_satp_mode_properties (obj);
@@ -2084,10 +2140,13 @@ static void riscv_cpu_add_user_properties(Object *obj)riscv_cpu_add_misa_properties (obj); -riscv_cpu_add_qdev_prop_array (dev, riscv_cpu_extensions); -riscv_cpu_add_qdev_prop_array (dev, riscv_cpu_options); -riscv_cpu_add_qdev_prop_array (dev, riscv_cpu_vendor_exts); -riscv_cpu_add_qdev_prop_array (dev, riscv_cpu_experimental_exts); +riscv_cpu_add_multiext_prop_array (obj, riscv_cpu_extensions); +riscv_cpu_add_multiext_prop_array (obj, riscv_cpu_vendor_exts); +riscv_cpu_add_multiext_prop_array (obj, riscv_cpu_experimental_exts); + +for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) { +qdev_property_add_static (DEVICE (obj), prop); + } }/*
@@ -2098,7 +2157,7 @@ static void riscv_init_max_cpu_extensions(Object *obj) {RISCVCPU *cpu =RISCV_CPU (obj);CPURISCVState *env = &cpu->env; -Property *prop; +RISCVCPUMultiExtConfig *prop;/* Enable RVG, RVJ and RVV that are disabled by default */ set_misa (env, env->misa_mxl, env->misa_ext |RVG |RVJ |RVV );
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 17/20] target/riscv/cpu.c: introduce RISCVCPUMultiExtConfig
Quoted 33 lines · Daniel Henrique Barboza
If we want to make better decisions when auto-enabling extensions during
realize()we need a way to tell if an user set an extension manually. The RISC-V KVM driver has its own solution via a KVMCPUConfig struct that has an 'user_set' flag that is set during the Propertyset()callback. Theset()callback also doesinit()time validations based on the current KVM driver capabilities.For TCG we would want a '
user_set' mechanic too, but we would look ad-hoc viacpu_cfg_ext_auto_update()if a certain extension was user set or not. If we copy what was made in the KVM side we would look for 'user_set' for one into 60+ extension structs spreaded in 3 arrays (riscv_cpu_extensions,riscv_cpu_experimental_exts,riscv_cpu_vendor_exts).We'll still need an extension struct but we won't be using the '
user_set' flag:- 'RISCVCPUMultiExtConfig' will be our specialized structure, similar to what we're already doing with the MISA extensions in 'RISCVCPUMisaExtConfig'.
DEFINE_PROP_BOOL()for all 3 extensions arrays were replaced byMULTI_EXT_CFG_BOOL(), a macro that will init our specialized struct;- the '
multi_ext_user_opts' hash will be used to store the offset of each extension that the user set via theset()callback,cpu_set_multi_ext_cfg(). For now we're just initializing and populating it - next patch will use it to determine if a certain extension was user set;-
cpu_add_multi_ext_prop()is a new helper that will replace theqdev_property_add_static()calls that our macros are doing to populate user properties. The macro was renamed toADD_CPU_MULTIEXT_PROPS_ARRAY()for clarity. Note that the non-extension properties in riscv_cpu_options[] still need to be declared viaqdev().
Quoted 6 lines
Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- target/riscv/cpu.c | 257 ++++++++++++++++++++++++++++----------------- 1 file changed, 158 insertions(+), 99 deletions(-)
PATCH RESEND v8 18/20 target/riscv: use isa_ext_update_enabled() in init_max_cpu_extensions()
In reply to Daniel Henrique BarbozaBefore adding support to detect if an extension was user set we need to handle how we're enabling extensions in riscv_init_max_cpu_extensions(). object_property_set_bool() calls the set() callback for the property, and we're going to use this callback to set the 'multi_ext_user_opts' hash.
This means that, as is today, all extensions we're setting for the 'max' CPU will be seen as user set in the future. Let's change set_bool() to isa_ext_update_enabled() that will just enable/disable the flag on a certain offset.
--- target/riscv/cpu.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
target/riscv/cpu.c +9 -9
@@ -2163,24 +2163,24 @@ static void riscv_init_max_cpu_extensions(Object *obj)set_misa (env, env->misa_mxl, env->misa_ext |RVG |RVJ |RVV );for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { -object_property_set_bool (obj, prop->name, true,NULL ); +isa_ext_update_enabled (cpu, prop->offset, true); }/* set vector version */ env->vext_ver =VEXT_VERSION_1_00_0 ;/* Zfinx is not compatible with F. Disable it */ -object_property_set_bool (obj,"zfinx" , false,NULL ); -object_property_set_bool (obj,"zdinx" , false,NULL ); -object_property_set_bool (obj,"zhinx" , false,NULL ); -object_property_set_bool (obj,"zhinxmin" , false,NULL ); +isa_ext_update_enabled (cpu,CPU_CFG_OFFSET (ext_zfinx), false); +isa_ext_update_enabled (cpu,CPU_CFG_OFFSET (ext_zdinx), false); +isa_ext_update_enabled (cpu,CPU_CFG_OFFSET (ext_zhinx), false); +isa_ext_update_enabled (cpu,CPU_CFG_OFFSET (ext_zhinxmin), false); -object_property_set_bool (obj,"zce" , false,NULL ); -object_property_set_bool (obj,"zcmp" , false,NULL ); -object_property_set_bool (obj,"zcmt" , false,NULL ); +isa_ext_update_enabled (cpu,CPU_CFG_OFFSET (ext_zce), false); +isa_ext_update_enabled (cpu,CPU_CFG_OFFSET (ext_zcmp), false); +isa_ext_update_enabled (cpu,CPU_CFG_OFFSET (ext_zcmt), false);if (env->misa_mxl !=MXL_RV32 ) { -object_property_set_bool (obj,"zcf" , false,NULL ); +isa_ext_update_enabled (cpu,CPU_CFG_OFFSET (ext_zcf), false); } }
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 18/20] target/riscv: use isa_ext_update_enabled() in init_max_cpu_extensions()
Quoted 59 lines · Daniel Henrique Barboza
Before adding support to detect if an extension was user set we need to handle how we're enabling extensions in
riscv_init_max_cpu_extensions().object_property_set_bool()calls theset()callback for the property, and we're going to use this callback to set the 'multi_ext_user_opts' hash.This means that, as is today, all extensions we're setting for the 'max' CPU will be seen as user set in the future. Let's change
set_bool()toisa_ext_update_enabled()that will just enable/disable the flag on a certain offset.Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- target/riscv/cpu.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)target/riscv/cpu.c +9
-9@@ -2163,24 +2163,24 @@ static void riscv_init_max_cpu_extensions(Object *obj)set_misa (env, env->misa_mxl, env->misa_ext |RVG |RVJ |RVV );for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { -object_property_set_bool (obj, prop->name, true,NULL ); +isa_ext_update_enabled (cpu, prop->offset, true); }/* set vector version */ env->vext_ver =VEXT_VERSION_1_00_0 ;/* Zfinx is not compatible with F. Disable it */ -object_property_set_bool (obj,"zfinx" , false,NULL ); -object_property_set_bool (obj,"zdinx" , false,NULL ); -object_property_set_bool (obj,"zhinx" , false,NULL ); -object_property_set_bool (obj,"zhinxmin" , false,NULL ); +isa_ext_update_enabled (cpu,CPU_CFG_OFFSET (ext_zfinx), false); +isa_ext_update_enabled (cpu,CPU_CFG_OFFSET (ext_zdinx), false); +isa_ext_update_enabled (cpu,CPU_CFG_OFFSET (ext_zhinx), false); +isa_ext_update_enabled (cpu,CPU_CFG_OFFSET (ext_zhinxmin), false); -object_property_set_bool (obj,"zce" , false,NULL ); -object_property_set_bool (obj,"zcmp" , false,NULL ); -object_property_set_bool (obj,"zcmt" , false,NULL ); +isa_ext_update_enabled (cpu,CPU_CFG_OFFSET (ext_zce), false); +isa_ext_update_enabled (cpu,CPU_CFG_OFFSET (ext_zcmp), false); +isa_ext_update_enabled (cpu,CPU_CFG_OFFSET (ext_zcmt), false);if (env->misa_mxl !=MXL_RV32 ) { -object_property_set_bool (obj,"zcf" , false,NULL ); +isa_ext_update_enabled (cpu,CPU_CFG_OFFSET (ext_zcf), false); } }Signature · 1 line
2.41.0
PATCH RESEND v8 19/20 target/riscv/cpu.c: honor user choice in cpu_cfg_ext_auto_update()
In reply to Daniel Henrique BarbozaAdd a new cpu_cfg_ext_is_user_set() helper to check if an extension was set by the user in the command line. Use it inside cpu_cfg_ext_auto_update() to verify if the user set a certain extension and, if that's the case, do not change its value.
This will make us honor user choice instead of overwriting the values. Users will then be informed whether they're using an incompatible set of extensions instead of QEMU setting a magic value that works.
For example, we'll now error out if the user explictly set 'zce' to true and 'zca' to false:
$ ./build/qemu-system-riscv64 -M virt -cpu rv64,zce=true,zca=false -nographic qemu-system-riscv64: Zcf/Zcd/Zcb/Zcmp/Zcmt extensions require Zca extension
This didn't happen before because we were enabling 'zca' if 'zce' was enabled regardless if the user explictly set 'zca' to false.
--- target/riscv/cpu.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
target/riscv/cpu.c +10 -0
@@ -196,6 +196,12 @@ static int cpu_cfg_ext_get_min_version(uint32_t ext_offset)return PRIV_VERSION_1_10_0 ; } +static bool cpu_cfg_ext_is_user_set (uint32_t ext_offset) +{ +return g_hash_table_contains (multi_ext_user_opts, +GUINT_TO_POINTER (ext_offset)); +} +static void cpu_cfg_ext_auto_update (RISCVCPU *cpu,uint32_t ext_offset,bool value) {
@@ -207,6 +213,10 @@ static void cpu_cfg_ext_auto_update(RISCVCPU *cpu, uint32_t ext_offset,return ; } +if (cpu_cfg_ext_is_user_set (ext_offset)) { +return ; + } +if (value && env->priv_ver !=PRIV_VERSION_LATEST ) {/* Do not enable it if priv_ver is older than min_version */ min_version =cpu_cfg_ext_get_min_version (ext_offset);
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 19/20] target/riscv/cpu.c: honor user choice in cpu_cfg_ext_auto_update()
Quoted 56 lines · Daniel Henrique Barboza
Add a new
cpu_cfg_ext_is_user_set()helper to check if an extension was set by the user in the command line. Use it insidecpu_cfg_ext_auto_update()to verify if the user set a certain extension and, if that's the case, do not change its value.This will make us honor user choice instead of overwriting the values. Users will then be informed whether they're using an incompatible set of extensions instead of QEMU setting a magic value that works.
For example, we'll now error out if the user explictly set 'zce' to true and 'zca' to false:
$ ./build/qemu-system-riscv64 -M virt -cpu rv64,zce=true,zca=false -nographic qemu-system-riscv64: Zcf/Zcd/Zcb/Zcmp/Zcmt extensions require Zca extensionThis didn't happen before because we were enabling 'zca' if 'zce' was enabled regardless if the user explictly set 'zca' to false.
Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- target/riscv/cpu.c | 10 ++++++++++ 1 file changed, 10 insertions(+)target/riscv/cpu.c +10
-0@@ -196,6 +196,12 @@ static int cpu_cfg_ext_get_min_version(uint32_t ext_offset)return PRIV_VERSION_1_10_0 ; } +static bool cpu_cfg_ext_is_user_set (uint32_t ext_offset) +{ +return g_hash_table_contains (multi_ext_user_opts, +GUINT_TO_POINTER (ext_offset)); +} +static void cpu_cfg_ext_auto_update (RISCVCPU *cpu,uint32_t ext_offset,bool value) {@@ -207,6 +213,10 @@ static void cpu_cfg_ext_auto_update(RISCVCPU *cpu, uint32_t ext_offset,return ; } +if (cpu_cfg_ext_is_user_set (ext_offset)) { +return ; + } +if (value && env->priv_ver !=PRIV_VERSION_LATEST ) {/* Do not enable it if priv_ver is older than min_version */ min_version =cpu_cfg_ext_get_min_version (ext_offset);Signature · 1 line
2.41.0
PATCH RESEND v8 20/20 target/riscv/cpu.c: consider user option with RVG
In reply to Daniel Henrique BarbozaEnabling RVG will enable a set of extensions that we're not checking if the user was okay enabling or not. And in this case we want to error out, instead of ignoring, otherwise we will be inconsistent enabling RVG without all its extensions.
After this patch, disabling ifencei or icsr while enabling RVG will result in error:
$ ./build/qemu-system-riscv64 -M virt -cpu rv64,g=true,Zifencei=false --nographic qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei qemu-system-riscv64: RVG requires Zifencei but user set Zifencei to false
--- target/riscv/cpu.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
target/riscv/cpu.c +16 -2
@@ -1155,8 +1155,22 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) riscv_has_ext(env, RVD) && cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {warn_report ("Setting G will also set IMAFD_Zicsr_Zifencei" ); - cpu->cfg.ext_icsr = true; - cpu->cfg.ext_ifencei = true; + +if (cpu_cfg_ext_is_user_set (CPU_CFG_OFFSET (ext_icsr)) && + !cpu->cfg.ext_icsr) { +error_setg (errp,"RVG requires Zicsr but user set Zicsr to false" ); +return ; + } + +if (cpu_cfg_ext_is_user_set (CPU_CFG_OFFSET (ext_ifencei)) && + !cpu->cfg.ext_ifencei) { +error_setg (errp,"RVG requires Zifencei but user set " +"Zifencei to false" ); +return ; + } + +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_icsr), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_ifencei), true); env->misa_ext |=RVI |RVM |RVA |RVF |RVD ; env->misa_ext_mask |=RVI |RVM |RVA |RVF |RVD ;
Signature · 1 line
2.41.0
Re: [PATCH RESEND v8 20/20] target/riscv/cpu.c: consider user option with RVG
Quoted 11 lines · Daniel Henrique Barboza
Enabling RVG will enable a set of extensions that we're not checking if the user was okay enabling or not. And in this case we want to error out, instead of ignoring, otherwise we will be inconsistent enabling RVG without all its extensions.
After this patch, disabling ifencei or icsr while enabling RVG will result in error:
$ ./build/qemu-system-riscv64 -M virt -cpu rv64,g=true,Zifencei=false --nographic qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei qemu-system-riscv64: RVG requires Zifencei but user set Zifencei to false
The warning makes it sound like Zifencei is getting overridden, but then the error says "nope". So I think...
Quoted 12 lines
Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- target/riscv/cpu.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)target/riscv/cpu.c +0
-0
target/riscv/cpu.c void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) · line 1157
Quoted 3 lines
target/riscv/cpu.c +0
-0@@ -1155,8 +1155,22 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) riscv_has_ext(env, RVD) && cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {warn_report ("Setting G will also set IMAFD_Zicsr_Zifencei" );
...we want to move this warning down below the checks and we should even check if it should be issued with
if (!cpu->cfg.ext_icsr || !cpu->cfg.ext_ifencei || !cpu->cfg.a || ...)
warn_report(...)
Quoted 25 lines
target/riscv/cpu.c +16
-2- cpu->cfg.ext_icsr = true; - cpu->cfg.ext_ifencei = true; + +if (cpu_cfg_ext_is_user_set (CPU_CFG_OFFSET (ext_icsr)) && + !cpu->cfg.ext_icsr) { +error_setg (errp,"RVG requires Zicsr but user set Zicsr to false" ); +return ; + } + +if (cpu_cfg_ext_is_user_set (CPU_CFG_OFFSET (ext_ifencei)) && + !cpu->cfg.ext_ifencei) { +error_setg (errp,"RVG requires Zifencei but user set " +"Zifencei to false" ); +return ; + } + +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_icsr), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_ifencei), true); env->misa_ext |=RVI |RVM |RVA |RVF |RVD ; env->misa_ext_mask |=RVI |RVM |RVA |RVF |RVD ;Signature · 1 line
2.41.0
Thanks,
drew
Re: [PATCH RESEND v8 20/20] target/riscv/cpu.c: consider user option with RVG
Quoted 35 lines · Andrew Jones
Quoted 11 lines · Daniel Henrique Barboza
Enabling RVG will enable a set of extensions that we're not checking if the user was okay enabling or not. And in this case we want to error out, instead of ignoring, otherwise we will be inconsistent enabling RVG without all its extensions.
After this patch, disabling ifencei or icsr while enabling RVG will result in error:
$ ./build/qemu-system-riscv64 -M virt -cpu rv64,g=true,Zifencei=false --nographic qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei qemu-system-riscv64: RVG requires Zifencei but user set Zifencei to falseThe warning makes it sound like Zifencei is getting overridden, but then the error says "nope". So I think...
Quoted 12 lines
Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Alistair Francis <[email protected]>--- target/riscv/cpu.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)target/riscv/cpu.c +0
-0target/riscv/cpu.c void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) · line 1157
Quoted 3 lines
target/riscv/cpu.c +0
-0@@ -1155,8 +1155,22 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) riscv_has_ext(env, RVD) && cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {warn_report ("Setting G will also set IMAFD_Zicsr_Zifencei" );...we want to move this warning down below the checks and we should even check if it should be issued with
Quoted 2 lines
if (!cpu->cfg.ext_icsr || !cpu->cfg.ext_ifencei || !cpu->cfg.a || ...)
warn_report(...)Moving the warning makes sense. I'll do that.
About the check, it's not visible in the patch but right before this code we have:
/* Do some ISA extension error checking */
if (riscv_has_ext(env, RVG) &&
!(riscv_has_ext(env, RVI) && riscv_has_ext(env, RVM) &&
riscv_has_ext(env, RVA) && riscv_has_ext(env, RVF) &&
riscv_has_ext(env, RVD) &&
cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {
warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");So we're already checking if we have at least one G extension disabled before issuing the warning.
Thanks,
Daniel
Quoted 29 lines
Quoted 26 lines
target/riscv/cpu.c +16
-2- cpu->cfg.ext_icsr = true; - cpu->cfg.ext_ifencei = true; + +if (cpu_cfg_ext_is_user_set (CPU_CFG_OFFSET (ext_icsr)) && + !cpu->cfg.ext_icsr) { +error_setg (errp,"RVG requires Zicsr but user set Zicsr to false" ); +return ; + } + +if (cpu_cfg_ext_is_user_set (CPU_CFG_OFFSET (ext_ifencei)) && + !cpu->cfg.ext_ifencei) { +error_setg (errp,"RVG requires Zifencei but user set " +"Zifencei to false" ); +return ; + } + +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_icsr), true); +cpu_cfg_ext_auto_update (cpu,CPU_CFG_OFFSET (ext_ifencei), true); env->misa_ext |=RVI |RVM |RVA |RVF |RVD ; env->misa_ext_mask |=RVI |RVM |RVA |RVF |RVD ;Signature · 1 line
2.41.0Thanks,
drew
Quoted 17 lines · Daniel Henrique Barboza
Hi,
This is a resend of these two patch sets because they no longer apply into Alistair's riscv-to-apply.next:
[PATCH v8 00/12] riscv: add 'max' CPU, deprecate 'any'
https://lore.kernel.org/qemu-riscv/[email protected]/[PATCH v3 0/8] riscv: detecting user choice in TCG extensions
https://lore.kernel.org/qemu-riscv/[email protected]/They're being sent in a single package for convenience. No other changes made from their old versions aside from the rebase.
Patches missing acks: 4,7,8
Quoted 8 lines
Changes from v7:
- patch 7:
- addriscv_cpu_add_qdev_prop_array()function instead of a macro
- patch 8:
- addriscv_cpu_add_kvm_unavail_prop_array()function instead of a
macro
- v7 link: https://lore.kernel.org/qemu-riscv/[email protected]/
Tracking whether or not the user enabled/disabled an extension is good. I see we're still not tracking for the MISA extensions, though. If tracking isn't necessary for them, then a comment above riscv_cpu_add_misa_properties()
explaining why not would be good. If tracking may be necessary, then making them consistent with the multi extensions by using the hash would be good.
Thanks,
drew