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

[gaosong <gaosong@loongson.cn>]

Thanks for the comments !

在 2024/5/2 下午8:45, Fabiano Rosas 写道:
> Peter Xu <peterx@redhat.com> writes:
>
>> On Tue, Apr 30, 2024 at 11:00:24AM -0300, Fabiano Rosas wrote:
>>> 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.
>> But when !KVM it means there's no ".needed" and it'll still be migrated?
> I expressed myself poorly, I meant put the return from .needed under
> CONFIG_KVM. But that is not even necessary, kvm_enabled() is enough.
>
>> IMHO it depends on whether loongarch is in the state already of trying to
>> keep its ABI at all.  I think we should still try to enjoy the time when
>> that ABI is not required, then we can simply add whatever fields, and let
>> things break with no big deal.
>>
>> Note that if with CONFIG_KVM it means it can break migration between kvm
>> v.s. tcg when only one qemu enabled kvm when compile.
Just remove CONIFG_KVM  would be OK?

Thanks.
Song Gao
>>    Considering the
>> patch is from the maintainer (which seems to say "breaking that is all
>> fine!") I'd say the original patch looks good actually, as it allows kvm /
>> tcg migrations too as a baseline.
> I'm fine with this approach as well.
>
>> Thanks,