Message ID | 4AFB4059.8020607@web.de |
---|---|
State | New |
Headers | show |
Jan Kiszka <jan.kiszka@web.de> wrote: > This partly reverts 2be3783328: First, the conversion neglected to > update the opaque translation in fdc_pre_save/fdc_post_load which causes > memory corruptions on vmsave/restore. And second, we can't apply a > common translation here as DeviceState->fdctrl_t is different for sysbus > and ISA. I finished today the proper patch. Please don't apply this one. > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > > hw/fdc.c | 5 ++--- > 1 files changed, 2 insertions(+), 3 deletions(-) > > *** NOTE *** > 'git shortlog|grep "reset + vmsd"' shows 10 such conversions. I only > briefly checked the first one, and it looks similar broken. Could > someone have a second look at them? Maybe it is also better to define a > vmsd opaque in DeviceInfo, which would also allow to solve this issue > differently. It looks like a plan. I am in the middle of trying to get migration working, and have at least another 2 patches (appart from the one already in staging). I am in the last round of testing. Later, Juan.
Jan Kiszka <jan.kiszka@web.de> wrote: > > *** NOTE *** > 'git shortlog|grep "reset + vmsd"' shows 10 such conversions. I only > briefly checked the first one, and it looks similar broken. Could > someone have a second look at them? Maybe it is also better to define a > vmsd opaque in DeviceInfo, which would also allow to solve this issue > differently. > I searched for .qdev.vmsd, and all the other uses are right as far as I can see. Later, Juan.
Juan Quintela wrote: > Jan Kiszka <jan.kiszka@web.de> wrote: >> *** NOTE *** >> 'git shortlog|grep "reset + vmsd"' shows 10 such conversions. I only >> briefly checked the first one, and it looks similar broken. Could >> someone have a second look at them? Maybe it is also better to define a >> vmsd opaque in DeviceInfo, which would also allow to solve this issue >> differently. >> > > I searched for .qdev.vmsd, and all the other uses are right as far as I > can see. Maybe it works, but it doesn't look clean to me. E.g. tcx.c, vmstate_tcx_post_load: it should be called with the DeviceState as opaque value, right? Then I'm missing container_of(d, TCXState, busdev.qdev). Jan
Jan Kiszka <jan.kiszka@web.de> wrote: > Juan Quintela wrote: >> Jan Kiszka <jan.kiszka@web.de> wrote: >>> *** NOTE *** >>> 'git shortlog|grep "reset + vmsd"' shows 10 such conversions. I only >>> briefly checked the first one, and it looks similar broken. Could >>> someone have a second look at them? Maybe it is also better to define a >>> vmsd opaque in DeviceInfo, which would also allow to solve this issue >>> differently. >>> >> >> I searched for .qdev.vmsd, and all the other uses are right as far as I >> can see. > > Maybe it works, but it doesn't look clean to me. It is how qdev works :p > E.g. tcx.c, > vmstate_tcx_post_load: it should be called with the DeviceState as > opaque value, right? Then I'm missing container_of(d, TCXState, > busdev.qdev). typedef struct TCXState { SysBusDevice busdev; ... } struct SysBusDevice { DeviceState qdev; .... } As you can see, if you have a pointer to a TCXState, you also have a pointer to a DeviceState (some for PCIDevice). It needs to be the 1st value, tcx.c should really use DO_UPCAST() and not container_of. If the DeviceState is not the 1st field, qdev stops working. int qdev_init(DeviceState *dev) { ... qemu_register_reset(qdev_reset, dev); if (dev->info->vmsd) vmstate_register(-1, dev->info->vmsd, dev); .... } As you can see, if we are using qdev, what we need to check is that the type of vmstate_foo is the same that the qdev type. static const VMStateDescription vmstate_tcx = { ... .fields = (VMStateField []) { VMSTATE_UINT16(height, TCXState), ... } Important bit here is TCXState static SysBusDeviceInfo tcx_info = { ... .qdev.size = sizeof(TCXState), ^^^^^^^^ See that the value that we are creating is a TCXState, then things are right. .qdev.vmsd = &vmstate_tcx, .... }; qdev abuses void * to create OOP in C (vmstate does the same), there is not a simple way to typecheck more this. What we need is that the functions that we put in the SysBusDeviceInfo in this case, all expect a value of type TCXState in this case. It is ok that they use a subset from the start (SysBusDevice or DeviceState), but we can't do much more than that. What we do with reset: static void tcx_reset(DeviceState *d) { TCXState *s = container_of(d, TCXState, busdev.qdev); .... } is not different that static void tcx_reset(void *opaque) { TCXState *s = opaque; .... } And in the case of vmstate, we have to sent values that are not qdev based yet, i.e. we can't use this trick. We could de a vmstate_qdev_register() with the other type, but will not help so much (VMStateDescription has to still use void * inside). Later, Juan.
differently. diff --git a/hw/fdc.c b/hw/fdc.c index d2bfa71..1e1b827 100644 --- a/hw/fdc.c +++ b/hw/fdc.c @@ -1932,6 +1932,8 @@ static int fdctrl_init_common(fdctrl_t *fdctrl) DMA_register_channel(fdctrl->dma_chann, &fdctrl_transfer_handler, fdctrl); fdctrl_connect_drives(fdctrl); + vmstate_register(-1, &vmstate_fdc, fdctrl); + return 0; } @@ -1998,7 +2000,6 @@ static ISADeviceInfo isa_fdc_info = { .qdev.name = "isa-fdc", .qdev.size = sizeof(fdctrl_isabus_t), .qdev.no_user = 1, - .qdev.vmsd = &vmstate_fdc, .qdev.reset = fdctrl_external_reset_isa, .qdev.props = (Property[]) { DEFINE_PROP_DRIVE("driveA", fdctrl_isabus_t, state.drives[0].dinfo), @@ -2011,7 +2012,6 @@ static SysBusDeviceInfo sysbus_fdc_info = { .init = sysbus_fdc_init1, .qdev.name = "sysbus-fdc", .qdev.size = sizeof(fdctrl_sysbus_t), - .qdev.vmsd = &vmstate_fdc, .qdev.reset = fdctrl_external_reset_sysbus, .qdev.props = (Property[]) { DEFINE_PROP_DRIVE("driveA", fdctrl_sysbus_t, state.drives[0].dinfo), @@ -2024,7 +2024,6 @@ static SysBusDeviceInfo sun4m_fdc_info = { .init = sun4m_fdc_init1, .qdev.name = "SUNW,fdtwo", .qdev.size = sizeof(fdctrl_sysbus_t), - .qdev.vmsd = &vmstate_fdc, .qdev.reset = fdctrl_external_reset_sysbus, .qdev.props = (Property[]) { DEFINE_PROP_DRIVE("drive", fdctrl_sysbus_t, state.drives[0].dinfo),
This partly reverts 2be3783328: First, the conversion neglected to update the opaque translation in fdc_pre_save/fdc_post_load which causes memory corruptions on vmsave/restore. And second, we can't apply a common translation here as DeviceState->fdctrl_t is different for sysbus and ISA. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- hw/fdc.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-) *** NOTE *** 'git shortlog|grep "reset + vmsd"' shows 10 such conversions. I only briefly checked the first one, and it looks similar broken. Could someone have a second look at them? Maybe it is also better to define a vmsd opaque in DeviceInfo, which would also allow to solve this issue