From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58201) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bU8rW-0006Rh-Ku for qemu-devel@nongnu.org; Mon, 01 Aug 2016 04:46:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bU8rS-0004wY-AK for qemu-devel@nongnu.org; Mon, 01 Aug 2016 04:46:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59672) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bU8rS-0004wM-1O for qemu-devel@nongnu.org; Mon, 01 Aug 2016 04:46:10 -0400 References: <82468299.10757116.1469547105896.JavaMail.zimbra@redhat.com> <1469623198-177227-1-git-send-email-imammedo@redhat.com> From: Paolo Bonzini Message-ID: <3d7699b6-c60c-de52-eb42-43aab1916874@redhat.com> Date: Mon, 1 Aug 2016 10:46:04 +0200 MIME-Version: 1.0 In-Reply-To: <1469623198-177227-1-git-send-email-imammedo@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] i2c: fix migration regression introduced by broadcast support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov , qemu-devel@nongnu.org Cc: amit.shah@redhat.com, dgilbert@redhat.com, fred.konrad@greensocs.com, alistair.francis@xilinx.com, crosthwaite.peter@gmail.com, hyun.kwon@xilinx.com, peter.maydell@linaro.org On 27/07/2016 14:39, Igor Mammedov wrote: > QEMU fails migration with following error: > > qemu-system-x86_64: Missing section footer for i2c_bus > qemu-system-x86_64: load of migration failed: Invalid argument > > when migrating from: > qemu-system-x86_64-v2.6.0 -m 256M rhel72.img -M pc-i440fx-2.6 > to > qemu-system-x86_64-v2.7.0-rc0 -m 256M rhel72.img -M pc-i440fx-2.6 > > Regression is added by commit 2293c27f (i2c: implement broadcast write) > > Fix it by dropping 'broadcast' VMState introduced by 2293c27f and > reuse broadcast 0x00 address as broadcast flag in bus->saved_address. > Then if there were ongoing broadcast at migration time, set > bus->saved_address to it and at i2c_slave_post_load() time check > for it instead of transfering and using 'broadcast' VMState. > > As result of reusing existing saved_address VMState, no compat > glue will be needed to keep forward/backward compatiblity. which > makes fix much less intrusive. > > Signed-off-by: Igor Mammedov > --- > CC: fred.konrad@greensocs.com > CC: alistair.francis@xilinx.com > CC: crosthwaite.peter@gmail.com > CC: hyun.kwon@xilinx.com > CC: peter.maydell@linaro.org > > --- > hw/i2c/core.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/hw/i2c/core.c b/hw/i2c/core.c > index abb3efb..4afbe0b 100644 > --- a/hw/i2c/core.c > +++ b/hw/i2c/core.c > @@ -17,6 +17,8 @@ struct I2CNode { > QLIST_ENTRY(I2CNode) next; > }; > > +#define I2C_BROADCAST 0x00 > + > struct I2CBus > { > BusState qbus; > @@ -47,6 +49,8 @@ static void i2c_bus_pre_save(void *opaque) > if (!QLIST_EMPTY(&bus->current_devs)) { > if (!bus->broadcast) { > bus->saved_address = QLIST_FIRST(&bus->current_devs)->elt->address; > + } else { > + bus->saved_address = I2C_BROADCAST; > } > } > } > @@ -58,7 +62,6 @@ static const VMStateDescription vmstate_i2c_bus = { > .pre_save = i2c_bus_pre_save, > .fields = (VMStateField[]) { > VMSTATE_UINT8(saved_address, I2CBus), > - VMSTATE_BOOL(broadcast, I2CBus), > VMSTATE_END_OF_LIST() > } > }; > @@ -93,7 +96,7 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv) > I2CSlaveClass *sc; > I2CNode *node; > > - if (address == 0x00) { > + if (address == I2C_BROADCAST) { > /* > * This is a broadcast, the current_devs will be all the devices of the > * bus. > @@ -221,7 +224,8 @@ static int i2c_slave_post_load(void *opaque, int version_id) > I2CNode *node; > > bus = I2C_BUS(qdev_get_parent_bus(DEVICE(dev))); > - if ((bus->saved_address == dev->address) || (bus->broadcast)) { > + if ((bus->saved_address == dev->address) || > + (bus->saved_address == I2C_BROADCAST)) { > node = g_malloc(sizeof(struct I2CNode)); > node->elt = dev; > QLIST_INSERT_HEAD(&bus->current_devs, node, next); > That's better than both the v1 patch and my suggestion. Good! I've queued the patch and hope to send a pull request tomorrow. Paolo