diff mbox series

[PULL,39/53] ipmi: Use proper struct reference for KCS vmstate

Message ID 20180531171606.21604-1-pbonzini@redhat.com
State Accepted
Commit 7e57b82ec3f1a87f623e4baef9ef1ba2b157935c
Headers show
Series None | expand

Commit Message

Paolo Bonzini May 31, 2018, 5:15 p.m. UTC 
From: Corey Minyard <cminyard@mvista.com>


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

There were also some issues in the state transfer.  The inlen field
was not being transferred, so if a transaction was in process during
the transfer it would be messed up.  And the use_irq field was
transferred, but that should come from the configuration.

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

Signed-off-by: Corey Minyard <cminyard@mvista.com>

Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-Id: <1524670052-28373-3-git-send-email-minyard@acm.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

---
 hw/ipmi/isa_ipmi_kcs.c | 81 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 66 insertions(+), 15 deletions(-)

-- 
2.17.0

Comments

Eric Blake May 31, 2018, 7:07 p.m. UTC | #1 
On 05/31/2018 12:15 PM, Paolo Bonzini wrote:
> From: Laszlo Ersek <lersek@redhat.com>

> 

> Add a schema that describes the different uses and properties of virtual

> machine firmware.

> 

> Each firmware executable installed on a host system should come with at

> least one JSON file that conforms to this schema. Each file informs the

> management applications about

> - the firmware's properties and one possible use case / feature set,

> - configuration bits that are required to run the firmware binary.

> 

> In addition, define rules for management apps for picking the highest

> priority firmware JSON file when multiple such files match the search

> criteria.

> 

> Cc: "Daniel P. Berrange" <berrange@redhat.com>

> Cc: David Gibson <dgibson@redhat.com>

> Cc: Eric Blake <eblake@redhat.com>

> Cc: Gerd Hoffmann <kraxel@redhat.com>

> Cc: Kashyap Chamarthy <kchamart@redhat.com>

> Cc: Markus Armbruster <armbru@redhat.com>

> Cc: Paolo Bonzini <pbonzini@redhat.com>

> Cc: Thomas Huth <thuth@redhat.com>

> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

> Message-Id: <20180509152608.9343-1-lersek@redhat.com>

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


Not Laszlo's fault, and not the only such patch that needs it, but

> ---

>   docs/interop/firmware.json | 540 +++++++++++++++++++++++++++++++++++++

>   1 file changed, 540 insertions(+)

>   create mode 100644 docs/interop/firmware.json

> 

> diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json


> +#

> +# Since: 2.13


s/2.13/3.0/g in the interface files to match Peter's recent commits.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Greg Kurz June 8, 2018, 5:39 p.m. UTC | #2 
On Thu, 31 May 2018 19:16:05 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> From: Patryk Olszewski <patryk@fala.ehost.pl>

> 

> This patch fixes a bug in serial that made it almost impossible for guest

> to communicate with devices through host's serial.

> 

> OPOST flag in c_oflag enables output processing letting other flags in

> c_oflag take effect. Usually in c_oflag ONLCR flag is also set, which

> causes crlf to be sent in place of lf. This breaks binary transmissions.

> Unsetting OPOST flag turns off any output processing which fixes the bug.

> 


But it damages error reporting...

Without this patch:

$ qemu-system-ppc64 -serial stdio -kernel foo
foo: No such file or directory
qemu-system-ppc64: error loading foo: Failed to load ELF
$

With this patch:

$ .mbuild-ppc-for-3.0/obj/ppc64-softmmu/qemu-system-ppc64 -serial stdio -kernel foo
foo: No such file or directory
                              qemu-system-ppc64: error loading foo: Failed to load ELF
                                                                                      $

It is possible to patch vreport() to append an explicit CR:

     error_vprintf(fmt, ap);
-    error_printf("\n");
+    error_printf("\n\r");
 }

but it only fixes the trailing newline of error_report(). Any other newline,
eg when using error_append_hint(), will lack the CR... Not sure how to fix
this :-\

> Bug reports related:

> https://bugs.launchpad.net/qemu/+bug/1772086

> https://bugs.launchpad.net/qemu/+bug/1407813

> https://bugs.launchpad.net/qemu/+bug/1715296

> also

> https://lists.nongnu.org/archive/html/qemu-devel/2006-06/msg00196.html

> 

> Signed-off-by: Patryk Olszewski <patryk@fala.ehost.pl>

> Message-Id: <1527105041-21013-1-git-send-email-patryk@fala.ehost.pl>

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

> Reviewed-by: Thomas Huth <thuth@redhat.com>

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> ---

>  chardev/char-serial.c | 2 +-

>  chardev/char-stdio.c  | 2 +-

>  2 files changed, 2 insertions(+), 2 deletions(-)

> 

> diff --git a/chardev/char-serial.c b/chardev/char-serial.c

> index feb52e559d..ae548d28da 100644

> --- a/chardev/char-serial.c

> +++ b/chardev/char-serial.c

> @@ -139,7 +139,7 @@ static void tty_serial_init(int fd, int speed,

>  

>      tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP

>                       | INLCR | IGNCR | ICRNL | IXON);

> -    tty.c_oflag |= OPOST;

> +    tty.c_oflag &= ~OPOST;

>      tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN | ISIG);

>      tty.c_cflag &= ~(CSIZE | PARENB | PARODD | CRTSCTS | CSTOPB);

>      switch (data_bits) {

> diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c

> index 96375f2ab8..d83e60e787 100644

> --- a/chardev/char-stdio.c

> +++ b/chardev/char-stdio.c

> @@ -59,7 +59,7 @@ static void qemu_chr_set_echo_stdio(Chardev *chr, bool echo)

>      if (!echo) {

>          tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP

>                           | INLCR | IGNCR | ICRNL | IXON);

> -        tty.c_oflag |= OPOST;

> +        tty.c_oflag &= ~OPOST;

>          tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN);

>          tty.c_cflag &= ~(CSIZE | PARENB);

>          tty.c_cflag |= CS8;
Philippe Mathieu-Daudé June 8, 2018, 5:56 p.m. UTC | #3 
On 06/08/2018 02:39 PM, Greg Kurz wrote:
> On Thu, 31 May 2018 19:16:05 +0200

> Paolo Bonzini <pbonzini@redhat.com> wrote:

> 

>> From: Patryk Olszewski <patryk@fala.ehost.pl>

>>

>> This patch fixes a bug in serial that made it almost impossible for guest

>> to communicate with devices through host's serial.

>>

>> OPOST flag in c_oflag enables output processing letting other flags in

>> c_oflag take effect. Usually in c_oflag ONLCR flag is also set, which

>> causes crlf to be sent in place of lf. This breaks binary transmissions.

>> Unsetting OPOST flag turns off any output processing which fixes the bug.

>>

> 

> But it damages error reporting...

> 

> Without this patch:

> 

> $ qemu-system-ppc64 -serial stdio -kernel foo

> foo: No such file or directory

> qemu-system-ppc64: error loading foo: Failed to load ELF

> $

> 

> With this patch:

> 

> $ .mbuild-ppc-for-3.0/obj/ppc64-softmmu/qemu-system-ppc64 -serial stdio -kernel foo

> foo: No such file or directory

>                               qemu-system-ppc64: error loading foo: Failed to load ELF

>                                                                                       $

> 

> It is possible to patch vreport() to append an explicit CR:

> 

>      error_vprintf(fmt, ap);

> -    error_printf("\n");

> +    error_printf("\n\r");

>  }

> 

> but it only fixes the trailing newline of error_report(). Any other newline,

> eg when using error_append_hint(), will lack the CR... Not sure how to fix

> this :-\


Peter just pushed the fix (ed6b018ef7):

http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg02152.html

> 

>> Bug reports related:

>> https://bugs.launchpad.net/qemu/+bug/1772086

>> https://bugs.launchpad.net/qemu/+bug/1407813

>> https://bugs.launchpad.net/qemu/+bug/1715296

>> also

>> https://lists.nongnu.org/archive/html/qemu-devel/2006-06/msg00196.html

>>

>> Signed-off-by: Patryk Olszewski <patryk@fala.ehost.pl>

>> Message-Id: <1527105041-21013-1-git-send-email-patryk@fala.ehost.pl>

>> Reviewed-by: Markus Armbruster <armbru@redhat.com>

>> Reviewed-by: Thomas Huth <thuth@redhat.com>

>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

>> ---

>>  chardev/char-serial.c | 2 +-

>>  chardev/char-stdio.c  | 2 +-

>>  2 files changed, 2 insertions(+), 2 deletions(-)

>>

>> diff --git a/chardev/char-serial.c b/chardev/char-serial.c

>> index feb52e559d..ae548d28da 100644

>> --- a/chardev/char-serial.c

>> +++ b/chardev/char-serial.c

>> @@ -139,7 +139,7 @@ static void tty_serial_init(int fd, int speed,

>>  

>>      tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP

>>                       | INLCR | IGNCR | ICRNL | IXON);

>> -    tty.c_oflag |= OPOST;

>> +    tty.c_oflag &= ~OPOST;

>>      tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN | ISIG);

