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

[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(-)

[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

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 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>
> 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;
> +}

OK, this one looks fine; I'd personally add a printf or something in the
case where you're having to fix up the output/outlen just so you know;
but that's OK; and since you know the device I'll leave the use_irq etc
to you, so:


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> +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
> 
--
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

Re: [Qemu-devel] [PATCH v2 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>
> 
> 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.

I think that's a bigger problem; lets see below.

> 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()
> +    }
> +};

I've got the following, which is only build tested but:

+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(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_PREFIX "isa-kcs",
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .fields      = (VMStateField[]) {
+        VMSTATE_STRUCT(kcs, ISAIPMIKCSDevice, 2, vmstate_IPMIKCS, IPMIKCS),
+        VMSTATE_END_OF_LIST()
+    }
+};

Note how the outlen and inlen fields use the _V modifier and are only
bound to v2, and I leave the UNUSED in for use_irq, that means we can
then mae the vmstate_v1_ISAIPMIKCSDevice just have:

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_STRUCT(kcs, ISAIPMIKCSDevice, 1, vmstate_IPMIKCS, IPMIKCS),
        VMSTATE_END_OF_LIST()
    }
};

Note the '1'. in the VMSTATE_STRUCT.
As I say, that's untested, but if it works it saves a lot of
duplication.

