[Qemu-devel] [PATCH v4 5/9] hw/timer: QOM'ify m48txx_sysbus (pass 2)

[Qemu-devel] [PATCH v4 5/9] hw/timer: QOM'ify m48txx_sysbus (pass 2)

[xiaoqiang zhao]

assign DeviceClass::vmsd instead of using vmstate_register function

Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com>
---
 hw/timer/m48t59.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
index 3c683aa..b0cf79d 100644
--- a/hw/timer/m48t59.c
+++ b/hw/timer/m48t59.c
@@ -742,8 +742,6 @@ static void m48t59_realize_common(M48t59State *s, Error **errp)
         s->wd_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &watchdog_cb, s);
     }
     qemu_get_timedate(&s->alarm, 0);
-
-    vmstate_register(NULL, -1, &vmstate_m48t59, s);
 }
 
 static void m48t59_isa_realize(DeviceState *dev, Error **errp)
@@ -822,6 +820,7 @@ static void m48txx_isa_class_init(ObjectClass *klass, void *data)
     dc->realize = m48t59_isa_realize;
     dc->reset = m48t59_reset_isa;
     dc->props = m48t59_isa_properties;
+    dc->vmsd = &vmstate_m48t59;
     nc->read = m48txx_isa_read;
     nc->write = m48txx_isa_write;
     nc->toggle_lock = m48txx_isa_toggle_lock;
@@ -866,6 +865,7 @@ static void m48txx_sysbus_class_init(ObjectClass *klass, void *data)
     dc->realize = m48t59_realize;
     dc->reset = m48t59_reset_sysbus;
     dc->props = m48t59_sysbus_properties;
+    dc->vmsd = &vmstate_m48t59;
     nc->read = m48txx_sysbus_read;
     nc->write = m48txx_sysbus_write;
     nc->toggle_lock = m48txx_sysbus_toggle_lock;
-- 
2.1.4

Re: [Qemu-devel] [PATCH v4 5/9] hw/timer: QOM'ify m48txx_sysbus (pass 2)

[Peter Maydell]

On 22 February 2016 at 09:11, xiaoqiang zhao <zxq_yx_007@163.com> wrote:
> assign DeviceClass::vmsd instead of using vmstate_register function
>
> Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com>
> ---
>  hw/timer/m48t59.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
> index 3c683aa..b0cf79d 100644
> --- a/hw/timer/m48t59.c
> +++ b/hw/timer/m48t59.c
> @@ -742,8 +742,6 @@ static void m48t59_realize_common(M48t59State *s, Error **errp)
>          s->wd_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &watchdog_cb, s);
>      }
>      qemu_get_timedate(&s->alarm, 0);
> -
> -    vmstate_register(NULL, -1, &vmstate_m48t59, s);
>  }
>
>  static void m48t59_isa_realize(DeviceState *dev, Error **errp)
> @@ -822,6 +820,7 @@ static void m48txx_isa_class_init(ObjectClass *klass, void *data)
>      dc->realize = m48t59_isa_realize;
>      dc->reset = m48t59_reset_isa;
>      dc->props = m48t59_isa_properties;
> +    dc->vmsd = &vmstate_m48t59;
>      nc->read = m48txx_isa_read;
>      nc->write = m48txx_isa_write;
>      nc->toggle_lock = m48txx_isa_toggle_lock;
> @@ -866,6 +865,7 @@ static void m48txx_sysbus_class_init(ObjectClass *klass, void *data)
>      dc->realize = m48t59_realize;
>      dc->reset = m48t59_reset_sysbus;
>      dc->props = m48t59_sysbus_properties;
> +    dc->vmsd = &vmstate_m48t59;
>      nc->read = m48txx_sysbus_read;
>      nc->write = m48txx_sysbus_write;
>      nc->toggle_lock = m48txx_sysbus_toggle_lock;
> --
> 2.1.4
>

Just noticed this won't work as it is -- the vmstate
struct is for the M48t59State*, but the ISA and
Sysbus wrappers have their own structs which are
what the dc->vmsd will be wanting to operate on.
You'd need extra VMState structs I think and
somebody who knows migration better than me to say
whether that is a migration compat break.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 5/9] hw/timer: QOM'ify m48txx_sysbus (pass 2)

[hitmoon]

