[Qemu-devel] [PATCH v3] hw/mips: gt64xxx_pci: Add VMStateDescription

[Qemu-devel] [PATCH v3] hw/mips: gt64xxx_pci: Add VMStateDescription

[James Hogan]

From: Sanjay Lal <sanjayl@kymasys.com>

Add VMStateDescription for GT64120 PCI emulation used by the Malta
platform, to allow it to work with savevm/loadvm and live migration.

The entire register array is saved/restored using VMSTATE_UINT32_ARRAY
(fixed length GT_REGS = 1024).

Signed-off-by: Sanjay Lal <sanjayl@kymasys.com>
[james.hogan@imgtec.com: Convert to VMState]
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
---
This is based on "[Patch 03/12] KVM/MIPS: Add save/restore state APIs
for saving/restoring KVM guests."[1].

Changes in v3:
 - Save entire register array using VMSTATE_UINT32_ARRAY (which is fixed
   length of GT_REGS = 1024) rather than individual registers. This is
   safer in case an important register is missed or new emulated
   functionality is added. (Aurelien Jarno)

Changes in v2:
 - Expand commit message
 - Convert to VMState (Peter Maydell)

[1] https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg00195.html
---
 hw/mips/gt64xxx_pci.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 6398514c99d2..22f63ce0c8af 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -312,6 +312,27 @@ static void gt64120_pci_mapping(GT64120State *s)
     }
 }
 
