On Tue, May 13, 2025 at 2:33 AM <liu.xuem...@zte.com.cn> wrote:
> >
> > From: Xuemei Liu <liu.xuem...@zte.com.cn>
> >
> > Address an error in migration when aia is configured as 'aplic-imsic' in
> > riscv kvm vm by adding riscv_aplic_state_needed() and
> > riscv_imsic_state_needed() to determine whether the corresponding sates are
> > needed.
> >
> > Previously, the fields in the vmsds of 'riscv_aplic' and 'riscv_imsic' can
> > only be initialized under certain special conditions in commit 95a97b3fd2..
> > However, the corresponding ses of these vmsds are inserted into the
> > savevm_state.handlers unconditionally. This led to migration failure
> > characterized by uninitialized fields when save vm state:
> > qemu-system-riscv64: ../migration/vmstate.c:433: vmstate_save_state_v:
> > Assertion 'first_elem || !n_elems || !size' failed.
> >
> > Fixes: 95a97b3fd2 ("target/riscv: update APLIC and IMSIC to support KVM
> > AIA")
> >
> > Signed-off-by: Xuemei Liu <liu.xuem...@zte.com.cn>
> > ---
> >
> > Changes in v2:
> >
> > - Dropped the modifications in vmstate_register_with_alias_id()
> > - Added riscv_aplic_state_needed() and riscv_imsic_state_needed()
> >
> > hw/intc/riscv_aplic.c | 8 ++++++++
> > hw/intc/riscv_imsic.c | 6 ++++++
> > 2 files changed, 14 insertions(+)
> >
> > diff --git a/hw/intc/riscv_aplic.c b/hw/intc/riscv_aplic.c
> > index 8bcd9f4697..c9abef2e0f 100644
> > --- a/hw/intc/riscv_aplic.c
> > +++ b/hw/intc/riscv_aplic.c
> > @@ -962,10 +962,18 @@ static const Property riscv_aplic_properties[] = {
> > DEFINE_PROP_BOOL("mmode", RISCVAPLICState, mmode, 0),
> > };
> >
> > +static bool riscv_aplic_state_needed(void *opaque)
> > +{
> > + RISCVAPLICState *aplic = opaque;
> > +
> > + return riscv_use_emulated_aplic(aplic->msimode);
> > +}
> > +
> > static const VMStateDescription vmstate_riscv_aplic = {
> > .name = "riscv_aplic",
> > .version_id = 2,
> > .minimum_version_id = 2,
>
> I think these should also be incremented
>
> Alistair
Sir, I might not be able to agree with your opinion. The needed() function
is only used in states saveing stage to determine whether the vmsd needs
to be saved. And *version_id are mainly used in states loading stage
during migaration for compatibilty checks. The needed() has no effect
on migration compability. Personally speaking, I don't think it's necessary
to increase *version_id. If you have different opinions, please feel free
to correct me.
Xuemei
>
> > + .needed = riscv_aplic_state_needed,
> > .fields = (const VMStateField[]) {
> > VMSTATE_UINT32(domaincfg, RISCVAPLICState),
> > VMSTATE_UINT32(mmsicfgaddr, RISCVAPLICState),
> > diff --git a/hw/intc/riscv_imsic.c b/hw/intc/riscv_imsic.c
> > index 2169988167..92a7254076 100644
> > --- a/hw/intc/riscv_imsic.c
> > +++ b/hw/intc/riscv_imsic.c
> > @@ -398,10 +398,16 @@ static const Property riscv_imsic_properties[] = {
> > DEFINE_PROP_UINT32("num-irqs", RISCVIMSICState, num_irqs, 0),
> > };
> >
> > +static bool riscv_imsic_state_needed(void *opaque)
> > +{
> > + return !kvm_irqchip_in_kernel();
> > +}
> > +
> > static const VMStateDescription vmstate_riscv_imsic = {
> > .name = "riscv_imsic",
> > .version_id = 1,
> > .minimum_version_id = 1,
> > + .needed = riscv_imsic_state_needed,
> > .fields = (const VMStateField[]) {
> > VMSTATE_VARRAY_UINT32(eidelivery, RISCVIMSICState,
> > num_pages, 0,
> > --
> > 2.27.0
> >
Original
From: alistair23 <alistai...@gmail.com>
To: Liu Xuemei00333568;
Cc: palmer <pal...@dabbelt.com>;alistair.francis
<alistair.fran...@wdc.com>;liwei1518 <liwei1...@gmail.com>;dbarboza
<dbarb...@ventanamicro.com>;zhiwei_liu <zhiwei_...@linux.alibaba.com>;peterx
<pet...@redhat.com>;farosas <faro...@suse.de>;qemu-riscv
<qemu-ri...@nongnu.org>;qemu-devel <qemu-devel@nongnu.org>;
Date: 2025/05/19 08:33
Subject: Re: [PATCH v2] migration: Fix migration failure when aia is configured
as 'aplic-imsic'
On Tue, May 13, 2025 at 2:33 AM <liu.xuem...@zte.com.cn> wrote:
>
> From: Xuemei Liu <liu.xuem...@zte.com.cn>
>
> Address an error in migration when aia is configured as 'aplic-imsic' in
> riscv kvm vm by adding riscv_aplic_state_needed() and
> riscv_imsic_state_needed() to determine whether the corresponding sates are
> needed.
>
> Previously, the fields in the vmsds of 'riscv_aplic' and 'riscv_imsic' can
> only be initialized under certain special conditions in commit 95a97b3fd2..
> However, the corresponding ses of these vmsds are inserted into the
> savevm_state.handlers unconditionally. This led to migration failure
> characterized by uninitialized fields when save vm state:
> qemu-system-riscv64: ../migration/vmstate.c:433: vmstate_save_state_v:
> Assertion 'first_elem || !n_elems || !size' failed.
>
> Fixes: 95a97b3fd2 ("target/riscv: update APLIC and IMSIC to support KVM AIA")
>
> Signed-off-by: Xuemei Liu <liu.xuem...@zte.com.cn>
> ---
>
> Changes in v2:
>
> - Dropped the modifications in vmstate_register_with_alias_id()
> - Added riscv_aplic_state_needed() and riscv_imsic_state_needed()
>
> hw/intc/riscv_aplic.c | 8 ++++++++
> hw/intc/riscv_imsic.c | 6 ++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/hw/intc/riscv_aplic.c b/hw/intc/riscv_aplic.c
> index 8bcd9f4697..c9abef2e0f 100644
> --- a/hw/intc/riscv_aplic.c
> +++ b/hw/intc/riscv_aplic.c
> @@ -962,10 +962,18 @@ static const Property riscv_aplic_properties[] = {
> DEFINE_PROP_BOOL("mmode", RISCVAPLICState, mmode, 0),
> };
>
> +static bool riscv_aplic_state_needed(void *opaque)
> +{
> + RISCVAPLICState *aplic = opaque;
> +
> + return riscv_use_emulated_aplic(aplic->msimode);
> +}
> +
> static const VMStateDescription vmstate_riscv_aplic = {
> .name = "riscv_aplic",
> .version_id = 2,
> .minimum_version_id = 2,
I think these should also be incremented
Alistair
> + .needed = riscv_aplic_state_needed,
> .fields = (const VMStateField[]) {
> VMSTATE_UINT32(domaincfg, RISCVAPLICState),
> VMSTATE_UINT32(mmsicfgaddr, RISCVAPLICState),
> diff --git a/hw/intc/riscv_imsic.c b/hw/intc/riscv_imsic.c
> index 2169988167..92a7254076 100644
> --- a/hw/intc/riscv_imsic.c
> +++ b/hw/intc/riscv_imsic.c
> @@ -398,10 +398,16 @@ static const Property riscv_imsic_properties[] = {
> DEFINE_PROP_UINT32("num-irqs", RISCVIMSICState, num_irqs, 0),
> };
>
> +static bool riscv_imsic_state_needed(void *opaque)
> +{
> + return !kvm_irqchip_in_kernel();
> +}
> +
> static const VMStateDescription vmstate_riscv_imsic = {
> .name = "riscv_imsic",
> .version_id = 1,
> .minimum_version_id = 1,
> + .needed = riscv_imsic_state_needed,
> .fields = (const VMStateField[]) {
> VMSTATE_VARRAY_UINT32(eidelivery, RISCVIMSICState,
> num_pages, 0,
> --
> 2.27.0
>
