diff mbox series

[v2,10/16] target/riscv: create finalize_features() for KVM

Message ID 20231222122235.545235-11-dbarboza@ventanamicro.com
State New
Headers show
Series target/riscv: deprecate riscv_cpu_options[] | expand

Commit Message

Daniel Henrique Barboza Dec. 22, 2023, 12:22 p.m. UTC 
To turn cbom_blocksize and cboz_blocksize into class properties we need
KVM specific changes.

KVM is creating its own version of these options with a customized
setter() that prevents users from picking an invalid value during init()
time. This comes at the cost of duplicating each option that KVM
supports. This will keep happening for each new shared option KVM
implements in the future.

We can avoid that by using the same property TCG uses and adding
specific KVM handling during finalize() time, like TCG already does with
riscv_tcg_cpu_finalize_features(). To do that, the common CPU property
offers a way of knowing if an option was user set or not, sparing us
from doing unneeded syscalls.

riscv_kvm_cpu_finalize_features() is then created using the same
KVMScratch CPU we already use during init() time, since finalize() time
is still too early to use the official KVM CPU for it. cbom_blocksize
and cboz_blocksize are then handled during finalize() in the same way
they're handled by their KVM specific setter.

With this change we can proceed with the blocksize changes in the common
code without breaking the KVM driver.

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

Comments

Vladimir Isaev Dec. 29, 2023, 11:22 a.m. UTC | #1 
22.12.2023 15:22, Daniel Henrique Barboza wrote:
> To turn cbom_blocksize and cboz_blocksize into class properties we need
> KVM specific changes.
> 
> KVM is creating its own version of these options with a customized
> setter() that prevents users from picking an invalid value during init()
> time. This comes at the cost of duplicating each option that KVM
> supports. This will keep happening for each new shared option KVM
> implements in the future.
> 
> We can avoid that by using the same property TCG uses and adding
> specific KVM handling during finalize() time, like TCG already does with
> riscv_tcg_cpu_finalize_features(). To do that, the common CPU property
> offers a way of knowing if an option was user set or not, sparing us
> from doing unneeded syscalls.
> 
> riscv_kvm_cpu_finalize_features() is then created using the same
> KVMScratch CPU we already use during init() time, since finalize() time
> is still too early to use the official KVM CPU for it. cbom_blocksize
> and cboz_blocksize are then handled during finalize() in the same way
> they're handled by their KVM specific setter.
> 
> With this change we can proceed with the blocksize changes in the common
> code without breaking the KVM driver.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c           | 16 +++++++---
>  target/riscv/cpu.h           |  1 +
>  target/riscv/kvm/kvm-cpu.c   | 59 ++++++++++++++++++++++++++++++++++++
>  target/riscv/kvm/kvm_riscv.h |  1 +
>  4 files changed, 72 insertions(+), 5 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 8be619b6f1..f49d31d753 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -63,6 +63,11 @@ static void cpu_option_add_user_setting(const char *optname, uint32_t value)
>                          GUINT_TO_POINTER(value));
>  }
> 
> +bool riscv_cpu_option_set(const char *optname)
> +{
> +    return g_hash_table_contains(general_user_opts, optname);
> +}
> +

This function may work in unexpected way for future developer since we can check just somehow restricted
number of options using it, like vlen/elen/cbom/cboz size, but not vext_spec or pmu-num/mask.

