[Qemu-devel] [PATCH v3 0/3] ipmi: Fix vmstate transfer

[Qemu-devel] [PATCH v3 0/3] ipmi: Fix vmstate transfer

[minyard]

I was going to just re-send the previous versions of these, but after
looking I realized it would have the same misuse of vmstate_register(),
and I didn't have a way to work around it.  I had already done the
VMSTATE_VSTRUCT work earlier, so I merged that in.

Changes from v2:
  * Added VMSTATE_VSTRUCT types of vmstate macros that allow the
    specific structure version used to be specified.
  * Reworked the KCS vmstate code to use the new VMSTATE_VSTRUCT
    macros.

Changes from v1:
  * Validate the data values in pre_load functions.
  * For KCS, instead of an old function, create a separate vmstate
    structure for the new version.  The name on the old vmstate
    structure wasn't specific enough, so a new name was needed,
    The old structure is set up to never be sent, but it can be
    received.

[Qemu-devel] [PATCH v3 1/3] vmstate: Add a VSTRUCT type

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The VMS_STRUCT has no way to specify which version of a structure
to use.  Add a type and a new field to allow the specific version
of a structure to be used.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 include/migration/vmstate.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
 migration/vmstate.c         | 25 +++++++++++++++++++++----
 2 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index df463fd..efa0a11 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -143,6 +143,11 @@ enum VMStateFlags {
      * to determine the number of entries in the array. Only valid in
      * combination with one of VMS_VARRAY*. */
     VMS_MULTIPLY_ELEMENTS = 0x4000,
+
+    /* A structure field that is like VMS_STRUCT, but uses
+     * VMStateField.struct_version_id to tell which version of the
+     * structure we are referencing to use. */
+    VMS_VSTRUCT           = 0x8000,
 };
 
 typedef enum {
@@ -167,6 +172,7 @@ struct VMStateField {
     enum VMStateFlags flags;
     const VMStateDescription *vmsd;
     int version_id;
+    int struct_version_id;
     bool (*field_exists)(void *opaque, int version_id);
 };
 
@@ -248,6 +254,25 @@ extern const VMStateInfo vmstate_info_qtailq;
     vmstate_offset_array(_state, _field, uint8_t,                    \
                          sizeof(typeof_field(_state, _field)))
 
+/* In the macros below, if there is a _version, that means the macro's
+ * field will be processed only if the version being received is >=
+ * the _version specified.  In general, if you add a new field, you
+ * would increment the structure's version and put that version
+ * number into the new field so it would only be processed with the
+ * new version.
+ *
+ * In particular, for VMSTATE_STRUCT() and friends the _version does
+ * *NOT* pick the version of the sub-structure.  It works just as
+ * specified above.  The version of the top-level structure received
+ * is passed down to all sub-structures.  This means that the
+ * sub-structures must have version that are compatible with all the
+ * structures that use them.
+ *
+ * If you want to specify the version of the sub-structure, use
+ * VMSTATE_VSTRUCT(), which allows the specific sub-structure version
+ * to be directly specified.
+ */
+
 #define VMSTATE_SINGLE_TEST(_field, _state, _test, _version, _info, _type) { \
     .name         = (stringify(_field)),                             \
     .version_id   = (_version),                                      \
@@ -395,6 +420,17 @@ extern const VMStateInfo vmstate_info_qtailq;
     .offset     = offsetof(_state, _field),                          \
 }
 
+#define VMSTATE_VSTRUCT_TEST(_field, _state, _test, _version, _vmsd, _type, _struct_version) { \
+    .name         = (stringify(_field)),                             \
+    .version_id   = (_version),                                      \
+    .struct_version_id = (_struct_version),                          \
+    .field_exists = (_test),                                         \
+    .vmsd         = &(_vmsd),                                        \
+    .size         = sizeof(_type),                                   \
+    .flags        = VMS_VSTRUCT,                                     \
+    .offset       = vmstate_offset_value(_state, _field, _type),     \
+}
+
 #define VMSTATE_STRUCT_TEST(_field, _state, _test, _version, _vmsd, _type) { \
     .name         = (stringify(_field)),                             \
     .version_id   = (_version),                                      \
