[11/20] target/riscv: introduce KVM AccelCPUClass

Add a KVM accelerator class like we did with TCG. The difference is
that, at least for now, we won't be using a realize() implementation for
this accelerator.

We'll start by assiging kvm_riscv_cpu_add_kvm_properties(), renamed to
kvm_cpu_instance_init(), as a 'cpu_instance_init' implementation. Change
riscv_cpu_post_init() to invoke accel_cpu_instance_init(), which will go
through the 'cpu_instance_init' impl of the current acceleration (if
available) and execute it. The end result is that the KVM initial setup,
i.e. starting registers and adding its specific properties, will be done
via this hook.

riscv_cpu_add_user_properties() is still being called via the common
post_init() function, thus we still need the "if kvm then return" logic
inside it for now. We'll deal with it when TCG accel class get its own
'cpu_instance_init' implementation.

riscv_add_satp_mode_properties() is now being exported from cpu.c since
it's a common helper between KVM and TCG.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c       |  8 ++---
 target/riscv/cpu.h       |  1 +
 target/riscv/kvm.c       | 64 +++++++++++++++++++++++++++-------------
 target/riscv/kvm_riscv.h |  1 -
 4 files changed, 49 insertions(+), 25 deletions(-)

On 25/8/23 15:08, Daniel Henrique Barboza wrote:
> Add a KVM accelerator class like we did with TCG. The difference is
> that, at least for now, we won't be using a realize() implementation for
> this accelerator.
> 
> We'll start by assiging kvm_riscv_cpu_add_kvm_properties(), renamed to
> kvm_cpu_instance_init(), as a 'cpu_instance_init' implementation. Change
> riscv_cpu_post_init() to invoke accel_cpu_instance_init(), which will go
> through the 'cpu_instance_init' impl of the current acceleration (if
> available) and execute it. The end result is that the KVM initial setup,
> i.e. starting registers and adding its specific properties, will be done
> via this hook.
> 
> riscv_cpu_add_user_properties() is still being called via the common
> post_init() function, thus we still need the "if kvm then return" logic
> inside it for now. We'll deal with it when TCG accel class get its own
> 'cpu_instance_init' implementation.
> 
> riscv_add_satp_mode_properties() is now being exported from cpu.c since
> it's a common helper between KVM and TCG.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.c       |  8 ++---
>   target/riscv/cpu.h       |  1 +
>   target/riscv/kvm.c       | 64 +++++++++++++++++++++++++++-------------
>   target/riscv/kvm_riscv.h |  1 -
>   4 files changed, 49 insertions(+), 25 deletions(-)


