[Qemu-devel] [PATCH 0/2] Fix vmstate for IPMI devices

[Qemu-devel] [PATCH 0/2] Fix vmstate for IPMI devices

[minyard]

There were several issues with vmstate transfers in the IPMI devices.
This new version is heavily tested and should be much better.  It will
not be possible to migrate BT devices from older systems, it was too
broken to handle that.

Just to make sure I got this right, could somebody knowledgeable
take a glance at this?

Thanks,

-corey

[Qemu-devel] [PATCH 1/2] ipmi: Use proper struct reference for KCS vmstate

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The vmstate for isa_ipmi_kcs was referencing into the kcs structure,
instead create a kcs structure separate and use that.

There was also some issues in the state transfer.  The inlen field
was not being transferred, so if a transaction was in process during
the transfer it would be messed up.  And the use_irq field was
transferred, but that should come from the configuration.  This
also fixes those issues and is tested under heavy load.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/ipmi/isa_ipmi_kcs.c | 75 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 60 insertions(+), 15 deletions(-)

diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
index 689587b..3c942d6 100644
--- a/hw/ipmi/isa_ipmi_kcs.c
+++ b/hw/ipmi/isa_ipmi_kcs.c
@@ -422,24 +422,69 @@ static void ipmi_isa_realize(DeviceState *dev, Error **errp)
     isa_register_ioport(isadev, &iik->kcs.io, iik->kcs.io_base);
 }
 