> +
> +/*
> + * 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);

OK, a bit scary - I don't think anyone else does that trick of
registering it under two names with a '_needed' always saying false.

So I think we have the istuation where the old version had two things
with the same name, and if you instantiated both device types you
probably lost one of them?
In the new world both the 'bt' one is incompatible and renamed,
but this one has the hack to load the old kcs version;  so I guess
that's OK.

Could you just add a comment above that second vmstate_register
  ' old version had different (clashing) name, register both'

However it's worth asking if it's really worth it, if you just ekpt this
one called TYPE_IPMI_INTERFACE then you wouldn't need that hack, and the
only downside I can see is that loading an image that had a ipmi-bt
would fail badly, as opposed to failing clearly.

Dave

>  }
>  
>  static void *isa_ipmi_kcs_get_backend_data(IPMIInterface *ii)
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Corey Minyard]

On 03/05/2018 08:09 AM, Dr. David Alan Gilbert wrote:
> * minyard@acm.org (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.
>> And the
>> name on the man VMStateDescription was incorrect, it needed to be
>> differentiated from the BT one.
> I think that's a bigger problem; lets see below.
>
>> 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()
>> +    }
>> +};
> I've got the following, which is only build tested but:
>
> +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(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_PREFIX "isa-kcs",
> +    .version_id = 2,
> +    .minimum_version_id = 2,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_STRUCT(kcs, ISAIPMIKCSDevice, 2, vmstate_IPMIKCS, IPMIKCS),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
>
> Note how the outlen and inlen fields use the _V modifier and are only
> bound to v2, and I leave the UNUSED in for use_irq, that means we can
> then mae the vmstate_v1_ISAIPMIKCSDevice just have:
>
> 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_STRUCT(kcs, ISAIPMIKCSDevice, 1, vmstate_IPMIKCS, IPMIKCS),
>          VMSTATE_END_OF_LIST()
>      }
> };
>
> Note the '1'. in the VMSTATE_STRUCT.
> As I say, that's untested, but if it works it saves a lot of
> duplication.
>

Ah I misunderstood something.  The version is sent for the section, and 
the version
in the VMSTATE_STRUCT() is passed down when the structure is encoded and
decoded.  I was thinking the version was transferred in the protocol, 
but I can see
now that is not the case.

But....

I implement that, and it doesn't work.  It turns out that there is the 
following in
vmstate_load_state():

                } else if (field->flags & VMS_STRUCT) {
                     ret = vmstate_load_state(f, field->vmsd, curr_elem,
field->vmsd->version_id);

So it doesn't matter what is in field->version_id, (which is where you 
say "Note
the '1' in the VMSTATE_STRUCT).  It passes in whatever the most current
version is from the vmsd.

Plus that field value is used to compare for the incoming version. If you
expect that the be the field passed down to the sub-structure, it's going
to need to be a different field.

So this is not going to work, and the fix is not going to be easy, it 
appears.

>> +
>> +/*
>> + * 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);
> OK, a bit scary - I don't think anyone else does that trick of
> registering it under two names with a '_needed' always saying false.

Yes, I was wondering if I was abusing that interface.

> So I think we have the istuation where the old version had two things
> with the same name, and if you instantiated both device types you
> probably lost one of them?

Yes, something like that.  Or more likely, the transfer would just fail.

> In the new world both the 'bt' one is incompatible and renamed,
> but this one has the hack to load the old kcs version;  so I guess
> that's OK.
>
> Could you just add a comment above that second vmstate_register
>    ' old version had different (clashing) name, register both'

Certainly.

> However it's worth asking if it's really worth it, if you just ekpt this
> one called TYPE_IPMI_INTERFACE then you wouldn't need that hack, and the
> only downside I can see is that loading an image that had a ipmi-bt
> would fail badly, as opposed to failing clearly.

I was wondering that, too.  I'd be ok with that.

But with the above bug I'm not sure it's possible to fix.  There's no 
way to pass the
version being processed to the handling of vmstate_IPMIKCS.

-corey

>
> Dave
>
>>   }
>>   
>>   static void *isa_ipmi_kcs_get_backend_data(IPMIInterface *ii)
>> -- 
>> 2.7.4
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Corey Minyard]

On 03/05/2018 04:52 PM, Corey Minyard wrote:
> On 03/05/2018 08:09 AM, Dr. David Alan Gilbert wrote:
>> * minyard@acm.org (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.
>>> And the
>>> name on the man VMStateDescription was incorrect, it needed to be
>>> differentiated from the BT one.
>> I think that's a bigger problem; lets see below.
>>
>>> 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()
>>> +    }
>>> +};
>> I've got the following, which is only build tested but:
>>
>> +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(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_PREFIX "isa-kcs",
>> +    .version_id = 2,
>> +    .minimum_version_id = 2,
>> +    .fields      = (VMStateField[]) {
>> +        VMSTATE_STRUCT(kcs, ISAIPMIKCSDevice, 2, vmstate_IPMIKCS, 
>> IPMIKCS),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>>
>> Note how the outlen and inlen fields use the _V modifier and are only
>> bound to v2, and I leave the UNUSED in for use_irq, that means we can
>> then mae the vmstate_v1_ISAIPMIKCSDevice just have:
>>
>> 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_STRUCT(kcs, ISAIPMIKCSDevice, 1, vmstate_IPMIKCS, 
>> IPMIKCS),
>>          VMSTATE_END_OF_LIST()
>>      }
>> };
>>
>> Note the '1'. in the VMSTATE_STRUCT.
>> As I say, that's untested, but if it works it saves a lot of
>> duplication.
>>
>
> Ah I misunderstood something.  The version is sent for the section, 
> and the version
> in the VMSTATE_STRUCT() is passed down when the structure is encoded and
> decoded.  I was thinking the version was transferred in the protocol, 
> but I can see
> now that is not the case.
>
> But....
>
> I implement that, and it doesn't work.  It turns out that there is the 
> following in
> vmstate_load_state():
>
>                } else if (field->flags & VMS_STRUCT) {
>                     ret = vmstate_load_state(f, field->vmsd, curr_elem,
> field->vmsd->version_id);
>
> So it doesn't matter what is in field->version_id, (which is where you 
> say "Note
> the '1' in the VMSTATE_STRUCT).  It passes in whatever the most current
> version is from the vmsd.
>
> Plus that field value is used to compare for the incoming version. If you
> expect that the be the field passed down to the sub-structure, it's going
> to need to be a different field.
>
> So this is not going to work, and the fix is not going to be easy, it 
> appears.

I've spent some time looking at this, and my conclusion is that this only
works by accident.  There appears to be confusion about what this
field does all over the code.

I'm willing to put together a patch that clears up the confusion, but it's
going to be big and messy.  I could also create a new vmstate type,
like VMSTATE_VSTRUCT(), which does what I need, and then we convert
everything else over a piece at a time.

-corey

>
>>> +
>>> +/*
>>> + * 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);
>> OK, a bit scary - I don't think anyone else does that trick of
>> registering it under two names with a '_needed' always saying false.
>
> Yes, I was wondering if I was abusing that interface.
>
>> So I think we have the istuation where the old version had two things
>> with the same name, and if you instantiated both device types you
>> probably lost one of them?
>
> Yes, something like that.  Or more likely, the transfer would just fail.
>
>> In the new world both the 'bt' one is incompatible and renamed,
>> but this one has the hack to load the old kcs version;  so I guess
>> that's OK.
>>
>> Could you just add a comment above that second vmstate_register
>>    ' old version had different (clashing) name, register both'
>
> Certainly.
>
>> However it's worth asking if it's really worth it, if you just ekpt this
>> one called TYPE_IPMI_INTERFACE then you wouldn't need that hack, and the
>> only downside I can see is that loading an image that had a ipmi-bt
>> would fail badly, as opposed to failing clearly.
>
> I was wondering that, too.  I'd be ok with that.
>
> But with the above bug I'm not sure it's possible to fix.  There's no 
> way to pass the
> version being processed to the handling of vmstate_IPMIKCS.
>
> -corey
>
>>
>> Dave
>>
>>>   }
>>>     static void *isa_ipmi_kcs_get_backend_data(IPMIInterface *ii)
>>> -- 
>>> 2.7.4
>>>
>> -- 
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>

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

[Dr. David Alan Gilbert]

* Corey Minyard (minyard@acm.org) wrote:
> On 03/05/2018 08:09 AM, Dr. David Alan Gilbert wrote:
> > * minyard@acm.org (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.
> > > And the
> > > name on the man VMStateDescription was incorrect, it needed to be
> > > differentiated from the BT one.
> > I think that's a bigger problem; lets see below.
> > 
> > > 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()
> > > +    }
> > > +};
> > I've got the following, which is only build tested but:
> > 
> > +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(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_PREFIX "isa-kcs",
> > +    .version_id = 2,
> > +    .minimum_version_id = 2,
> > +    .fields      = (VMStateField[]) {
> > +        VMSTATE_STRUCT(kcs, ISAIPMIKCSDevice, 2, vmstate_IPMIKCS, IPMIKCS),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > 
> > Note how the outlen and inlen fields use the _V modifier and are only
> > bound to v2, and I leave the UNUSED in for use_irq, that means we can
> > then mae the vmstate_v1_ISAIPMIKCSDevice just have:
> > 
> > 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_STRUCT(kcs, ISAIPMIKCSDevice, 1, vmstate_IPMIKCS, IPMIKCS),
> >          VMSTATE_END_OF_LIST()
> >      }
> > };
> > 
> > Note the '1'. in the VMSTATE_STRUCT.
> > As I say, that's untested, but if it works it saves a lot of
> > duplication.
> > 
> 
> Ah I misunderstood something.  The version is sent for the section, and the
> version
> in the VMSTATE_STRUCT() is passed down when the structure is encoded and
> decoded.  I was thinking the version was transferred in the protocol, but I
> can see
> now that is not the case.
> 
> But....
> 
> I implement that, and it doesn't work.  It turns out that there is the
> following in
> vmstate_load_state():
> 
>                } else if (field->flags & VMS_STRUCT) {
>                     ret = vmstate_load_state(f, field->vmsd, curr_elem,
> field->vmsd->version_id);
> 
> So it doesn't matter what is in field->version_id, (which is where you say
> "Note
> the '1' in the VMSTATE_STRUCT).  It passes in whatever the most current
> version is from the vmsd.
> 
> Plus that field value is used to compare for the incoming version. If you
> expect that the be the field passed down to the sub-structure, it's going
> to need to be a different field.
> 
> So this is not going to work, and the fix is not going to be easy, it
> appears.

I think you're right; so rereading, the 'version' in the
VMSTATE_STRUCT macro is behaving like as if it was VMSTATE_STRUCT_V
(named like the other _V macros) - do you agree?
(In actual fact I can see somewhere I used it like that)

<from your follow on mail>

> I've spent some time looking at this, and my conclusion is that this only
> works by accident.  There appears to be confusion about what this
> field does all over the code.
> 
> I'm willing to put together a patch that clears up the confusion, but it's
> going to be big and messy.  I could also create a new vmstate type,
> like VMSTATE_VSTRUCT(), which does what I need, and then we convert
> everything else over a piece at a time.

Can you give me some more detail about what you found?
If as I say above the main problem is that field is really
operating like a VMSTATE_STRUCT_V then we just need a rename there.
Adding a new flag/macro like you say for actually selecting the version
to use in the substructure would be doable if it fits nicely but
that doesn't seem like as massive change.
Have you found any places that you think are actually calling it
with the wrong expectation.

> > > +
> > > +/*
> > > + * 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);
> > OK, a bit scary - I don't think anyone else does that trick of
> > registering it under two names with a '_needed' always saying false.
> 
> Yes, I was wondering if I was abusing that interface.
> 
> > So I think we have the istuation where the old version had two things
> > with the same name, and if you instantiated both device types you
> > probably lost one of them?
> 
> Yes, something like that.  Or more likely, the transfer would just fail.
> 
> > In the new world both the 'bt' one is incompatible and renamed,
> > but this one has the hack to load the old kcs version;  so I guess
> > that's OK.
> > 
> > Could you just add a comment above that second vmstate_register
> >    ' old version had different (clashing) name, register both'
> 
> Certainly.
> 
> > However it's worth asking if it's really worth it, if you just ekpt this
> > one called TYPE_IPMI_INTERFACE then you wouldn't need that hack, and the
> > only downside I can see is that loading an image that had a ipmi-bt
> > would fail badly, as opposed to failing clearly.
> 
> I was wondering that, too.  I'd be ok with that.
> 
> But with the above bug I'm not sure it's possible to fix.  There's no way to
> pass the
> version being processed to the handling of vmstate_IPMIKCS.

I think I'd take a fix for that as long as it was relatively small;
I've got less stomach for a big fix for that field all over the code -
some well placed comemnts explaining what's going on to the unwary
would be welcome though.

Dave

> -corey
> 
> > 
> > Dave
> > 
> > >   }
> > >   static void *isa_ipmi_kcs_get_backend_data(IPMIInterface *ii)
> > > -- 
> > > 2.7.4
> > > 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Corey Minyard]

On 04/24/2018 10:32 AM, Dr. David Alan Gilbert wrote:
> * Corey Minyard (minyard@acm.org) wrote:
>> On 03/05/2018 08:09 AM, Dr. David Alan Gilbert wrote:
>>> * minyard@acm.org (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.
>>>> And the
>>>> name on the man VMStateDescription was incorrect, it needed to be
>>>> differentiated from the BT one.
>>> I think that's a bigger problem; lets see below.
>>>
>>>> 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()
>>>> +    }
>>>> +};
>>> I've got the following, which is only build tested but:
>>>
>>> +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(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_PREFIX "isa-kcs",
>>> +    .version_id = 2,
>>> +    .minimum_version_id = 2,
>>> +    .fields      = (VMStateField[]) {
>>> +        VMSTATE_STRUCT(kcs, ISAIPMIKCSDevice, 2, vmstate_IPMIKCS, IPMIKCS),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>>
>>> Note how the outlen and inlen fields use the _V modifier and are only
>>> bound to v2, and I leave the UNUSED in for use_irq, that means we can
>>> then mae the vmstate_v1_ISAIPMIKCSDevice just have:
>>>
>>> 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_STRUCT(kcs, ISAIPMIKCSDevice, 1, vmstate_IPMIKCS, IPMIKCS),
>>>           VMSTATE_END_OF_LIST()
>>>       }
>>> };
>>>
>>> Note the '1'. in the VMSTATE_STRUCT.
>>> As I say, that's untested, but if it works it saves a lot of
>>> duplication.
>>>
>> Ah I misunderstood something.  The version is sent for the section, and the
>> version
>> in the VMSTATE_STRUCT() is passed down when the structure is encoded and
>> decoded.  I was thinking the version was transferred in the protocol, but I
>> can see
>> now that is not the case.
>>
>> But....
>>
>> I implement that, and it doesn't work.  It turns out that there is the
>> following in
>> vmstate_load_state():
>>
>>                 } else if (field->flags & VMS_STRUCT) {
>>                      ret = vmstate_load_state(f, field->vmsd, curr_elem,
>> field->vmsd->version_id);
>>
>> So it doesn't matter what is in field->version_id, (which is where you say
>> "Note
>> the '1' in the VMSTATE_STRUCT).  It passes in whatever the most current
>> version is from the vmsd.
>>
>> Plus that field value is used to compare for the incoming version. If you
>> expect that the be the field passed down to the sub-structure, it's going
>> to need to be a different field.
>>
>> So this is not going to work, and the fix is not going to be easy, it
>> appears.
> I think you're right; so rereading, the 'version' in the
> VMSTATE_STRUCT macro is behaving like as if it was VMSTATE_STRUCT_V
> (named like the other _V macros) - do you agree?
> (In actual fact I can see somewhere I used it like that)

Yes, I agree.

>
> <from your follow on mail>
>
>> I've spent some time looking at this, and my conclusion is that this only
>> works by accident.  There appears to be confusion about what this
>> field does all over the code.
>>
>> I'm willing to put together a patch that clears up the confusion, but it's
>> going to be big and messy.  I could also create a new vmstate type,
>> like VMSTATE_VSTRUCT(), which does what I need, and then we convert
>> everything else over a piece at a time.
> Can you give me some more detail about what you found?
> If as I say above the main problem is that field is really
> operating like a VMSTATE_STRUCT_V then we just need a rename there.
> Adding a new flag/macro like you say for actually selecting the version
> to use in the substructure would be doable if it fits nicely but
> that doesn't seem like as massive change.
> Have you found any places that you think are actually calling it
> with the wrong expectation.

After looking at it again, I'm not sure what I was referring to. But 
there are
places, like the VMSTATE_IDE_xxx() macros in include/hw/ide/internal.h, 
where
the versions seem to be referring to the versions in the target 
structure.  But
that would mean that the structure using the macros would have to be the
same version as what is in the macros for it to all work correctly. And 
I think
it is.

>>>> +
>>>> +/*
>>>> + * 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);
>>> OK, a bit scary - I don't think anyone else does that trick of
>>> registering it under two names with a '_needed' always saying false.
>> Yes, I was wondering if I was abusing that interface.
>>
>>> So I think we have the istuation where the old version had two things
>>> with the same name, and if you instantiated both device types you
>>> probably lost one of them?
>> Yes, something like that.  Or more likely, the transfer would just fail.
>>
>>> In the new world both the 'bt' one is incompatible and renamed,
>>> but this one has the hack to load the old kcs version;  so I guess
>>> that's OK.
>>>
>>> Could you just add a comment above that second vmstate_register
>>>     ' old version had different (clashing) name, register both'
>> Certainly.
>>
>>> However it's worth asking if it's really worth it, if you just ekpt this
>>> one called TYPE_IPMI_INTERFACE then you wouldn't need that hack, and the
>>> only downside I can see is that loading an image that had a ipmi-bt
>>> would fail badly, as opposed to failing clearly.
>> I was wondering that, too.  I'd be ok with that.
>>
>> But with the above bug I'm not sure it's possible to fix.  There's no way to
>> pass the
>> version being processed to the handling of vmstate_IPMIKCS.
> I think I'd take a fix for that as long as it was relatively small;
> I've got less stomach for a big fix for that field all over the code -
> some well placed comemnts explaining what's going on to the unwary
> would be welcome though.

The use of the _version field in the macros is fairly inconsistent. 
VMSTATE_SINGLE_TEST
has _version, VMSTATE_POINTER has it, VMSTATE_POINTER_TEST does not have 
it, etc.
It would be nice to make it consistent, and I would be willing to work 
on it, but I can
understand why you wouldn't want to do that.  I don't think there is any 
way to do a
small fix that would make any sense, unless it was a bunch of small fixes.

I could add a comment, something like:

/* 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.
  */

Perhaps not ideal, but probably better than nothing.

I guess I need to re-post my updates to the IPMI driver, then.  I also have
a patch to add a VMSTATE_VSTRUCT() macro that lets you specify the
sub-structure version, and a modified IPMI driver to use it.  It does cut
out a bunch of code.  I'd prefer to wait on those and get the main
changes in first, though.

Thanks,

-corey

>
> Dave
>
>> -corey
>>
>>> Dave
>>>
>>>>    }
>>>>    static void *isa_ipmi_kcs_get_backend_data(IPMIInterface *ii)
>>>> -- 
>>>> 2.7.4
>>>>
>>> --
>>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[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