Re: [RFC PATCH 09/10] target/riscv: Restrict KVM-specific fields from ArchCPU

Daniel Henrique Barboza <dbarboza@xxxxxxxxxxxxxxxx> · Wed, 5 Apr 2023 17:44:34 -0300

On 4/5/23 13:04, Philippe Mathieu-Daudé wrote:
These fields shouldn't be accessed when KVM is not available.

Signed-off-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>
---
RFC: The migration part is likely invalid...

kvmtimer_needed() is defined in target/riscv/machine.c as

   static bool kvmtimer_needed(void *opaque)
   {
       return kvm_enabled();
   }

which depends on a host feature.


kvm_enabled() can be false even when CONFIG_KVM is true when a KVM capable host
is running a TCG guest, for example. In that case env->kvm_timer_* states exist
but aren't initialized, and shouldn't be migrated.

Thus it's not just a host feature, but a host feature + accel option. I think
this is fine.

---
  target/riscv/cpu.h     | 2 ++
  target/riscv/machine.c | 4 ++++
  2 files changed, 6 insertions(+)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 638e47c75a..82939235ab 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -377,12 +377,14 @@ struct CPUArchState {
      hwaddr kernel_addr;
      hwaddr fdt_addr;
  
+#ifdef CONFIG_KVM
      /* kvm timer */
      bool kvm_timer_dirty;
      uint64_t kvm_timer_time;
      uint64_t kvm_timer_compare;
      uint64_t kvm_timer_state;
      uint64_t kvm_timer_frequency;
+#endif /* CONFIG_KVM */
  };
  
  OBJECT_DECLARE_CPU_TYPE(RISCVCPU, RISCVCPUClass, RISCV_CPU)
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index 9c455931d8..e45d564ec3 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -201,10 +201,12 @@ static bool kvmtimer_needed(void *opaque)
  
  static int cpu_post_load(void *opaque, int version_id)
  {
+#ifdef CONFIG_KVM
      RISCVCPU *cpu = opaque;
      CPURISCVState *env = &cpu->env;
  
      env->kvm_timer_dirty = true;
+#endif
      return 0;
  }
  
@@ -215,9 +217,11 @@ static const VMStateDescription vmstate_kvmtimer = {
      .needed = kvmtimer_needed,
      .post_load = cpu_post_load,
      .fields = (VMStateField[]) {
+#ifdef CONFIG_KVM
          VMSTATE_UINT64(env.kvm_timer_time, RISCVCPU),
          VMSTATE_UINT64(env.kvm_timer_compare, RISCVCPU),
          VMSTATE_UINT64(env.kvm_timer_state, RISCVCPU),
+#endif

Here you're creating an empty 'cpu/kvmtimer' vmstate that won't be migrated anyway
because kvmtimer_needed (== kvm_enabled()) will be always false if CONFIG_KVM=n.

I'd say it's better to just get rid of the whole vmstate in this case, but I don't
like the precedence of having vmstates being gated by build flags.


Reviewed-by: Daniel Henrique Barboza <dbarboza@xxxxxxxxxxxxxxxx>



          VMSTATE_END_OF_LIST()
      }
  };