> -static void riscv_add_satp_mode_properties(Object *obj)
> +void riscv_add_satp_mode_properties(Object *obj)
>   {
>       RISCVCPU *cpu = RISCV_CPU(obj);
>   
> @@ -1199,6 +1199,8 @@ static void riscv_cpu_post_init(Object *obj)
>       RISCVCPU *cpu = RISCV_CPU(obj);
>       RISCVCPUClass *rcc = RISCV_CPU_GET_CLASS(cpu);
>   
> +    accel_cpu_instance_init(CPU(obj));
> +
>       if (rcc->user_extension_properties) {
>           riscv_cpu_add_user_properties(obj);
>       }
> @@ -1561,12 +1563,10 @@ static void riscv_cpu_add_multiext_prop_array(Object *obj,
>   static void riscv_cpu_add_user_properties(Object *obj)
>   {
>   #ifndef CONFIG_USER_ONLY
> -    riscv_add_satp_mode_properties(obj);
> -
>       if (kvm_enabled()) {
> -        kvm_riscv_cpu_add_kvm_properties(obj);
>           return;
>       }

Maybe in a preliminary patch:

if (tcg_enabled()) {

> +    riscv_add_satp_mode_properties(obj);

}

then remove the 'if kvm_enabled' in this patch?

>   #endif

On 8/28/23 13:38, Philippe Mathieu-Daudé wrote:
> On 25/8/23 15:08, Daniel Henrique Barboza wrote:
>> Add a KVM accelerator class like we did with TCG. The difference is
>> that, at least for now, we won't be using a realize() implementation for
>> this accelerator.
>>
>> We'll start by assiging kvm_riscv_cpu_add_kvm_properties(), renamed to
>> kvm_cpu_instance_init(), as a 'cpu_instance_init' implementation. Change
>> riscv_cpu_post_init() to invoke accel_cpu_instance_init(), which will go
>> through the 'cpu_instance_init' impl of the current acceleration (if
>> available) and execute it. The end result is that the KVM initial setup,
>> i.e. starting registers and adding its specific properties, will be done
>> via this hook.
>>
>> riscv_cpu_add_user_properties() is still being called via the common
>> post_init() function, thus we still need the "if kvm then return" logic
>> inside it for now. We'll deal with it when TCG accel class get its own
>> 'cpu_instance_init' implementation.
>>
>> riscv_add_satp_mode_properties() is now being exported from cpu.c since
>> it's a common helper between KVM and TCG.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c       |  8 ++---
>>   target/riscv/cpu.h       |  1 +
>>   target/riscv/kvm.c       | 64 +++++++++++++++++++++++++++-------------
>>   target/riscv/kvm_riscv.h |  1 -
>>   4 files changed, 49 insertions(+), 25 deletions(-)
> 
> 
>> -static void riscv_add_satp_mode_properties(Object *obj)
>> +void riscv_add_satp_mode_properties(Object *obj)
>>   {
>>       RISCVCPU *cpu = RISCV_CPU(obj);
>> @@ -1199,6 +1199,8 @@ static void riscv_cpu_post_init(Object *obj)
>>       RISCVCPU *cpu = RISCV_CPU(obj);
>>       RISCVCPUClass *rcc = RISCV_CPU_GET_CLASS(cpu);
>> +    accel_cpu_instance_init(CPU(obj));
>> +
>>       if (rcc->user_extension_properties) {
>>           riscv_cpu_add_user_properties(obj);
>>       }
>> @@ -1561,12 +1563,10 @@ static void riscv_cpu_add_multiext_prop_array(Object *obj,
>>   static void riscv_cpu_add_user_properties(Object *obj)
>>   {
>>   #ifndef CONFIG_USER_ONLY
>> -    riscv_add_satp_mode_properties(obj);
>> -
>>       if (kvm_enabled()) {
>> -        kvm_riscv_cpu_add_kvm_properties(obj);
>>           return;
>>       }
> 
> Maybe in a preliminary patch:
> 
> if (tcg_enabled()) {
> 
>> +    riscv_add_satp_mode_properties(obj);
> 
> }
> 
> then remove the 'if kvm_enabled' in this patch?

I'll add a preliminary patch to make kvm_riscv_cpu_add_kvm_properties() call
riscv_add_satp_mode_properties() and change the order they're being called here
(i.e. call kvm_riscv_cpu_add_kvm_properties() first). Then this patch can safely
remove the 'if kvm_enabled' block as you suggested.


Thanks,

Daniel


> 
>>   #endif
>

On Fri, Aug 25, 2023 at 10:08:44AM -0300, Daniel Henrique Barboza wrote:
> Add a KVM accelerator class like we did with TCG. The difference is
> that, at least for now, we won't be using a realize() implementation for
> this accelerator.
> 
> We'll start by assiging kvm_riscv_cpu_add_kvm_properties(), renamed to
> kvm_cpu_instance_init(), as a 'cpu_instance_init' implementation. Change
> riscv_cpu_post_init() to invoke accel_cpu_instance_init(), which will go
> through the 'cpu_instance_init' impl of the current acceleration (if
> available) and execute it. The end result is that the KVM initial setup,
> i.e. starting registers and adding its specific properties, will be done
> via this hook.
> 
> riscv_cpu_add_user_properties() is still being called via the common
> post_init() function, thus we still need the "if kvm then return" logic
> inside it for now. We'll deal with it when TCG accel class get its own
> 'cpu_instance_init' implementation.
> 
> riscv_add_satp_mode_properties() is now being exported from cpu.c since
> it's a common helper between KVM and TCG.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c       |  8 ++---
>  target/riscv/cpu.h       |  1 +
>  target/riscv/kvm.c       | 64 +++++++++++++++++++++++++++-------------
>  target/riscv/kvm_riscv.h |  1 -
>  4 files changed, 49 insertions(+), 25 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 58b0ef2af8..04c6bfaeef 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1112,7 +1112,7 @@ static void cpu_riscv_set_satp(Object *obj, Visitor *v, const char *name,
>      satp_map->init |= 1 << satp;
>  }
>  
> -static void riscv_add_satp_mode_properties(Object *obj)
> +void riscv_add_satp_mode_properties(Object *obj)
>  {
>      RISCVCPU *cpu = RISCV_CPU(obj);
>  
> @@ -1199,6 +1199,8 @@ static void riscv_cpu_post_init(Object *obj)
>      RISCVCPU *cpu = RISCV_CPU(obj);
>      RISCVCPUClass *rcc = RISCV_CPU_GET_CLASS(cpu);
>  
> +    accel_cpu_instance_init(CPU(obj));
> +
>      if (rcc->user_extension_properties) {
>          riscv_cpu_add_user_properties(obj);
>      }
> @@ -1561,12 +1563,10 @@ static void riscv_cpu_add_multiext_prop_array(Object *obj,
>  static void riscv_cpu_add_user_properties(Object *obj)
>  {
>  #ifndef CONFIG_USER_ONLY
> -    riscv_add_satp_mode_properties(obj);
> -
>      if (kvm_enabled()) {
> -        kvm_riscv_cpu_add_kvm_properties(obj);
>          return;
>      }
> +    riscv_add_satp_mode_properties(obj);
>  #endif
>  
>      riscv_cpu_add_misa_properties(obj);
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index b9c4bea3f7..950c2301f2 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -726,6 +726,7 @@ extern const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[];
>  extern Property riscv_cpu_options[];
>  
>  void riscv_cpu_add_misa_properties(Object *cpu_obj);
> +void riscv_add_satp_mode_properties(Object *obj);
>  
>  /* CSR function table */
>  extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 7e67121456..3c4fa43cee 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -31,6 +31,7 @@
>  #include "sysemu/kvm_int.h"
>  #include "cpu.h"
>  #include "trace.h"
> +#include "hw/core/accel-cpu.h"
>  #include "hw/pci/pci.h"
>  #include "exec/memattrs.h"
>  #include "exec/address-spaces.h"
> @@ -1262,26 +1263,6 @@ void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
>      kvm_msi_via_irqfd_allowed = kvm_irqfds_enabled();
>  }
>  
> -void kvm_riscv_cpu_add_kvm_properties(Object *obj)
> -{
> -    DeviceState *dev = DEVICE(obj);
> -
> -    riscv_init_user_properties(obj);
> -    riscv_cpu_add_misa_properties(obj);
> -
> -    riscv_cpu_add_kvm_unavail_prop_array(obj, riscv_cpu_extensions);
> -    riscv_cpu_add_kvm_unavail_prop_array(obj, riscv_cpu_vendor_exts);
> -    riscv_cpu_add_kvm_unavail_prop_array(obj, riscv_cpu_experimental_exts);
> -
> -    for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) {
> -        /* Check if KVM created the property already */
> -        if (object_property_find(obj, prop->name)) {
> -            continue;
> -        }
> -        qdev_property_add_static(dev, prop);
> -    }
> -}
> -
>  static void riscv_host_cpu_init(Object *obj)
>  {
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> @@ -1310,3 +1291,46 @@ static const TypeInfo riscv_kvm_cpu_type_infos[] = {
>  };
>  
>  DEFINE_TYPES(riscv_kvm_cpu_type_infos)
> +
> +static void kvm_cpu_instance_init(CPUState *cs)
> +{
> +    Object *obj = OBJECT(RISCV_CPU(cs));
> +    DeviceState *dev = DEVICE(obj);
> +
> +    riscv_init_user_properties(obj);
> +
> +    riscv_add_satp_mode_properties(obj);
> +    riscv_cpu_add_misa_properties(obj);
> +
> +    riscv_cpu_add_kvm_unavail_prop_array(obj, riscv_cpu_extensions);
> +    riscv_cpu_add_kvm_unavail_prop_array(obj, riscv_cpu_vendor_exts);
> +    riscv_cpu_add_kvm_unavail_prop_array(obj, riscv_cpu_experimental_exts);
> +
> +    for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) {
> +        /* Check if we have a specific KVM handler for the option */
> +        if (object_property_find(obj, prop->name)) {
> +            continue;
> +        }
> +        qdev_property_add_static(dev, prop);
> +    }
> +}
> +
> +static void kvm_cpu_accel_class_init(ObjectClass *oc, void *data)
> +{
> +    AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
> +
> +    acc->cpu_instance_init = kvm_cpu_instance_init;
> +}
> +
> +static const TypeInfo kvm_cpu_accel_type_info = {
> +    .name = ACCEL_CPU_NAME("kvm"),
> +
> +    .parent = TYPE_ACCEL_CPU,
> +    .class_init = kvm_cpu_accel_class_init,
> +    .abstract = true,
> +};

blank line here

> +static void kvm_cpu_accel_register_types(void)
> +{
> +    type_register_static(&kvm_cpu_accel_type_info);
> +}
> +type_init(kvm_cpu_accel_register_types);
> diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
> index 81e08b8359..a0ea1a7505 100644
> --- a/target/riscv/kvm_riscv.h
> +++ b/target/riscv/kvm_riscv.h
> @@ -19,7 +19,6 @@
>  #ifndef QEMU_KVM_RISCV_H
>  #define QEMU_KVM_RISCV_H
>  
> -void kvm_riscv_cpu_add_kvm_properties(Object *obj);
>  void kvm_riscv_reset_vcpu(RISCVCPU *cpu);
>  void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level);
>  void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
> -- 
> 2.41.0
> 
>

I'll wait for v2 to complete the review of this one.

Thanks,
drew