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

[Andrew Jones]

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