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

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

2018-04-24 Thread 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

2018-04-24 Thread 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

2018-03-06 Thread 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

2018-03-05 Thread 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

2018-03-05 Thread 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,

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

2018-03-02 Thread minyard

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.

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()
+}
+};
+
+/*
+ * 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(>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 **) >kcs.bmc);
 
 vmstate_register(NULL, 0, _ISAIPMIKCSDevice, iik);
+vmstate_register(NULL, 0, _v1_ISAIPMIKCSDevice, iik);
 }
 
 static void *isa_ipmi_kcs_get_backend_data(IPMIInterface *ii)
-- 
2.7.4

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

2018-02-14 Thread minyard

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.

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()
+}
+};
+
+/*
+ * 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(>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 **) >kcs.bmc);
 
 vmstate_register(NULL, 0, _ISAIPMIKCSDevice, iik);
+vmstate_register(NULL, 0, _v1_ISAIPMIKCSDevice, iik);
 }
 
 static void *isa_ipmi_kcs_get_backend_data(IPMIInterface *ii)
-- 
2.7.4

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

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

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

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

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

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

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

7 matches

Site Navigation

Mail list logo

Footer information