>  #define ISA_EXT_DATA_ENTRY(_name, _min_ver, _prop) \
>      {#_name, _min_ver, CPU_CFG_OFFSET(_prop)}
> 
> @@ -1056,17 +1061,18 @@ void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
>  {
>      Error *local_err = NULL;
> 
> -    /*
> -     * KVM accel does not have a specialized finalize()
> -     * callback because its extensions are validated
> -     * in the get()/set() callbacks of each property.
> -     */
>      if (tcg_enabled()) {
>          riscv_tcg_cpu_finalize_features(cpu, &local_err);
>          if (local_err != NULL) {
>              error_propagate(errp, local_err);
>              return;
>          }
> +    } else if (kvm_enabled()) {
> +        riscv_kvm_cpu_finalize_features(cpu, &local_err);
> +        if (local_err != NULL) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
>      }
> 
>  #ifndef CONFIG_USER_ONLY
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 53101b82c5..988471c7ba 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -495,6 +495,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                          bool probe, uintptr_t retaddr);
>  char *riscv_isa_string(RISCVCPU *cpu);
>  void riscv_cpu_list(void);
> +bool riscv_cpu_option_set(const char *optname);
> 
>  #define cpu_list riscv_cpu_list
>  #define cpu_mmu_index riscv_cpu_mmu_index
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 62a1e51f0a..70fb075846 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1490,6 +1490,65 @@ static void kvm_cpu_instance_init(CPUState *cs)
>      }
>  }
> 
> +void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
> +{
> +    CPURISCVState *env = &cpu->env;
> +    KVMScratchCPU kvmcpu;
> +    struct kvm_one_reg reg;
> +    uint64_t val;
> +    int ret;
> +
> +    /* short-circuit without spinning the scratch CPU */
> +    if (!cpu->cfg.ext_zicbom && !cpu->cfg.ext_zicboz) {
> +        return;
> +    }
> +
> +    if (!kvm_riscv_create_scratch_vcpu(&kvmcpu)) {
> +        error_setg(errp, "Unable to create scratch KVM cpu");
> +        return;
> +    }
> +
> +    if (cpu->cfg.ext_zicbom &&
> +        riscv_cpu_option_set(kvm_cbom_blocksize.name)) {
> +
> +        reg.id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG,
> +                                        kvm_cbom_blocksize.kvm_reg_id);
> +        reg.addr = (uint64_t)&val;
> +        ret = ioctl(kvmcpu.cpufd, KVM_GET_ONE_REG, &reg);
> +        if (ret != 0) {
> +            error_setg(errp, "Unable to read cbom_blocksize, error %d", errno);
> +            return;
> +        }
> +
> +        if (cpu->cfg.cbom_blocksize != val) {
> +            error_setg(errp, "Unable to set cbom_blocksize to a different "
> +                       "value than the host (%lu)", val);
> +            return;
> +        }
> +    }
> +
> +    if (cpu->cfg.ext_zicboz &&
> +        riscv_cpu_option_set(kvm_cboz_blocksize.name)) {
> +
> +        reg.id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG,
> +                                        kvm_cboz_blocksize.kvm_reg_id);
> +        reg.addr = (uint64_t)&val;
> +        ret = ioctl(kvmcpu.cpufd, KVM_GET_ONE_REG, &reg);
> +        if (ret != 0) {
> +            error_setg(errp, "Unable to read cbom_blocksize, error %d", errno);
> +            return;
> +        }
> +
> +        if (cpu->cfg.cboz_blocksize != val) {
> +            error_setg(errp, "Unable to set cboz_blocksize to a different "
> +                       "value than the host (%lu)", val);
> +            return;
> +        }
> +    }
> +
> +    kvm_riscv_destroy_scratch_vcpu(&kvmcpu);
> +}
> +
>  static void kvm_cpu_accel_class_init(ObjectClass *oc, void *data)
>  {
>      AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
> diff --git a/target/riscv/kvm/kvm_riscv.h b/target/riscv/kvm/kvm_riscv.h
> index 8329cfab82..4bd98fddc7 100644
> --- a/target/riscv/kvm/kvm_riscv.h
> +++ b/target/riscv/kvm/kvm_riscv.h
> @@ -27,5 +27,6 @@ void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
>                            uint64_t guest_num);
>  void riscv_kvm_aplic_request(void *opaque, int irq, int level);
>  int kvm_riscv_sync_mpstate_to_kvm(RISCVCPU *cpu, int state);
> +void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp);
> 
>  #endif
> --
> 2.43.0
> 
>
Daniel Henrique Barboza Jan. 2, 2024, 12:29 p.m. UTC | #2 
On 12/29/23 08:22, Vladimir Isaev wrote:
> 22.12.2023 15:22, Daniel Henrique Barboza wrote:
>> To turn cbom_blocksize and cboz_blocksize into class properties we need
>> KVM specific changes.
>>
>> KVM is creating its own version of these options with a customized
>> setter() that prevents users from picking an invalid value during init()
>> time. This comes at the cost of duplicating each option that KVM
>> supports. This will keep happening for each new shared option KVM
>> implements in the future.
>>
>> We can avoid that by using the same property TCG uses and adding
>> specific KVM handling during finalize() time, like TCG already does with
>> riscv_tcg_cpu_finalize_features(). To do that, the common CPU property
>> offers a way of knowing if an option was user set or not, sparing us
>> from doing unneeded syscalls.
>>
>> riscv_kvm_cpu_finalize_features() is then created using the same
>> KVMScratch CPU we already use during init() time, since finalize() time
>> is still too early to use the official KVM CPU for it. cbom_blocksize
>> and cboz_blocksize are then handled during finalize() in the same way
>> they're handled by their KVM specific setter.
>>
>> With this change we can proceed with the blocksize changes in the common
>> code without breaking the KVM driver.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c           | 16 +++++++---
>>   target/riscv/cpu.h           |  1 +
>>   target/riscv/kvm/kvm-cpu.c   | 59 ++++++++++++++++++++++++++++++++++++
>>   target/riscv/kvm/kvm_riscv.h |  1 +
>>   4 files changed, 72 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 8be619b6f1..f49d31d753 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -63,6 +63,11 @@ static void cpu_option_add_user_setting(const char *optname, uint32_t value)
>>                           GUINT_TO_POINTER(value));
>>   }
>>
>> +bool riscv_cpu_option_set(const char *optname)
>> +{
>> +    return g_hash_table_contains(general_user_opts, optname);
>> +}
>> +
> 
> This function may work in unexpected way for future developer since we can check just somehow restricted
> number of options using it, like vlen/elen/cbom/cboz size, but not vext_spec or pmu-num/mask.

