[Qemu-devel] [PATCH] i2c: factor out VMSD to parent class

[Qemu-devel] [PATCH] i2c: factor out VMSD to parent class

[Peter A. G. Crosthwaite]

Hi All. PMM raised a query on a recent series of mine (the SSI series) about
handling VMSD for devices which define state at multiple levels of the QOM
heirachy. Rather than complicate the discussion over in my series im trying to
start the discussion with an existing subsystem - i2c. This patch is a first
attempt at trying to get the VMSD for generic I2C state factored out of the
individual devices and handled transparently by the super class (I2C_SLAVE).

I have applied the change to only the one I2C device (max7310). If we were going
to run with this, the change pattern would be applied to all I2C devices.

This patch is not a merge proposal it is RFC only.

Please review and let us know if this is flawed or not. What needs to be done to
get this multi-level VMSD going?

I will use whatever review I get to fix my SSI series as well as fix I2C.

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 hw/i2c.c     |    2 ++
 hw/i2c.h     |    8 --------
 hw/max7310.c |    1 -
 3 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/hw/i2c.c b/hw/i2c.c
index 296bece..17e1633 100644
--- a/hw/i2c.c
+++ b/hw/i2c.c
@@ -207,6 +207,8 @@ static int i2c_slave_qdev_init(DeviceState *dev)
     I2CSlave *s = I2C_SLAVE_FROM_QDEV(dev);
     I2CSlaveClass *sc = I2C_SLAVE_GET_CLASS(s);
 
+    vmstate_register(NULL, 0, &vmstate_i2c_slave, s);
+
     return sc->init(s);
 }
 
diff --git a/hw/i2c.h b/hw/i2c.h
index 0f5682b..5b75ecc 100644
--- a/hw/i2c.h
+++ b/hw/i2c.h
@@ -81,12 +81,4 @@ void lm832x_key_event(DeviceState *dev, int key, int state);
 
 extern const VMStateDescription vmstate_i2c_slave;
 
-#define VMSTATE_I2C_SLAVE(_field, _state) {                          \
-    .name       = (stringify(_field)),                               \
-    .size       = sizeof(I2CSlave),                                  \
-    .vmsd       = &vmstate_i2c_slave,                                \
-    .flags      = VMS_STRUCT,                                        \
-    .offset     = vmstate_offset_value(_state, _field, I2CSlave),    \
-}
-
 #endif
diff --git a/hw/max7310.c b/hw/max7310.c
index 1ed18ba..9375691 100644
--- a/hw/max7310.c
+++ b/hw/max7310.c
@@ -156,7 +156,6 @@ static const VMStateDescription vmstate_max7310 = {
         VMSTATE_UINT8(polarity, MAX7310State),
         VMSTATE_UINT8(status, MAX7310State),
         VMSTATE_UINT8(command, MAX7310State),
-        VMSTATE_I2C_SLAVE(i2c, MAX7310State),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH] i2c: factor out VMSD to parent class

[Juan Quintela]

"Peter A. G. Crosthwaite" <peter.crosthwaite@petalogix.com> wrote:
> Hi All. PMM raised a query on a recent series of mine (the SSI series) about
> handling VMSD for devices which define state at multiple levels of the QOM
> heirachy. Rather than complicate the discussion over in my series im trying to
> start the discussion with an existing subsystem - i2c. This patch is a first
> attempt at trying to get the VMSD for generic I2C state factored out of the
> individual devices and handled transparently by the super class (I2C_SLAVE).
>
> I have applied the change to only the one I2C device (max7310). If we were going
> to run with this, the change pattern would be applied to all I2C devices.
>
> This patch is not a merge proposal it is RFC only.
>
> Please review and let us know if this is flawed or not. What needs to be done to
> get this multi-level VMSD going?
>
> I will use whatever review I get to fix my SSI series as well as fix I2C.
>
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>

