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

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

[Peng Liang]

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),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.26.2

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

[Igor Mammedov]

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()
>      }
>  };

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

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

[Igor Mammedov]

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()
> > >      }
> > >  };  
> >   

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

[Dr. David Alan Gilbert]

* 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

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

[Peng Liang]

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

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

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

[Igor Mammedov]

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
> >   

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

[Dr. David Alan Gilbert]

* 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

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

[Peng Liang]

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
>>>>   
>>