@@ -712,6 +748,13 @@ extern const VMStateInfo vmstate_info_qtailq;
 #define VMSTATE_SINGLE(_field, _state, _version, _info, _type)        \
     VMSTATE_SINGLE_TEST(_field, _state, NULL, _version, _info, _type)
 
+#define VMSTATE_VSTRUCT(_field, _state, _vmsd, _type, _struct_version)\
+    VMSTATE_VSTRUCT_TEST(_field, _state, NULL, 0, _vmsd, _type, _struct_version)
+
+#define VMSTATE_VSTRUCT_V(_field, _state, _version, _vmsd, _type, _struct_version) \
+    VMSTATE_VSTRUCT_TEST(_field, _state, NULL, _version, _vmsd, _type, \
+                         _struct_version)
+
 #define VMSTATE_STRUCT(_field, _state, _version, _vmsd, _type)        \
     VMSTATE_STRUCT_TEST(_field, _state, NULL, _version, _vmsd, _type)
 
@@ -1000,6 +1043,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                        void *opaque, int version_id);
 int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                        void *opaque, QJSON *vmdesc);
+int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
+                         void *opaque, QJSON *vmdesc, int version_id);
 
 bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);
 
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 0b3282c..f0b07f4 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -136,6 +136,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                 } else if (field->flags & VMS_STRUCT) {
                     ret = vmstate_load_state(f, field->vmsd, curr_elem,
                                              field->vmsd->version_id);
+                } else if (field->flags & VMS_VSTRUCT) {
+                    ret = vmstate_load_state(f, field->vmsd, curr_elem,
+                                             field->struct_version_id);
                 } else {
                     ret = field->info->get(f, curr_elem, size, field);
                 }
@@ -209,6 +212,8 @@ static const char *vmfield_get_type_name(VMStateField *field)
 
     if (field->flags & VMS_STRUCT) {
         type = "struct";
+    } else if (field->flags & VMS_VSTRUCT) {
+        type = "vstruct";
     } else if (field->info->name) {
         type = field->info->name;
     }
@@ -309,7 +314,13 @@ bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque)
 
 
 int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
