| Message ID | 20201019093156.2993284-1-liangpeng10@huawei.com |
|---|---|
| State | Accepted |
| Commit | 136fc6aa2cf38205fa3b47e155ebac11baccc789 |
| Headers | show |
| Series | ACPI: Avoid infinite recursion when dump-vmstate | expand |
On Mon, 19 Oct 2020 17:31:56 +0800 Peng Liang <liangpeng10@huawei.com> wrote: > There is a field with vmstate_ghes_state as vmsd in vmstate_ghes_state, > which will lead to infinite recursion in dump_vmstate_vmsd. > > Fixes: a08a64627b ("ACPI: Record the Generic Error Status Block address") > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > --- > hw/acpi/generic_event_device.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c > index 6df400e1ee16..4b6867300a55 100644 > --- a/hw/acpi/generic_event_device.c > +++ b/hw/acpi/generic_event_device.c > @@ -334,8 +334,7 @@ static const VMStateDescription vmstate_ghes_state = { > .minimum_version_id = 1, > .needed = ghes_needed, > .fields = (VMStateField[]) { > - VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, > - vmstate_ghes_state, AcpiGhesState), > + VMSTATE_UINT64(ghes_state.ghes_addr_le, AcpiGedState), not sure its' ok handle it this way, see how it is done with another structure: static const VMStateDescription vmstate_ged_state = { .name = "acpi-ged-state", .version_id = 1, .minimum_version_id = 1, .fields = (VMStateField[]) { VMSTATE_UINT32(sel, GEDState), VMSTATE_END_OF_LIST() } }; ... VMSTATE_STRUCT(ged_state, AcpiGedState, 1, vmstate_ged_state, GEDState), i.e. it looks like we are missing structure definition for AcpiGhesState CCing David, to help with migration magic in case I'm wrong or missed something > VMSTATE_END_OF_LIST() > } > };
* Igor Mammedov (imammedo@redhat.com) wrote: > On Mon, 19 Oct 2020 17:31:56 +0800 > Peng Liang <liangpeng10@huawei.com> wrote: > > > There is a field with vmstate_ghes_state as vmsd in vmstate_ghes_state, > > which will lead to infinite recursion in dump_vmstate_vmsd. > > > > Fixes: a08a64627b ("ACPI: Record the Generic Error Status Block address") > > Reported-by: Euler Robot <euler.robot@huawei.com> > > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > > --- > > hw/acpi/generic_event_device.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c > > index 6df400e1ee16..4b6867300a55 100644 > > --- a/hw/acpi/generic_event_device.c > > +++ b/hw/acpi/generic_event_device.c > > @@ -334,8 +334,7 @@ static const VMStateDescription vmstate_ghes_state = { > > .minimum_version_id = 1, > > .needed = ghes_needed, > > .fields = (VMStateField[]) { > > - VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, > > - vmstate_ghes_state, AcpiGhesState), > > + VMSTATE_UINT64(ghes_state.ghes_addr_le, AcpiGedState), > > not sure its' ok handle it this way, > > see how it is done with another structure: > > static const VMStateDescription vmstate_ged_state = { > .name = "acpi-ged-state", > .version_id = 1, > .minimum_version_id = 1, > .fields = (VMStateField[]) { > VMSTATE_UINT32(sel, GEDState), > VMSTATE_END_OF_LIST() > } > }; > > ... > > VMSTATE_STRUCT(ged_state, AcpiGedState, 1, vmstate_ged_state, GEDState), > > i.e. it looks like we are missing structure definition for AcpiGhesState > > CCing David, > to help with migration magic in case I'm wrong or missed something Yeh that's confusing :-) Given a: VMSTATE_STRUCT(a, B, 1, vmstate_c, C) We're saying there's a field 'a' in type B, and field 'a' should be of type C and be serialised using vmstate_c. That also means that in any vmstate_c, we're expecting it to be passed a type C generally. Having said that; you don't need a struct - you can get away with that VMSTATE_UINT64, there's two problems: a) That assumes that your ghes always stays that simple. b) If you wanted to store a Ghes from a number of different parent structures then you're stuck because your vmstate_ghes_state is bound to being a strict field of AcpiGedState. So yes, it's neatest to do it using a VMSD for AcpiGhesState And congratulations on finding a loop; I don't think we've ever had one before :-) Dave > > VMSTATE_END_OF_LIST() > > } > > }; > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, 23 Oct 2020 19:54:41 +0100 "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Igor Mammedov (imammedo@redhat.com) wrote: > > On Mon, 19 Oct 2020 17:31:56 +0800 > > Peng Liang <liangpeng10@huawei.com> wrote: > > > > > There is a field with vmstate_ghes_state as vmsd in vmstate_ghes_state, > > > which will lead to infinite recursion in dump_vmstate_vmsd. > > > > > > Fixes: a08a64627b ("ACPI: Record the Generic Error Status Block address") > > > Reported-by: Euler Robot <euler.robot@huawei.com> > > > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > > > --- > > > hw/acpi/generic_event_device.c | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c > > > index 6df400e1ee16..4b6867300a55 100644 > > > --- a/hw/acpi/generic_event_device.c > > > +++ b/hw/acpi/generic_event_device.c > > > @@ -334,8 +334,7 @@ static const VMStateDescription vmstate_ghes_state = { > > > .minimum_version_id = 1, > > > .needed = ghes_needed, > > > .fields = (VMStateField[]) { > > > - VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, > > > - vmstate_ghes_state, AcpiGhesState), > > > + VMSTATE_UINT64(ghes_state.ghes_addr_le, AcpiGedState), > > > > not sure its' ok handle it this way, > > > > see how it is done with another structure: > > > > static const VMStateDescription vmstate_ged_state = { > > .name = "acpi-ged-state", > > .version_id = 1, > > .minimum_version_id = 1, > > .fields = (VMStateField[]) { > > VMSTATE_UINT32(sel, GEDState), > > VMSTATE_END_OF_LIST() > > } > > }; > > > > ... > > > > VMSTATE_STRUCT(ged_state, AcpiGedState, 1, vmstate_ged_state, GEDState), > > > > i.e. it looks like we are missing structure definition for AcpiGhesState > > > > CCing David, > > to help with migration magic in case I'm wrong or missed something > > Yeh that's confusing :-) > > Given a: > > VMSTATE_STRUCT(a, B, 1, vmstate_c, C) > > We're saying there's a field 'a' in type B, and field 'a' > should be of type C and be serialised using vmstate_c. > > That also means that in any vmstate_c, we're expecting it > to be passed a type C generally. > > Having said that; you don't need a struct - you can get away > with that VMSTATE_UINT64, there's two problems: > > a) That assumes that your ghes always stays that simple. > b) If you wanted to store a Ghes from a number of different > parent structures then you're stuck because your vmstate_ghes_state > is bound to being a strict field of AcpiGedState. > > So yes, it's neatest to do it using a VMSD for AcpiGhesState > > And congratulations on finding a loop; I don't think we've ever had one > before :-) can we make compilation fail in case VMSTATE_STRUCT is used but is not actually provided like it was in this case? > > Dave > > > > VMSTATE_END_OF_LIST() > > > } > > > }; > >
On 10/24/2020 2:54 AM, Dr. David Alan Gilbert wrote: > * Igor Mammedov (imammedo@redhat.com) wrote: >> On Mon, 19 Oct 2020 17:31:56 +0800 >> Peng Liang <liangpeng10@huawei.com> wrote: >> >>> There is a field with vmstate_ghes_state as vmsd in vmstate_ghes_state, >>> which will lead to infinite recursion in dump_vmstate_vmsd. >>> >>> Fixes: a08a64627b ("ACPI: Record the Generic Error Status Block address") >>> Reported-by: Euler Robot <euler.robot@huawei.com> >>> Signed-off-by: Peng Liang <liangpeng10@huawei.com> >>> --- >>> hw/acpi/generic_event_device.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c >>> index 6df400e1ee16..4b6867300a55 100644 >>> --- a/hw/acpi/generic_event_device.c >>> +++ b/hw/acpi/generic_event_device.c >>> @@ -334,8 +334,7 @@ static const VMStateDescription vmstate_ghes_state = { >>> .minimum_version_id = 1, >>> .needed = ghes_needed, >>> .fields = (VMStateField[]) { >>> - VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, >>> - vmstate_ghes_state, AcpiGhesState), >>> + VMSTATE_UINT64(ghes_state.ghes_addr_le, AcpiGedState), >> >> not sure its' ok handle it this way, >> >> see how it is done with another structure: >> >> static const VMStateDescription vmstate_ged_state = { >> .name = "acpi-ged-state", >> .version_id = 1, >> .minimum_version_id = 1, >> .fields = (VMStateField[]) { >> VMSTATE_UINT32(sel, GEDState), >> VMSTATE_END_OF_LIST() >> } >> }; >> >> ... >> >> VMSTATE_STRUCT(ged_state, AcpiGedState, 1, vmstate_ged_state, GEDState), >> >> i.e. it looks like we are missing structure definition for AcpiGhesState >> >> CCing David, >> to help with migration magic in case I'm wrong or missed something > > Yeh that's confusing :-) > > Given a: > > VMSTATE_STRUCT(a, B, 1, vmstate_c, C) > > We're saying there's a field 'a' in type B, and field 'a' > should be of type C and be serialised using vmstate_c. > > That also means that in any vmstate_c, we're expecting it > to be passed a type C generally. > > Having said that; you don't need a struct - you can get away > with that VMSTATE_UINT64, there's two problems: > > a) That assumes that your ghes always stays that simple. > b) If you wanted to store a Ghes from a number of different > parent structures then you're stuck because your vmstate_ghes_state > is bound to being a strict field of AcpiGedState. > > So yes, it's neatest to do it using a VMSD for AcpiGhesState > > And congratulations on finding a loop; I don't think we've ever had one > before :-) > > Dave > >>> VMSTATE_END_OF_LIST() >>> } >>> }; >> Do you mean that we need another VMStateDescription to describe AcpiGhesState instead of using VMSTATE_UINT64 directly? Maybe like this: diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c index 6df400e1ee16..5454be67d5f0 100644 --- a/hw/acpi/generic_event_device.c +++ b/hw/acpi/generic_event_device.c @@ -322,6 +322,16 @@ static const VMStateDescription vmstate_ged_state = { } }; +static const VMStateDescription vmstate_ghes = { + .name = "acpi-ghes", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT64(ghes_addr_le, AcpiGhesState), + VMSTATE_END_OF_LIST() + }, +}; + static bool ghes_needed(void *opaque) { AcpiGedState *s = opaque; @@ -335,7 +345,7 @@ static const VMStateDescription vmstate_ghes_state = { .needed = ghes_needed, .fields = (VMStateField[]) { VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, - vmstate_ghes_state, AcpiGhesState), + vmstate_ghes, AcpiGhesState), VMSTATE_END_OF_LIST() } }; -- Thanks, Peng
* Igor Mammedov (imammedo@redhat.com) wrote: > On Fri, 23 Oct 2020 19:54:41 +0100 > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > > * Igor Mammedov (imammedo@redhat.com) wrote: > > > On Mon, 19 Oct 2020 17:31:56 +0800 > > > Peng Liang <liangpeng10@huawei.com> wrote: > > > > > > > There is a field with vmstate_ghes_state as vmsd in vmstate_ghes_state, > > > > which will lead to infinite recursion in dump_vmstate_vmsd. > > > > > > > > Fixes: a08a64627b ("ACPI: Record the Generic Error Status Block address") > > > > Reported-by: Euler Robot <euler.robot@huawei.com> > > > > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > > > > --- > > > > hw/acpi/generic_event_device.c | 3 +-- > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c > > > > index 6df400e1ee16..4b6867300a55 100644 > > > > --- a/hw/acpi/generic_event_device.c > > > > +++ b/hw/acpi/generic_event_device.c > > > > @@ -334,8 +334,7 @@ static const VMStateDescription vmstate_ghes_state = { > > > > .minimum_version_id = 1, > > > > .needed = ghes_needed, > > > > .fields = (VMStateField[]) { > > > > - VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, > > > > - vmstate_ghes_state, AcpiGhesState), > > > > + VMSTATE_UINT64(ghes_state.ghes_addr_le, AcpiGedState), > > > > > > not sure its' ok handle it this way, > > > > > > see how it is done with another structure: > > > > > > static const VMStateDescription vmstate_ged_state = { > > > .name = "acpi-ged-state", > > > .version_id = 1, > > > .minimum_version_id = 1, > > > .fields = (VMStateField[]) { > > > VMSTATE_UINT32(sel, GEDState), > > > VMSTATE_END_OF_LIST() > > > } > > > }; > > > > > > ... > > > > > > VMSTATE_STRUCT(ged_state, AcpiGedState, 1, vmstate_ged_state, GEDState), > > > > > > i.e. it looks like we are missing structure definition for AcpiGhesState > > > > > > CCing David, > > > to help with migration magic in case I'm wrong or missed something > > > > Yeh that's confusing :-) > > > > Given a: > > > > VMSTATE_STRUCT(a, B, 1, vmstate_c, C) > > > > We're saying there's a field 'a' in type B, and field 'a' > > should be of type C and be serialised using vmstate_c. > > > > That also means that in any vmstate_c, we're expecting it > > to be passed a type C generally. > > > > Having said that; you don't need a struct - you can get away > > with that VMSTATE_UINT64, there's two problems: > > > > a) That assumes that your ghes always stays that simple. > > b) If you wanted to store a Ghes from a number of different > > parent structures then you're stuck because your vmstate_ghes_state > > is bound to being a strict field of AcpiGedState. > > > > So yes, it's neatest to do it using a VMSD for AcpiGhesState > > > > And congratulations on finding a loop; I don't think we've ever had one > > before :-) > > can we make compilation fail in case VMSTATE_STRUCT is used but > is not actually provided like it was in this case? There's some type checking by the vmstate_offset_value macro; which I think would have verified that the 'ghes_state' field in AcpiGedState really is of type AcpiGhesState; but what we don't have is a type associated with a given VMStateDescription to know what type of state it's supposed to be called on. Dave > > > > Dave > > > > > > VMSTATE_END_OF_LIST() > > > > } > > > > }; > > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Is someone taking a fix for this in 5.2 - it's breaking vmstate comparison. Dave * Peng Liang (liangpeng10@huawei.com) wrote: > On 10/24/2020 2:54 AM, Dr. David Alan Gilbert wrote: > > * Igor Mammedov (imammedo@redhat.com) wrote: > >> On Mon, 19 Oct 2020 17:31:56 +0800 > >> Peng Liang <liangpeng10@huawei.com> wrote: > >> > >>> There is a field with vmstate_ghes_state as vmsd in vmstate_ghes_state, > >>> which will lead to infinite recursion in dump_vmstate_vmsd. > >>> > >>> Fixes: a08a64627b ("ACPI: Record the Generic Error Status Block address") > >>> Reported-by: Euler Robot <euler.robot@huawei.com> > >>> Signed-off-by: Peng Liang <liangpeng10@huawei.com> > >>> --- > >>> hw/acpi/generic_event_device.c | 3 +-- > >>> 1 file changed, 1 insertion(+), 2 deletions(-) > >>> > >>> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c > >>> index 6df400e1ee16..4b6867300a55 100644 > >>> --- a/hw/acpi/generic_event_device.c > >>> +++ b/hw/acpi/generic_event_device.c > >>> @@ -334,8 +334,7 @@ static const VMStateDescription vmstate_ghes_state = { > >>> .minimum_version_id = 1, > >>> .needed = ghes_needed, > >>> .fields = (VMStateField[]) { > >>> - VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, > >>> - vmstate_ghes_state, AcpiGhesState), > >>> + VMSTATE_UINT64(ghes_state.ghes_addr_le, AcpiGedState), > >> > >> not sure its' ok handle it this way, > >> > >> see how it is done with another structure: > >> > >> static const VMStateDescription vmstate_ged_state = { > >> .name = "acpi-ged-state", > >> .version_id = 1, > >> .minimum_version_id = 1, > >> .fields = (VMStateField[]) { > >> VMSTATE_UINT32(sel, GEDState), > >> VMSTATE_END_OF_LIST() > >> } > >> }; > >> > >> ... > >> > >> VMSTATE_STRUCT(ged_state, AcpiGedState, 1, vmstate_ged_state, GEDState), > >> > >> i.e. it looks like we are missing structure definition for AcpiGhesState > >> > >> CCing David, > >> to help with migration magic in case I'm wrong or missed something > > > > Yeh that's confusing :-) > > > > Given a: > > > > VMSTATE_STRUCT(a, B, 1, vmstate_c, C) > > > > We're saying there's a field 'a' in type B, and field 'a' > > should be of type C and be serialised using vmstate_c. > > > > That also means that in any vmstate_c, we're expecting it > > to be passed a type C generally. > > > > Having said that; you don't need a struct - you can get away > > with that VMSTATE_UINT64, there's two problems: > > > > a) That assumes that your ghes always stays that simple. > > b) If you wanted to store a Ghes from a number of different > > parent structures then you're stuck because your vmstate_ghes_state > > is bound to being a strict field of AcpiGedState. > > > > So yes, it's neatest to do it using a VMSD for AcpiGhesState > > > > And congratulations on finding a loop; I don't think we've ever had one > > before :-) > > > > Dave > > > >>> VMSTATE_END_OF_LIST() > >>> } > >>> }; > >> > > Do you mean that we need another VMStateDescription to describe > AcpiGhesState instead of using VMSTATE_UINT64 directly? Maybe like this: > > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c > index 6df400e1ee16..5454be67d5f0 100644 > --- a/hw/acpi/generic_event_device.c > +++ b/hw/acpi/generic_event_device.c > @@ -322,6 +322,16 @@ static const VMStateDescription vmstate_ged_state = { > } > }; > > +static const VMStateDescription vmstate_ghes = { > + .name = "acpi-ghes", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT64(ghes_addr_le, AcpiGhesState), > + VMSTATE_END_OF_LIST() > + }, > +}; > + > static bool ghes_needed(void *opaque) > { > AcpiGedState *s = opaque; > @@ -335,7 +345,7 @@ static const VMStateDescription vmstate_ghes_state = { > .needed = ghes_needed, > .fields = (VMStateField[]) { > VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, > - vmstate_ghes_state, AcpiGhesState), > + vmstate_ghes, AcpiGhesState), > VMSTATE_END_OF_LIST() > } > }; > > -- > Thanks, > Peng > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Wed, 11 Nov 2020 14:01:12 +0000 "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > Is someone taking a fix for this in 5.2 - it's breaking vmstate > comparison. can you merge it via migration tree? [...] for fixed up version below Acked-by: Igor Mammedov <imammedo@redhat.com> > > > > Do you mean that we need another VMStateDescription to describe > > AcpiGhesState instead of using VMSTATE_UINT64 directly? Maybe like this: > > > > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c > > index 6df400e1ee16..5454be67d5f0 100644 > > --- a/hw/acpi/generic_event_device.c > > +++ b/hw/acpi/generic_event_device.c > > @@ -322,6 +322,16 @@ static const VMStateDescription vmstate_ged_state = { > > } > > }; > > > > +static const VMStateDescription vmstate_ghes = { > > + .name = "acpi-ghes", > > + .version_id = 1, > > + .minimum_version_id = 1, > > + .fields = (VMStateField[]) { > > + VMSTATE_UINT64(ghes_addr_le, AcpiGhesState), > > + VMSTATE_END_OF_LIST() > > + }, > > +}; > > + > > static bool ghes_needed(void *opaque) > > { > > AcpiGedState *s = opaque; > > @@ -335,7 +345,7 @@ static const VMStateDescription vmstate_ghes_state = { > > .needed = ghes_needed, > > .fields = (VMStateField[]) { > > VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, > > - vmstate_ghes_state, AcpiGhesState), > > + vmstate_ghes, AcpiGhesState), > > VMSTATE_END_OF_LIST() > > } > > }; > > > > -- > > Thanks, > > Peng > >
* Igor Mammedov (imammedo@redhat.com) wrote: > On Wed, 11 Nov 2020 14:01:12 +0000 > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > > Is someone taking a fix for this in 5.2 - it's breaking vmstate > > comparison. > can you merge it via migration tree? I could; Peng: Could you give a sign-off for this version ? Dave > [...] > > for fixed up version below > Acked-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > Do you mean that we need another VMStateDescription to describe > > > AcpiGhesState instead of using VMSTATE_UINT64 directly? Maybe like this: > > > > > > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c > > > index 6df400e1ee16..5454be67d5f0 100644 > > > --- a/hw/acpi/generic_event_device.c > > > +++ b/hw/acpi/generic_event_device.c > > > @@ -322,6 +322,16 @@ static const VMStateDescription vmstate_ged_state = { > > > } > > > }; > > > > > > +static const VMStateDescription vmstate_ghes = { > > > + .name = "acpi-ghes", > > > + .version_id = 1, > > > + .minimum_version_id = 1, > > > + .fields = (VMStateField[]) { > > > + VMSTATE_UINT64(ghes_addr_le, AcpiGhesState), > > > + VMSTATE_END_OF_LIST() > > > + }, > > > +}; > > > + > > > static bool ghes_needed(void *opaque) > > > { > > > AcpiGedState *s = opaque; > > > @@ -335,7 +345,7 @@ static const VMStateDescription vmstate_ghes_state = { > > > .needed = ghes_needed, > > > .fields = (VMStateField[]) { > > > VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, > > > - vmstate_ghes_state, AcpiGhesState), > > > + vmstate_ghes, AcpiGhesState), > > > VMSTATE_END_OF_LIST() > > > } > > > }; > > > > > > -- > > > Thanks, > > > Peng > > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 11/12/2020 1:26 AM, Dr. David Alan Gilbert wrote: > * Igor Mammedov (imammedo@redhat.com) wrote: >> On Wed, 11 Nov 2020 14:01:12 +0000 >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >> >>> Is someone taking a fix for this in 5.2 - it's breaking vmstate >>> comparison. >> can you merge it via migration tree? > > I could; Peng: Could you give a sign-off for this version ? > > Dave OK, I'll send it as soon as possible. Thanks, Peng > >> [...] >> >> for fixed up version below >> Acked-by: Igor Mammedov <imammedo@redhat.com> >> >>>> >>>> Do you mean that we need another VMStateDescription to describe >>>> AcpiGhesState instead of using VMSTATE_UINT64 directly? Maybe like this: >>>> >>>> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c >>>> index 6df400e1ee16..5454be67d5f0 100644 >>>> --- a/hw/acpi/generic_event_device.c >>>> +++ b/hw/acpi/generic_event_device.c >>>> @@ -322,6 +322,16 @@ static const VMStateDescription vmstate_ged_state = { >>>> } >>>> }; >>>> >>>> +static const VMStateDescription vmstate_ghes = { >>>> + .name = "acpi-ghes", >>>> + .version_id = 1, >>>> + .minimum_version_id = 1, >>>> + .fields = (VMStateField[]) { >>>> + VMSTATE_UINT64(ghes_addr_le, AcpiGhesState), >>>> + VMSTATE_END_OF_LIST() >>>> + }, >>>> +}; >>>> + >>>> static bool ghes_needed(void *opaque) >>>> { >>>> AcpiGedState *s = opaque; >>>> @@ -335,7 +345,7 @@ static const VMStateDescription vmstate_ghes_state = { >>>> .needed = ghes_needed, >>>> .fields = (VMStateField[]) { >>>> VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, >>>> - vmstate_ghes_state, AcpiGhesState), >>>> + vmstate_ghes, AcpiGhesState), >>>> VMSTATE_END_OF_LIST() >>>> } >>>> }; >>>> >>>> -- >>>> Thanks, >>>> Peng >>>> >>
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c index 6df400e1ee16..4b6867300a55 100644 --- a/hw/acpi/generic_event_device.c +++ b/hw/acpi/generic_event_device.c @@ -334,8 +334,7 @@ static const VMStateDescription vmstate_ghes_state = { .minimum_version_id = 1, .needed = ghes_needed, .fields = (VMStateField[]) { - VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, - vmstate_ghes_state, AcpiGhesState), + VMSTATE_UINT64(ghes_state.ghes_addr_le, AcpiGedState), VMSTATE_END_OF_LIST() } };
There is a field with vmstate_ghes_state as vmsd in vmstate_ghes_state, which will lead to infinite recursion in dump_vmstate_vmsd. Fixes: a08a64627b ("ACPI: Record the Generic Error Status Block address") Reported-by: Euler Robot <euler.robot@huawei.com> Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- hw/acpi/generic_event_device.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)