Makes sense. I'll add all options in this hash to make it consistent.


Thanks,


Daniel

> 
>>   #define ISA_EXT_DATA_ENTRY(_name, _min_ver, _prop) \
>>       {#_name, _min_ver, CPU_CFG_OFFSET(_prop)}
>>
>> @@ -1056,17 +1061,18 @@ void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
>>   {
>>       Error *local_err = NULL;
>>
>> -    /*
>> -     * KVM accel does not have a specialized finalize()
>> -     * callback because its extensions are validated
>> -     * in the get()/set() callbacks of each property.
>> -     */
>>       if (tcg_enabled()) {
>>           riscv_tcg_cpu_finalize_features(cpu, &local_err);
>>           if (local_err != NULL) {
>>               error_propagate(errp, local_err);
>>               return;
>>           }
>> +    } else if (kvm_enabled()) {
>> +        riscv_kvm_cpu_finalize_features(cpu, &local_err);
>> +        if (local_err != NULL) {
>> +            error_propagate(errp, local_err);
>> +            return;
>> +        }
>>       }
>>
>>   #ifndef CONFIG_USER_ONLY
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 53101b82c5..988471c7ba 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -495,6 +495,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>>                           bool probe, uintptr_t retaddr);
>>   char *riscv_isa_string(RISCVCPU *cpu);
>>   void riscv_cpu_list(void);
>> +bool riscv_cpu_option_set(const char *optname);
>>
>>   #define cpu_list riscv_cpu_list
>>   #define cpu_mmu_index riscv_cpu_mmu_index
>> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
>> index 62a1e51f0a..70fb075846 100644
>> --- a/target/riscv/kvm/kvm-cpu.c
>> +++ b/target/riscv/kvm/kvm-cpu.c
>> @@ -1490,6 +1490,65 @@ static void kvm_cpu_instance_init(CPUState *cs)
>>       }
>>   }
>>
>> +void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
>> +{
>> +    CPURISCVState *env = &cpu->env;
>> +    KVMScratchCPU kvmcpu;
>> +    struct kvm_one_reg reg;
>> +    uint64_t val;
>> +    int ret;
>> +
>> +    /* short-circuit without spinning the scratch CPU */
>> +    if (!cpu->cfg.ext_zicbom && !cpu->cfg.ext_zicboz) {
>> +        return;
>> +    }
>> +
>> +    if (!kvm_riscv_create_scratch_vcpu(&kvmcpu)) {
>> +        error_setg(errp, "Unable to create scratch KVM cpu");
>> +        return;
>> +    }
>> +
>> +    if (cpu->cfg.ext_zicbom &&
>> +        riscv_cpu_option_set(kvm_cbom_blocksize.name)) {
>> +
>> +        reg.id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG,
>> +                                        kvm_cbom_blocksize.kvm_reg_id);
>> +        reg.addr = (uint64_t)&val;
>> +        ret = ioctl(kvmcpu.cpufd, KVM_GET_ONE_REG, &reg);
>> +        if (ret != 0) {
>> +            error_setg(errp, "Unable to read cbom_blocksize, error %d", errno);
>> +            return;
>> +        }
>> +
>> +        if (cpu->cfg.cbom_blocksize != val) {
>> +            error_setg(errp, "Unable to set cbom_blocksize to a different "
>> +                       "value than the host (%lu)", val);
>> +            return;
>> +        }
>> +    }
>> +
>> +    if (cpu->cfg.ext_zicboz &&
>> +        riscv_cpu_option_set(kvm_cboz_blocksize.name)) {
>> +
>> +        reg.id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG,
>> +                                        kvm_cboz_blocksize.kvm_reg_id);
>> +        reg.addr = (uint64_t)&val;
>> +        ret = ioctl(kvmcpu.cpufd, KVM_GET_ONE_REG, &reg);
>> +        if (ret != 0) {
>> +            error_setg(errp, "Unable to read cbom_blocksize, error %d", errno);
>> +            return;
>> +        }
>> +
>> +        if (cpu->cfg.cboz_blocksize != val) {
>> +            error_setg(errp, "Unable to set cboz_blocksize to a different "
>> +                       "value than the host (%lu)", val);
>> +            return;
>> +        }
>> +    }
>> +
>> +    kvm_riscv_destroy_scratch_vcpu(&kvmcpu);
>> +}
>> +
>>   static void kvm_cpu_accel_class_init(ObjectClass *oc, void *data)
>>   {
>>       AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
>> diff --git a/target/riscv/kvm/kvm_riscv.h b/target/riscv/kvm/kvm_riscv.h
>> index 8329cfab82..4bd98fddc7 100644
>> --- a/target/riscv/kvm/kvm_riscv.h
>> +++ b/target/riscv/kvm/kvm_riscv.h
>> @@ -27,5 +27,6 @@ void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
>>                             uint64_t guest_num);
>>   void riscv_kvm_aplic_request(void *opaque, int irq, int level);
>>   int kvm_riscv_sync_mpstate_to_kvm(RISCVCPU *cpu, int state);
>> +void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp);
>>
>>   #endif
>> --
>> 2.43.0
>>
>>
diff mbox series