-                        void *opaque, QJSON *vmdesc)
+                       void *opaque, QJSON *vmdesc_id)
+{
+    return vmstate_save_state_v(f, vmsd, opaque, vmdesc_id, vmsd->version_id);
+}
+
+int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
+                         void *opaque, QJSON *vmdesc, int version_id)
 {
     int ret = 0;
     VMStateField *field = vmsd->fields;
@@ -327,13 +338,15 @@ int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
 
     if (vmdesc) {
         json_prop_str(vmdesc, "vmsd_name", vmsd->name);
-        json_prop_int(vmdesc, "version", vmsd->version_id);
+        json_prop_int(vmdesc, "version", version_id);
         json_start_array(vmdesc, "fields");
     }
 
     while (field->name) {
-        if (!field->field_exists ||
-            field->field_exists(opaque, vmsd->version_id)) {
+        if ((field->field_exists &&
+             field->field_exists(opaque, version_id)) ||
+            (!field->field_exists &&
+             field->version_id <= version_id)) {
             void *first_elem = opaque + field->offset;
             int i, n_elems = vmstate_n_elems(opaque, field);
             int size = vmstate_size(opaque, field);
@@ -363,6 +376,10 @@ int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                 } else if (field->flags & VMS_STRUCT) {
                     ret = vmstate_save_state(f, field->vmsd, curr_elem,
                                              vmdesc_loop);
+                } else if (field->flags & VMS_VSTRUCT) {
+                    ret = vmstate_save_state_v(f, field->vmsd, curr_elem,
+                                               vmdesc_loop,
+                                               field->struct_version_id);
                 } else {
                     ret = field->info->put(f, curr_elem, size, field,
                                      vmdesc_loop);
-- 
2.7.4

[Qemu-devel] [PATCH v3 2/3] 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 were 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.

To fix this, the new VMS_VSTRUCT macros are used so the exact
version of the structure can be specified, depending on what
version was being received.  So an upgrade should work for KCS.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/ipmi/isa_ipmi_kcs.c | 81 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 66 insertions(+), 15 deletions(-)

diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
index 689587b..a794315 100644
--- a/hw/ipmi/isa_ipmi_kcs.c
+++ b/hw/ipmi/isa_ipmi_kcs.c
@@ -22,6 +22,7 @@
  * THE SOFTWARE.
  */
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "qapi/error.h"
 #include "hw/hw.h"
 #include "hw/ipmi/ipmi.h"
@@ -422,24 +423,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 = {
+static int ipmi_kcs_vmstate_post_load(void *opaque, int version)
+{
+    IPMIKCS *ik = opaque;
+
+    /* Make sure all the values are sane. */
+    if (ik->outpos >= MAX_IPMI_MSG_SIZE || ik->outlen >= MAX_IPMI_MSG_SIZE ||
+        ik->outpos >= ik->outlen) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "ipmi:kcs: vmstate transfer received bad out values: %d %d\n",
+                      ik->outpos, ik->outlen);
+        ik->outpos = 0;
+        ik->outlen = 0;
+    }
+
+    if (ik->inlen >= MAX_IPMI_MSG_SIZE) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "ipmi:kcs: vmstate transfer received bad in value: %d\n",
+                      ik->inlen);
+        ik->inlen = 0;
+    }
+
+    return 0;
+}
+
+static bool vmstate_kcs_before_version2(void *opaque, int version)
+{
+    return version <= 1;
+}
+
+static const VMStateDescription vmstate_IPMIKCS = {
+    .name = TYPE_IPMI_INTERFACE_PREFIX "kcs",
+    .version_id = 2,
+    .minimum_version_id = 1,
+    .post_load = ipmi_kcs_vmstate_post_load,
+    .fields      = (VMStateField[]) {
+        VMSTATE_BOOL(obf_irq_set, IPMIKCS),
+        VMSTATE_BOOL(atn_irq_set, IPMIKCS),
+        VMSTATE_UNUSED_TEST(vmstate_kcs_before_version2, 1), /* Was use_irq */
+        VMSTATE_BOOL(irqs_enabled, IPMIKCS),
+        VMSTATE_UINT32(outpos, IPMIKCS),
+        VMSTATE_UINT32_V(outlen, IPMIKCS, 2),
+        VMSTATE_UINT8_ARRAY(outmsg, IPMIKCS, MAX_IPMI_MSG_SIZE),
+        VMSTATE_UINT32_V(inlen, IPMIKCS, 2),
+        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 const VMStateDescription vmstate_ISAIPMIKCSDevice = {
     .name = TYPE_IPMI_INTERFACE,
-    .version_id = 1,
+    .version_id = 2,
     .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_VSTRUCT_TEST(kcs, ISAIPMIKCSDevice, vmstate_kcs_before_version2,
+                             0, vmstate_IPMIKCS, IPMIKCS, 1),
+        VMSTATE_VSTRUCT_V(kcs, ISAIPMIKCSDevice, 2, vmstate_IPMIKCS,
+                          IPMIKCS, 2),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -450,6 +496,11 @@ static void isa_ipmi_kcs_init(Object *obj)
 
     ipmi_bmc_find_and_link(obj, (Object **) &iik->kcs.bmc);
 
+    /*
+     * Version 1 had an incorrect name, it clashed with the BT
+     * IPMI device, so receive it, but transmit a different
+     * version.
+     */
     vmstate_register(NULL, 0, &vmstate_ISAIPMIKCSDevice, iik);
 }
 
-- 
2.7.4

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

[Marc-André Lureau]

Hi Corey

On Wed, Apr 25, 2018 at 5:27 PM,  <minyard@acm.org> wrote:
> 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 were 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.
>
> To fix this, the new VMS_VSTRUCT macros are used so the exact
> version of the structure can be specified, depending on what
> version was being received.  So an upgrade should work for KCS.

Looks good overall,

You could easily split this patch further to help review/bisecting etc.

Introduce VMSTATE_STRUCT, unuse use_irq, introduce version 2, add the
postload checks.

You could also help reviewers by giving your test setup, so we can
more easily reproduce the fix and/or try variants.

I also wonder if you could have used subsections, but the VSTRUCT type
seems a good approach to me, David would have to review it though.


> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/ipmi/isa_ipmi_kcs.c | 81 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 66 insertions(+), 15 deletions(-)
>
> diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
> index 689587b..a794315 100644
> --- a/hw/ipmi/isa_ipmi_kcs.c
> +++ b/hw/ipmi/isa_ipmi_kcs.c
> @@ -22,6 +22,7 @@
>   * THE SOFTWARE.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/log.h"
>  #include "qapi/error.h"
>  #include "hw/hw.h"
>  #include "hw/ipmi/ipmi.h"
> @@ -422,24 +423,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 = {
> +static int ipmi_kcs_vmstate_post_load(void *opaque, int version)
> +{
> +    IPMIKCS *ik = opaque;
> +
> +    /* Make sure all the values are sane. */
> +    if (ik->outpos >= MAX_IPMI_MSG_SIZE || ik->outlen >= MAX_IPMI_MSG_SIZE ||
> +        ik->outpos >= ik->outlen) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "ipmi:kcs: vmstate transfer received bad out values: %d %d\n",
> +                      ik->outpos, ik->outlen);
> +        ik->outpos = 0;
> +        ik->outlen = 0;
> +    }
> +
> +    if (ik->inlen >= MAX_IPMI_MSG_SIZE) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "ipmi:kcs: vmstate transfer received bad in value: %d\n",
> +                      ik->inlen);
> +        ik->inlen = 0;
> +    }
> +
> +    return 0;
> +}
> +
> +static bool vmstate_kcs_before_version2(void *opaque, int version)
> +{
> +    return version <= 1;
> +}
> +
> +static const VMStateDescription vmstate_IPMIKCS = {
> +    .name = TYPE_IPMI_INTERFACE_PREFIX "kcs",
> +    .version_id = 2,
> +    .minimum_version_id = 1,
> +    .post_load = ipmi_kcs_vmstate_post_load,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_BOOL(obf_irq_set, IPMIKCS),
> +        VMSTATE_BOOL(atn_irq_set, IPMIKCS),
> +        VMSTATE_UNUSED_TEST(vmstate_kcs_before_version2, 1), /* Was use_irq */
> +        VMSTATE_BOOL(irqs_enabled, IPMIKCS),
> +        VMSTATE_UINT32(outpos, IPMIKCS),
> +        VMSTATE_UINT32_V(outlen, IPMIKCS, 2),
> +        VMSTATE_UINT8_ARRAY(outmsg, IPMIKCS, MAX_IPMI_MSG_SIZE),
> +        VMSTATE_UINT32_V(inlen, IPMIKCS, 2),
> +        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 const VMStateDescription vmstate_ISAIPMIKCSDevice = {
>      .name = TYPE_IPMI_INTERFACE,
> -    .version_id = 1,
> +    .version_id = 2,
>      .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_VSTRUCT_TEST(kcs, ISAIPMIKCSDevice, vmstate_kcs_before_version2,
> +                             0, vmstate_IPMIKCS, IPMIKCS, 1),
> +        VMSTATE_VSTRUCT_V(kcs, ISAIPMIKCSDevice, 2, vmstate_IPMIKCS,
> +                          IPMIKCS, 2),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> @@ -450,6 +496,11 @@ static void isa_ipmi_kcs_init(Object *obj)
>
>      ipmi_bmc_find_and_link(obj, (Object **) &iik->kcs.bmc);
>
> +    /*
> +     * Version 1 had an incorrect name, it clashed with the BT
> +     * IPMI device, so receive it, but transmit a different
> +     * version.
> +     */
>      vmstate_register(NULL, 0, &vmstate_ISAIPMIKCSDevice, iik);
>  }
>
> --
> 2.7.4
>
>



-- 
Marc-André Lureau

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

[Corey Minyard]

On 05/18/2018 10:45 AM, Marc-André Lureau wrote:
> Hi Corey
>
> On Wed, Apr 25, 2018 at 5:27 PM,  <minyard@acm.org> wrote:
>> 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 were 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.
>>
>> To fix this, the new VMS_VSTRUCT macros are used so the exact
>> version of the structure can be specified, depending on what
>> version was being received.  So an upgrade should work for KCS.
> Looks good overall,
>
> You could easily split this patch further to help review/bisecting etc.
>
> Introduce VMSTATE_STRUCT, unuse use_irq, introduce version 2, add the
> postload checks.

That's probably fair.  I'll do that for v4.

> You could also help reviewers by giving your test setup, so we can
> more easily reproduce the fix and/or try variants.

Hmm.  That's a little hard.  I'll see what I can do.  Maybe it's not too 
bad,
most distros should have the openipmi library available.

>
> I also wonder if you could have used subsections, but the VSTRUCT type
> seems a good approach to me, David would have to review it though.

Yeah, I think we talked about subsections at one point, but this seemed 
better.
That will have to wait for David.

Thanks,

-corey

>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> ---
>>   hw/ipmi/isa_ipmi_kcs.c | 81 ++++++++++++++++++++++++++++++++++++++++----------
>>   1 file changed, 66 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
>> index 689587b..a794315 100644
>> --- a/hw/ipmi/isa_ipmi_kcs.c
>> +++ b/hw/ipmi/isa_ipmi_kcs.c
>> @@ -22,6 +22,7 @@
>>    * THE SOFTWARE.
>>    */
>>   #include "qemu/osdep.h"
>> +#include "qemu/log.h"
>>   #include "qapi/error.h"
>>   #include "hw/hw.h"
>>   #include "hw/ipmi/ipmi.h"
>> @@ -422,24 +423,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 = {
>> +static int ipmi_kcs_vmstate_post_load(void *opaque, int version)
>> +{
>> +    IPMIKCS *ik = opaque;
>> +
>> +    /* Make sure all the values are sane. */
>> +    if (ik->outpos >= MAX_IPMI_MSG_SIZE || ik->outlen >= MAX_IPMI_MSG_SIZE ||
>> +        ik->outpos >= ik->outlen) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "ipmi:kcs: vmstate transfer received bad out values: %d %d\n",
>> +                      ik->outpos, ik->outlen);
>> +        ik->outpos = 0;
>> +        ik->outlen = 0;
>> +    }
>> +
>> +    if (ik->inlen >= MAX_IPMI_MSG_SIZE) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "ipmi:kcs: vmstate transfer received bad in value: %d\n",
>> +                      ik->inlen);
>> +        ik->inlen = 0;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static bool vmstate_kcs_before_version2(void *opaque, int version)
>> +{
>> +    return version <= 1;
>> +}
>> +
>> +static const VMStateDescription vmstate_IPMIKCS = {
>> +    .name = TYPE_IPMI_INTERFACE_PREFIX "kcs",
>> +    .version_id = 2,
>> +    .minimum_version_id = 1,
>> +    .post_load = ipmi_kcs_vmstate_post_load,
>> +    .fields      = (VMStateField[]) {
>> +        VMSTATE_BOOL(obf_irq_set, IPMIKCS),
>> +        VMSTATE_BOOL(atn_irq_set, IPMIKCS),
>> +        VMSTATE_UNUSED_TEST(vmstate_kcs_before_version2, 1), /* Was use_irq */
>> +        VMSTATE_BOOL(irqs_enabled, IPMIKCS),
>> +        VMSTATE_UINT32(outpos, IPMIKCS),
>> +        VMSTATE_UINT32_V(outlen, IPMIKCS, 2),
>> +        VMSTATE_UINT8_ARRAY(outmsg, IPMIKCS, MAX_IPMI_MSG_SIZE),
>> +        VMSTATE_UINT32_V(inlen, IPMIKCS, 2),
>> +        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 const VMStateDescription vmstate_ISAIPMIKCSDevice = {
>>       .name = TYPE_IPMI_INTERFACE,
>> -    .version_id = 1,
>> +    .version_id = 2,
>>       .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_VSTRUCT_TEST(kcs, ISAIPMIKCSDevice, vmstate_kcs_before_version2,
>> +                             0, vmstate_IPMIKCS, IPMIKCS, 1),
>> +        VMSTATE_VSTRUCT_V(kcs, ISAIPMIKCSDevice, 2, vmstate_IPMIKCS,
>> +                          IPMIKCS, 2),
>>           VMSTATE_END_OF_LIST()
>>       }
>>   };
>> @@ -450,6 +496,11 @@ static void isa_ipmi_kcs_init(Object *obj)
>>
>>       ipmi_bmc_find_and_link(obj, (Object **) &iik->kcs.bmc);
>>
>> +    /*
>> +     * Version 1 had an incorrect name, it clashed with the BT
>> +     * IPMI device, so receive it, but transmit a different
>> +     * version.
>> +     */
>>       vmstate_register(NULL, 0, &vmstate_ISAIPMIKCSDevice, iik);
>>   }
>>
>> --
>> 2.7.4
>>
>>
>
>