>>      tty.c_cflag &= ~(CSIZE | PARENB | PARODD | CRTSCTS | CSTOPB);

>>      switch (data_bits) {

>> diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c

>> index 96375f2ab8..d83e60e787 100644

>> --- a/chardev/char-stdio.c

>> +++ b/chardev/char-stdio.c

>> @@ -59,7 +59,7 @@ static void qemu_chr_set_echo_stdio(Chardev *chr, bool echo)

>>      if (!echo) {

>>          tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP

>>                           | INLCR | IGNCR | ICRNL | IXON);

>> -        tty.c_oflag |= OPOST;

>> +        tty.c_oflag &= ~OPOST;

>>          tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN);

>>          tty.c_cflag &= ~(CSIZE | PARENB);

>>          tty.c_cflag |= CS8;

>
Patryk Olszewski June 8, 2018, 6:08 p.m. UTC | #4 
W dniu 08.06.2018 o 19:39, Greg Kurz pisze:
> On Thu, 31 May 2018 19:16:05 +0200

> Paolo Bonzini <pbonzini@redhat.com> wrote:

>

>> From: Patryk Olszewski <patryk@fala.ehost.pl>

>>

>> This patch fixes a bug in serial that made it almost impossible for guest

>> to communicate with devices through host's serial.

>>

>> OPOST flag in c_oflag enables output processing letting other flags in

>> c_oflag take effect. Usually in c_oflag ONLCR flag is also set, which

>> causes crlf to be sent in place of lf. This breaks binary transmissions.

>> Unsetting OPOST flag turns off any output processing which fixes the bug.

>>

> But it damages error reporting...

>

> Without this patch:

>

> $ qemu-system-ppc64 -serial stdio -kernel foo

> foo: No such file or directory

> qemu-system-ppc64: error loading foo: Failed to load ELF

> $

>

> With this patch:

>

> $ .mbuild-ppc-for-3.0/obj/ppc64-softmmu/qemu-system-ppc64 -serial stdio -kernel foo

> foo: No such file or directory

>                               qemu-system-ppc64: error loading foo: Failed to load ELF

>                                                                                       $

>

> It is possible to patch vreport() to append an explicit CR:

>

>      error_vprintf(fmt, ap);

> -    error_printf("\n");

> +    error_printf("\n\r");

>  }

>

> but it only fixes the trailing newline of error_report(). Any other newline,

> eg when using error_append_hint(), will lack the CR... Not sure how to fix

> this :-\

>

>> Bug reports related:

>> https://bugs.launchpad.net/qemu/+bug/1772086

>> https://bugs.launchpad.net/qemu/+bug/1407813

>> https://bugs.launchpad.net/qemu/+bug/1715296

>> also

>> https://lists.nongnu.org/archive/html/qemu-devel/2006-06/msg00196.html

>>

>> Signed-off-by: Patryk Olszewski <patryk@fala.ehost.pl>

>> Message-Id: <1527105041-21013-1-git-send-email-patryk@fala.ehost.pl>

>> Reviewed-by: Markus Armbruster <armbru@redhat.com>

>> Reviewed-by: Thomas Huth <thuth@redhat.com>

>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

>> ---

>>  chardev/char-serial.c | 2 +-

>>  chardev/char-stdio.c  | 2 +-

>>  2 files changed, 2 insertions(+), 2 deletions(-)

>>

>> diff --git a/chardev/char-serial.c b/chardev/char-serial.c

>> index feb52e559d..ae548d28da 100644

>> --- a/chardev/char-serial.c

>> +++ b/chardev/char-serial.c

>> @@ -139,7 +139,7 @@ static void tty_serial_init(int fd, int speed,

>>  

>>      tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP

>>                       | INLCR | IGNCR | ICRNL | IXON);

>> -    tty.c_oflag |= OPOST;

>> +    tty.c_oflag &= ~OPOST;

>>      tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN | ISIG);

>>      tty.c_cflag &= ~(CSIZE | PARENB | PARODD | CRTSCTS | CSTOPB);

>>      switch (data_bits) {

>> diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c

>> index 96375f2ab8..d83e60e787 100644

>> --- a/chardev/char-stdio.c

>> +++ b/chardev/char-stdio.c

>> @@ -59,7 +59,7 @@ static void qemu_chr_set_echo_stdio(Chardev *chr, bool echo)

>>      if (!echo) {

>>          tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP

>>                           | INLCR | IGNCR | ICRNL | IXON);

>> -        tty.c_oflag |= OPOST;

>> +        tty.c_oflag &= ~OPOST;

>>          tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN);

>>          tty.c_cflag &= ~(CSIZE | PARENB);

