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

[Dr. David Alan Gilbert]

* 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