[Qemu-devel] [PATCH] fdc: Fix vmsave/restore regression

[Qemu-devel] [PATCH] fdc: Fix vmsave/restore regression

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2220 bytes --]

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
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),


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] fdc: Fix vmsave/restore regression

[Juan Quintela]

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.

[Qemu-devel] Re: [PATCH] fdc: Fix vmsave/restore regression

[Juan Quintela]

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.

[Qemu-devel] Re: [PATCH] fdc: Fix vmsave/restore regression

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] fdc: Fix vmsave/restore regression

[Juan Quintela]

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.