[v6] riscv: add 'max' CPU, deprecate 'any'

[PATCH v6 04/12] target/riscv/cpu.c: del DEFINE_PROP_END_OF_LIST() from riscv_cpu_extensions

Posted by Daniel Henrique Barboza 2 years, 6 months ago

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 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.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
---
 target/riscv/cpu.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f1a292d967..33a2e9328c 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -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

Posted by Alistair Francis 2 years, 6 months ago

On Thu, Jul 27, 2023 at 6:20 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 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 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.

Is this the right approach? This seem different to the rest of QEMU

Alistair

>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> ---
>  target/riscv/cpu.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f1a292d967..33a2e9328c 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -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

Posted by Daniel Henrique Barboza 2 years, 5 months ago


On 8/10/23 14:49, Alistair Francis wrote:
> On Thu, Jul 27, 2023 at 6:20 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> 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 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.
> 
> 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


> 
> Alistair
> 
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> ---
>>   target/riscv/cpu.c | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index f1a292d967..33a2e9328c 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -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

Posted by Peter Maydell 2 years, 5 months ago

On Tue, 15 Aug 2023 at 13:44, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 8/10/23 14:49, Alistair Francis wrote:
> > On Thu, Jul 27, 2023 at 6:20 PM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >> 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 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.
> >
> > 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

Posted by Daniel Henrique Barboza 2 years, 5 months ago


On 8/15/23 10:15, Peter Maydell wrote:
> On Tue, 15 Aug 2023 at 13:44, Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>>
>>
>> On 8/10/23 14:49, Alistair Francis wrote:
>>> On Thu, Jul 27, 2023 at 6:20 PM Daniel Henrique Barboza
>>> <dbarboza@ventanamicro.com> wrote:
>>>>
>>>> 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 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.
>>>
>>> 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 for the explanation. Having a non-terminated property array is another
reason to revisit this patch.


Thanks,

Daniel

> 
> thanks
> -- PMM