Is someone taking a fix for this in 5.2 - it's breaking vmstate
comparison.
Dave
* Peng Liang (liangpen...@huawei.com) wrote:
> On 10/24/2020 2:54 AM, Dr. David Alan Gilbert wrote:
> > * Igor Mammedov (imamm...@redhat.com) wrote:
> >> On Mon, 19 Oct 2020 17:31:56 +0800
> >> Peng Liang <liangpen...@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.ro...@huawei.com>
> >>> Signed-off-by: Peng Liang <liangpen...@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 / dgilb...@redhat.com / Manchester, UK