-const VMStateDescription vmstate_ISAIPMIKCSDevice = {
-    .name = TYPE_IPMI_INTERFACE,
+static const VMStateDescription vmstate_IPMIKCS = {
+    .name = TYPE_IPMI_INTERFACE_PREFIX "kcs",
     .version_id = 1,
     .minimum_version_id = 1,
     .fields      = (VMStateField[]) {
-        VMSTATE_BOOL(kcs.obf_irq_set, ISAIPMIKCSDevice),
-        VMSTATE_BOOL(kcs.atn_irq_set, ISAIPMIKCSDevice),
-        VMSTATE_BOOL(kcs.use_irq, ISAIPMIKCSDevice),
-        VMSTATE_BOOL(kcs.irqs_enabled, ISAIPMIKCSDevice),
-        VMSTATE_UINT32(kcs.outpos, ISAIPMIKCSDevice),
-        VMSTATE_UINT8_ARRAY(kcs.outmsg, ISAIPMIKCSDevice, MAX_IPMI_MSG_SIZE),
-        VMSTATE_UINT8_ARRAY(kcs.inmsg, ISAIPMIKCSDevice, MAX_IPMI_MSG_SIZE),
-        VMSTATE_BOOL(kcs.write_end, ISAIPMIKCSDevice),
-        VMSTATE_UINT8(kcs.status_reg, ISAIPMIKCSDevice),
-        VMSTATE_UINT8(kcs.data_out_reg, ISAIPMIKCSDevice),
-        VMSTATE_INT16(kcs.data_in_reg, ISAIPMIKCSDevice),
-        VMSTATE_INT16(kcs.cmd_reg, ISAIPMIKCSDevice),
-        VMSTATE_UINT8(kcs.waiting_rsp, ISAIPMIKCSDevice),
+        VMSTATE_BOOL(obf_irq_set, IPMIKCS),
+        VMSTATE_BOOL(atn_irq_set, IPMIKCS),
+        VMSTATE_BOOL(use_irq, IPMIKCS),
+        VMSTATE_BOOL(irqs_enabled, IPMIKCS),
+        VMSTATE_UINT32(outpos, IPMIKCS),
+        VMSTATE_UINT8_ARRAY(outmsg, IPMIKCS, MAX_IPMI_MSG_SIZE),
+        VMSTATE_UINT32(inlen, IPMIKCS),
+        VMSTATE_UINT8_ARRAY(inmsg, IPMIKCS, MAX_IPMI_MSG_SIZE),
+        VMSTATE_BOOL(write_end, IPMIKCS),
+        VMSTATE_UINT8(status_reg, IPMIKCS),
+        VMSTATE_UINT8(data_out_reg, IPMIKCS),
+        VMSTATE_INT16(data_in_reg, IPMIKCS),
+        VMSTATE_INT16(cmd_reg, IPMIKCS),
+        VMSTATE_UINT8(waiting_rsp, IPMIKCS),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static int isa_ipmi_kcs_load_old(QEMUFile *f, void *opaque, int version_id)
+{
+    ISAIPMIKCSDevice *iik = opaque;
+    IPMIKCS *k = &iik->kcs;
+    unsigned int i;
+
+    if (version_id != 1) {
+        return -EINVAL;
+    }
+
+    k->obf_irq_set = qemu_get_byte(f);
+    k->atn_irq_set = qemu_get_byte(f);
+    qemu_get_byte(f); /* Used to be use_irq, but that's not a good idea. */
+    k->irqs_enabled = qemu_get_byte(f);
+    k->outpos = qemu_get_be32(f);
+    for (i = 0; i < MAX_IPMI_MSG_SIZE; i++) {
+        k->outmsg[i] = qemu_get_byte(f);
+    }
+    k->inlen = 0; /* This was forgotten on version 1, just reset it. */
+    for (i = 0; i < MAX_IPMI_MSG_SIZE; i++) {
+        k->inmsg[i] = qemu_get_byte(f);
+    }
+    k->write_end = qemu_get_byte(f);
+    k->status_reg = qemu_get_byte(f);
+    k->data_out_reg = qemu_get_byte(f);
+    k->data_in_reg = qemu_get_be16(f);
+    k->cmd_reg = qemu_get_be16(f);
+    k->waiting_rsp = qemu_get_byte(f);
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_ISAIPMIKCSDevice = {
+    .name = TYPE_IPMI_INTERFACE_PREFIX "isa-kcs",
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 1,
+    .load_state_old = isa_ipmi_kcs_load_old,
+    .fields      = (VMStateField[]) {
+        VMSTATE_STRUCT(kcs, ISAIPMIKCSDevice, 1, vmstate_IPMIKCS, IPMIKCS),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] ipmi: Use proper struct reference for BT vmstate

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The vmstate for isa_ipmi_bt was referencing into the bt structure,
instead create a bt structure separate and use that.

The version 1 of the BT transfer was fairly broken, if a migration
occured during an IPMI operation, it is likely the migration would
be corrupted because I misunderstood the VMSTATE_VBUFFER_UINT32()
handling, I thought it handled transferring the length field,
too.  So I just remove support for that.  I doubt anyone is using
it at this point.

This also removes the transfer of use_irq, since that should come
from configuration.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/ipmi/isa_ipmi_bt.c | 43 ++++++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index e946030..a990ab7 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -450,22 +450,39 @@ static void isa_ipmi_bt_realize(DeviceState *dev, Error **errp)
     isa_register_ioport(isadev, &iib->bt.io, iib->bt.io_base);
 }
 
-static const VMStateDescription vmstate_ISAIPMIBTDevice = {
-    .name = TYPE_IPMI_INTERFACE,
+
+const VMStateDescription vmstate_IPMIBT = {
+    .name = TYPE_IPMI_INTERFACE_PREFIX "bt",
     .version_id = 1,
     .minimum_version_id = 1,
     .fields      = (VMStateField[]) {
-        VMSTATE_BOOL(bt.obf_irq_set, ISAIPMIBTDevice),
-        VMSTATE_BOOL(bt.atn_irq_set, ISAIPMIBTDevice),
-        VMSTATE_BOOL(bt.use_irq, ISAIPMIBTDevice),
-        VMSTATE_BOOL(bt.irqs_enabled, ISAIPMIBTDevice),
-        VMSTATE_UINT32(bt.outpos, ISAIPMIBTDevice),
-        VMSTATE_VBUFFER_UINT32(bt.outmsg, ISAIPMIBTDevice, 1, NULL, bt.outlen),
-        VMSTATE_VBUFFER_UINT32(bt.inmsg, ISAIPMIBTDevice, 1, NULL, bt.inlen),
-        VMSTATE_UINT8(bt.control_reg, ISAIPMIBTDevice),
-        VMSTATE_UINT8(bt.mask_reg, ISAIPMIBTDevice),
-        VMSTATE_UINT8(bt.waiting_rsp, ISAIPMIBTDevice),
-        VMSTATE_UINT8(bt.waiting_seq, ISAIPMIBTDevice),
+        VMSTATE_BOOL(obf_irq_set, IPMIBT),
+        VMSTATE_BOOL(atn_irq_set, IPMIBT),
+        VMSTATE_BOOL(irqs_enabled, IPMIBT),
+        VMSTATE_UINT32(outpos, IPMIBT),
+        VMSTATE_UINT32(outlen, IPMIBT),
+        VMSTATE_UINT8_ARRAY(outmsg, IPMIBT, MAX_IPMI_MSG_SIZE),
+        VMSTATE_UINT32(inlen, IPMIBT),
+        VMSTATE_UINT8_ARRAY(inmsg, IPMIBT, MAX_IPMI_MSG_SIZE),
+        VMSTATE_UINT8(control_reg, IPMIBT),
+        VMSTATE_UINT8(mask_reg, IPMIBT),
+        VMSTATE_UINT8(waiting_rsp, IPMIBT),
+        VMSTATE_UINT8(waiting_seq, IPMIBT),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_ISAIPMIBTDevice = {
+    .name = TYPE_IPMI_INTERFACE_PREFIX "isa-bt",
+    .version_id = 2,
+    .minimum_version_id = 2,
+    /*
+     * Version 1 had messed up the array transfer, it's not even usable
+     * because it used VMSTATE_VBUFFER_UINT32, but it did not transfer
+     * the buffer length, so random things would happen.
+     */
+    .fields      = (VMStateField[]) {
+        VMSTATE_STRUCT(bt, ISAIPMIBTDevice, 1, vmstate_IPMIBT, IPMIBT),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] ipmi: Use proper struct reference for KCS vmstate

[Dr. David Alan Gilbert]

* minyard@acm.org (minyard@acm.org) wrote:
> From: Corey Minyard <cminyard@mvista.com>

Hi Corey,

> The vmstate for isa_ipmi_kcs was referencing into the kcs structure,
> instead create a kcs structure separate and use that.
> 
> There was also some issues in the state transfer.  The inlen field
> was not being transferred, so if a transaction was in process during
> the transfer it would be messed up.  And the use_irq field was
> transferred, but that should come from the configuration.  This
> also fixes those issues and is tested under heavy load.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  hw/ipmi/isa_ipmi_kcs.c | 75 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 60 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
> index 689587b..3c942d6 100644
> --- a/hw/ipmi/isa_ipmi_kcs.c
> +++ b/hw/ipmi/isa_ipmi_kcs.c
> @@ -422,24 +422,69 @@ static void ipmi_isa_realize(DeviceState *dev, Error **errp)
>      isa_register_ioport(isadev, &iik->kcs.io, iik->kcs.io_base);
>  }
>  
> -const VMStateDescription vmstate_ISAIPMIKCSDevice = {
> -    .name = TYPE_IPMI_INTERFACE,
> +static const VMStateDescription vmstate_IPMIKCS = {
> +    .name = TYPE_IPMI_INTERFACE_PREFIX "kcs",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .fields      = (VMStateField[]) {
> -        VMSTATE_BOOL(kcs.obf_irq_set, ISAIPMIKCSDevice),
> -        VMSTATE_BOOL(kcs.atn_irq_set, ISAIPMIKCSDevice),
> -        VMSTATE_BOOL(kcs.use_irq, ISAIPMIKCSDevice),
> -        VMSTATE_BOOL(kcs.irqs_enabled, ISAIPMIKCSDevice),
> -        VMSTATE_UINT32(kcs.outpos, ISAIPMIKCSDevice),
> -        VMSTATE_UINT8_ARRAY(kcs.outmsg, ISAIPMIKCSDevice, MAX_IPMI_MSG_SIZE),
> -        VMSTATE_UINT8_ARRAY(kcs.inmsg, ISAIPMIKCSDevice, MAX_IPMI_MSG_SIZE),
> -        VMSTATE_BOOL(kcs.write_end, ISAIPMIKCSDevice),
> -        VMSTATE_UINT8(kcs.status_reg, ISAIPMIKCSDevice),
> -        VMSTATE_UINT8(kcs.data_out_reg, ISAIPMIKCSDevice),
> -        VMSTATE_INT16(kcs.data_in_reg, ISAIPMIKCSDevice),
> -        VMSTATE_INT16(kcs.cmd_reg, ISAIPMIKCSDevice),
> -        VMSTATE_UINT8(kcs.waiting_rsp, ISAIPMIKCSDevice),
> +        VMSTATE_BOOL(obf_irq_set, IPMIKCS),
> +        VMSTATE_BOOL(atn_irq_set, IPMIKCS),
> +        VMSTATE_BOOL(use_irq, IPMIKCS),
> +        VMSTATE_BOOL(irqs_enabled, IPMIKCS),
> +        VMSTATE_UINT32(outpos, IPMIKCS),
> +        VMSTATE_UINT8_ARRAY(outmsg, IPMIKCS, MAX_IPMI_MSG_SIZE),
> +        VMSTATE_UINT32(inlen, IPMIKCS),
> +        VMSTATE_UINT8_ARRAY(inmsg, IPMIKCS, MAX_IPMI_MSG_SIZE),

There's an easier way to do this, which means that you don't need the
load_old hook; you can do:

  VMSTATE_UINT32_V(inlen, IPMIKCS, 2);

and that field will only be loaded when loading a v2.


Now, the other thing you could do, if it's rare then inlen is none-0,
is you could add it as a subsection that only gets sent when it's
none-0; that way migration to an old qemu would work in most cases.

Dave

> +        VMSTATE_BOOL(write_end, IPMIKCS),
> +        VMSTATE_UINT8(status_reg, IPMIKCS),
> +        VMSTATE_UINT8(data_out_reg, IPMIKCS),
> +        VMSTATE_INT16(data_in_reg, IPMIKCS),
> +        VMSTATE_INT16(cmd_reg, IPMIKCS),
> +        VMSTATE_UINT8(waiting_rsp, IPMIKCS),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static int isa_ipmi_kcs_load_old(QEMUFile *f, void *opaque, int version_id)
> +{
> +    ISAIPMIKCSDevice *iik = opaque;
> +    IPMIKCS *k = &iik->kcs;
> +    unsigned int i;
> +
> +    if (version_id != 1) {
> +        return -EINVAL;
> +    }
> +
> +    k->obf_irq_set = qemu_get_byte(f);
> +    k->atn_irq_set = qemu_get_byte(f);
> +    qemu_get_byte(f); /* Used to be use_irq, but that's not a good idea. */
> +    k->irqs_enabled = qemu_get_byte(f);
> +    k->outpos = qemu_get_be32(f);
> +    for (i = 0; i < MAX_IPMI_MSG_SIZE; i++) {
> +        k->outmsg[i] = qemu_get_byte(f);
> +    }
> +    k->inlen = 0; /* This was forgotten on version 1, just reset it. */
> +    for (i = 0; i < MAX_IPMI_MSG_SIZE; i++) {
> +        k->inmsg[i] = qemu_get_byte(f);
> +    }
> +    k->write_end = qemu_get_byte(f);
> +    k->status_reg = qemu_get_byte(f);
> +    k->data_out_reg = qemu_get_byte(f);
> +    k->data_in_reg = qemu_get_be16(f);
> +    k->cmd_reg = qemu_get_be16(f);
> +    k->waiting_rsp = qemu_get_byte(f);
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_ISAIPMIKCSDevice = {
> +    .name = TYPE_IPMI_INTERFACE_PREFIX "isa-kcs",
> +    .version_id = 2,
> +    .minimum_version_id = 2,
> +    .minimum_version_id_old = 1,
> +    .load_state_old = isa_ipmi_kcs_load_old,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_STRUCT(kcs, ISAIPMIKCSDevice, 1, vmstate_IPMIKCS, IPMIKCS),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> -- 
> 2.7.4
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/2] ipmi: Use proper struct reference for BT vmstate

[Dr. David Alan Gilbert]

* minyard@acm.org (minyard@acm.org) wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> The vmstate for isa_ipmi_bt was referencing into the bt structure,
> instead create a bt structure separate and use that.
> 
> The version 1 of the BT transfer was fairly broken, if a migration
> occured during an IPMI operation, it is likely the migration would
> be corrupted because I misunderstood the VMSTATE_VBUFFER_UINT32()
> handling, I thought it handled transferring the length field,
> too.  So I just remove support for that.  I doubt anyone is using
> it at this point.
> 
> This also removes the transfer of use_irq, since that should come
> from configuration.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  hw/ipmi/isa_ipmi_bt.c | 43 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
> index e946030..a990ab7 100644
> --- a/hw/ipmi/isa_ipmi_bt.c
> +++ b/hw/ipmi/isa_ipmi_bt.c
> @@ -450,22 +450,39 @@ static void isa_ipmi_bt_realize(DeviceState *dev, Error **errp)
>      isa_register_ioport(isadev, &iib->bt.io, iib->bt.io_base);
>  }
>  
> -static const VMStateDescription vmstate_ISAIPMIBTDevice = {
> -    .name = TYPE_IPMI_INTERFACE,
> +
> +const VMStateDescription vmstate_IPMIBT = {
> +    .name = TYPE_IPMI_INTERFACE_PREFIX "bt",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .fields      = (VMStateField[]) {
> -        VMSTATE_BOOL(bt.obf_irq_set, ISAIPMIBTDevice),
> -        VMSTATE_BOOL(bt.atn_irq_set, ISAIPMIBTDevice),
> -        VMSTATE_BOOL(bt.use_irq, ISAIPMIBTDevice),
> -        VMSTATE_BOOL(bt.irqs_enabled, ISAIPMIBTDevice),
> -        VMSTATE_UINT32(bt.outpos, ISAIPMIBTDevice),
> -        VMSTATE_VBUFFER_UINT32(bt.outmsg, ISAIPMIBTDevice, 1, NULL, bt.outlen),
> -        VMSTATE_VBUFFER_UINT32(bt.inmsg, ISAIPMIBTDevice, 1, NULL, bt.inlen),
> -        VMSTATE_UINT8(bt.control_reg, ISAIPMIBTDevice),
> -        VMSTATE_UINT8(bt.mask_reg, ISAIPMIBTDevice),
> -        VMSTATE_UINT8(bt.waiting_rsp, ISAIPMIBTDevice),
> -        VMSTATE_UINT8(bt.waiting_seq, ISAIPMIBTDevice),
> +        VMSTATE_BOOL(obf_irq_set, IPMIBT),
> +        VMSTATE_BOOL(atn_irq_set, IPMIBT),
> +        VMSTATE_BOOL(irqs_enabled, IPMIBT),
> +        VMSTATE_UINT32(outpos, IPMIBT),
> +        VMSTATE_UINT32(outlen, IPMIBT),
> +        VMSTATE_UINT8_ARRAY(outmsg, IPMIBT, MAX_IPMI_MSG_SIZE),
> +        VMSTATE_UINT32(inlen, IPMIBT),
> +        VMSTATE_UINT8_ARRAY(inmsg, IPMIBT, MAX_IPMI_MSG_SIZE),
> +        VMSTATE_UINT8(control_reg, IPMIBT),
> +        VMSTATE_UINT8(mask_reg, IPMIBT),
> +        VMSTATE_UINT8(waiting_rsp, IPMIBT),
> +        VMSTATE_UINT8(waiting_seq, IPMIBT),

OK, without knowing anything about the internals of the IPMI model, this
looks simpler; and yes, vbuffer isn't very smart.

You should probably consider either a postload to verify the data,
or be a bit paranoid in the uses.  For example, imagine that 'outpos'
and 'outlen' were for some reason completely bogus huge values and then
you got to ipmi_bt_ioport_read case 1, I think you could read off the
end of outmsg.

Dave

> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription vmstate_ISAIPMIBTDevice = {
> +    .name = TYPE_IPMI_INTERFACE_PREFIX "isa-bt",
> +    .version_id = 2,
> +    .minimum_version_id = 2,
> +    /*
> +     * Version 1 had messed up the array transfer, it's not even usable
> +     * because it used VMSTATE_VBUFFER_UINT32, but it did not transfer
> +     * the buffer length, so random things would happen.
> +     */
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_STRUCT(bt, ISAIPMIBTDevice, 1, vmstate_IPMIBT, IPMIBT),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> -- 
> 2.7.4
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK