Re: [Qemu-devel] [PATCH v7 02/13] ssi: Implemented CS behaviour

[Peter Crosthwaite]

On Mon, Sep 24, 2012 at 8:19 PM, Peter Crosthwaite
<address@hidden> wrote:
> On Mon, Sep 24, 2012 at 7:29 PM, Peter Maydell <address@hidden> wrote:
>> On 24 September 2012 10:18, Peter A. G. Crosthwaite
>> <address@hidden> wrote:
>>> @@ -296,10 +297,13 @@ static void ssd0323_save(QEMUFile *f, void *opaque)
>>>      qemu_put_be32(f, s->remap);
>>>      qemu_put_be32(f, s->mode);
>>>      qemu_put_buffer(f, s->framebuffer, sizeof(s->framebuffer));
>>> +
>>> +    qemu_put_be32(f, ss->cs);
>>>  }
>>>
>>>  static int ssd0323_load(QEMUFile *f, void *opaque, int version_id)
>>>  {
>>> +    SSISlave *ss = SSI_SLAVE(opaque);
>>>      ssd0323_state *s = (ssd0323_state *)opaque;
>>>      int i;
>>>
>>> @@ -321,6 +325,8 @@ static int ssd0323_load(QEMUFile *f, void *opaque, int 
>>> version_id)
>>>      s->mode = qemu_get_be32(f);
>>>      qemu_get_buffer(f, s->framebuffer, sizeof(s->framebuffer));
>>>
>>> +    ss->cs = qemu_get_be32(f);
>>> +
>>>      return 0;
>>>  }
>>
>> This looks odd. The cs field isn't part of the ssd0323_state,
>> so it shouldn't be ssd0303_save/load's job to save and restore
>> it. Contrast the way the vmstate-based devices don't directly
>> save/restore cs but defer to VMSTATE_SSI_SLAVE.
>>
>
> I took a "when in rome" philosophy here, and did it the way the device
> model already does VMSD. The rest of the device state is encapsulated
> using this mechanism. Im guessing this is an old way of doing VMSD
> with the whole register_savevm() thing. Is it possible to mix and
> match this old file-style VMSD with the newer struct approach? I cant
> see an easy way without completely rewriting the VMSD implementation
> of this device model.
>

Hi Juan,

We have another curly VMSD question on this series. The hw/ssd0323.c
uses the old school register_savevm() scheme for VMSD. With this
series, this device now inherits from another device (TYPE_SSI_SLAVE)
which has proper VMSD support. The question is can, we mix and match
the old and new VMSD to avoid having to rewrite this device models
VMSD? Can we leave this device model vmsd as is and add the vm_state
with the single SSI_SLAVE field? This vmsd would then save/load the
inherited state and co-exist with the register_savevm() implementation
for the non-inherited state.

Regards,
Peter

> Regards,
> Peter
>
>> -- PMM