Re: [Qemu-devel] [PATCH v2] i2c: fix migration regression introduced by broadcast support

[Paolo Bonzini]

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 <address@hidden>
> ---
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
> 
> ---
>  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