在 2016年02月22日 17:24, Peter Maydell 写道:
> On 22 February 2016 at 09:11, xiaoqiang zhao <zxq_yx_007@163.com> wrote:
>> assign DeviceClass::vmsd instead of using vmstate_register function
>>
>> Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com>
>> ---
>>   hw/timer/m48t59.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
>> index 3c683aa..b0cf79d 100644
>> --- a/hw/timer/m48t59.c
>> +++ b/hw/timer/m48t59.c
>> @@ -742,8 +742,6 @@ static void m48t59_realize_common(M48t59State *s, Error **errp)
>>           s->wd_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &watchdog_cb, s);
>>       }
>>       qemu_get_timedate(&s->alarm, 0);
>> -
>> -    vmstate_register(NULL, -1, &vmstate_m48t59, s);
>>   }
>>
>>   static void m48t59_isa_realize(DeviceState *dev, Error **errp)
>> @@ -822,6 +820,7 @@ static void m48txx_isa_class_init(ObjectClass *klass, void *data)
>>       dc->realize = m48t59_isa_realize;
>>       dc->reset = m48t59_reset_isa;
>>       dc->props = m48t59_isa_properties;
>> +    dc->vmsd = &vmstate_m48t59;
>>       nc->read = m48txx_isa_read;
>>       nc->write = m48txx_isa_write;
>>       nc->toggle_lock = m48txx_isa_toggle_lock;
>> @@ -866,6 +865,7 @@ static void m48txx_sysbus_class_init(ObjectClass *klass, void *data)
>>       dc->realize = m48t59_realize;
>>       dc->reset = m48t59_reset_sysbus;
>>       dc->props = m48t59_sysbus_properties;
>> +    dc->vmsd = &vmstate_m48t59;
>>       nc->read = m48txx_sysbus_read;
>>       nc->write = m48txx_sysbus_write;
>>       nc->toggle_lock = m48txx_sysbus_toggle_lock;
>> --
>> 2.1.4
>>
> Just noticed this won't work as it is -- the vmstate
> struct is for the M48t59State*, but the ISA and
> Sysbus wrappers have their own structs which are
> what the dc->vmsd will be wanting to operate on.
> You'd need extra VMState structs I think and
> somebody who knows migration better than me to say
> whether that is a migration compat break.
>
> thanks
> -- PMM
It seems that the old code also use the same vmstate structure. Maybe 
it's a common structure which will not be used at the same time.

Re: [Qemu-devel] [PATCH v4 5/9] hw/timer: QOM'ify m48txx_sysbus (pass 2)

[Peter Maydell]

On 22 February 2016 at 10:28, hitmoon <zxq_yx_007@163.com> wrote:
> 在 2016年02月22日 17:24, Peter Maydell 写道:
>> Just noticed this won't work as it is -- the vmstate
>> struct is for the M48t59State*, but the ISA and
>> Sysbus wrappers have their own structs which are
>> what the dc->vmsd will be wanting to operate on.
>> You'd need extra VMState structs I think and
>> somebody who knows migration better than me to say
>> whether that is a migration compat break.

> It seems that the old code also use the same vmstate structure. Maybe it's a
> common structure which will not be used at the same time.

The old code passes vmstate_register() a pointer to the
M48t59State, and so the offsets in the vmstate line up correctly
with the fields it wants to access. If you use dc->vmsd then the
pointer that is (implicitly) used is the pointer to the device
structure itself, which is not the same address as the
M48t59State embedded inside that struct.

You'd basically need to have extra VMState structures for the
devices themselves which just said "inside this M48txxISAState
is an M48t59State", and "inside this M48txxSysBusState is a
M48t59State" and referred to the existing vmstate for the
M48t59State.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 5/9] hw/timer: QOM'ify m48txx_sysbus (pass 2)

[xiaoqiang zhao]

>> 在 2016年2月22日，19:03，Peter Maydell <peter.maydell@linaro.org> 写道：
>> 
>>> On 22 February 2016 at 10:28, hitmoon <zxq_yx_007@163.com> wrote:
>>> 在 2016年02月22日 17:24, Peter Maydell 写道:
>>> Just noticed this won't work as it is -- the vmstate
>>> struct is for the M48t59State*, but the ISA and
>>> Sysbus wrappers have their own structs which are
>>> what the dc->vmsd will be wanting to operate on.
>>> You'd need extra VMState structs I think and
>>> somebody who knows migration better than me to say
>>> whether that is a migration compat break.
> 
>> It seems that the old code also use the same vmstate structure. Maybe it's a
>> common structure which will not be used at the same time.
> 
> The old code passes vmstate_register() a pointer to the
> M48t59State, and so the offsets in the vmstate line up correctly
> with the fields it wants to access. If you use dc->vmsd then the
> pointer that is (implicitly) used is the pointer to the device
> structure itself, which is not the same address as the
> M48t59State embedded inside that struct.
> 
> You'd basically need to have extra VMState structures for the
> devices themselves which just said "inside this M48txxISAState
> is an M48t59State", and "inside this M48txxSysBusState is a
> M48t59State" and referred to the existing vmstate for the
> M48t59State.
> 
> thanks
> -- PMM