Patch

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 8be619b6f1..f49d31d753 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -63,6 +63,11 @@  static void cpu_option_add_user_setting(const char *optname, uint32_t value)
                         GUINT_TO_POINTER(value));
 }
 
+bool riscv_cpu_option_set(const char *optname)
+{
+    return g_hash_table_contains(general_user_opts, optname);
+}
+
 #define ISA_EXT_DATA_ENTRY(_name, _min_ver, _prop) \
     {#_name, _min_ver, CPU_CFG_OFFSET(_prop)}
 
@@ -1056,17 +1061,18 @@  void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
 {
     Error *local_err = NULL;
 
-    /*
-     * KVM accel does not have a specialized finalize()
-     * callback because its extensions are validated
-     * in the get()/set() callbacks of each property.
-     */
     if (tcg_enabled()) {
         riscv_tcg_cpu_finalize_features(cpu, &local_err);
         if (local_err != NULL) {
             error_propagate(errp, local_err);
             return;
         }
+    } else if (kvm_enabled()) {
+        riscv_kvm_cpu_finalize_features(cpu, &local_err);
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 53101b82c5..988471c7ba 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -495,6 +495,7 @@  bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                         bool probe, uintptr_t retaddr);
 char *riscv_isa_string(RISCVCPU *cpu);
 void riscv_cpu_list(void);
+bool riscv_cpu_option_set(const char *optname);
 
 #define cpu_list riscv_cpu_list
 #define cpu_mmu_index riscv_cpu_mmu_index
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 62a1e51f0a..70fb075846 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1490,6 +1490,65 @@  static void kvm_cpu_instance_init(CPUState *cs)
     }
 }
 
