[PATCH] target/riscv: Convert priv_spec property from string to enum

khaled saleh posted 1 patch 1 week, 5 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260517192800.112905-1-khaled.saleh.req@gmail.com
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, Chao Liu <chao.liu.zevorn@gmail.com>
target/riscv/cpu.c         | 59 +++++++++++---------------------------
target/riscv/cpu.h         |  1 +
target/riscv/tcg/tcg-cpu.c |  3 +-
3 files changed, 20 insertions(+), 43 deletions(-)
[PATCH] target/riscv: Convert priv_spec property from string to enum
Posted by khaled saleh 1 week, 5 days ago
The priv_spec property accepts a fixed set of values (v1.10.0, v1.11.0, v1.12.0, v1.13.0) but was implemented as a string type with manual string-to-enum conversion in custom getter/setter functions.

Convert it to use QEnumLookup with visit_type_enum() for:
- Automatic input validation by the visitor framework
- QMP introspection support (valid values are discoverable)
- Reduced boilerplate (priv_spec_from_str/to_str removed)

This resolves the "FIXME enum?" comment in cpu.c.

Signed-off-by: khaled saleh <khaled.saleh.req@gmail.com>
---
 target/riscv/cpu.c         | 59 +++++++++++---------------------------
 target/riscv/cpu.h         |  1 +
 target/riscv/tcg/tcg-cpu.c |  3 +-
 3 files changed, 20 insertions(+), 43 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 506a018d52..8365471572 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -26,6 +26,7 @@
 #include "internals.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
+#include "qapi/util.h"
 #include "qemu/error-report.h"
 #include "hw/core/qdev-properties.h"
 #include "hw/core/qdev-prop-internal.h"
@@ -1655,51 +1656,25 @@ static const PropertyInfo prop_pmp_granularity = {
     .set = prop_pmp_granularity_set,
 };
 
-static int priv_spec_from_str(const char *priv_spec_str)
-{
-    int priv_version = -1;
-
-    if (!g_strcmp0(priv_spec_str, PRIV_VER_1_13_0_STR)) {
-        priv_version = PRIV_VERSION_1_13_0;
-    } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_12_0_STR)) {
-        priv_version = PRIV_VERSION_1_12_0;
-    } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_11_0_STR)) {
-        priv_version = PRIV_VERSION_1_11_0;
-    } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_10_0_STR)) {
-        priv_version = PRIV_VERSION_1_10_0;
-    }
-
-    return priv_version;
-}
+static const char *const priv_spec_str[] = {
+    [PRIV_VERSION_1_10_0] = PRIV_VER_1_10_0_STR,
+    [PRIV_VERSION_1_11_0] = PRIV_VER_1_11_0_STR,
+    [PRIV_VERSION_1_12_0] = PRIV_VER_1_12_0_STR,
+    [PRIV_VERSION_1_13_0] = PRIV_VER_1_13_0_STR,
+};
 
