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

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]

On 2012-08-28 08:58, 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.
> 
> v2: Do not save the data that cannot affect the guest code behavior.
> 
> Signed-off-by: Pavel Dovgalyuk<pavel.dovgaluk@gmail.com>
> ---
>  hw/pcspk.c |   17 +++++++++++++++--
>  1 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pcspk.c b/hw/pcspk.c
> index e430324..07a3de0 100644
> --- a/hw/pcspk.c
> +++ b/hw/pcspk.c
> @@ -46,8 +46,8 @@ typedef struct {
>      unsigned int pit_count;
>      unsigned int samples;
>      unsigned int play_pos;
> -    int data_on;
> -    int dummy_refresh_clock;
> +    uint8_t data_on;
> +    uint8_t dummy_refresh_clock;
>  } PCSpkState;
>  
>  static const char *s_spk = "pcspk";
> @@ -159,6 +159,18 @@ 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(data_on, PCSpkState),
> +        VMSTATE_UINT8(dummy_refresh_clock, PCSpkState),

Just to understand the plan: The intention is to leave the audio
disabled after resume, just saving those bits that can be read back via
IO port, correct? Then please document this in a comment, otherwise fix it.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

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

[Pavel Dovgaluk]

> -----Original Message-----
> From: Jan Kiszka [mailto:jan.kiszka@web.de]
> Sent: Tuesday, August 28, 2012 11:13 AM
> To: Pavel Dovgaluk
> Cc: 'qemu-devel'
> Subject: Re: [PATCH] Save/load PC speaker internal state (v2)
> 
> On 2012-08-28 08:58, 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.
> >
> > v2: Do not save the data that cannot affect the guest code behavior.
> >
> > Signed-off-by: Pavel Dovgalyuk<pavel.dovgaluk@gmail.com>
> > ---
> >  hw/pcspk.c |   17 +++++++++++++++--
> >  1 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/pcspk.c b/hw/pcspk.c
> > index e430324..07a3de0 100644
> > --- a/hw/pcspk.c
> > +++ b/hw/pcspk.c
> > @@ -46,8 +46,8 @@ typedef struct {
> >      unsigned int pit_count;
> >      unsigned int samples;
> >      unsigned int play_pos;
> > -    int data_on;
> > -    int dummy_refresh_clock;
> > +    uint8_t data_on;
> > +    uint8_t dummy_refresh_clock;
> >  } PCSpkState;
> >
> >  static const char *s_spk = "pcspk";
> > @@ -159,6 +159,18 @@ 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(data_on, PCSpkState),
> > +        VMSTATE_UINT8(dummy_refresh_clock, PCSpkState),
> 
> Just to understand the plan: The intention is to leave the audio
> disabled after resume, just saving those bits that can be read back via
> IO port, correct? Then please document this in a comment, otherwise fix it.

  The plan is to make guest system behavior deterministic.
  Resuming the audio is not needed for this but could be a useful side effect.
  I thought about solutions of this problem and got the following:
we cannot reinitialize the buffer in post_load function, because pit used for
samples generation could be not reloaded yet at that moment and the buffer's state
will be invalid in this case.
  So the only solution is saving and loading the complete state of the audio buffer.

Pavel Dovgaluk

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]

On 2012-08-29 09:05, Pavel Dovgaluk wrote:
>> -----Original Message-----
>> From: Jan Kiszka [mailto:jan.kiszka@web.de]
>> Sent: Tuesday, August 28, 2012 11:13 AM
>> To: Pavel Dovgaluk
>> Cc: 'qemu-devel'
>> Subject: Re: [PATCH] Save/load PC speaker internal state (v2)
>>
>> On 2012-08-28 08:58, 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.
>>>
>>> v2: Do not save the data that cannot affect the guest code behavior.
>>>
>>> Signed-off-by: Pavel Dovgalyuk<pavel.dovgaluk@gmail.com>
>>> ---
>>>  hw/pcspk.c |   17 +++++++++++++++--
>>>  1 files changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/pcspk.c b/hw/pcspk.c
>>> index e430324..07a3de0 100644
>>> --- a/hw/pcspk.c
>>> +++ b/hw/pcspk.c
>>> @@ -46,8 +46,8 @@ typedef struct {
>>>      unsigned int pit_count;
>>>      unsigned int samples;
>>>      unsigned int play_pos;
>>> -    int data_on;
>>> -    int dummy_refresh_clock;
>>> +    uint8_t data_on;
>>> +    uint8_t dummy_refresh_clock;
>>>  } PCSpkState;
>>>
>>>  static const char *s_spk = "pcspk";
>>> @@ -159,6 +159,18 @@ 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(data_on, PCSpkState),
>>> +        VMSTATE_UINT8(dummy_refresh_clock, PCSpkState),
>>
>> Just to understand the plan: The intention is to leave the audio
>> disabled after resume, just saving those bits that can be read back via
>> IO port, correct? Then please document this in a comment, otherwise fix it.
> 
>   The plan is to make guest system behavior deterministic.
>   Resuming the audio is not needed for this but could be a useful side effect.
>   I thought about solutions of this problem and got the following:
> we cannot reinitialize the buffer in post_load function, because pit used for
> samples generation could be not reloaded yet at that moment and the buffer's state
> will be invalid in this case.
>   So the only solution is saving and loading the complete state of the audio buffer.

Theoretically, you could save the required PIT state inside the pcspk
VMSD to solve this (still less bytes than saving the buffer). However, I
see no relevant loss in leaving audio off until the guest fiddles with
the controls again. I'm just asking for documenting this.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

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

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

v2: Do not save the data that cannot affect the guest code behavior.

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

diff --git a/hw/pcspk.c b/hw/pcspk.c
index e430324..07a3de0 100644
--- a/hw/pcspk.c
+++ b/hw/pcspk.c
@@ -46,8 +46,8 @@ typedef struct {
     unsigned int pit_count;
     unsigned int samples;
     unsigned int play_pos;
-    int data_on;
-    int dummy_refresh_clock;
+    uint8_t data_on;
+    uint8_t dummy_refresh_clock;
 } PCSpkState;
 
 static const char *s_spk = "pcspk";
@@ -159,6 +159,18 @@ 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(data_on, PCSpkState),
+        VMSTATE_UINT8(dummy_refresh_clock, PCSpkState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static int pcspk_initfn(ISADevice *dev)
 {
     PCSpkState *s = DO_UPCAST(PCSpkState, dev, dev);
@@ -185,6 +197,7 @@ static void pcspk_class_initfn(ObjectClass *klass, void *data)
     ic->init = pcspk_initfn;
     dc->no_user = 1;
     dc->props = pcspk_properties;
+    dc->vmsd = &vmstate_spk;
 }
 
 static TypeInfo pcspk_info = {