I will have a closer look. 

Re: [Qemu-devel] [PATCH v4 5/9] hw/timer: QOM'ify m48txx_sysbus (pass 2)

[xiaoqiang zhao]

>> 在 2016年2月22日，19:03，Peter Maydell <peter.maydell@linaro.org> 写道：
>> 
>>> On 22 February 2016 at 10:28, hitmoon <zxq_yx_007@163.com> wrote:
>>> 在 2016年02月22日 17:24, Peter Maydell 写道:
>>> Just noticed this won't work as it is -- the vmstate
>>> struct is for the M48t59State*, but the ISA and
>>> Sysbus wrappers have their own structs which are
>>> what the dc->vmsd will be wanting to operate on.
>>> You'd need extra VMState structs I think and
>>> somebody who knows migration better than me to say
>>> whether that is a migration compat break.
> 
>> It seems that the old code also use the same vmstate structure. Maybe it's a
>> common structure which will not be used at the same time.
> 
> The old code passes vmstate_register() a pointer to the
> M48t59State, and so the offsets in the vmstate line up correctly
> with the fields it wants to access. If you use dc->vmsd then the
> pointer that is (implicitly) used is the pointer to the device
> structure itself, which is not the same address as the
> M48t59State embedded inside that struct.
> 
> You'd basically need to have extra VMState structures for the
> devices themselves which just said "inside this M48txxISAState
> is an M48t59State", and "inside this M48txxSysBusState is a
> M48t59State" and referred to the existing vmstate for the
> M48t59State.
> 
> thanks
> -- PMM

I will have a closer look.

Re: [Qemu-devel] [PATCH v4 5/9] hw/timer: QOM'ify m48txx_sysbus (pass 2)

[hitmoon]

在 2016年02月22日 19:03, Peter Maydell 写道:
> On 22 February 2016 at 10:28, hitmoon <zxq_yx_007@163.com> wrote:
>> 在 2016年02月22日 17:24, Peter Maydell 写道:
>>> Just noticed this won't work as it is -- the vmstate
>>> struct is for the M48t59State*, but the ISA and
>>> Sysbus wrappers have their own structs which are
>>> what the dc->vmsd will be wanting to operate on.
>>> You'd need extra VMState structs I think and
>>> somebody who knows migration better than me to say
>>> whether that is a migration compat break.
>> It seems that the old code also use the same vmstate structure. Maybe it's a
>> common structure which will not be used at the same time.
> The old code passes vmstate_register() a pointer to the
> M48t59State, and so the offsets in the vmstate line up correctly
> with the fields it wants to access. If you use dc->vmsd then the
> pointer that is (implicitly) used is the pointer to the device
> structure itself, which is not the same address as the
> M48t59State embedded inside that struct.
>
> You'd basically need to have extra VMState structures for the
> devices themselves which just said "inside this M48txxISAState
> is an M48t59State", and "inside this M48txxSysBusState is a
> M48t59State" and referred to the existing vmstate for the
> M48t59State.
>
> thanks
> -- PMM

I add a new  structure for m48t59_isa as follows:

static const VMStateDescription vmstate_m48t59_isa = {
     .name = "m48t59-isa",
     .version_id = 1,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(state.lock, M48txxISAState),
         VMSTATE_UINT16(state.addr, M48txxISAState),
         VMSTATE_VBUFFER_UINT32(state.buffer, M48txxISAState, 0, NULL, 
0, state.size),
         VMSTATE_END_OF_LIST()
     }
};

is this correct?

Re: [Qemu-devel] [PATCH v4 5/9] hw/timer: QOM'ify m48txx_sysbus (pass 2)

[Peter Maydell]

