From: Corey Minyard <cminyard@mvista.com>

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

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

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

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/ipmi/isa_ipmi_bt.c | 68 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 55 insertions(+), 13 deletions(-)

diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index e946030..8bbb1fa 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -22,6 +22,7 @@
  * THE SOFTWARE.
  */
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "qapi/error.h"
 #include "hw/hw.h"
 #include "hw/ipmi/ipmi.h"
@@ -450,22 +451,63 @@ static void isa_ipmi_bt_realize(DeviceState *dev, Error **errp)
     isa_register_ioport(isadev, &iib->bt.io, iib->bt.io_base);
 }
 
-static const VMStateDescription vmstate_ISAIPMIBTDevice = {
-    .name = TYPE_IPMI_INTERFACE,
+static int ipmi_bt_vmstate_post_load(void *opaque, int version)
+{
+    IPMIBT *ib = opaque;
+
+    /* Make sure all the values are sane. */
+    if (ib->outpos >= MAX_IPMI_MSG_SIZE || ib->outlen >= MAX_IPMI_MSG_SIZE ||
+        ib->outpos >= ib->outlen) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "ipmi:bt: vmstate transfer received bad out values: %d %d\n",
+                      ib->outpos, ib->outlen);
+        ib->outpos = 0;
+        ib->outlen = 0;
+    }
+
+    if (ib->inlen >= MAX_IPMI_MSG_SIZE) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "ipmi:bt: vmstate transfer received bad in value: %d\n",
+                      ib->inlen);
+        ib->inlen = 0;
+    }
+
+    return 0;
+}
+
+const VMStateDescription vmstate_IPMIBT = {
+    .name = TYPE_IPMI_INTERFACE_PREFIX "bt",
     .version_id = 1,
     .minimum_version_id = 1,
+    .post_load = ipmi_bt_vmstate_post_load,
+    .fields      = (VMStateField[]) {
+        VMSTATE_BOOL(obf_irq_set, IPMIBT),
+        VMSTATE_BOOL(atn_irq_set, IPMIBT),
+        VMSTATE_BOOL(irqs_enabled, IPMIBT),
+        VMSTATE_UINT32(outpos, IPMIBT),
+        VMSTATE_UINT32(outlen, IPMIBT),
+        VMSTATE_UINT8_ARRAY(outmsg, IPMIBT, MAX_IPMI_MSG_SIZE),
+        VMSTATE_UINT32(inlen, IPMIBT),
+        VMSTATE_UINT8_ARRAY(inmsg, IPMIBT, MAX_IPMI_MSG_SIZE),
+        VMSTATE_UINT8(control_reg, IPMIBT),
+        VMSTATE_UINT8(mask_reg, IPMIBT),
+        VMSTATE_UINT8(waiting_rsp, IPMIBT),
+        VMSTATE_UINT8(waiting_seq, IPMIBT),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_ISAIPMIBTDevice = {
+    .name = TYPE_IPMI_INTERFACE_PREFIX "isa-bt",
+    .version_id = 2,
+    .minimum_version_id = 2,
+    /*
+     * Version 1 had messed up the array transfer, it's not even usable
+     * because it used VMSTATE_VBUFFER_UINT32, but it did not transfer
+     * the buffer length, so random things would happen.
+     */
     .fields      = (VMStateField[]) {
-        VMSTATE_BOOL(bt.obf_irq_set, ISAIPMIBTDevice),
-        VMSTATE_BOOL(bt.atn_irq_set, ISAIPMIBTDevice),
-        VMSTATE_BOOL(bt.use_irq, ISAIPMIBTDevice),
-        VMSTATE_BOOL(bt.irqs_enabled, ISAIPMIBTDevice),
-        VMSTATE_UINT32(bt.outpos, ISAIPMIBTDevice),
-        VMSTATE_VBUFFER_UINT32(bt.outmsg, ISAIPMIBTDevice, 1, NULL, bt.outlen),
-        VMSTATE_VBUFFER_UINT32(bt.inmsg, ISAIPMIBTDevice, 1, NULL, bt.inlen),
-        VMSTATE_UINT8(bt.control_reg, ISAIPMIBTDevice),
-        VMSTATE_UINT8(bt.mask_reg, ISAIPMIBTDevice),
-        VMSTATE_UINT8(bt.waiting_rsp, ISAIPMIBTDevice),
-        VMSTATE_UINT8(bt.waiting_seq, ISAIPMIBTDevice),
+        VMSTATE_STRUCT(bt, ISAIPMIBTDevice, 1, vmstate_IPMIBT, IPMIBT),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.7.4

Hi Corey,

On 04/25/2018 12:27 PM, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> The vmstate for isa_ipmi_bt was referencing into the bt structure,
> instead create a bt structure separate and use that.
> 
> The version 1 of the BT transfer was fairly broken, if a migration
> occured during an IPMI operation, it is likely the migration would
> be corrupted because I misunderstood the VMSTATE_VBUFFER_UINT32()
> handling, I thought it handled transferring the length field,
> too.  So I just remove support for that.  I doubt anyone is using
> it at this point.
> 
> This also removes the transfer of use_irq, since that should come
> from configuration.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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

> ---
>  hw/ipmi/isa_ipmi_bt.c | 68 +++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 55 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
> index e946030..8bbb1fa 100644
> --- a/hw/ipmi/isa_ipmi_bt.c
> +++ b/hw/ipmi/isa_ipmi_bt.c
> @@ -22,6 +22,7 @@
>   * THE SOFTWARE.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/log.h"
>  #include "qapi/error.h"
>  #include "hw/hw.h"
>  #include "hw/ipmi/ipmi.h"
> @@ -450,22 +451,63 @@ static void isa_ipmi_bt_realize(DeviceState *dev, Error **errp)
>      isa_register_ioport(isadev, &iib->bt.io, iib->bt.io_base);
>  }
>  
> -static const VMStateDescription vmstate_ISAIPMIBTDevice = {
> -    .name = TYPE_IPMI_INTERFACE,
> +static int ipmi_bt_vmstate_post_load(void *opaque, int version)
> +{
> +    IPMIBT *ib = opaque;
> +
> +    /* Make sure all the values are sane. */
> +    if (ib->outpos >= MAX_IPMI_MSG_SIZE || ib->outlen >= MAX_IPMI_MSG_SIZE ||
> +        ib->outpos >= ib->outlen) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "ipmi:bt: vmstate transfer received bad out values: %d %d\n",
> +                      ib->outpos, ib->outlen);
> +        ib->outpos = 0;
> +        ib->outlen = 0;
> +    }
> +
> +    if (ib->inlen >= MAX_IPMI_MSG_SIZE) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "ipmi:bt: vmstate transfer received bad in value: %d\n",
> +                      ib->inlen);
> +        ib->inlen = 0;
> +    }
> +
> +    return 0;
> +}
> +
> +const VMStateDescription vmstate_IPMIBT = {
> +    .name = TYPE_IPMI_INTERFACE_PREFIX "bt",
>      .version_id = 1,
>      .minimum_version_id = 1,
> +    .post_load = ipmi_bt_vmstate_post_load,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_BOOL(obf_irq_set, IPMIBT),
> +        VMSTATE_BOOL(atn_irq_set, IPMIBT),
> +        VMSTATE_BOOL(irqs_enabled, IPMIBT),
> +        VMSTATE_UINT32(outpos, IPMIBT),
> +        VMSTATE_UINT32(outlen, IPMIBT),
> +        VMSTATE_UINT8_ARRAY(outmsg, IPMIBT, MAX_IPMI_MSG_SIZE),
> +        VMSTATE_UINT32(inlen, IPMIBT),
> +        VMSTATE_UINT8_ARRAY(inmsg, IPMIBT, MAX_IPMI_MSG_SIZE),
> +        VMSTATE_UINT8(control_reg, IPMIBT),
> +        VMSTATE_UINT8(mask_reg, IPMIBT),
> +        VMSTATE_UINT8(waiting_rsp, IPMIBT),
> +        VMSTATE_UINT8(waiting_seq, IPMIBT),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription vmstate_ISAIPMIBTDevice = {
> +    .name = TYPE_IPMI_INTERFACE_PREFIX "isa-bt",
> +    .version_id = 2,
> +    .minimum_version_id = 2,
> +    /*
> +     * Version 1 had messed up the array transfer, it's not even usable
> +     * because it used VMSTATE_VBUFFER_UINT32, but it did not transfer
> +     * the buffer length, so random things would happen.
> +     */
>      .fields      = (VMStateField[]) {
> -        VMSTATE_BOOL(bt.obf_irq_set, ISAIPMIBTDevice),
> -        VMSTATE_BOOL(bt.atn_irq_set, ISAIPMIBTDevice),
> -        VMSTATE_BOOL(bt.use_irq, ISAIPMIBTDevice),
> -        VMSTATE_BOOL(bt.irqs_enabled, ISAIPMIBTDevice),
> -        VMSTATE_UINT32(bt.outpos, ISAIPMIBTDevice),
> -        VMSTATE_VBUFFER_UINT32(bt.outmsg, ISAIPMIBTDevice, 1, NULL, bt.outlen),
> -        VMSTATE_VBUFFER_UINT32(bt.inmsg, ISAIPMIBTDevice, 1, NULL, bt.inlen),
> -        VMSTATE_UINT8(bt.control_reg, ISAIPMIBTDevice),
> -        VMSTATE_UINT8(bt.mask_reg, ISAIPMIBTDevice),
> -        VMSTATE_UINT8(bt.waiting_rsp, ISAIPMIBTDevice),
> -        VMSTATE_UINT8(bt.waiting_seq, ISAIPMIBTDevice),
> +        VMSTATE_STRUCT(bt, ISAIPMIBTDevice, 1, vmstate_IPMIBT, IPMIBT),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> 

On 04/25/2018 03:27 PM, Philippe Mathieu-Daudé wrote:
> Hi Corey,
>
> On 04/25/2018 12:27 PM, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> The vmstate for isa_ipmi_bt was referencing into the bt structure,
>> instead create a bt structure separate and use that.
>>
>> The version 1 of the BT transfer was fairly broken, if a migration
>> occured during an IPMI operation, it is likely the migration would
>> be corrupted because I misunderstood the VMSTATE_VBUFFER_UINT32()
>> handling, I thought it handled transferring the length field,
>> too.  So I just remove support for that.  I doubt anyone is using
>> it at this point.
>>
>> This also removes the transfer of use_irq, since that should come
>> from configuration.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> It seems you mis-pasted Signed-off-by -> Reviewed-by.

Yes, I did. Thanks.

-corey

>> ---
>>   hw/ipmi/isa_ipmi_bt.c | 68 +++++++++++++++++++++++++++++++++++++++++----------
>>   1 file changed, 55 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
>> index e946030..8bbb1fa 100644
>> --- a/hw/ipmi/isa_ipmi_bt.c
>> +++ b/hw/ipmi/isa_ipmi_bt.c
>> @@ -22,6 +22,7 @@
>>    * THE SOFTWARE.
>>    */
>>   #include "qemu/osdep.h"
>> +#include "qemu/log.h"
>>   #include "qapi/error.h"
>>   #include "hw/hw.h"
>>   #include "hw/ipmi/ipmi.h"
>> @@ -450,22 +451,63 @@ static void isa_ipmi_bt_realize(DeviceState *dev, Error **errp)
>>       isa_register_ioport(isadev, &iib->bt.io, iib->bt.io_base);
>>   }
>>   
>> -static const VMStateDescription vmstate_ISAIPMIBTDevice = {
>> -    .name = TYPE_IPMI_INTERFACE,
>> +static int ipmi_bt_vmstate_post_load(void *opaque, int version)
>> +{
>> +    IPMIBT *ib = opaque;
>> +
>> +    /* Make sure all the values are sane. */
>> +    if (ib->outpos >= MAX_IPMI_MSG_SIZE || ib->outlen >= MAX_IPMI_MSG_SIZE ||
>> +        ib->outpos >= ib->outlen) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "ipmi:bt: vmstate transfer received bad out values: %d %d\n",
>> +                      ib->outpos, ib->outlen);
>> +        ib->outpos = 0;
>> +        ib->outlen = 0;
>> +    }
>> +
>> +    if (ib->inlen >= MAX_IPMI_MSG_SIZE) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "ipmi:bt: vmstate transfer received bad in value: %d\n",
>> +                      ib->inlen);
>> +        ib->inlen = 0;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +const VMStateDescription vmstate_IPMIBT = {
>> +    .name = TYPE_IPMI_INTERFACE_PREFIX "bt",
>>       .version_id = 1,
>>       .minimum_version_id = 1,
>> +    .post_load = ipmi_bt_vmstate_post_load,
>> +    .fields      = (VMStateField[]) {
>> +        VMSTATE_BOOL(obf_irq_set, IPMIBT),
>> +        VMSTATE_BOOL(atn_irq_set, IPMIBT),
>> +        VMSTATE_BOOL(irqs_enabled, IPMIBT),
>> +        VMSTATE_UINT32(outpos, IPMIBT),
>> +        VMSTATE_UINT32(outlen, IPMIBT),
>> +        VMSTATE_UINT8_ARRAY(outmsg, IPMIBT, MAX_IPMI_MSG_SIZE),
>> +        VMSTATE_UINT32(inlen, IPMIBT),
>> +        VMSTATE_UINT8_ARRAY(inmsg, IPMIBT, MAX_IPMI_MSG_SIZE),
>> +        VMSTATE_UINT8(control_reg, IPMIBT),
>> +        VMSTATE_UINT8(mask_reg, IPMIBT),
>> +        VMSTATE_UINT8(waiting_rsp, IPMIBT),
>> +        VMSTATE_UINT8(waiting_seq, IPMIBT),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static const VMStateDescription vmstate_ISAIPMIBTDevice = {
>> +    .name = TYPE_IPMI_INTERFACE_PREFIX "isa-bt",
>> +    .version_id = 2,
>> +    .minimum_version_id = 2,
>> +    /*
>> +     * Version 1 had messed up the array transfer, it's not even usable
>> +     * because it used VMSTATE_VBUFFER_UINT32, but it did not transfer
>> +     * the buffer length, so random things would happen.
>> +     */
>>       .fields      = (VMStateField[]) {
>> -        VMSTATE_BOOL(bt.obf_irq_set, ISAIPMIBTDevice),
>> -        VMSTATE_BOOL(bt.atn_irq_set, ISAIPMIBTDevice),
>> -        VMSTATE_BOOL(bt.use_irq, ISAIPMIBTDevice),
>> -        VMSTATE_BOOL(bt.irqs_enabled, ISAIPMIBTDevice),
>> -        VMSTATE_UINT32(bt.outpos, ISAIPMIBTDevice),
>> -        VMSTATE_VBUFFER_UINT32(bt.outmsg, ISAIPMIBTDevice, 1, NULL, bt.outlen),
>> -        VMSTATE_VBUFFER_UINT32(bt.inmsg, ISAIPMIBTDevice, 1, NULL, bt.inlen),
>> -        VMSTATE_UINT8(bt.control_reg, ISAIPMIBTDevice),
>> -        VMSTATE_UINT8(bt.mask_reg, ISAIPMIBTDevice),
>> -        VMSTATE_UINT8(bt.waiting_rsp, ISAIPMIBTDevice),
>> -        VMSTATE_UINT8(bt.waiting_seq, ISAIPMIBTDevice),
>> +        VMSTATE_STRUCT(bt, ISAIPMIBTDevice, 1, vmstate_IPMIBT, IPMIBT),
>>           VMSTATE_END_OF_LIST()
>>       }
>>   };
>>