This series move data from one part to another (obvious), now the
questions:
- how do you know that one part of the data relates to the same device
  on the other side?  I don't know about i2c, I am hoping for an answer
  O;-)  This will make that the state of the device will be sent in two
  chunks, so we should make sure that the two chunks ends in the same
  device.

- This makes the change completely uncompatible.  What boards use
  i2c/ssi?  My understanding is that it is only used outside of
  x86(_64), if so, we can live without backward compatibility.  We
  should increase the version numbers.  Something like that on addition.

static const VMStateDescription vmstate_max7310 = {
    .name = "max7310",
-   .version_id = 0,
-   .minimum_version_id = 0,
-   .minimum_version_id_old = 0,
+   .version_id = 1,
+   .minimum_version_id = 1,
+   .minimum_version_id_old = 1,


   And that should make it.

- If you ask me, I would very much preffer something like PCI devices,
  where the 1st field of any specific device is the i2c part.  This
  would achieve two things:
   * all i2c devices would have the common fields at the beggining
   * we sent the data for one device in one go, so we will never had
     trouble making sure that both devices arrive at the same time, in
     the right order, etc.

- I guess there is same reasy why you want to split the device state,
  it could be on the other series where I haven't read it though.

Later, Juan.

Re: [Qemu-devel] [PATCH] i2c: factor out VMSD to parent class

[Peter Maydell]

On 14 August 2012 09:27, Juan Quintela <quintela@redhat.com> wrote:
> "Peter A. G. Crosthwaite" <peter.crosthwaite@petalogix.com> wrote:
>> Hi All. PMM raised a query on a recent series of mine (the SSI series) about
>> handling VMSD for devices which define state at multiple levels of the QOM
>> heirachy.

> - If you ask me, I would very much preffer something like PCI devices,
>   where the 1st field of any specific device is the i2c part.  This
>   would achieve two things:
>    * all i2c devices would have the common fields at the beggining
>    * we sent the data for one device in one go, so we will never had
>      trouble making sure that both devices arrive at the same time, in
>      the right order, etc.
>
> - I guess there is same reasy why you want to split the device state,
>   it could be on the other series where I haven't read it though.

Really I'm just trying to get clarification on how class hierarchies should
handle vmstate. At the moment any device which is a subclass of i2c
has a VMSTATE_I2C_SLAVE field corresponding to the element in
its struct which is its parent object. This seems a bit odd because
surely the parent class should be responsible for its own save/load?
I'm also not sure we do this consistently through the whole QOM
hierarchy.

So what I'm after is not necessarily a patch so much as a decision about
which way this should be handled. This would probably be good to put
in a section of Anthony's QOM style guide...

-- PMM

Re: [Qemu-devel] [PATCH] i2c: factor out VMSD to parent class

[Peter Crosthwaite]

On Tue, Aug 14, 2012 at 6:46 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 14 August 2012 09:27, Juan Quintela <quintela@redhat.com> wrote:
>> "Peter A. G. Crosthwaite" <peter.crosthwaite@petalogix.com> wrote:
>>> Hi All. PMM raised a query on a recent series of mine (the SSI series) about
>>> handling VMSD for devices which define state at multiple levels of the QOM
>>> heirachy.
>
>> - If you ask me, I would very much preffer something like PCI devices,
>>   where the 1st field of any specific device is the i2c part.  This
>>   would achieve two things:
>>    * all i2c devices would have the common fields at the beggining
>>    * we sent the data for one device in one go, so we will never had
>>      trouble making sure that both devices arrive at the same time, in
>>      the right order, etc.
>>
>> - I guess there is same reasy why you want to split the device state,
>>   it could be on the other series where I haven't read it though.

So this is exactly what I have done in the SSI. Correct me if I am
wrong but it is the same setup as PCI where the VMSTATE_PCI_DEVICE
(VMSTATE_SSI_SLAVE in my case) is the first field. All I need to do is
bump version numbers?

Regards,
Peter

>
> Really I'm just trying to get clarification on how class hierarchies should
> handle vmstate. At the moment any device which is a subclass of i2c
> has a VMSTATE_I2C_SLAVE field corresponding to the element in
> its struct which is its parent object. This seems a bit odd because
> surely the parent class should be responsible for its own save/load?

Yes I agree. Means you have to define the super class in two places
rather than one (one in the class definition and one in the VMSD). The
problem is if people stick to the guns over backwards compatibility
then this proposed cleanup will never happen. So unless we can fix it
globally and bite the bullet on a pritty much a global version number
bump, then we might as well stick with the current system for the sake
of consistency rather than PCI and I2C working one way and SSI using
something completely different.

> I'm also not sure we do this consistently through the whole QOM
> hierarchy.
>

Ditto on getting consistency going, someone is going to have to sign
off on a backwards incompatibility.

Regards,
Peter

> So what I'm after is not necessarily a patch so much as a decision about
> which way this should be handled. This would probably be good to put
> in a section of Anthony's QOM style guide...
>
> -- PMM

Re: [Qemu-devel] [PATCH] i2c: factor out VMSD to parent class

[Juan Quintela]

Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote:
> On Tue, Aug 14, 2012 at 6:46 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 14 August 2012 09:27, Juan Quintela <quintela@redhat.com> wrote:
>>> "Peter A. G. Crosthwaite" <peter.crosthwaite@petalogix.com> wrote:
>>>> Hi All. PMM raised a query on a recent series of mine (the SSI series) about
>>>> handling VMSD for devices which define state at multiple levels of the QOM
>>>> heirachy.
>>
>>> - If you ask me, I would very much preffer something like PCI devices,
>>>   where the 1st field of any specific device is the i2c part.  This
>>>   would achieve two things:
>>>    * all i2c devices would have the common fields at the beggining
>>>    * we sent the data for one device in one go, so we will never had
>>>      trouble making sure that both devices arrive at the same time, in
>>>      the right order, etc.
>>>
>>> - I guess there is same reasy why you want to split the device state,
>>>   it could be on the other series where I haven't read it though.
>
> So this is exactly what I have done in the SSI. Correct me if I am
> wrong but it is the same setup as PCI where the VMSTATE_PCI_DEVICE
> (VMSTATE_SSI_SLAVE in my case) is the first field. All I need to do is
> bump version numbers?

I think so.  What boards normally use SSI?

Later, Juan.

Re: [Qemu-devel] [PATCH] i2c: factor out VMSD to parent class

[Peter Crosthwaite]

>>>>
>>>> - I guess there is same reasy why you want to split the device state,
>>>>   it could be on the other series where I haven't read it though.
>>
>> So this is exactly what I have done in the SSI. Correct me if I am
>> wrong but it is the same setup as PCI where the VMSTATE_PCI_DEVICE
>> (VMSTATE_SSI_SLAVE in my case) is the first field. All I need to do is
>> bump version numbers?
>
> I think so.  What boards normally use SSI?
>

Tracing all the calls to ssi_create_bus() at the moment we have
stellaris, spitz, highbank, gumstix, mainstone, z2, tosa & collie.

Regards,
Peter

> Later, Juan.

Re: [Qemu-devel] [PATCH] i2c: factor out VMSD to parent class

[Juan Quintela]

Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote:
>>>>>
>>>>> - I guess there is same reasy why you want to split the device state,
>>>>>   it could be on the other series where I haven't read it though.
>>>
>>> So this is exactly what I have done in the SSI. Correct me if I am
>>> wrong but it is the same setup as PCI where the VMSTATE_PCI_DEVICE
>>> (VMSTATE_SSI_SLAVE in my case) is the first field. All I need to do is
>>> bump version numbers?
>>
>> I think so.  What boards normally use SSI?
>>
>
> Tracing all the calls to ssi_create_bus() at the moment we have
> stellaris, spitz, highbank, gumstix, mainstone, z2, tosa & collie.

Then we can change it without regards to backwards compatibility
(migration is still not working correctly there), so backwards
compatibility is the less of the troubles.

Later, Juan.