[Qemu-devel] [PATCH v3 3/3] 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>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/ipmi/isa_ipmi_bt.c | 68 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 55 insertions(+), 13 deletions(-)

diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index e946030..8bbb1fa 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -22,6 +22,7 @@
  * THE SOFTWARE.
  */
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "qapi/error.h"
 #include "hw/hw.h"
 #include "hw/ipmi/ipmi.h"
@@ -450,22 +451,63 @@ 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,
+static int ipmi_bt_vmstate_post_load(void *opaque, int version)
+{
+    IPMIBT *ib = opaque;
+
+    /* Make sure all the values are sane. */
+    if (ib->outpos >= MAX_IPMI_MSG_SIZE || ib->outlen >= MAX_IPMI_MSG_SIZE ||
+        ib->outpos >= ib->outlen) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "ipmi:bt: vmstate transfer received bad out values: %d %d\n",
+                      ib->outpos, ib->outlen);
+        ib->outpos = 0;
+        ib->outlen = 0;
+    }
+
+    if (ib->inlen >= MAX_IPMI_MSG_SIZE) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "ipmi:bt: vmstate transfer received bad in value: %d\n",
+                      ib->inlen);
+        ib->inlen = 0;
+    }
+
+    return 0;
+}
+
+const VMStateDescription vmstate_IPMIBT = {
+    .name = TYPE_IPMI_INTERFACE_PREFIX "bt",
     .version_id = 1,
     .minimum_version_id = 1,
+    .post_load = ipmi_bt_vmstate_post_load,
+    .fields      = (VMStateField[]) {
+        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_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_STRUCT(bt, ISAIPMIBTDevice, 1, vmstate_IPMIBT, IPMIBT),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.7.4

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

[Philippe Mathieu-Daudé]

Hi Corey,

On 04/25/2018 12:27 PM, 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>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

It seems you mis-pasted Signed-off-by -> Reviewed-by.

> ---
>  hw/ipmi/isa_ipmi_bt.c | 68 +++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 55 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
> index e946030..8bbb1fa 100644
> --- a/hw/ipmi/isa_ipmi_bt.c
> +++ b/hw/ipmi/isa_ipmi_bt.c
> @@ -22,6 +22,7 @@
>   * THE SOFTWARE.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/log.h"
>  #include "qapi/error.h"
>  #include "hw/hw.h"
>  #include "hw/ipmi/ipmi.h"
> @@ -450,22 +451,63 @@ 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,
> +static int ipmi_bt_vmstate_post_load(void *opaque, int version)
> +{
> +    IPMIBT *ib = opaque;
> +
> +    /* Make sure all the values are sane. */
> +    if (ib->outpos >= MAX_IPMI_MSG_SIZE || ib->outlen >= MAX_IPMI_MSG_SIZE ||
> +        ib->outpos >= ib->outlen) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "ipmi:bt: vmstate transfer received bad out values: %d %d\n",
> +                      ib->outpos, ib->outlen);
> +        ib->outpos = 0;
> +        ib->outlen = 0;
> +    }
> +
> +    if (ib->inlen >= MAX_IPMI_MSG_SIZE) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "ipmi:bt: vmstate transfer received bad in value: %d\n",
> +                      ib->inlen);
> +        ib->inlen = 0;
> +    }
> +
> +    return 0;
> +}
> +
> +const VMStateDescription vmstate_IPMIBT = {
> +    .name = TYPE_IPMI_INTERFACE_PREFIX "bt",
>      .version_id = 1,
>      .minimum_version_id = 1,
> +    .post_load = ipmi_bt_vmstate_post_load,
> +    .fields      = (VMStateField[]) {
> +        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_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_STRUCT(bt, ISAIPMIBTDevice, 1, vmstate_IPMIBT, IPMIBT),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> 

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

[Corey Minyard]

On 04/25/2018 03:27 PM, Philippe Mathieu-Daudé wrote:
> Hi Corey,
>
> On 04/25/2018 12:27 PM, 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>
>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> It seems you mis-pasted Signed-off-by -> Reviewed-by.

Yes, I did. Thanks.

-corey

>> ---
>>   hw/ipmi/isa_ipmi_bt.c | 68 +++++++++++++++++++++++++++++++++++++++++----------
>>   1 file changed, 55 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
>> index e946030..8bbb1fa 100644
>> --- a/hw/ipmi/isa_ipmi_bt.c
>> +++ b/hw/ipmi/isa_ipmi_bt.c
>> @@ -22,6 +22,7 @@
>>    * THE SOFTWARE.
>>    */
>>   #include "qemu/osdep.h"
>> +#include "qemu/log.h"
>>   #include "qapi/error.h"
>>   #include "hw/hw.h"
>>   #include "hw/ipmi/ipmi.h"
>> @@ -450,22 +451,63 @@ 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,
>> +static int ipmi_bt_vmstate_post_load(void *opaque, int version)
>> +{
>> +    IPMIBT *ib = opaque;
>> +
>> +    /* Make sure all the values are sane. */
>> +    if (ib->outpos >= MAX_IPMI_MSG_SIZE || ib->outlen >= MAX_IPMI_MSG_SIZE ||
>> +        ib->outpos >= ib->outlen) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "ipmi:bt: vmstate transfer received bad out values: %d %d\n",
>> +                      ib->outpos, ib->outlen);
>> +        ib->outpos = 0;
>> +        ib->outlen = 0;
>> +    }
>> +
>> +    if (ib->inlen >= MAX_IPMI_MSG_SIZE) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "ipmi:bt: vmstate transfer received bad in value: %d\n",
>> +                      ib->inlen);
>> +        ib->inlen = 0;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +const VMStateDescription vmstate_IPMIBT = {
>> +    .name = TYPE_IPMI_INTERFACE_PREFIX "bt",
>>       .version_id = 1,
>>       .minimum_version_id = 1,
>> +    .post_load = ipmi_bt_vmstate_post_load,
>> +    .fields      = (VMStateField[]) {
>> +        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_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_STRUCT(bt, ISAIPMIBTDevice, 1, vmstate_IPMIBT, IPMIBT),
>>           VMSTATE_END_OF_LIST()
>>       }
>>   };
>>