+void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
+{
+    CPURISCVState *env = &cpu->env;
+    KVMScratchCPU kvmcpu;
+    struct kvm_one_reg reg;
+    uint64_t val;
+    int ret;
+
+    /* short-circuit without spinning the scratch CPU */
+    if (!cpu->cfg.ext_zicbom && !cpu->cfg.ext_zicboz) {
+        return;
+    }
+
+    if (!kvm_riscv_create_scratch_vcpu(&kvmcpu)) {
+        error_setg(errp, "Unable to create scratch KVM cpu");
+        return;
+    }
+
+    if (cpu->cfg.ext_zicbom &&
+        riscv_cpu_option_set(kvm_cbom_blocksize.name)) {
+
+        reg.id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG,
+                                        kvm_cbom_blocksize.kvm_reg_id);
+        reg.addr = (uint64_t)&val;
+        ret = ioctl(kvmcpu.cpufd, KVM_GET_ONE_REG, &reg);
+        if (ret != 0) {
+            error_setg(errp, "Unable to read cbom_blocksize, error %d", errno);
+            return;
+        }
+
+        if (cpu->cfg.cbom_blocksize != val) {
+            error_setg(errp, "Unable to set cbom_blocksize to a different "
+                       "value than the host (%lu)", val);
+            return;
+        }
+    }
+
+    if (cpu->cfg.ext_zicboz &&
+        riscv_cpu_option_set(kvm_cboz_blocksize.name)) {
+
+        reg.id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG,
+                                        kvm_cboz_blocksize.kvm_reg_id);
+        reg.addr = (uint64_t)&val;
+        ret = ioctl(kvmcpu.cpufd, KVM_GET_ONE_REG, &reg);
+        if (ret != 0) {
+            error_setg(errp, "Unable to read cbom_blocksize, error %d", errno);
+            return;
+        }
+
+        if (cpu->cfg.cboz_blocksize != val) {
+            error_setg(errp, "Unable to set cboz_blocksize to a different "
+                       "value than the host (%lu)", val);
+            return;
+        }
+    }
+
+    kvm_riscv_destroy_scratch_vcpu(&kvmcpu);
+}
+
 static void kvm_cpu_accel_class_init(ObjectClass *oc, void *data)
 {
     AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
diff --git a/target/riscv/kvm/kvm_riscv.h b/target/riscv/kvm/kvm_riscv.h
index 8329cfab82..4bd98fddc7 100644
--- a/target/riscv/kvm/kvm_riscv.h
+++ b/target/riscv/kvm/kvm_riscv.h
@@ -27,5 +27,6 @@  void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
                           uint64_t guest_num);
 void riscv_kvm_aplic_request(void *opaque, int irq, int level);
 int kvm_riscv_sync_mpstate_to_kvm(RISCVCPU *cpu, int state);
+void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp);
 
 #endif