From: Fabiano Rosas <farosas@suse.de>
To: Peter Xu <peterx@redhat.com>
Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>,
"Song Gao" <gaosong@loongson.cn>,
qemu-devel@nongnu.org, "Tianrui Zhao" <zhaotianrui@loongson.cn>,
pbonzini@redhat.com, peter.maydell@linaro.org,
richard.henderson@linaro.org, maobibo@loongson.cn,
lixianglai@loongso.cn
Subject: Re: [PATCH] target/loongarch/kvm: Fix VM recovery from disk failures
Date: Thu, 02 May 2024 09:45:40 -0300 [thread overview]
Message-ID: <87o79oo00b.fsf@suse.de> (raw)
In-Reply-To: <ZjJjl2fIU1s24uFD@x1n>
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. 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,
next prev parent reply other threads:[~2024-05-02 12:47 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-30 1:23 [PATCH] target/loongarch/kvm: Fix VM recovery from disk failures Song Gao
2024-04-30 8:53 ` Philippe Mathieu-Daudé
2024-04-30 14:00 ` Fabiano Rosas
2024-05-01 15:45 ` Peter Xu
2024-05-02 12:45 ` Fabiano Rosas [this message]
2024-05-07 8:12 ` gaosong
2024-05-07 15:47 ` Peter Xu
2024-05-07 15:52 ` Peter Maydell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87o79oo00b.fsf@suse.de \
--to=farosas@suse.de \
--cc=gaosong@loongson.cn \
--cc=lixianglai@loongso.cn \
--cc=maobibo@loongson.cn \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=peterx@redhat.com \
--cc=philmd@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=richard.henderson@linaro.org \
--cc=zhaotianrui@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).