[PULL,48/65] target/riscv/cpu.c: split kvm prop handling to its own helper

Commit Message

Alistair Francis Sept. 8, 2023, 6:04 a.m. UTC

From: Daniel Henrique Barboza <[e-mail]>

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

Signed-off-by: Daniel Henrique Barboza <[e-mail]>
Reviewed-by: Weiwei Li <[e-mail]>
Reviewed-by: Alistair Francis <[e-mail]>
Reviewed-by: Andrew Jones <[e-mail]>
Message-ID: <[e-mail]>
Signed-off-by: Alistair Francis <[e-mail]>
---
 target/riscv/cpu.c | 65 ++++++++++++++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 23 deletions(-)

Comments

Philippe Mathieu-Daudé Sept. 8, 2023, 1:21 p.m. UTC | #1

On 8/9/23 08:04, Alistair Francis wrote:
> From: Daniel Henrique Barboza <[e-mail]>
> 
> 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 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.
> 
> Signed-off-by: Daniel Henrique Barboza <[e-mail]>
> Reviewed-by: Weiwei Li <[e-mail]>
> Reviewed-by: Alistair Francis <[e-mail]>
> Reviewed-by: Andrew Jones <[e-mail]>
> Message-ID: <[e-mail]>
> Signed-off-by: Alistair Francis <[e-mail]>
> ---
>   target/riscv/cpu.c | 65 ++++++++++++++++++++++++++++++----------------
>   1 file changed, 42 insertions(+), 23 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index db640e7460..8e6d316500 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1943,6 +1943,46 @@ static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
>   }
>   #endif
>   
> +#ifndef CONFIG_USER_ONLY

#ifdef CONFIG_KVM ?

> +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.
>    *
> @@ -1958,39 +1998,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]);
>       }
>   }

Daniel Henrique Barboza Sept. 10, 2023, 8:58 a.m. UTC | #2

On 9/8/23 10:21, Philippe Mathieu-Daudé wrote:
> On 8/9/23 08:04, Alistair Francis wrote:
>> From: Daniel Henrique Barboza <[e-mail]>
>>
>> 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 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.
>>
>> Signed-off-by: Daniel Henrique Barboza <[e-mail]>
>> Reviewed-by: Weiwei Li <[e-mail]>
>> Reviewed-by: Alistair Francis <[e-mail]>
>> Reviewed-by: Andrew Jones <[e-mail]>
>> Message-ID: <[e-mail]>
>> Signed-off-by: Alistair Francis <[e-mail]>
>> ---
>>   target/riscv/cpu.c | 65 ++++++++++++++++++++++++++++++----------------
>>   1 file changed, 42 insertions(+), 23 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index db640e7460..8e6d316500 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1943,6 +1943,46 @@ static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
>>   }
>>   #endif
>> +#ifndef CONFIG_USER_ONLY
> 
> #ifdef CONFIG_KVM ?

Yeah, CONFIG_KVM would be more fitting here. We're moving all this code to kvm.c
in "[PATCH v2 00/19] riscv: split TCG/KVM accelerators from cpu.c", and we won't
need neither in the end, so for now it's simpler to temporarily leave it like
this.


Thanks,

Daniel

> 
>> +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.
>>    *
>> @@ -1958,39 +1998,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]);
>>       }
>>   }
>

Alistair Francis Sept. 11, 2023, 2:15 a.m. UTC | #3

On Sun, Sep 10, 2023 at 6:58 PM Daniel Henrique Barboza
<[e-mail]> wrote:
>
>
>
> On 9/8/23 10:21, Philippe Mathieu-Daudé wrote:
> > On 8/9/23 08:04, Alistair Francis wrote:
> >> From: Daniel Henrique Barboza <[e-mail]>
> >>
> >> 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 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.
> >>
> >> Signed-off-by: Daniel Henrique Barboza <[e-mail]>
> >> Reviewed-by: Weiwei Li <[e-mail]>
> >> Reviewed-by: Alistair Francis <[e-mail]>
> >> Reviewed-by: Andrew Jones <[e-mail]>
> >> Message-ID: <[e-mail]>
> >> Signed-off-by: Alistair Francis <[e-mail]>
> >> ---
> >>   target/riscv/cpu.c | 65 ++++++++++++++++++++++++++++++----------------
> >>   1 file changed, 42 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index db640e7460..8e6d316500 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -1943,6 +1943,46 @@ static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
> >>   }
> >>   #endif
> >> +#ifndef CONFIG_USER_ONLY
> >
> > #ifdef CONFIG_KVM ?
>
> Yeah, CONFIG_KVM would be more fitting here. We're moving all this code to kvm.c
> in "[PATCH v2 00/19] riscv: split TCG/KVM accelerators from cpu.c", and we won't
> need neither in the end, so for now it's simpler to temporarily leave it like
> this.

There are CI failures, so I have converted this to #ifdef CONFIG_KVM
which should fix them.

Alistair

>
>
> Thanks,
>
> Daniel
>
> >
> >> +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.
> >>    *
> >> @@ -1958,39 +1998,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]);
> >>       }
> >>   }
> >

Patch

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index db640e7460..8e6d316500 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1943,6 +1943,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.
  *
@@ -1958,39 +1998,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]);
     }
 }