Re: [PATCH] ACPI: Avoid infinite recursion when dump-vmstate

[Dr. David Alan Gilbert]

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