[Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer

[Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer

[minyard]

This took a while because I had other distractions and I spent some
time studying how all this vmstate code works, and that itself got
to be a distraction.

I'm not 100% sure what I have done is right.  Maybe there is a way to
do a backwards migration for KCS, but I'm not sure how.

Changes from v1:
  * Validate the data values in a 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 v2 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 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.  And the
name on the man VMStateDescription was incorrect, it needed to be
differentiated from the BT one.

To fix this, a new VMStateDescription is added that is hopefully
correct, and the old one is kept (modified to remove use_irq) in
a way that it can be received from the remote but will not be sent.
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 | 77 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 75 insertions(+), 2 deletions(-)

diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
index 689587b..2a2784d 100644
--- a/hw/ipmi/isa_ipmi_kcs.c
+++ b/hw/ipmi/isa_ipmi_kcs.c
@@ -422,14 +422,86 @@ 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) {
+        ik->outpos = 0;
+        ik->outlen = 0;
+    }
+
+    if (ik->inlen >= MAX_IPMI_MSG_SIZE) {
+        ik->inlen = 0;
+    }
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_IPMIKCS = {
+    .name = TYPE_IPMI_INTERFACE_PREFIX "kcs",
+    .version_id = 1,
+    .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_BOOL(irqs_enabled, IPMIKCS),
+        VMSTATE_UINT32(outpos, IPMIKCS),
+        VMSTATE_UINT32(outlen, 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 const VMStateDescription vmstate_ISAIPMIKCSDevice = {
+    .name = TYPE_IPMI_INTERFACE_PREFIX "isa-kcs",
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .fields      = (VMStateField[]) {
+        VMSTATE_STRUCT(kcs, ISAIPMIKCSDevice, 1, vmstate_IPMIKCS, IPMIKCS),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+/*
+ * Old version of the vmstate transfer that has a number of issues.
+ * We changed the vm state description name, so we need a separate
+ * structure and need to register it separately.
+ */
+static int ipmi_kcs_v1_vmstate_post_load(void *opaque, int version)
+{
+    ISAIPMIKCSDevice *iik = opaque;
+
+    return ipmi_kcs_vmstate_post_load(&iik->kcs, version);
+}
+
+static bool ipmi_kcs_v1_vmstate_needed(void *opaque)
+{
+    /* Never transmit this, it is just for receiving old versions. */
+    return false;
+}
+
+const VMStateDescription vmstate_v1_ISAIPMIKCSDevice = {
     .name = TYPE_IPMI_INTERFACE,
     .version_id = 1,
     .minimum_version_id = 1,
+    .post_load = ipmi_kcs_v1_vmstate_post_load,
+    .needed = ipmi_kcs_v1_vmstate_needed,
     .fields      = (VMStateField[]) {
         VMSTATE_BOOL(kcs.obf_irq_set, ISAIPMIKCSDevice),
         VMSTATE_BOOL(kcs.atn_irq_set, ISAIPMIKCSDevice),
-        VMSTATE_BOOL(kcs.use_irq, ISAIPMIKCSDevice),
+        VMSTATE_UNUSED(1), /* Was use_irq */
         VMSTATE_BOOL(kcs.irqs_enabled, ISAIPMIKCSDevice),
         VMSTATE_UINT32(kcs.outpos, ISAIPMIKCSDevice),
         VMSTATE_UINT8_ARRAY(kcs.outmsg, ISAIPMIKCSDevice, MAX_IPMI_MSG_SIZE),
@@ -451,6 +523,7 @@ static void isa_ipmi_kcs_init(Object *obj)
     ipmi_bmc_find_and_link(obj, (Object **) &iik->kcs.bmc);
 
     vmstate_register(NULL, 0, &vmstate_ISAIPMIKCSDevice, iik);
+    vmstate_register(NULL, 0, &vmstate_v1_ISAIPMIKCSDevice, iik);
 }
 
 static void *isa_ipmi_kcs_get_backend_data(IPMIInterface *ii)
-- 
2.7.4

[Qemu-devel] [PATCH v2 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>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/ipmi/isa_ipmi_bt.c | 61 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 13 deletions(-)

diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index e946030..b64dce2 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -450,22 +450,57 @@ 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) {
+        ib->outpos = 0;
+        ib->outlen = 0;
+    }
+
+    if (ib->inlen >= MAX_IPMI_MSG_SIZE) {
+        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

[Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer

[minyard]

I apologize for the resend, I left the list off the previous post.

This is unchanged since the previous post, two weeks ago.  I received
no comments, so I guess it's ok.  It's fairly broken now, so I would
like this fixed.

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.

The following changes since commit 427cbc7e4136a061628cb4315cc8182ea36d772f:

  Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2018-03-01 18:46:41 +0000)

are available in the git repository at:

  https://github.com/cminyard/qemu.git tags/ipmi-vmstate-fixes

for you to fetch changes up to 90797371d9a3138657e7b1f7ab4425eb67d6fd0a:

  ipmi: Use proper struct reference for BT vmstate (2018-03-02 07:48:39 -0600)

----------------------------------------------------------------
Fix the IPMI vmstate code to work correctly in all cases.  Heavily
tested under load.

----------------------------------------------------------------
Corey Minyard (2):
      ipmi: Use proper struct reference for KCS vmstate
      ipmi: Use proper struct reference for BT vmstate

 hw/ipmi/isa_ipmi_bt.c  | 61 ++++++++++++++++++++++++++++++---------
 hw/ipmi/isa_ipmi_kcs.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 123 insertions(+), 15 deletions(-)

Re: [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer

[Dr. David Alan Gilbert]

* minyard@acm.org (minyard@acm.org) wrote:
> I apologize for the resend, I left the list off the previous post.
> 
> This is unchanged since the previous post, two weeks ago.  I received
> no comments, so I guess it's ok.  It's fairly broken now, so I would
> like this fixed.

Sorry, I'll look at it on Monday; I was out last week and hadn't got
around to this set.

Dave

> 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.
> 
> The following changes since commit 427cbc7e4136a061628cb4315cc8182ea36d772f:
> 
>   Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2018-03-01 18:46:41 +0000)
> 
> are available in the git repository at:
> 
>   https://github.com/cminyard/qemu.git tags/ipmi-vmstate-fixes
> 
> for you to fetch changes up to 90797371d9a3138657e7b1f7ab4425eb67d6fd0a:
> 
>   ipmi: Use proper struct reference for BT vmstate (2018-03-02 07:48:39 -0600)
> 
> ----------------------------------------------------------------
> Fix the IPMI vmstate code to work correctly in all cases.  Heavily
> tested under load.
> 
> ----------------------------------------------------------------
> Corey Minyard (2):
>       ipmi: Use proper struct reference for KCS vmstate
>       ipmi: Use proper struct reference for BT vmstate
> 
>  hw/ipmi/isa_ipmi_bt.c  | 61 ++++++++++++++++++++++++++++++---------
>  hw/ipmi/isa_ipmi_kcs.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 123 insertions(+), 15 deletions(-)
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer

[Corey Minyard]

On 03/02/2018 02:02 PM, Dr. David Alan Gilbert wrote:
> * minyard@acm.org (minyard@acm.org) wrote:
>> I apologize for the resend, I left the list off the previous post.
>>
>> This is unchanged since the previous post, two weeks ago.  I received
>> no comments, so I guess it's ok.  It's fairly broken now, so I would
>> like this fixed.
> Sorry, I'll look at it on Monday; I was out last week and hadn't got
> around to this set.

Thanks a bunch.  I have some doubt about how I handled the backwards
compatibility in the KCS code.  It works, but I'm not sure it's right.

-corey

> Dave
>
>> 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.
>>
>> The following changes since commit 427cbc7e4136a061628cb4315cc8182ea36d772f:
>>
>>    Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2018-03-01 18:46:41 +0000)
>>
>> are available in the git repository at:
>>
>>    https://github.com/cminyard/qemu.git tags/ipmi-vmstate-fixes
>>
>> for you to fetch changes up to 90797371d9a3138657e7b1f7ab4425eb67d6fd0a:
>>
>>    ipmi: Use proper struct reference for BT vmstate (2018-03-02 07:48:39 -0600)
>>
>> ----------------------------------------------------------------
>> Fix the IPMI vmstate code to work correctly in all cases.  Heavily
>> tested under load.
>>
>> ----------------------------------------------------------------
>> Corey Minyard (2):
>>        ipmi: Use proper struct reference for KCS vmstate
>>        ipmi: Use proper struct reference for BT vmstate
>>
>>   hw/ipmi/isa_ipmi_bt.c  | 61 ++++++++++++++++++++++++++++++---------
>>   hw/ipmi/isa_ipmi_kcs.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++--
>>   2 files changed, 123 insertions(+), 15 deletions(-)
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer

[Peter Maydell]

On 2 March 2018 at 15:26,  <minyard@acm.org> wrote:
> I apologize for the resend, I left the list off the previous post.
>
> This is unchanged since the previous post, two weeks ago.  I received
> no comments, so I guess it's ok.  It's fairly broken now, so I would
> like this fixed.
>
> 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.
>
> The following changes since commit 427cbc7e4136a061628cb4315cc8182ea36d772f:
>
>   Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2018-03-01 18:46:41 +0000)
>
> are available in the git repository at:
>
>   https://github.com/cminyard/qemu.git tags/ipmi-vmstate-fixes
>
> for you to fetch changes up to 90797371d9a3138657e7b1f7ab4425eb67d6fd0a:
>
>   ipmi: Use proper struct reference for BT vmstate (2018-03-02 07:48:39 -0600)
>

If you could avoid sending pull request emails for patchsets, I'd
appreciate it -- they automatically go into my queue of
things-to-process-for-master, and if I'm not paying enough attention
they might actually get applied...

thanks
-- PMM