riscv: add 'max' CPU, deprecate 'any'
v6 cover riscv: add 'max' CPU, deprecate 'any'
Hi,
I decided to spin a new version of this work after chatting with Conor about some log messages about 'vector version is not specified' message when using the 'max' CPU.
For this particular CPU we should set an appropriate value for the vector version right at cpu_init(), but doing that isn't enough to avoid that log message. A new patch (9) was added to tune in RVV validation code to skip the message if a CPU already set env->vext_ver.
Patches missing acks: 9, 11, 12
Changes from v6:
- patch 9 (new)
- change RVV vext_ver handling to avoid msg logs if the CPU sets a
specific env->vext_ver
- patch 10 (former 9)
- set env->vext_ver in riscv_init_max_cpu_extensions()
- v5 link: https://lore.kernel.org/qemu-riscv/[email protected]/
Daniel Henrique Barboza (12):
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/cpu.c: del DEFINE_PROP_END_OF_LIST() from
riscv_cpu_extensions
target/riscv/cpu.c: split vendor exts from riscv_cpu_extensions[] target/riscv/cpu.c: split non-ratified exts from
riscv_cpu_extensions[]
target/riscv/cpu.c: add ADD_CPU_QDEV_PROPERTIES_ARRAY() macro target/riscv/cpu.c: add ADD_UNAVAIL_KVM_PROP_ARRAY() macro 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
docs/about/deprecated.rst | 12 +++ target/riscv/cpu-qom.h | 1 + target/riscv/cpu.c | 188 +++++++++++++++++++++++++-------- tests/avocado/riscv_opensbi.py | 16 +++ 4 files changed, 171 insertions(+), 46 deletions(-)
Signature · 1 line
2.41.0
PATCH v6 1/12 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
@@ -1759,7 +1759,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),
@@ -1771,15 +1770,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),
@@ -1810,9 +1802,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),
@@ -1856,6 +1846,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,
@@ -1924,6 +1929,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
PATCH v6 2/12 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
@@ -1914,17 +1914,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 v6 3/12 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
@@ -1881,6 +1881,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. *
@@ -1896,39 +1936,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 v6 03/12] target/riscv/cpu.c: split kvm prop handling to its own helper
Quoted 7 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.
Quoted 8 lines
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.Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Weiwei Li <[email protected]>
Alistair
Quoted 101 lines
--- target/riscv/cpu.c | 65 ++++++++++++++++++++++++++++++---------------- 1 file changed, 42 insertions(+), 23 deletions(-)target/riscv/cpu.c +42
-24@@ -1881,6 +1881,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. * @@ -1896,39 +1936,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]); } } --2.41.0
PATCH v6 4/12 target/riscv/cpu.c: del DEFINE_PROP_END_OF_LIST() from riscv_cpu_extensions
In reply to Daniel Henrique BarbozaThis last blank element is used by the 'for' loop to check if a property has a valid name.
Remove it and use ARRAY_SIZE() instead like riscv_cpu_options is already using. All future arrays will also do the same and we'll able to encapsulate more repetitions in macros later on.
--- target/riscv/cpu.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
target/riscv/cpu.c +4 -8
@@ -1842,8 +1842,6 @@ static Property riscv_cpu_extensions[] = {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), - - DEFINE_PROP_END_OF_LIST(), }; static Property riscv_cpu_options[] = {
@@ -1901,14 +1899,13 @@ static void riscv_cpu_add_kvm_unavail_prop(Object *obj, const char *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_extensions); i++) { +riscv_cpu_add_kvm_unavail_prop (obj, riscv_cpu_extensions[i].name); }for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_options); i++) {
@@ -1929,7 +1926,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
@@ -1943,8 +1939,8 @@ 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 (int i =0 ; i <ARRAY_SIZE (riscv_cpu_extensions); i++) { +qdev_property_add_static (dev, &riscv_cpu_extensions[i]); }for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_options); i++) {
Signature · 1 line
2.41.0
Re: [PATCH v6 04/12] target/riscv/cpu.c: del DEFINE_PROP_END_OF_LIST() from riscv_cpu_extensions
Quoted 7 lines · Daniel Henrique Barboza
This last blank element is used by the 'for' loop to check if a property has a valid name.
Remove it and use
ARRAY_SIZE()instead likeriscv_cpu_optionsis already using. All future arrays will also do the same and we'll able to encapsulate more repetitions in macros later on.
Is this the right approach? This seem different to the rest of QEMU
Alistair
Quoted 60 lines
Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Weiwei Li <[email protected]>--- target/riscv/cpu.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)target/riscv/cpu.c +4
-9@@ -1842,8 +1842,6 @@ static Property riscv_cpu_extensions[] = {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), - - DEFINE_PROP_END_OF_LIST(), }; static Property riscv_cpu_options[] = {@@ -1901,14 +1899,13 @@ static void riscv_cpu_add_kvm_unavail_prop(Object *obj, const char *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_extensions); i++) { +riscv_cpu_add_kvm_unavail_prop (obj, riscv_cpu_extensions[i].name); }for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_options); i++) {@@ -1929,7 +1926,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 @@ -1943,8 +1939,8 @@ 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 (int i =0 ; i <ARRAY_SIZE (riscv_cpu_extensions); i++) { +qdev_property_add_static (dev, &riscv_cpu_extensions[i]); }for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_options); i++) { --2.41.0
Re: [PATCH v6 04/12] target/riscv/cpu.c: del DEFINE_PROP_END_OF_LIST() from riscv_cpu_extensions
Quoted 11 lines · Alistair Francis
Quoted 7 lines · Daniel Henrique Barboza
This last blank element is used by the 'for' loop to check if a property has a valid name.
Remove it and use
ARRAY_SIZE()instead likeriscv_cpu_optionsis already using. All future arrays will also do the same and we'll able to encapsulate more repetitions in macros later on.Is this the right approach? This seem different to the rest of QEMU
I am not sure if we have a 'right approach' in this case or not. I see both being used in QEMU.
All this said, I'm cooking another series in which I had to remove the ARRAY_SIZE() of all these arrays, going back to add the empty element at the end of each one, because I ended up exporting them (making them extern) to other files and ARRAY_SIZE() doesn't work in that case.
I can do this right now in this patch with the excuse that we're going to export them in the future. As long as we do the same thing to all arrays we should be able to eliminate code repetition anyway.
Thanks,
Daniel
Quoted 63 lines
Alistair
Quoted 60 lines
Signed-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Weiwei Li <[email protected]>--- target/riscv/cpu.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)target/riscv/cpu.c +4
-9@@ -1842,8 +1842,6 @@ static Property riscv_cpu_extensions[] = {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), - - DEFINE_PROP_END_OF_LIST(), }; static Property riscv_cpu_options[] = {@@ -1901,14 +1899,13 @@ static void riscv_cpu_add_kvm_unavail_prop(Object *obj, const char *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_extensions); i++) { +riscv_cpu_add_kvm_unavail_prop (obj, riscv_cpu_extensions[i].name); }for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_options); i++) {@@ -1929,7 +1926,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 @@ -1943,8 +1939,8 @@ 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 (int i =0 ; i <ARRAY_SIZE (riscv_cpu_extensions); i++) { +qdev_property_add_static (dev, &riscv_cpu_extensions[i]); }for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_options); i++) { --2.41.0
Re: [PATCH v6 04/12] target/riscv/cpu.c: del DEFINE_PROP_END_OF_LIST() from riscv_cpu_extensions
Quoted 10 lines · Daniel Henrique Barboza
Quoted 6 lines · Alistair Francis
Quoted 4 lines · Daniel Henrique Barboza
This last blank element is used by the 'for' loop to check if a property has a valid name.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Remove it and use
ARRAY_SIZE()instead likeriscv_cpu_optionsis already using. All future arrays will also do the same and we'll able to encapsulate more repetitions in macros later on.Is this the right approach? This seem different to the rest of QEMU
I am not sure if we have a 'right approach' in this case or not. I see both being used in QEMU.
The major use of the DEFINE_PROP_* macros is for creating a property list to pass to device_class_set_props(). Those lists must be terminated with the DEFINE_PROP_END_OF_LIST() marker (because the function takes a pointer and can't tell the size of the list with ARRAY_SIZE()). For cases like this where you're writing code locally to manually iterate through the array and never pass it to any other code in QEMU, both approaches work. But it does seem to me a little confusing to have a non-terminated property array.
thanks
-- PMM
Re: [PATCH v6 04/12] target/riscv/cpu.c: del DEFINE_PROP_END_OF_LIST() from riscv_cpu_extensions
Quoted 21 lines · Peter Maydell
Quoted 10 lines · Daniel Henrique Barboza
Quoted 6 lines · Alistair Francis
Quoted 4 lines · Daniel Henrique Barboza
This last blank element is used by the 'for' loop to check if a property has a valid name.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Remove it and use
ARRAY_SIZE()instead likeriscv_cpu_optionsis already using. All future arrays will also do the same and we'll able to encapsulate more repetitions in macros later on.Is this the right approach? This seem different to the rest of QEMU
I am not sure if we have a 'right approach' in this case or not. I see both being used in QEMU.
Quoted 9 lines
The major use of the DEFINE_PROP_* macros is for creating a property list to pass to
device_class_set_props(). Those lists must be terminated with theDEFINE_PROP_END_OF_LIST()marker (because the function takes a pointer and can't tell the size of the list withARRAY_SIZE()). For cases like this where you're writing code locally to manually iterate through the array and never pass it to any other code in QEMU, both approaches work. But it does seem to me a little confusing to have a non-terminated property array.
Thanks for the explanation. Having a non-terminated property array is another reason to revisit this patch.
Thanks,
Daniel
Quoted 3 lines
thanks
-- PMM
PATCH v6 5/12 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 | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-)
target/riscv/cpu.c +23 -14
@@ -1814,20 +1814,6 @@ 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_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), -/* These are experimental so mark with 'x-' */ DEFINE_PROP_BOOL("x-zicond", RISCVCPU, cfg.ext_zicond, false),
@@ -1844,6 +1830,21 @@ static Property riscv_cpu_extensions[] = { DEFINE_PROP_BOOL("x-zvfbfwma", RISCVCPU, cfg.ext_zvfbfwma, false), }; +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 Property riscv_cpu_options[] = {DEFINE_PROP_UINT8 ("pmu-num" ,RISCVCPU , cfg.pmu_num,16 ),
@@ -1908,6 +1909,10 @@ static void riscv_cpu_add_kvm_properties(Object *obj)riscv_cpu_add_kvm_unavail_prop (obj, riscv_cpu_extensions[i].name); } +for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_vendor_exts); i++) { +riscv_cpu_add_kvm_unavail_prop (obj, riscv_cpu_vendor_exts[i].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)) {
@@ -1946,6 +1951,10 @@ static void riscv_cpu_add_user_properties(Object *obj)for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_options); i++) {qdev_property_add_static (dev, &riscv_cpu_options[i]); } + +for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_vendor_exts); i++) { +qdev_property_add_static (dev, &riscv_cpu_vendor_exts[i]); + } }static Property riscv_cpu_properties[] = {
Signature · 1 line
2.41.0
PATCH v6 6/12 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 | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-)
target/riscv/cpu.c +26 -15
@@ -1813,21 +1813,6 @@ static Property riscv_cpu_extensions[] = {DEFINE_PROP_BOOL ("zcf" ,RISCVCPU , cfg.ext_zcf, false),DEFINE_PROP_BOOL ("zcmp" ,RISCVCPU , cfg.ext_zcmp, false), DEFINE_PROP_BOOL("zcmt", RISCVCPU, cfg.ext_zcmt, false), - -/* These are experimental so mark with 'x-' */ -DEFINE_PROP_BOOL ("x-zicond" ,RISCVCPU , cfg.ext_zicond, false), - -/* ePMP 0.9.3 */ -DEFINE_PROP_BOOL ("x-epmp" ,RISCVCPU , cfg.epmp, false), -DEFINE_PROP_BOOL ("x-smaia" ,RISCVCPU , cfg.ext_smaia, false), -DEFINE_PROP_BOOL ("x-ssaia" ,RISCVCPU , cfg.ext_ssaia, false), - -DEFINE_PROP_BOOL ("x-zvfh" ,RISCVCPU , cfg.ext_zvfh, false), -DEFINE_PROP_BOOL ("x-zvfhmin" ,RISCVCPU , cfg.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), }; static Property riscv_cpu_vendor_exts[] = {
@@ -1845,6 +1830,23 @@ static Property riscv_cpu_vendor_exts[] = { DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps, false), }; +/* 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 */ +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), + +DEFINE_PROP_BOOL ("x-zvfh" ,RISCVCPU , cfg.ext_zvfh, false), +DEFINE_PROP_BOOL ("x-zvfhmin" ,RISCVCPU , cfg.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), +}; +static Property riscv_cpu_options[] = {DEFINE_PROP_UINT8 ("pmu-num" ,RISCVCPU , cfg.pmu_num,16 ),
@@ -1913,6 +1915,11 @@ static void riscv_cpu_add_kvm_properties(Object *obj)riscv_cpu_add_kvm_unavail_prop (obj, riscv_cpu_vendor_exts[i].name); } +for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_experimental_exts); i++) { +riscv_cpu_add_kvm_unavail_prop (obj, + riscv_cpu_experimental_exts[i].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)) {
@@ -1955,6 +1962,10 @@ static void riscv_cpu_add_user_properties(Object *obj)for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_vendor_exts); i++) {qdev_property_add_static (dev, &riscv_cpu_vendor_exts[i]); } + +for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_experimental_exts); i++) { +qdev_property_add_static (dev, &riscv_cpu_experimental_exts[i]); + } }static Property riscv_cpu_properties[] = {
Signature · 1 line
2.41.0
PATCH v6 7/12 target/riscv/cpu.c: add ADD_CPU_QDEV_PROPERTIES_ARRAY() macro
In reply to Daniel Henrique BarbozaThe code inside riscv_cpu_add_user_properties() became quite repetitive after recent changes. Add a macro to hide the repetition away.
--- target/riscv/cpu.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)
target/riscv/cpu.c +11 -15
@@ -1882,6 +1882,13 @@ static void cpu_set_cfg_unavailable(Object *obj, Visitor *v, }#endif +#define ADD_CPU_QDEV_PROPERTIES_ARRAY (_dev, _array) \ + do { \ + for (int i = 0; i < ARRAY_SIZE(_array); i++) { \ + qdev_property_add_static(_dev, &_array[i]); \ + } \ + } while (0) +#ifndef CONFIG_USER_ONLY static void riscv_cpu_add_kvm_unavail_prop (Object *obj,const char *prop_name) {
@@ -1951,21 +1958,10 @@ static void riscv_cpu_add_user_properties(Object *obj)riscv_cpu_add_misa_properties (obj); -for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_extensions); i++) { -qdev_property_add_static (dev, &riscv_cpu_extensions[i]); - } - -for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_options); i++) { -qdev_property_add_static (dev, &riscv_cpu_options[i]); - } - -for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_vendor_exts); i++) { -qdev_property_add_static (dev, &riscv_cpu_vendor_exts[i]); - } - -for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_experimental_exts); i++) { -qdev_property_add_static (dev, &riscv_cpu_experimental_exts[i]); - } +ADD_CPU_QDEV_PROPERTIES_ARRAY (dev, riscv_cpu_extensions); +ADD_CPU_QDEV_PROPERTIES_ARRAY (dev, riscv_cpu_options); +ADD_CPU_QDEV_PROPERTIES_ARRAY (dev, riscv_cpu_vendor_exts); +ADD_CPU_QDEV_PROPERTIES_ARRAY (dev, riscv_cpu_experimental_exts); }static Property riscv_cpu_properties[] = {
Signature · 1 line
2.41.0
PATCH v6 8/12 target/riscv/cpu.c: add ADD_UNAVAIL_KVM_PROP_ARRAY() macro
In reply to Daniel Henrique BarbozaUse a macro in riscv_cpu_add_kvm_properties() to eliminate some of its code repetition, similar to what we're already doing with ADD_CPU_QDEV_PROPERTIES_ARRAY().
--- target/riscv/cpu.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)
target/riscv/cpu.c +10 -12
@@ -1907,6 +1907,13 @@ static void riscv_cpu_add_kvm_unavail_prop(Object *obj, const char *prop_name) NULL, (void *)prop_name); } +#define ADD_UNAVAIL_KVM_PROP_ARRAY (_obj, _array) \ + do { \ + for (int i = 0; i < ARRAY_SIZE(_array); i++) { \ + riscv_cpu_add_kvm_unavail_prop(_obj, _array[i].name); \ + } \ + } while (0) +static void riscv_cpu_add_kvm_properties (Object *obj) {DeviceState *dev =DEVICE (obj);
@@ -1914,18 +1921,9 @@ static void riscv_cpu_add_kvm_properties(Object *obj)kvm_riscv_init_user_properties (obj);riscv_cpu_add_misa_properties (obj); -for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_extensions); i++) { -riscv_cpu_add_kvm_unavail_prop (obj, riscv_cpu_extensions[i].name); - } - -for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_vendor_exts); i++) { -riscv_cpu_add_kvm_unavail_prop (obj, riscv_cpu_vendor_exts[i].name); - } - -for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_experimental_exts); i++) { -riscv_cpu_add_kvm_unavail_prop (obj, - riscv_cpu_experimental_exts[i].name); - } +ADD_UNAVAIL_KVM_PROP_ARRAY (obj, riscv_cpu_extensions); +ADD_UNAVAIL_KVM_PROP_ARRAY (obj, riscv_cpu_vendor_exts); +ADD_UNAVAIL_KVM_PROP_ARRAY (obj, riscv_cpu_experimental_exts);for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_options); i++) {/* Check if KVM created the property already */
Signature · 1 line
2.41.0
PATCH v6 9/12 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
@@ -939,8 +939,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 ;
@@ -963,17 +961,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
Re: [PATCH v6 09/12] target/riscv/cpu.c: limit cfg->vext_spec log message
Quoted 11 lines · Daniel Henrique Barboza
Inside
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_verwill be set in thecpu_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.
Quoted 5 lines
Change
riscv_cpu_validate_v()to not throw this log message ifenv->vext_veris already set.Signed-off-by Daniel Henrique Barboza <[email protected]>---
Weiwei Li
Quoted 38 lines
target/riscv/cpu.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)target/riscv/cpu.c +4
-5@@ -939,8 +939,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 ;@@ -963,17 +961,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)
Re: [PATCH v6 09/12] target/riscv/cpu.c: limit cfg->vext_spec log message
In reply to Daniel Henrique BarbozaQuoted 12 lines · Daniel Henrique Barboza
Inside
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_verwill be set in thecpu_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.
Quoted 4 lines
Change
riscv_cpu_validate_v()to not throw this log message ifenv->vext_veris already set.Signed-off-by Daniel Henrique Barboza <[email protected]>
Alistair
Quoted 43 lines
--- target/riscv/cpu.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)target/riscv/cpu.c +4
-6@@ -939,8 +939,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 ;@@ -963,17 +961,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) --2.41.0
PATCH v6 10/12 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 v6 10/12] target/riscv: add 'max' CPU type
Quoted 24 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:
Quoted 7 lines
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_svpbmtSigned-off-by Daniel Henrique Barboza <[email protected]>Reviewed-by Weiwei Li <[email protected]>
Alistair
Quoted 109 lines
--- 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
-1@@ -250,6 +250,7 @@ static const char * const riscv_intr_names[] = { };static void riscv_cpu_add_user_properties (Object *obj); +static void riscv_init_max_cpu_extensions (Object *obj);const char *riscv_cpu_get_trap_name (target_ulong cause,bool async) {@@ -376,6 +377,25 @@ static void riscv_any_cpu_init(Object *obj) cpu->cfg.pmp = true; } +static void riscv_max_cpu_init (Object *obj) +{ +RISCVCPU *cpu =RISCV_CPU (obj); +CPURISCVState *env = &cpu->env; +RISCVMXL mlx =MXL_RV64 ; + +#ifdef TARGET_RISCV32 + mlx =MXL_RV32 ; +#endif +set_misa (env, mlx,0 ); +riscv_cpu_add_user_properties (obj); +riscv_init_max_cpu_extensions (obj); + env->priv_ver =PRIV_VERSION_LATEST ; +#ifndef CONFIG_USER_ONLY +set_satp_mode_max_supported (RISCV_CPU (obj), mlx ==MXL_RV32 ? +VM_1_10_SV32 :VM_1_10_SV57 ); +#endif +} +#if defined(TARGET_RISCV64 )static void rv64_base_cpu_init (Object *obj) {@@ -1961,6 +1981,41 @@ static void riscv_cpu_add_user_properties(Object *obj)ADD_CPU_QDEV_PROPERTIES_ARRAY (dev, riscv_cpu_experimental_exts); } +/* +* The 'max' type CPU will have all possible ratified +* non-vendor extensions enabled. +*/ +static void riscv_init_max_cpu_extensions (Object *obj) +{ +RISCVCPU *cpu =RISCV_CPU (obj); +CPURISCVState *env = &cpu->env; + +/* Enable RVG, RVJ and RVV that are disabled by default */ +set_misa (env, env->misa_mxl, env->misa_ext |RVG |RVJ |RVV ); + +for (int i =0 ; i <ARRAY_SIZE (riscv_cpu_extensions); i++) { +object_property_set_bool (obj, riscv_cpu_extensions[i].name, + true,NULL ); + } + +/* 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 ); + +object_property_set_bool (obj,"zce" , false,NULL ); +object_property_set_bool (obj,"zcmp" , false,NULL ); +object_property_set_bool (obj,"zcmt" , false,NULL ); + +if (env->misa_mxl !=MXL_RV32 ) { +object_property_set_bool (obj,"zcf" , false,NULL ); + } +} +static Property riscv_cpu_properties[] = {DEFINE_PROP_BOOL ("debug" ,RISCVCPU , cfg.debug, true),@@ -2299,6 +2354,7 @@ static const TypeInfo riscv_cpu_type_infos[] = { .abstract = true, }, DEFINE_DYNAMIC_CPU(TYPE_RISCV_CPU_ANY, riscv_any_cpu_init), + DEFINE_DYNAMIC_CPU(TYPE_RISCV_CPU_MAX, riscv_max_cpu_init),#if defined(CONFIG_KVM ) DEFINE_CPU(TYPE_RISCV_CPU_HOST, riscv_host_cpu_init),#endif --2.41.0
PATCH v6 11/12 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 v6 11/12] avocado, risc-v: add opensbi tests for 'max' CPU
Quoted 5 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.
Signed-off-by Daniel Henrique Barboza <[email protected]>
Alistair
Quoted 32 lines
--- tests/avocado/riscv_opensbi.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)tests/avocado/riscv_opensbi.py +16
-1@@ -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() --2.41.0
PATCH v6 12/12 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 reimplemention 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
@@ -1477,6 +1477,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 v6 12/12] target/riscv: deprecate the 'any' CPU type
Quoted 16 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.
Quoted 8 lines
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]>
Alistair
Quoted 48 lines
--- 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 reimplemention 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
-1@@ -1477,6 +1477,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); --2.41.0