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