Re: [Qemu-devel] [PATCH] Save/load PC speaker internal state

Re: [Qemu-devel] [PATCH] Save/load PC speaker internal state

[Jan Kiszka]

On 2012-08-27 14:21, Pavel Dovgaluk wrote:
> Save PC speaker state to remove differences between system
> states after saving the snapshot and after loading it again.
> This patch is needed for deterministic replay of the execution.
> 
> Signed-off-by: Pavel Dovgalyuk<pavel.dovgaluk@gmail.com>
> ---
>  hw/pcspk.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pcspk.c b/hw/pcspk.c
> index e430324..3fb3dd1 100644
> --- a/hw/pcspk.c
> +++ b/hw/pcspk.c
> @@ -159,10 +159,28 @@ static const MemoryRegionOps pcspk_io_ops = {
>      },
>  };
>  
> +static const VMStateDescription vmstate_spk = {
> +    .name = "pcspk",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_UINT8_ARRAY(sample_buf, PCSpkState, PCSPK_BUF_LEN),

Can be regenerated on load.

> +        VMSTATE_UINT32(pit_count, PCSpkState),
> +        VMSTATE_UINT32(samples, PCSpkState),

Same here.

> +        VMSTATE_UINT32(play_pos, PCSpkState),
> +        VMSTATE_INT32(data_on, PCSpkState),
> +        VMSTATE_INT32(dummy_refresh_clock, PCSpkState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static int pcspk_initfn(ISADevice *dev)
>  {
>      PCSpkState *s = DO_UPCAST(PCSpkState, dev, dev);
>  
> +    vmstate_register(NULL, 0, &vmstate_spk, s);
> +

This function is for legacy users only. You have to register the vmstate
via pcspk_class_initfn (set vmsd of the device class).

>      memory_region_init_io(&s->ioport, &pcspk_io_ops, s, "elcr", 1);
>      isa_register_ioport(dev, &s->ioport, s->iobase);
> 
> 
> 
> 

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] Save/load PC speaker internal state

[Peter Maydell]

On 27 August 2012 13:21, Pavel Dovgaluk <Pavel.Dovgaluk@ispras.ru> wrote:
> Save PC speaker state to remove differences between system
> states after saving the snapshot and after loading it again.
> This patch is needed for deterministic replay of the execution.
>
> Signed-off-by: Pavel Dovgalyuk<pavel.dovgaluk@gmail.com>

Hi Pavel; thanks for this patch. Couple of minor issues:

> +static const VMStateDescription vmstate_spk = {
> +    .name = "pcspk",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_UINT8_ARRAY(sample_buf, PCSpkState, PCSPK_BUF_LEN),
> +        VMSTATE_UINT32(pit_count, PCSpkState),
> +        VMSTATE_UINT32(samples, PCSpkState),
> +        VMSTATE_UINT32(play_pos, PCSpkState),
> +        VMSTATE_INT32(data_on, PCSpkState),
> +        VMSTATE_INT32(dummy_refresh_clock, PCSpkState),

I think that you need also to update the types in the PCSpkState
struct from int/unsigned int to int32_t/uint32_t, otherwise this
won't compile on a 64 bit system.

> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static int pcspk_initfn(ISADevice *dev)
>  {
>      PCSpkState *s = DO_UPCAST(PCSpkState, dev, dev);
>
> +    vmstate_register(NULL, 0, &vmstate_spk, s);
> +

It's nicer to register the vmstate by setting
    dc->vmsd = &vmstate_spk;
in pcspk_class_initfn(); then you don't need to explicitly call
vmstate_register().

>      memory_region_init_io(&s->ioport, &pcspk_io_ops, s, "elcr", 1);
>      isa_register_ioport(dev, &s->ioport, s->iobase);

-- PMM

Re: [Qemu-devel] [PATCH] Save/load PC speaker internal state

[Pavel Dovgaluk]

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Monday, August 27, 2012 4:50 PM
> To: Pavel Dovgaluk
> Cc: qemu-devel
> Subject: Re: [Qemu-devel] [PATCH] Save/load PC speaker internal state
> 
> On 27 August 2012 13:21, Pavel Dovgaluk <Pavel.Dovgaluk@ispras.ru> wrote:
> > Save PC speaker state to remove differences between system
> > states after saving the snapshot and after loading it again.
> > This patch is needed for deterministic replay of the execution.
> >
> > Signed-off-by: Pavel Dovgalyuk<pavel.dovgaluk@gmail.com>
> 
> Hi Pavel; thanks for this patch. Couple of minor issues:
> 
> > +static const VMStateDescription vmstate_spk = {
> > +    .name = "pcspk",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .minimum_version_id_old = 1,
> > +    .fields      = (VMStateField[]) {
> > +        VMSTATE_UINT8_ARRAY(sample_buf, PCSpkState, PCSPK_BUF_LEN),
> > +        VMSTATE_UINT32(pit_count, PCSpkState),
> > +        VMSTATE_UINT32(samples, PCSpkState),
> > +        VMSTATE_UINT32(play_pos, PCSpkState),
> > +        VMSTATE_INT32(data_on, PCSpkState),
> > +        VMSTATE_INT32(dummy_refresh_clock, PCSpkState),
> 
> I think that you need also to update the types in the PCSpkState
> struct from int/unsigned int to int32_t/uint32_t, otherwise this
> won't compile on a 64 bit system.

  Peter, Jan, thank you for the reviews. I've sent new version of the patch.

Pavel Dovgaluk

[Qemu-devel] [PATCH] Save/load PC speaker internal state

[Pavel Dovgaluk]

Save PC speaker state to remove differences between system
states after saving the snapshot and after loading it again.
This patch is needed for deterministic replay of the execution.

Signed-off-by: Pavel Dovgalyuk<pavel.dovgaluk@gmail.com>
---
 hw/pcspk.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/hw/pcspk.c b/hw/pcspk.c
index e430324..3fb3dd1 100644
--- a/hw/pcspk.c
+++ b/hw/pcspk.c
@@ -159,10 +159,28 @@ static const MemoryRegionOps pcspk_io_ops = {
     },
 };
 
+static const VMStateDescription vmstate_spk = {
+    .name = "pcspk",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT8_ARRAY(sample_buf, PCSpkState, PCSPK_BUF_LEN),
+        VMSTATE_UINT32(pit_count, PCSpkState),
+        VMSTATE_UINT32(samples, PCSpkState),
+        VMSTATE_UINT32(play_pos, PCSpkState),
+        VMSTATE_INT32(data_on, PCSpkState),
+        VMSTATE_INT32(dummy_refresh_clock, PCSpkState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static int pcspk_initfn(ISADevice *dev)
 {
     PCSpkState *s = DO_UPCAST(PCSpkState, dev, dev);
 
+    vmstate_register(NULL, 0, &vmstate_spk, s);
+
     memory_region_init_io(&s->ioport, &pcspk_io_ops, s, "elcr", 1);
     isa_register_ioport(dev, &s->ioport, s->iobase);

Re: [Qemu-devel] [PATCH] Save/load PC speaker internal state

[Andreas Färber]

Am 27.08.2012 14:21, schrieb Pavel Dovgaluk:
> Save PC speaker state to remove differences between system
> states after saving the snapshot and after loading it again.
> This patch is needed for deterministic replay of the execution.
> 
> Signed-off-by: Pavel Dovgalyuk<pavel.dovgaluk@gmail.com>
> ---
>  hw/pcspk.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pcspk.c b/hw/pcspk.c
> index e430324..3fb3dd1 100644
> --- a/hw/pcspk.c
> +++ b/hw/pcspk.c
> @@ -159,10 +159,28 @@ static const MemoryRegionOps pcspk_io_ops = {
>      },
>  };
>  
> +static const VMStateDescription vmstate_spk = {
> +    .name = "pcspk",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_UINT8_ARRAY(sample_buf, PCSpkState, PCSPK_BUF_LEN),
> +        VMSTATE_UINT32(pit_count, PCSpkState),
> +        VMSTATE_UINT32(samples, PCSpkState),
> +        VMSTATE_UINT32(play_pos, PCSpkState),
> +        VMSTATE_INT32(data_on, PCSpkState),
> +        VMSTATE_INT32(dummy_refresh_clock, PCSpkState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static int pcspk_initfn(ISADevice *dev)
>  {
>      PCSpkState *s = DO_UPCAST(PCSpkState, dev, dev);
>  
> +    vmstate_register(NULL, 0, &vmstate_spk, s);
> +
>      memory_region_init_io(&s->ioport, &pcspk_io_ops, s, "elcr", 1);
>      isa_register_ioport(dev, &s->ioport, s->iobase);
> 

What about pc-1.0 etc. machines? Does this need to be backported to
stable branches first for migration from v1.2 to work? Or is such
incoming excess data silently ignored? Or any compat magic required on top?

Andreas

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

Re: [Qemu-devel] [PATCH] Save/load PC speaker internal state

[Juan Quintela]

Andreas Färber <afaerber@suse.de> wrote:
> Am 27.08.2012 14:21, schrieb Pavel Dovgaluk:
>> Save PC speaker state to remove differences between system
>> states after saving the snapshot and after loading it again.
>> This patch is needed for deterministic replay of the execution.
>> 
>
> What about pc-1.0 etc. machines? Does this need to be backported to
> stable branches first for migration from v1.2 to work? Or is such
> incoming excess data silently ignored? Or any compat magic required on top?

Is there a way to detect if we have ever used the speaker?  Otherwise,
it is difficult to know if we should sent it or not.

Later, Juan.