diff mbox series

[v10,03/20] target/riscv/cpu.c: split kvm prop handling to its own helper

Message ID 20230912132423.268494-4-dbarboza@ventanamicro.com
State New
Headers show
Series riscv: 'max' CPU, detect user choice in TCG | expand

Commit Message

Daniel Henrique Barboza Sept. 12, 2023, 1:24 p.m. UTC 
Future patches will split the existing Property arrays even further, and
the existing code in riscv_cpu_add_user_properties() will start to scale
bad with it because it's dealing with KVM constraints mixed in with TCG
constraints. We're going to pay a high price to share a couple of common
lines of code between the two.

Create a new kvm_riscv_cpu_add_kvm_properties() helper 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.

The helper was declared in kvm_stubs.h, while being implemented in
cpu.c, to allow '--enable-debug' builds to work. The compiler won't
remove the kvm_riscv_cpu_add_kvm_properties() reference when
'kvm_enabled()' is false if we end up with an unused function. Even
though being a KVM only helper we can't implement it in kvm.c due to its
many dependencies inside cpu.c, so make it public in kvm_riscv.h and
keep its implementation in cpu.c for now. We'll move it to kvm.c in the
near future.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c       | 65 +++++++++++++++++++++++++---------------
 target/riscv/kvm_riscv.h |  3 ++
 2 files changed, 44 insertions(+), 24 deletions(-)

Comments

Andrew Jones Sept. 15, 2023, 12:04 p.m. UTC | #1 
On Tue, Sep 12, 2023 at 10:24:06AM -0300, Daniel Henrique Barboza wrote:
> Future patches will split the existing Property arrays even further, and
> the existing code in riscv_cpu_add_user_properties() will start to scale
> bad with it because it's dealing with KVM constraints mixed in with TCG
> constraints. We're going to pay a high price to share a couple of common
> lines of code between the two.
> 
> Create a new kvm_riscv_cpu_add_kvm_properties() helper 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.
> 
> The helper was declared in kvm_stubs.h, while being implemented in
> cpu.c, to allow '--enable-debug' builds to work. The compiler won't
> remove the kvm_riscv_cpu_add_kvm_properties() reference when
> 'kvm_enabled()' is false if we end up with an unused function. Even
> though being a KVM only helper we can't implement it in kvm.c due to its
> many dependencies inside cpu.c, so make it public in kvm_riscv.h and
> keep its implementation in cpu.c for now. We'll move it to kvm.c in the
> near future.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c       | 65 +++++++++++++++++++++++++---------------
>  target/riscv/kvm_riscv.h |  3 ++
>  2 files changed, 44 insertions(+), 24 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index db640e7460..7b7c5649e7 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1924,7 +1924,7 @@ static Property riscv_cpu_options[] = {
>      DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
>  };
>  
> -#ifndef CONFIG_USER_ONLY
> +#ifdef CONFIG_KVM
>  static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
>                                      const char *name,
>                                      void *opaque, Error **errp)
> @@ -1941,6 +1941,44 @@ static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
>                     propname);
>      }
>  }
> +
> +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);
> +}
> +
> +void kvm_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

IIUC, we're leaving kvm_riscv_cpu_add_kvm_properties() in cpu.c due to its
dependencies, such as riscv_cpu_add_misa_properties(), but I don't
understand why it needs to be public, since it's still only called from
this file. Also, to handle the clang debug build I'd guess we still need
a !CONFIG_KVM stub, so, in this file, I suggest we do something like,

#ifdef CONFIG_KVM
static void kvm_riscv_cpu_add_kvm_properties(obj)
{ ... }
#else
static void kvm_riscv_cpu_add_kvm_properties(obj) {}
#endif

But, all that said, I know this is all getting reworked in another
in-flight series, so I'm not real worried about exactly how it comes
together at the moment. Assuming we pass all compile tests like the
clang debug test, then

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew
diff mbox series

Patch

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index db640e7460..7b7c5649e7 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1924,7 +1924,7 @@  static Property riscv_cpu_options[] = {
     DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
 };
 
-#ifndef CONFIG_USER_ONLY
+#ifdef CONFIG_KVM
 static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
                                     const char *name,
                                     void *opaque, Error **errp)
@@ -1941,6 +1941,44 @@  static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
                    propname);
     }
 }
+
+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);
+}
+
+void kvm_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
 
 /*
@@ -1958,39 +1996,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);
+        kvm_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]);
     }
 }
diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
index de8c209ebc..69e807fbfb 100644
--- a/target/riscv/kvm_riscv.h
+++ b/target/riscv/kvm_riscv.h
@@ -19,6 +19,9 @@ 
 #ifndef QEMU_KVM_RISCV_H
 #define QEMU_KVM_RISCV_H
 
+/* Temporarily implemented in cpu.c */
+void kvm_riscv_cpu_add_kvm_properties(Object *obj);
+
 void kvm_riscv_init_user_properties(Object *cpu_obj);
 void kvm_riscv_reset_vcpu(RISCVCPU *cpu);
 void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level);