| Message ID | 20230405160454.97436-10-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | accel/kvm: Spring cleaning | expand |
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() > } > };
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~
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 --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() } };
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(+)