* [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer
@ 2018-03-02 15:26 minyard
2018-03-02 15:26 ` [Qemu-devel] [PATCH v2 1/2] ipmi: Use proper struct reference for KCS vmstate minyard
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: minyard @ 2018-03-02 15:26 UTC (permalink / raw)
To: qemu-devel, Peter Maydell; +Cc: Dr . David Alan Gilbert
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(-)
^ permalink raw reply [flat|nested] 13+ messages in thread* [Qemu-devel] [PATCH v2 1/2] ipmi: Use proper struct reference for KCS vmstate 2018-03-02 15:26 [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer minyard @ 2018-03-02 15:26 ` minyard 2018-03-05 14:09 ` Dr. David Alan Gilbert 2018-03-02 15:26 ` [Qemu-devel] [PATCH v2 2/2] ipmi: Use proper struct reference for BT vmstate minyard ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: minyard @ 2018-03-02 15:26 UTC (permalink / raw) To: qemu-devel, Peter Maydell; +Cc: Dr . David Alan Gilbert, Corey 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] ipmi: Use proper struct reference for KCS vmstate 2018-03-02 15:26 ` [Qemu-devel] [PATCH v2 1/2] ipmi: Use proper struct reference for KCS vmstate minyard @ 2018-03-05 14:09 ` Dr. David Alan Gilbert 2018-03-05 22:52 ` Corey Minyard 0 siblings, 1 reply; 13+ messages in thread From: Dr. David Alan Gilbert @ 2018-03-05 14:09 UTC (permalink / raw) To: minyard; +Cc: qemu-devel, Peter Maydell, Corey Minyard * 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] ipmi: Use proper struct reference for KCS vmstate 2018-03-05 14:09 ` Dr. David Alan Gilbert @ 2018-03-05 22:52 ` Corey Minyard 2018-03-06 16:46 ` Corey Minyard 2018-04-24 15:32 ` Dr. David Alan Gilbert 0 siblings, 2 replies; 13+ messages in thread From: Corey Minyard @ 2018-03-05 22:52 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: qemu-devel, Peter Maydell, 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] ipmi: Use proper struct reference for KCS vmstate 2018-03-05 22:52 ` Corey Minyard @ 2018-03-06 16:46 ` Corey Minyard 2018-04-24 15:32 ` Dr. David Alan Gilbert 1 sibling, 0 replies; 13+ messages in thread From: Corey Minyard @ 2018-03-06 16:46 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: qemu-devel, Peter Maydell, 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 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] ipmi: Use proper struct reference for KCS vmstate 2018-03-05 22:52 ` Corey Minyard 2018-03-06 16:46 ` Corey Minyard @ 2018-04-24 15:32 ` Dr. David Alan Gilbert 2018-04-24 21:08 ` Corey Minyard 1 sibling, 1 reply; 13+ messages in thread From: Dr. David Alan Gilbert @ 2018-04-24 15:32 UTC (permalink / raw) To: Corey Minyard; +Cc: qemu-devel, Peter Maydell, Corey Minyard * 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] ipmi: Use proper struct reference for KCS vmstate 2018-04-24 15:32 ` Dr. David Alan Gilbert @ 2018-04-24 21:08 ` Corey Minyard 0 siblings, 0 replies; 13+ messages in thread From: Corey Minyard @ 2018-04-24 21:08 UTC (permalink / raw) To: Dr. David Alan Gilbert, Corey Minyard; +Cc: qemu-devel, Peter Maydell 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] ipmi: Use proper struct reference for BT vmstate 2018-03-02 15:26 [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer minyard 2018-03-02 15:26 ` [Qemu-devel] [PATCH v2 1/2] ipmi: Use proper struct reference for KCS vmstate minyard @ 2018-03-02 15:26 ` minyard 2018-03-05 12:00 ` Dr. David Alan Gilbert 2018-03-02 20:02 ` [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer Dr. David Alan Gilbert 2018-03-05 13:29 ` Peter Maydell 3 siblings, 1 reply; 13+ messages in thread From: minyard @ 2018-03-02 15:26 UTC (permalink / raw) To: qemu-devel, Peter Maydell; +Cc: Dr . David Alan Gilbert, Corey 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] ipmi: Use proper struct reference for BT vmstate 2018-03-02 15:26 ` [Qemu-devel] [PATCH v2 2/2] ipmi: Use proper struct reference for BT vmstate minyard @ 2018-03-05 12:00 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 13+ messages in thread From: Dr. David Alan Gilbert @ 2018-03-05 12:00 UTC (permalink / raw) To: minyard; +Cc: qemu-devel, Peter Maydell, Corey Minyard * 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer 2018-03-02 15:26 [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer minyard 2018-03-02 15:26 ` [Qemu-devel] [PATCH v2 1/2] ipmi: Use proper struct reference for KCS vmstate minyard 2018-03-02 15:26 ` [Qemu-devel] [PATCH v2 2/2] ipmi: Use proper struct reference for BT vmstate minyard @ 2018-03-02 20:02 ` Dr. David Alan Gilbert 2018-03-02 20:09 ` Corey Minyard 2018-03-05 13:29 ` Peter Maydell 3 siblings, 1 reply; 13+ messages in thread From: Dr. David Alan Gilbert @ 2018-03-02 20:02 UTC (permalink / raw) To: minyard; +Cc: qemu-devel, Peter Maydell * 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer 2018-03-02 20:02 ` [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer Dr. David Alan Gilbert @ 2018-03-02 20:09 ` Corey Minyard 0 siblings, 0 replies; 13+ messages in thread From: Corey Minyard @ 2018-03-02 20:09 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: qemu-devel, Peter Maydell 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer 2018-03-02 15:26 [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer minyard ` (2 preceding siblings ...) 2018-03-02 20:02 ` [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer Dr. David Alan Gilbert @ 2018-03-05 13:29 ` Peter Maydell 3 siblings, 0 replies; 13+ messages in thread From: Peter Maydell @ 2018-03-05 13:29 UTC (permalink / raw) To: Corey Minyard; +Cc: QEMU Developers, Dr . David Alan Gilbert 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer
@ 2018-02-14 18:23 minyard
2018-02-14 18:23 ` [Qemu-devel] [PATCH v2 1/2] ipmi: Use proper struct reference for KCS vmstate minyard
0 siblings, 1 reply; 13+ messages in thread
From: minyard @ 2018-02-14 18:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Dr . David Alan Gilbert
This took a while because I had other distractions and I spent some
time studying how all this vmstate code works, and that itself got
to be a distraction.
I'm not 100% sure what I have done is right. Maybe there is a way to
do a backwards migration for KCS, but I'm not sure how.
Changes from v1:
* Validate the data values in a 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.
^ permalink raw reply [flat|nested] 13+ messages in thread* [Qemu-devel] [PATCH v2 1/2] ipmi: Use proper struct reference for KCS vmstate 2018-02-14 18:23 minyard @ 2018-02-14 18:23 ` minyard 0 siblings, 0 replies; 13+ messages in thread From: minyard @ 2018-02-14 18:23 UTC (permalink / raw) To: qemu-devel; +Cc: Dr . David Alan Gilbert, Corey 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-04-24 21:08 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-03-02 15:26 [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer minyard 2018-03-02 15:26 ` [Qemu-devel] [PATCH v2 1/2] ipmi: Use proper struct reference for KCS vmstate minyard 2018-03-05 14:09 ` Dr. David Alan Gilbert 2018-03-05 22:52 ` Corey Minyard 2018-03-06 16:46 ` Corey Minyard 2018-04-24 15:32 ` Dr. David Alan Gilbert 2018-04-24 21:08 ` Corey Minyard 2018-03-02 15:26 ` [Qemu-devel] [PATCH v2 2/2] ipmi: Use proper struct reference for BT vmstate minyard 2018-03-05 12:00 ` Dr. David Alan Gilbert 2018-03-02 20:02 ` [Qemu-devel] [PATCH v2 0/2] ipmi: Fix vmstate transfer Dr. David Alan Gilbert 2018-03-02 20:09 ` Corey Minyard 2018-03-05 13:29 ` Peter Maydell -- strict thread matches above, loose matches on Subject: below -- 2018-02-14 18:23 minyard 2018-02-14 18:23 ` [Qemu-devel] [PATCH v2 1/2] ipmi: Use proper struct reference for KCS vmstate minyard
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.