-const char *priv_spec_to_str(int priv_version)
-{
-    switch (priv_version) {
-    case PRIV_VERSION_1_10_0:
-        return PRIV_VER_1_10_0_STR;
-    case PRIV_VERSION_1_11_0:
-        return PRIV_VER_1_11_0_STR;
-    case PRIV_VERSION_1_12_0:
-        return PRIV_VER_1_12_0_STR;
-    case PRIV_VERSION_1_13_0:
-        return PRIV_VER_1_13_0_STR;
-    default:
-        return NULL;
-    }
-}
+const QEnumLookup priv_spec_lookup = {
+    .array = priv_spec_str,
+    .size = ARRAY_SIZE(priv_spec_str),
+};
 
 static void prop_priv_spec_set(Object *obj, Visitor *v, const char *name,
                                void *opaque, Error **errp)
 {
     RISCVCPU *cpu = RISCV_CPU(obj);
-    g_autofree char *value = NULL;
-    int priv_version = -1;
-
-    visit_type_str(v, name, &value, errp);
+    int priv_version;
 
-    priv_version = priv_spec_from_str(value);
-    if (priv_version < 0) {
-        error_setg(errp, "Unsupported privilege spec version '%s'", value);
+    if (!visit_type_enum(v, name, &priv_version, &priv_spec_lookup, errp)) {
         return;
     }
 
@@ -1718,15 +1693,15 @@ static void prop_priv_spec_get(Object *obj, Visitor *v, const char *name,
                                void *opaque, Error **errp)
 {
     RISCVCPU *cpu = RISCV_CPU(obj);
-    const char *value = priv_spec_to_str(cpu->env.priv_ver);
+    int value = cpu->env.priv_ver;
 
-    visit_type_str(v, name, (char **)&value, errp);
+    visit_type_enum(v, name, &value, &priv_spec_lookup, errp);
 }
 
 static const PropertyInfo prop_priv_spec = {
-    .type = "str",
+    .type = "RISCVPrivSpec",
     .description = "priv_spec",
-    /* FIXME enum? */
+    .enum_table = &priv_spec_lookup,
     .get = prop_priv_spec_get,
     .set = prop_priv_spec_set,
 };
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index fae839cade..522b63016f 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -70,6 +70,7 @@ typedef struct CPUArchState CPURISCVState;
 #define RVG RV('G')
 #define RVB RV('B')
 
+extern const QEnumLookup priv_spec_lookup;
 extern const uint32_t misa_bits[];
 const char *riscv_get_misa_ext_name(uint32_t bit);
 const char *riscv_get_misa_ext_description(uint32_t bit);
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 02c98cc2db..2ec4b8e98e 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -27,6 +27,7 @@
 #include "time_helper.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
+#include "qapi/util.h"
 #include "qemu/accel.h"
 #include "qemu/error-report.h"
 #include "qemu/log.h"
@@ -86,7 +87,7 @@ static void riscv_cpu_write_misa_bit(RISCVCPU *cpu, uint32_t bit,
 
 static const char *cpu_priv_ver_to_str(int priv_ver)
 {
-    const char *priv_spec_str = priv_spec_to_str(priv_ver);
+    const char *priv_spec_str = qapi_enum_lookup(&priv_spec_lookup, priv_ver);
 
     g_assert(priv_spec_str);
 
-- 
2.34.1
Re: [PATCH] target/riscv: Convert priv_spec property from string to enum
Posted by Daniel Henrique Barboza 1 week, 5 days ago

On 5/17/2026 4:28 PM, khaled saleh wrote:
> The priv_spec property accepts a fixed set of values (v1.10.0, v1.11.0, v1.12.0, v1.13.0) but was implemented as a string type with manual string-to-enum conversion in custom getter/setter functions.
> 
> Convert it to use QEnumLookup with visit_type_enum() for:
> - Automatic input validation by the visitor framework
> - QMP introspection support (valid values are discoverable)
> - Reduced boilerplate (priv_spec_from_str/to_str removed)
> 
> This resolves the "FIXME enum?" comment in cpu.c.
> 
> Signed-off-by: khaled saleh <khaled.saleh.req@gmail.com>
> ---

We usually send patches rebased on top of this tree:

https://github.com/alistair23/qemu/tree/riscv-to-apply.next

Using that tree this patch yields a trivial conflict in cpu.h.
If you can re-send it based on the tree above that would be great.


Patch LGTM otherwise. Thanks,
Daniel


>   target/riscv/cpu.c         | 59 +++++++++++---------------------------
>   target/riscv/cpu.h         |  1 +
>   target/riscv/tcg/tcg-cpu.c |  3 +-
>   3 files changed, 20 insertions(+), 43 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 506a018d52..8365471572 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -26,6 +26,7 @@
>   #include "internals.h"
>   #include "qapi/error.h"
>   #include "qapi/visitor.h"
> +#include "qapi/util.h"
>   #include "qemu/error-report.h"
>   #include "hw/core/qdev-properties.h"
>   #include "hw/core/qdev-prop-internal.h"
> @@ -1655,51 +1656,25 @@ static const PropertyInfo prop_pmp_granularity = {
>       .set = prop_pmp_granularity_set,
>   };
>   
> -static int priv_spec_from_str(const char *priv_spec_str)
> -{
> -    int priv_version = -1;
> -
> -    if (!g_strcmp0(priv_spec_str, PRIV_VER_1_13_0_STR)) {
> -        priv_version = PRIV_VERSION_1_13_0;
> -    } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_12_0_STR)) {
> -        priv_version = PRIV_VERSION_1_12_0;
> -    } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_11_0_STR)) {
> -        priv_version = PRIV_VERSION_1_11_0;
> -    } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_10_0_STR)) {
> -        priv_version = PRIV_VERSION_1_10_0;
> -    }
> -
> -    return priv_version;
> -}
> +static const char *const priv_spec_str[] = {
> +    [PRIV_VERSION_1_10_0] = PRIV_VER_1_10_0_STR,
> +    [PRIV_VERSION_1_11_0] = PRIV_VER_1_11_0_STR,
> +    [PRIV_VERSION_1_12_0] = PRIV_VER_1_12_0_STR,
> +    [PRIV_VERSION_1_13_0] = PRIV_VER_1_13_0_STR,
> +};
>   
> -const char *priv_spec_to_str(int priv_version)
> -{
> -    switch (priv_version) {
> -    case PRIV_VERSION_1_10_0:
> -        return PRIV_VER_1_10_0_STR;
> -    case PRIV_VERSION_1_11_0:
> -        return PRIV_VER_1_11_0_STR;
> -    case PRIV_VERSION_1_12_0:
> -        return PRIV_VER_1_12_0_STR;
> -    case PRIV_VERSION_1_13_0:
> -        return PRIV_VER_1_13_0_STR;
> -    default:
> -        return NULL;
> -    }
> -}
> +const QEnumLookup priv_spec_lookup = {
> +    .array = priv_spec_str,
> +    .size = ARRAY_SIZE(priv_spec_str),
> +};
>   
>   static void prop_priv_spec_set(Object *obj, Visitor *v, const char *name,
>                                  void *opaque, Error **errp)
>   {
>       RISCVCPU *cpu = RISCV_CPU(obj);
> -    g_autofree char *value = NULL;
> -    int priv_version = -1;
> -
> -    visit_type_str(v, name, &value, errp);
> +    int priv_version;
>   
> -    priv_version = priv_spec_from_str(value);
> -    if (priv_version < 0) {
> -        error_setg(errp, "Unsupported privilege spec version '%s'", value);
> +    if (!visit_type_enum(v, name, &priv_version, &priv_spec_lookup, errp)) {
>           return;
>       }
>   
> @@ -1718,15 +1693,15 @@ static void prop_priv_spec_get(Object *obj, Visitor *v, const char *name,
>                                  void *opaque, Error **errp)
>   {
>       RISCVCPU *cpu = RISCV_CPU(obj);
> -    const char *value = priv_spec_to_str(cpu->env.priv_ver);
> +    int value = cpu->env.priv_ver;
>   
> -    visit_type_str(v, name, (char **)&value, errp);
> +    visit_type_enum(v, name, &value, &priv_spec_lookup, errp);
>   }
>   
>   static const PropertyInfo prop_priv_spec = {
> -    .type = "str",
> +    .type = "RISCVPrivSpec",
>       .description = "priv_spec",
> -    /* FIXME enum? */
> +    .enum_table = &priv_spec_lookup,
>       .get = prop_priv_spec_get,
>       .set = prop_priv_spec_set,
>   };
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index fae839cade..522b63016f 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -70,6 +70,7 @@ typedef struct CPUArchState CPURISCVState;
>   #define RVG RV('G')
>   #define RVB RV('B')
>   
> +extern const QEnumLookup priv_spec_lookup;
>   extern const uint32_t misa_bits[];
>   const char *riscv_get_misa_ext_name(uint32_t bit);
>   const char *riscv_get_misa_ext_description(uint32_t bit);
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 02c98cc2db..2ec4b8e98e 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -27,6 +27,7 @@
>   #include "time_helper.h"
>   #include "qapi/error.h"
>   #include "qapi/visitor.h"
> +#include "qapi/util.h"
>   #include "qemu/accel.h"
>   #include "qemu/error-report.h"
>   #include "qemu/log.h"
> @@ -86,7 +87,7 @@ static void riscv_cpu_write_misa_bit(RISCVCPU *cpu, uint32_t bit,
>   
>   static const char *cpu_priv_ver_to_str(int priv_ver)
>   {
> -    const char *priv_spec_str = priv_spec_to_str(priv_ver);
> +    const char *priv_spec_str = qapi_enum_lookup(&priv_spec_lookup, priv_ver);
>   
>       g_assert(priv_spec_str);
>
Re: [PATCH] target/riscv: Convert priv_spec property from string to enum
Posted by Daniel Henrique Barboza 1 week, 5 days ago
Ccing Drew Jones

On 5/17/2026 4:28 PM, khaled saleh wrote:
> The priv_spec property accepts a fixed set of values (v1.10.0, v1.11.0, v1.12.0, v1.13.0) but was implemented as a string type with manual string-to-enum conversion in custom getter/setter functions.
> 
> Convert it to use QEnumLookup with visit_type_enum() for:
> - Automatic input validation by the visitor framework
> - QMP introspection support (valid values are discoverable)
> - Reduced boilerplate (priv_spec_from_str/to_str removed)
> 
> This resolves the "FIXME enum?" comment in cpu.c.
> 
> Signed-off-by: khaled saleh <khaled.saleh.req@gmail.com>
> ---

This would be an improvement from what we have but I'm not sure if 'enum'
is what we want either.  We tend to go towards on/off flags that are more
QMP friendly.  So instead of enum property called 'priv_spec' we would have
booleans like "priv_ver_1_11 = on/off" and etc.

And I believe we can't just get rid of the existing string properties all of
a sudden too.  The string option would need to go through a deprecation
cycle first, and I'm not even sure if we can re-use the same property name
with 'enum' instead of string (my guess is we can't).  So it seems to me that
the 'priv_ver' property is basically doomed to be a string type until it goes
away.


Thanks,
Daniel

>   target/riscv/cpu.c         | 59 +++++++++++---------------------------
>   target/riscv/cpu.h         |  1 +
>   target/riscv/tcg/tcg-cpu.c |  3 +-
>   3 files changed, 20 insertions(+), 43 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 506a018d52..8365471572 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -26,6 +26,7 @@
>   #include "internals.h"
>   #include "qapi/error.h"
>   #include "qapi/visitor.h"
> +#include "qapi/util.h"
>   #include "qemu/error-report.h"
>   #include "hw/core/qdev-properties.h"
>   #include "hw/core/qdev-prop-internal.h"
> @@ -1655,51 +1656,25 @@ static const PropertyInfo prop_pmp_granularity = {
>       .set = prop_pmp_granularity_set,
>   };
>   
> -static int priv_spec_from_str(const char *priv_spec_str)
> -{
> -    int priv_version = -1;
> -
> -    if (!g_strcmp0(priv_spec_str, PRIV_VER_1_13_0_STR)) {
> -        priv_version = PRIV_VERSION_1_13_0;
> -    } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_12_0_STR)) {
> -        priv_version = PRIV_VERSION_1_12_0;
> -    } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_11_0_STR)) {
> -        priv_version = PRIV_VERSION_1_11_0;
> -    } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_10_0_STR)) {
> -        priv_version = PRIV_VERSION_1_10_0;
> -    }
> -
> -    return priv_version;
> -}
> +static const char *const priv_spec_str[] = {
> +    [PRIV_VERSION_1_10_0] = PRIV_VER_1_10_0_STR,
> +    [PRIV_VERSION_1_11_0] = PRIV_VER_1_11_0_STR,
> +    [PRIV_VERSION_1_12_0] = PRIV_VER_1_12_0_STR,
> +    [PRIV_VERSION_1_13_0] = PRIV_VER_1_13_0_STR,
> +};
>   
> -const char *priv_spec_to_str(int priv_version)
> -{
> -    switch (priv_version) {
> -    case PRIV_VERSION_1_10_0:
> -        return PRIV_VER_1_10_0_STR;
> -    case PRIV_VERSION_1_11_0:
> -        return PRIV_VER_1_11_0_STR;
> -    case PRIV_VERSION_1_12_0:
> -        return PRIV_VER_1_12_0_STR;
> -    case PRIV_VERSION_1_13_0:
> -        return PRIV_VER_1_13_0_STR;
> -    default:
> -        return NULL;
> -    }
> -}
> +const QEnumLookup priv_spec_lookup = {
> +    .array = priv_spec_str,
> +    .size = ARRAY_SIZE(priv_spec_str),
> +};
>   
>   static void prop_priv_spec_set(Object *obj, Visitor *v, const char *name,
>                                  void *opaque, Error **errp)
>   {
>       RISCVCPU *cpu = RISCV_CPU(obj);
> -    g_autofree char *value = NULL;
> -    int priv_version = -1;
> -
> -    visit_type_str(v, name, &value, errp);
> +    int priv_version;
>   
> -    priv_version = priv_spec_from_str(value);
> -    if (priv_version < 0) {
> -        error_setg(errp, "Unsupported privilege spec version '%s'", value);
> +    if (!visit_type_enum(v, name, &priv_version, &priv_spec_lookup, errp)) {
>           return;
>       }
>   
> @@ -1718,15 +1693,15 @@ static void prop_priv_spec_get(Object *obj, Visitor *v, const char *name,
>                                  void *opaque, Error **errp)
>   {
>       RISCVCPU *cpu = RISCV_CPU(obj);
> -    const char *value = priv_spec_to_str(cpu->env.priv_ver);
> +    int value = cpu->env.priv_ver;
>   
> -    visit_type_str(v, name, (char **)&value, errp);
> +    visit_type_enum(v, name, &value, &priv_spec_lookup, errp);
>   }
>   
>   static const PropertyInfo prop_priv_spec = {
> -    .type = "str",
> +    .type = "RISCVPrivSpec",
>       .description = "priv_spec",
> -    /* FIXME enum? */
> +    .enum_table = &priv_spec_lookup,
>       .get = prop_priv_spec_get,
>       .set = prop_priv_spec_set,
>   };
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index fae839cade..522b63016f 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -70,6 +70,7 @@ typedef struct CPUArchState CPURISCVState;
>   #define RVG RV('G')
>   #define RVB RV('B')
>   
> +extern const QEnumLookup priv_spec_lookup;
>   extern const uint32_t misa_bits[];
>   const char *riscv_get_misa_ext_name(uint32_t bit);
>   const char *riscv_get_misa_ext_description(uint32_t bit);
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 02c98cc2db..2ec4b8e98e 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -27,6 +27,7 @@
>   #include "time_helper.h"
>   #include "qapi/error.h"
>   #include "qapi/visitor.h"
> +#include "qapi/util.h"
>   #include "qemu/accel.h"
>   #include "qemu/error-report.h"
>   #include "qemu/log.h"
> @@ -86,7 +87,7 @@ static void riscv_cpu_write_misa_bit(RISCVCPU *cpu, uint32_t bit,
>   
>   static const char *cpu_priv_ver_to_str(int priv_ver)
>   {
> -    const char *priv_spec_str = priv_spec_to_str(priv_ver);
> +    const char *priv_spec_str = qapi_enum_lookup(&priv_spec_lookup, priv_ver);
>   
>       g_assert(priv_spec_str);
>
Re: [PATCH] target/riscv: Convert priv_spec property from string to enum
Posted by Markus Armbruster 1 week, 5 days ago
Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com> writes:

> Ccing Drew Jones
>
> On 5/17/2026 4:28 PM, khaled saleh wrote:
>> The priv_spec property accepts a fixed set of values (v1.10.0, v1.11.0, v1.12.0, v1.13.0) but was implemented as a string type with manual string-to-enum conversion in custom getter/setter functions.
>>
>> Convert it to use QEnumLookup with visit_type_enum() for:
>> - Automatic input validation by the visitor framework
>> - QMP introspection support (valid values are discoverable)
>> - Reduced boilerplate (priv_spec_from_str/to_str removed)
>>
>> This resolves the "FIXME enum?" comment in cpu.c.
>>
>> Signed-off-by: khaled saleh <khaled.saleh.req@gmail.com>
>> ---
>
> This would be an improvement from what we have but I'm not sure if 'enum'
> is what we want either.  We tend to go towards on/off flags that are more
> QMP friendly.  So instead of enum property called 'priv_spec' we would have
> booleans like "priv_ver_1_11 = on/off" and etc.
>
> And I believe we can't just get rid of the existing string properties all of
> a sudden too.  The string option would need to go through a deprecation
> cycle first, and I'm not even sure if we can re-use the same property name
> with 'enum' instead of string (my guess is we can't).  So it seems to me that
> the 'priv_ver' property is basically doomed to be a string type until it goes
> away.

Enums are strings on the wire.  Changing a QMP command argument or QOM
property that accepts only a finite set of values from string to enum
with the same set of values is compatible.

In the case of QMP, introspection with query-qmp-schema gets more
precise, which could conveivably confuse management applications.
Non-issue here, I believe.
Re: [PATCH] target/riscv: Convert priv_spec property from string to enum
Posted by Daniel Henrique Barboza 1 week, 5 days ago

On 5/18/2026 9:17 AM, Markus Armbruster wrote:
> Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com> writes:
> 
>> Ccing Drew Jones
>>
>> On 5/17/2026 4:28 PM, khaled saleh wrote:
>>> The priv_spec property accepts a fixed set of values (v1.10.0, v1.11.0, v1.12.0, v1.13.0) but was implemented as a string type with manual string-to-enum conversion in custom getter/setter functions.
>>>
>>> Convert it to use QEnumLookup with visit_type_enum() for:
>>> - Automatic input validation by the visitor framework
>>> - QMP introspection support (valid values are discoverable)
>>> - Reduced boilerplate (priv_spec_from_str/to_str removed)
>>>
>>> This resolves the "FIXME enum?" comment in cpu.c.
>>>
>>> Signed-off-by: khaled saleh <khaled.saleh.req@gmail.com>
>>> ---
>>
>> This would be an improvement from what we have but I'm not sure if 'enum'
>> is what we want either.  We tend to go towards on/off flags that are more
>> QMP friendly.  So instead of enum property called 'priv_spec' we would have
>> booleans like "priv_ver_1_11 = on/off" and etc.
>>
>> And I believe we can't just get rid of the existing string properties all of
>> a sudden too.  The string option would need to go through a deprecation
>> cycle first, and I'm not even sure if we can re-use the same property name
>> with 'enum' instead of string (my guess is we can't).  So it seems to me that
>> the 'priv_ver' property is basically doomed to be a string type until it goes
>> away.
> 
> Enums are strings on the wire.  Changing a QMP command argument or QOM
> property that accepts only a finite set of values from string to enum
> with the same set of values is compatible.
> 
> In the case of QMP, introspection with query-qmp-schema gets more
> precise, which could conveivably confuse management applications.
> Non-issue here, I believe.

Thanks for the QMP/QOM infos!

If QMP doesn't care that much, and in a closer look seems like users won't
be impacted (since it'll be the same strings used as input), I believe my
initial reservations are uncalled for and it is ok to go ahead with this
change.


Thanks,
Daniel


>
Re: [PATCH] target/riscv: Convert priv_spec property from string to enum
Posted by Philippe Mathieu-Daudé 1 week, 5 days ago
Cc'ing QAPI maintainer

On 17/5/26 21:28, khaled saleh wrote:
> The priv_spec property accepts a fixed set of values (v1.10.0, v1.11.0, v1.12.0, v1.13.0) but was implemented as a string type with manual string-to-enum conversion in custom getter/setter functions.
> 
> Convert it to use QEnumLookup with visit_type_enum() for:
> - Automatic input validation by the visitor framework
> - QMP introspection support (valid values are discoverable)
> - Reduced boilerplate (priv_spec_from_str/to_str removed)
> 
> This resolves the "FIXME enum?" comment in cpu.c.
> 
> Signed-off-by: khaled saleh <khaled.saleh.req@gmail.com>
> ---
>   target/riscv/cpu.c         | 59 +++++++++++---------------------------
>   target/riscv/cpu.h         |  1 +
>   target/riscv/tcg/tcg-cpu.c |  3 +-
>   3 files changed, 20 insertions(+), 43 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 506a018d52..8365471572 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -26,6 +26,7 @@
>   #include "internals.h"
>   #include "qapi/error.h"
>   #include "qapi/visitor.h"
> +#include "qapi/util.h"
>   #include "qemu/error-report.h"
>   #include "hw/core/qdev-properties.h"
>   #include "hw/core/qdev-prop-internal.h"
> @@ -1655,51 +1656,25 @@ static const PropertyInfo prop_pmp_granularity = {
>       .set = prop_pmp_granularity_set,
>   };
>   
> -static int priv_spec_from_str(const char *priv_spec_str)
> -{
> -    int priv_version = -1;
> -
> -    if (!g_strcmp0(priv_spec_str, PRIV_VER_1_13_0_STR)) {
> -        priv_version = PRIV_VERSION_1_13_0;
> -    } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_12_0_STR)) {
> -        priv_version = PRIV_VERSION_1_12_0;
> -    } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_11_0_STR)) {
> -        priv_version = PRIV_VERSION_1_11_0;
> -    } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_10_0_STR)) {
> -        priv_version = PRIV_VERSION_1_10_0;
> -    }
> -
> -    return priv_version;
> -}
> +static const char *const priv_spec_str[] = {
> +    [PRIV_VERSION_1_10_0] = PRIV_VER_1_10_0_STR,
> +    [PRIV_VERSION_1_11_0] = PRIV_VER_1_11_0_STR,
> +    [PRIV_VERSION_1_12_0] = PRIV_VER_1_12_0_STR,
> +    [PRIV_VERSION_1_13_0] = PRIV_VER_1_13_0_STR,
> +};
>   
> -const char *priv_spec_to_str(int priv_version)
> -{
> -    switch (priv_version) {
> -    case PRIV_VERSION_1_10_0:
> -        return PRIV_VER_1_10_0_STR;
> -    case PRIV_VERSION_1_11_0:
> -        return PRIV_VER_1_11_0_STR;
> -    case PRIV_VERSION_1_12_0:
> -        return PRIV_VER_1_12_0_STR;
> -    case PRIV_VERSION_1_13_0:
> -        return PRIV_VER_1_13_0_STR;
> -    default:
> -        return NULL;
> -    }
> -}
> +const QEnumLookup priv_spec_lookup = {
> +    .array = priv_spec_str,
> +    .size = ARRAY_SIZE(priv_spec_str),
> +};
>   
>   static void prop_priv_spec_set(Object *obj, Visitor *v, const char *name,
>                                  void *opaque, Error **errp)
>   {
>       RISCVCPU *cpu = RISCV_CPU(obj);
> -    g_autofree char *value = NULL;
> -    int priv_version = -1;
> -
> -    visit_type_str(v, name, &value, errp);
> +    int priv_version;
>   
> -    priv_version = priv_spec_from_str(value);
> -    if (priv_version < 0) {
> -        error_setg(errp, "Unsupported privilege spec version '%s'", value);
> +    if (!visit_type_enum(v, name, &priv_version, &priv_spec_lookup, errp)) {
>           return;
>       }
>   
> @@ -1718,15 +1693,15 @@ static void prop_priv_spec_get(Object *obj, Visitor *v, const char *name,
>                                  void *opaque, Error **errp)
>   {
>       RISCVCPU *cpu = RISCV_CPU(obj);
> -    const char *value = priv_spec_to_str(cpu->env.priv_ver);
> +    int value = cpu->env.priv_ver;
>   
> -    visit_type_str(v, name, (char **)&value, errp);
> +    visit_type_enum(v, name, &value, &priv_spec_lookup, errp);
>   }
>   
>   static const PropertyInfo prop_priv_spec = {
> -    .type = "str",
> +    .type = "RISCVPrivSpec",
>       .description = "priv_spec",
> -    /* FIXME enum? */
> +    .enum_table = &priv_spec_lookup,
>       .get = prop_priv_spec_get,
>       .set = prop_priv_spec_set,
>   };
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index fae839cade..522b63016f 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -70,6 +70,7 @@ typedef struct CPUArchState CPURISCVState;
>   #define RVG RV('G')
>   #define RVB RV('B')
>   
> +extern const QEnumLookup priv_spec_lookup;
>   extern const uint32_t misa_bits[];
>   const char *riscv_get_misa_ext_name(uint32_t bit);
>   const char *riscv_get_misa_ext_description(uint32_t bit);
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 02c98cc2db..2ec4b8e98e 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -27,6 +27,7 @@
>   #include "time_helper.h"
>   #include "qapi/error.h"
>   #include "qapi/visitor.h"
> +#include "qapi/util.h"
>   #include "qemu/accel.h"
>   #include "qemu/error-report.h"
>   #include "qemu/log.h"
> @@ -86,7 +87,7 @@ static void riscv_cpu_write_misa_bit(RISCVCPU *cpu, uint32_t bit,
>   
>   static const char *cpu_priv_ver_to_str(int priv_ver)
>   {
> -    const char *priv_spec_str = priv_spec_to_str(priv_ver);
> +    const char *priv_spec_str = qapi_enum_lookup(&priv_spec_lookup, priv_ver);
>   
>       g_assert(priv_spec_str);
>