Re: [Qemu-devel] [PATCH v3 0/3] ipmi: Fix vmstate transfer

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1524670052-28373-1-git-send-email-minyard@acm.org
Subject: [Qemu-devel] [PATCH v3 0/3] ipmi: Fix vmstate transfer

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/1524670052-28373-1-git-send-email-minyard@acm.org -> patchew/1524670052-28373-1-git-send-email-minyard@acm.org
 t [tag update]            patchew/20180420145249.32435-1-peter.maydell@linaro.org -> patchew/20180420145249.32435-1-peter.maydell@linaro.org
 t [tag update]            patchew/20180423223337.82366-1-eblake@redhat.com -> patchew/20180423223337.82366-1-eblake@redhat.com
Auto packing the repository in background for optimum performance.
See "git help gc" for manual housekeeping.
Switched to a new branch 'test'
782859090c ipmi: Use proper struct reference for BT vmstate
4324fae8c3 ipmi: Use proper struct reference for KCS vmstate
a72462b176 vmstate: Add a VSTRUCT type

=== OUTPUT BEGIN ===
Checking PATCH 1/3: vmstate: Add a VSTRUCT type...
ERROR: line over 90 characters
#67: FILE: include/migration/vmstate.h:423:
+#define VMSTATE_VSTRUCT_TEST(_field, _state, _test, _version, _vmsd, _type, _struct_version) { \

WARNING: line over 80 characters
#88: FILE: include/migration/vmstate.h:754:
+#define VMSTATE_VSTRUCT_V(_field, _state, _version, _vmsd, _type, _struct_version) \

total: 1 errors, 1 warnings, 140 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/3: ipmi: Use proper struct reference for KCS vmstate...
Checking PATCH 3/3: ipmi: Use proper struct reference for BT vmstate...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com