Re: [PATCH] target/loongarch/kvm: Fix VM recovery from disk failures

[Fabiano Rosas]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> (Cc'ing migration maintainers)
>
> On 30/4/24 03:23, Song Gao wrote:
>> vmstate does not save kvm_state_conter,
>> which can cause VM recovery from disk to fail.
>
> Cc: qemu-stable@nongnu.org
> Fixes: d11681c94f ("target/loongarch: Implement kvm_arch_init_vcpu")
>
>> Signed-off-by: Song Gao <gaosong@loongson.cn>
>> ---
>>   target/loongarch/machine.c | 2 ++
>>   1 file changed, 2 insertions(+)
>> 
>> diff --git a/target/loongarch/machine.c b/target/loongarch/machine.c
>> index c7029fb9b4..4cd1bf06ff 100644
>> --- a/target/loongarch/machine.c
>> +++ b/target/loongarch/machine.c
>> @@ -191,6 +191,8 @@ const VMStateDescription vmstate_loongarch_cpu = {
>>           VMSTATE_STRUCT_ARRAY(env.tlb, LoongArchCPU, LOONGARCH_TLB_MAX,
>>                                0, vmstate_tlb, LoongArchTLB),
>>   
>> +        VMSTATE_UINT64(kvm_state_counter, LoongArchCPU),
>> +
>>           VMSTATE_END_OF_LIST()
>>       },
>>       .subsections = (const VMStateDescription * const []) {
>
> The migration stream is versioned, so you should increase it,
> but this field is only relevant for KVM (it shouldn't be there
> in non-KVM builds). IMHO the correct migration way to fix that
> is (untested):
>
> -- >8 --
> diff --git a/target/loongarch/machine.c b/target/loongarch/machine.c
> index c7029fb9b4..08032c6d71 100644
> --- a/target/loongarch/machine.c
> +++ b/target/loongarch/machine.c
> @@ -8,8 +8,27 @@
>   #include "qemu/osdep.h"
>   #include "cpu.h"
>   #include "migration/cpu.h"
> +#include "sysemu/kvm.h"
>   #include "vec.h"
>
> +#ifdef CONFIG_KVM
> +static bool kvmcpu_needed(void *opaque)
> +{
> +    return kvm_enabled();
> +}
> +
> +static const VMStateDescription vmstate_kvmtimer = {
> +    .name = "cpu/kvmtimer",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = kvmcpu_needed,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_UINT64(kvm_state_counter, LoongArchCPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +#endif /* CONFIG_KVM */
> +
>   static const VMStateDescription vmstate_fpu_reg = {
>       .name = "fpu_reg",
>       .version_id = 1,
> @@ -194,6 +213,9 @@ const VMStateDescription vmstate_loongarch_cpu = {
>           VMSTATE_END_OF_LIST()
>       },
>       .subsections = (const VMStateDescription * const []) {
> +#ifdef CONFIG_KVM
> +        &vmstate_kvmcpu,
> +#endif
>           &vmstate_fpu,
>           &vmstate_lsx,
>           &vmstate_lasx,
> ---

LGTM, I'd just leave only the .needed function under CONFIG_KVM instead
of the whole subsection.