Message ID | 20230727220927.62950-5-dbarboza@ventanamicro.com |
---|---|
State | New |
Headers | show |
Series | riscv: add 'max' CPU, deprecate 'any' | expand |
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 > >
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 >> >>
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
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
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++) {