Re: [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code

[Corey Minyard <minyard@acm.org>]

On 11/15/18 5:01 PM, Philippe Mathieu-Daudé wrote:
> Hi Corey,
>
> On 15/11/18 20:24, minyard@acm.org wrote:
>> These changes allow SMBus access while doing a state transfer.
>> Seems like a good idea to me in general.
>>
>> I have these queued for the SMBus IPMI driver work, of course.
>>
>> I had submitted this before and then lost track of the work since I
>> started finding all kinds of broken things in the I2C code.  I
>> have fixed the broken things I found first, and then added the
>> previous patches.
>>
>> I have tested this in q35 and it works without issue.  On piix4 the
>> pm_smbus code is broken on a migration, however. The device disappears
>> from the PCI bus on a migration, from what I can tell.  It's not the
>> fault of this code, something more fundamental is going on.  The
>> following comment in piix4.c may have something to do with it:
>>
>> /* qemu-kvm 1.2 uses version 3 but advertised as 2
>>   * To support incoming qemu-kvm 1.2 migration, change version_id
>>   * and minimum_version_id to 2 below (which breaks migration from
>>   * qemu 1.2).
>>
>> Anyway, I need to chase that down.
>>
>> I'm primarily submitting this to make sure I'm doing the backwards
>> compatability with .needed correctly.  I'm adding a new field in
>> the machine class and setting it in the initialization code for
>> older versions.  David, is this what you wanted?  It will have to
>> be adjusted for the proper version if/when it really goes in, of
>> course.  You can see those in the following commits:
>>    boards.h: Ignore migration for SMBus devices on
>>    i2c:pm_smbus: Fix state transfer
>>    i2c: Add vmstate handling to the smbus eeprom
>> I thought about adding a field to the pm_smbus code to only transfer
>> if it was accessed, but I'm assuming that most modern OSes will
>> at least initialized the device based on its presence on the PCI
>> bus.  So that didn't seem like it would add any value.
>>
>> I'm also submitting to see if all the fixes and cleanups look ok.
>> That's the first 5 commits.
>
> $ git diff origin/master --summary
>  delete mode 100644 hw/i2c/smbus.c
>  create mode 100644 hw/i2c/smbus_master.c
>  create mode 100644 hw/i2c/smbus_slave.c
>  create mode 100644 include/hw/i2c/smbus_eeprom.h
>  rename include/hw/i2c/{smbus.h => smbus_master.h} (56%)
>  create mode 100644 include/hw/i2c/smbus_slave.h
>
> Can you add the following files in the MAINTAINERS file:
> - hw/i2c/smbus_master.c
> - hw/i2c/smbus_slave.c
> - include/hw/i2c/smbus_eeprom.h
> - include/hw/i2c/smbus_master.h
> - include/hw/i2c/smbus_slave.h
>
Ok, but who should the maintainer be?  I guess I can take
it, if that is what you are implying.  But most of the files in
i2c are not maintained.

Thanks,

-corey