>>          tty.c_cflag |= CS8;


The change to char-stdio.c wasn't actually introduced by me.
(https://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg05416.html).
Anyway, I haven't yet investigated it thoroughly but right now I think the problem is with that error reporting system. After all serial device shouldn't alter data coming from the guest. You never know when somebody will come up with crazy idea of pushing binary data through stdout.
Greg Kurz June 9, 2018, 7:31 a.m. UTC | #5 
On Fri, 8 Jun 2018 14:56:20 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 06/08/2018 02:39 PM, Greg Kurz wrote:

> > On Thu, 31 May 2018 19:16:05 +0200

> > Paolo Bonzini <pbonzini@redhat.com> wrote:

> >   

> >> From: Patryk Olszewski <patryk@fala.ehost.pl>

> >>

> >> This patch fixes a bug in serial that made it almost impossible for guest

> >> to communicate with devices through host's serial.

> >>

> >> OPOST flag in c_oflag enables output processing letting other flags in

> >> c_oflag take effect. Usually in c_oflag ONLCR flag is also set, which

> >> causes crlf to be sent in place of lf. This breaks binary transmissions.

> >> Unsetting OPOST flag turns off any output processing which fixes the bug.

> >>  

> > 

> > But it damages error reporting...

> > 

> > Without this patch:

> > 

> > $ qemu-system-ppc64 -serial stdio -kernel foo

> > foo: No such file or directory

> > qemu-system-ppc64: error loading foo: Failed to load ELF

> > $

> > 

> > With this patch:

> > 

> > $ .mbuild-ppc-for-3.0/obj/ppc64-softmmu/qemu-system-ppc64 -serial stdio -kernel foo

> > foo: No such file or directory

> >                               qemu-system-ppc64: error loading foo: Failed to load ELF

> >                                                                                       $

> > 

> > It is possible to patch vreport() to append an explicit CR:

> > 

> >      error_vprintf(fmt, ap);

> > -    error_printf("\n");

> > +    error_printf("\n\r");

> >  }

> > 

> > but it only fixes the trailing newline of error_report(). Any other newline,

> > eg when using error_append_hint(), will lack the CR... Not sure how to fix

> > this :-\  

> 

> Peter just pushed the fix (ed6b018ef7):

> 

> http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg02152.html

> 


Ah, cool ! :)

> >   

> >> Bug reports related:

> >> https://bugs.launchpad.net/qemu/+bug/1772086

> >> https://bugs.launchpad.net/qemu/+bug/1407813

> >> https://bugs.launchpad.net/qemu/+bug/1715296

> >> also

> >> https://lists.nongnu.org/archive/html/qemu-devel/2006-06/msg00196.html

> >>

> >> Signed-off-by: Patryk Olszewski <patryk@fala.ehost.pl>

> >> Message-Id: <1527105041-21013-1-git-send-email-patryk@fala.ehost.pl>

> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>

> >> Reviewed-by: Thomas Huth <thuth@redhat.com>

> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> >> ---

> >>  chardev/char-serial.c | 2 +-

> >>  chardev/char-stdio.c  | 2 +-

> >>  2 files changed, 2 insertions(+), 2 deletions(-)

> >>

> >> diff --git a/chardev/char-serial.c b/chardev/char-serial.c

> >> index feb52e559d..ae548d28da 100644

> >> --- a/chardev/char-serial.c

> >> +++ b/chardev/char-serial.c

> >> @@ -139,7 +139,7 @@ static void tty_serial_init(int fd, int speed,

> >>  

> >>      tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP

> >>                       | INLCR | IGNCR | ICRNL | IXON);

> >> -    tty.c_oflag |= OPOST;

> >> +    tty.c_oflag &= ~OPOST;

> >>      tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN | ISIG);

> >>      tty.c_cflag &= ~(CSIZE | PARENB | PARODD | CRTSCTS | CSTOPB);

> >>      switch (data_bits) {

> >> diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c

> >> index 96375f2ab8..d83e60e787 100644

> >> --- a/chardev/char-stdio.c

> >> +++ b/chardev/char-stdio.c

> >> @@ -59,7 +59,7 @@ static void qemu_chr_set_echo_stdio(Chardev *chr, bool echo)

> >>      if (!echo) {

> >>          tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP

> >>                           | INLCR | IGNCR | ICRNL | IXON);

> >> -        tty.c_oflag |= OPOST;

> >> +        tty.c_oflag &= ~OPOST;

> >>          tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN);

> >>          tty.c_cflag &= ~(CSIZE | PARENB);

> >>          tty.c_cflag |= CS8;  

> >
diff mbox series

Patch

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