diff mbox series

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

Message ID 20230405160454.97436-10-philmd@linaro.org
State New
Headers show
Series accel/kvm: Spring cleaning | expand

Commit Message

Philippe Mathieu-Daudé April 5, 2023, 4:04 p.m. UTC 
These fields shouldn't be accessed when KVM is not available.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
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.
---
 target/riscv/cpu.h     | 2 ++
 target/riscv/machine.c | 4 ++++
 2 files changed, 6 insertions(+)

Comments

Daniel Henrique Barboza April 5, 2023, 8:44 p.m. UTC | #1 
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@linaro.org>
> ---
> 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@ventanamicro.com>



>           VMSTATE_END_OF_LIST()
>       }
>   };
Richard Henderson April 8, 2023, 4:28 a.m. UTC | #2 
On 4/5/23 09:04, Philippe Mathieu-Daudé wrote:
> These fields shouldn't be accessed when KVM is not available.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
> 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.
> ---
>   target/riscv/cpu.h     | 2 ++
>   target/riscv/machine.c | 4 ++++
>   2 files changed, 6 insertions(+)

Yeah, the kvm parts need to be extracted to their own subsection.


r~
Richard Henderson April 8, 2023, 4:29 a.m. UTC | #3 
On 4/7/23 21:28, Richard Henderson wrote:
> On 4/5/23 09:04, Philippe Mathieu-Daudé wrote:
>> These fields shouldn't be accessed when KVM is not available.
>>
>> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
>> ---
>> 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.
>> ---
>>   target/riscv/cpu.h     | 2 ++
>>   target/riscv/machine.c | 4 ++++
>>   2 files changed, 6 insertions(+)
> 
> Yeah, the kvm parts need to be extracted to their own subsection.

Oh, but they are.  Ho hum, it's getting late.


r~
diff mbox series

Patch

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
         VMSTATE_END_OF_LIST()
     }
 };