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 (miny...@acm.org) wrote:

On 03/05/2018 08:09 AM, Dr. David Alan Gilbert wrote:

* miny...@acm.org (miny...@acm.org) wrote:

From: Corey Minyard 

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 
Cc: Dr. David Alan Gilbert 
---
   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, >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,
  

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

[Dr. David Alan Gilbert]

* Corey Minyard (miny...@acm.org) wrote:
> On 03/05/2018 08:09 AM, Dr. David Alan Gilbert wrote:
> > * miny...@acm.org (miny...@acm.org) wrote:
> > > From: Corey Minyard 
> > > 
> > > 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 
> > > Cc: Dr. David Alan Gilbert 
> > > ---
> > >   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, >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 

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:

* miny...@acm.org (miny...@acm.org) wrote:

From: Corey Minyard 

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 
Cc: Dr. David Alan Gilbert 
---
  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, >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 = 

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:

* miny...@acm.org (miny...@acm.org) wrote:

From: Corey Minyard 

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 
Cc: Dr. David Alan Gilbert 
---
  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, >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,
 

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

[Dr. David Alan Gilbert]

* miny...@acm.org (miny...@acm.org) wrote:
> From: Corey Minyard 
> 
> 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 
> Cc: Dr. David Alan Gilbert 
> ---
>  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, >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,