On 23 February 2016 at 05:11, hitmoon <zxq_yx_007@163.com> wrote:
>
> 在 2016年02月22日 19:03, Peter Maydell 写道:
>> You'd basically need to have extra VMState structures for the
>> devices themselves which just said "inside this M48txxISAState
>> is an M48t59State", and "inside this M48txxSysBusState is a
>> M48t59State" and referred to the existing vmstate for the
>> M48t59State.

> I add a new  structure for m48t59_isa as follows:
>
> static const VMStateDescription vmstate_m48t59_isa = {
>     .name = "m48t59-isa",
>     .version_id = 1,
>     .minimum_version_id = 1,
>     .fields = (VMStateField[]) {
>         VMSTATE_UINT8(state.lock, M48txxISAState),
>         VMSTATE_UINT16(state.addr, M48txxISAState),
>         VMSTATE_VBUFFER_UINT32(state.buffer, M48txxISAState, 0, NULL, 0,
> state.size),
>         VMSTATE_END_OF_LIST()
>     }
> };
>
> is this correct?

No. I said you needed to have a vmstate struct that said
"inside this M48txxISAState is a M48t59State", and your
suggested struct is not doing that.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 5/9] hw/timer: QOM'ify m48txx_sysbus (pass 2)

[hitmoon]

在 2016年02月23日 17:04, Peter Maydell 写道:
> On 23 February 2016 at 05:11, hitmoon <zxq_yx_007@163.com> wrote:
>> 在 2016年02月22日 19:03, Peter Maydell 写道:
>>> You'd basically need to have extra VMState structures for the
>>> devices themselves which just said "inside this M48txxISAState
>>> is an M48t59State", and "inside this M48txxSysBusState is a
>>> M48t59State" and referred to the existing vmstate for the
>>> M48t59State.
>> I add a new  structure for m48t59_isa as follows:
>>
>> static const VMStateDescription vmstate_m48t59_isa = {
>>      .name = "m48t59-isa",
>>      .version_id = 1,
>>      .minimum_version_id = 1,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_UINT8(state.lock, M48txxISAState),
>>          VMSTATE_UINT16(state.addr, M48txxISAState),
>>          VMSTATE_VBUFFER_UINT32(state.buffer, M48txxISAState, 0, NULL, 0,
>> state.size),
>>          VMSTATE_END_OF_LIST()
>>      }
>> };
>>
>> is this correct?
> No. I said you needed to have a vmstate struct that said
> "inside this M48txxISAState is a M48t59State", and your
> suggested struct is not doing that.
>
> thanks
> -- PMM
I still can NOT understand your intention properly. Can you explain more 
clearly?

Re: [Qemu-devel] [PATCH v4 5/9] hw/timer: QOM'ify m48txx_sysbus (pass 2)

[Peter Maydell]

On 23 February 2016 at 10:02, hitmoon <zxq_yx_007@163.com> wrote:
> I still can NOT understand your intention properly. Can you explain more
> clearly?

VMState structures should generally mirror the structs
they are saving/restoring. The M48txxISAState has an entry
that just says "state is an M48t59State struct. Your
VMState for M48txxISAState should therefore also have an
entry saying "state is an M48t59State struct", it should
not manually include all the fields in state. If you
look for uses of VMSTATE_STRUCT in the codebase you'll
find some examples.

You also need to be able to test that migration works
if you save data on a QEMU built before you make this change
and then reload the data after it. So you need to find a test
setup which uses this device (ie a working command line for
a machine that uses it, preferably with a real guest) and make
sure you can do vmsave/load.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 5/9] hw/timer: QOM'ify m48txx_sysbus (pass 2)

[hitmoon]

在 2016年02月23日 18:26, Peter Maydell 写道:
> On 23 February 2016 at 10:02, hitmoon <zxq_yx_007@163.com> wrote:
>> I still can NOT understand your intention properly. Can you explain more
>> clearly?
> VMState structures should generally mirror the structs
> they are saving/restoring. The M48txxISAState has an entry
> that just says "state is an M48t59State struct. Your
> VMState for M48txxISAState should therefore also have an
> entry saying "state is an M48t59State struct", it should
> not manually include all the fields in state. If you
> look for uses of VMSTATE_STRUCT in the codebase you'll
> find some examples.
>
> You also need to be able to test that migration works
> if you save data on a QEMU built before you make this change
> and then reload the data after it. So you need to find a test
> setup which uses this device (ie a working command line for
> a machine that uses it, preferably with a real guest) and make
> sure you can do vmsave/load.
>
> thanks
> -- PMM
  Got it !
  Thanks PMM !