+static int gt64120_post_load(void *opaque, int version_id)
+{
+    GT64120State *s = opaque;
+
+    gt64120_isd_mapping(s);
+    gt64120_pci_mapping(s);
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_gt64120 = {
+    .name = "gt64120",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .post_load = gt64120_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(regs, GT64120State, GT_REGS),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void gt64120_writel (void *opaque, hwaddr addr,
                             uint64_t val, unsigned size)
 {
@@ -1174,9 +1195,11 @@ static const TypeInfo gt64120_pci_info = {
 
 static void gt64120_class_init(ObjectClass *klass, void *data)
 {
+    DeviceClass *dc = DEVICE_CLASS(klass);
     SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
 
     sdc->init = gt64120_init;
+    dc->vmsd = &vmstate_gt64120;
 }
 
 static const TypeInfo gt64120_info = {
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3] hw/mips: gt64xxx_pci: Add VMStateDescription

[James Hogan]

On 20/06/14 11:25, James Hogan wrote:
> From: Sanjay Lal <sanjayl@kymasys.com>
> 
> Add VMStateDescription for GT64120 PCI emulation used by the Malta
> platform, to allow it to work with savevm/loadvm and live migration.
> 
> The entire register array is saved/restored using VMSTATE_UINT32_ARRAY
> (fixed length GT_REGS = 1024).
> 
> Signed-off-by: Sanjay Lal <sanjayl@kymasys.com>
> [james.hogan@imgtec.com: Convert to VMState]
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> ---
> This is based on "[Patch 03/12] KVM/MIPS: Add save/restore state APIs
> for saving/restoring KVM guests."[1].
> 
> Changes in v3:
>  - Save entire register array using VMSTATE_UINT32_ARRAY (which is fixed
>    length of GT_REGS = 1024) rather than individual registers. This is
>    safer in case an important register is missed or new emulated
>    functionality is added. (Aurelien Jarno)

For the record, Aurelien's question about saving all registers or only
specific ones is still open (I also don't have enough experience to be
sure, although I prefer the approach in v3).

On 19/06/14 16:51, Aurelien Jarno wrote:
> That said there might be some drawbacks in doing that, and I don't
> really have enough experience to judge about that. It would be nice if
> some more people can give their opinion there.

Note, new emulated functionality may still require VMState versioning to
handle unexpected zero register values in restored state.

Cheers
James

Re: [Qemu-devel] [PATCH v3] hw/mips: gt64xxx_pci: Add VMStateDescription

[Aurelien Jarno]

On Fri, Jun 20, 2014 at 11:25:47AM +0100, James Hogan wrote:
> From: Sanjay Lal <sanjayl@kymasys.com>
> 
> Add VMStateDescription for GT64120 PCI emulation used by the Malta
> platform, to allow it to work with savevm/loadvm and live migration.
> 
> The entire register array is saved/restored using VMSTATE_UINT32_ARRAY
> (fixed length GT_REGS = 1024).
> 
> Signed-off-by: Sanjay Lal <sanjayl@kymasys.com>
> [james.hogan@imgtec.com: Convert to VMState]
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> ---
> This is based on "[Patch 03/12] KVM/MIPS: Add save/restore state APIs
> for saving/restoring KVM guests."[1].
> 
> Changes in v3:
>  - Save entire register array using VMSTATE_UINT32_ARRAY (which is fixed
>    length of GT_REGS = 1024) rather than individual registers. This is
>    safer in case an important register is missed or new emulated
>    functionality is added. (Aurelien Jarno)
> 
> Changes in v2:
>  - Expand commit message
>  - Convert to VMState (Peter Maydell)
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg00195.html
> ---
>  hw/mips/gt64xxx_pci.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> index 6398514c99d2..22f63ce0c8af 100644
> --- a/hw/mips/gt64xxx_pci.c
> +++ b/hw/mips/gt64xxx_pci.c
> @@ -312,6 +312,27 @@ static void gt64120_pci_mapping(GT64120State *s)
>      }
>  }
>  
> +static int gt64120_post_load(void *opaque, int version_id)
> +{
> +    GT64120State *s = opaque;
> +
> +    gt64120_isd_mapping(s);
> +    gt64120_pci_mapping(s);
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_gt64120 = {
> +    .name = "gt64120",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .post_load = gt64120_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(regs, GT64120State, GT_REGS),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static void gt64120_writel (void *opaque, hwaddr addr,
>                              uint64_t val, unsigned size)
>  {
> @@ -1174,9 +1195,11 @@ static const TypeInfo gt64120_pci_info = {
>  
>  static void gt64120_class_init(ObjectClass *klass, void *data)
>  {
> +    DeviceClass *dc = DEVICE_CLASS(klass);
>      SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
>  
>      sdc->init = gt64120_init;
> +    dc->vmsd = &vmstate_gt64120;
>  }
>  
>  static const TypeInfo gt64120_info = {


Thanks for this new version, I'll apply it in the next days unless
someone comes with good arguments why we should not do that.

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH v3] hw/mips: gt64xxx_pci: Add VMStateDescription

[Paolo Bonzini]

Il 20/06/2014 12:58, Aurelien Jarno ha scritto:
> On Fri, Jun 20, 2014 at 11:25:47AM +0100, James Hogan wrote:
>> From: Sanjay Lal <sanjayl@kymasys.com>
>>
>> Add VMStateDescription for GT64120 PCI emulation used by the Malta
>> platform, to allow it to work with savevm/loadvm and live migration.
>>
>> The entire register array is saved/restored using VMSTATE_UINT32_ARRAY
>> (fixed length GT_REGS = 1024).
>>
>> Signed-off-by: Sanjay Lal <sanjayl@kymasys.com>
>> [james.hogan@imgtec.com: Convert to VMState]
>> Signed-off-by: James Hogan <james.hogan@imgtec.com>
>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>> ---
>> This is based on "[Patch 03/12] KVM/MIPS: Add save/restore state APIs
>> for saving/restoring KVM guests."[1].
>>
>> Changes in v3:
>>  - Save entire register array using VMSTATE_UINT32_ARRAY (which is fixed
>>    length of GT_REGS = 1024) rather than individual registers. This is
>>    safer in case an important register is missed or new emulated
>>    functionality is added. (Aurelien Jarno)
>>
>> Changes in v2:
>>  - Expand commit message
>>  - Convert to VMState (Peter Maydell)
>>
>> [1] https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg00195.html
>> ---
>>  hw/mips/gt64xxx_pci.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
>> index 6398514c99d2..22f63ce0c8af 100644
>> --- a/hw/mips/gt64xxx_pci.c
>> +++ b/hw/mips/gt64xxx_pci.c
>> @@ -312,6 +312,27 @@ static void gt64120_pci_mapping(GT64120State *s)
>>      }
>>  }
>>
>> +static int gt64120_post_load(void *opaque, int version_id)
>> +{
>> +    GT64120State *s = opaque;
>> +
>> +    gt64120_isd_mapping(s);
>> +    gt64120_pci_mapping(s);
>> +
>> +    return 0;
>> +}
>> +
>> +static const VMStateDescription vmstate_gt64120 = {
>> +    .name = "gt64120",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .post_load = gt64120_post_load,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32_ARRAY(regs, GT64120State, GT_REGS),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>  static void gt64120_writel (void *opaque, hwaddr addr,
>>                              uint64_t val, unsigned size)
>>  {
>> @@ -1174,9 +1195,11 @@ static const TypeInfo gt64120_pci_info = {
>>
>>  static void gt64120_class_init(ObjectClass *klass, void *data)
>>  {
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>      SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
>>
>>      sdc->init = gt64120_init;
>> +    dc->vmsd = &vmstate_gt64120;
>>  }
>>
>>  static const TypeInfo gt64120_info = {
>
>
> Thanks for this new version, I'll apply it in the next days unless
> someone comes with good arguments why we should not do that.
>
> Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
>

FWIW it looks good to me.  It's simpler this way.

Paolo

Re: [Qemu-devel] [PATCH v3] hw/mips: gt64xxx_pci: Add VMStateDescription

[Andreas Färber]

Am 20.06.2014 12:58, schrieb Aurelien Jarno:
> Thanks for this new version, I'll apply it in the next days unless
> someone comes with good arguments why we should not do that.
> 
> Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

No objection to the code, I just think that "hw/mips:" is redundant in
the subject, although there's no precedent yet.
http://git.qemu-project.org/?p=qemu.git;a=history;f=hw/mips/gt64xxx_pci.c;h=6398514c99d2b15cd047149c8457717e352e9347;hb=HEAD

Same applies to the other MIPS patch.

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg