| Message ID | 1456132310-4826-1-git-send-email-zxq_yx_007@163.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
? 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.
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
>> ? 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.
>> ? 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.
? 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?
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
? 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?
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
? 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 !
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;
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(-)