[Qemu-devel] [PATCH v3 00/16] Fix/add vmstate handling in some I2C code

[Qemu-devel] [PATCH v3 00/16] Fix/add vmstate handling in some I2C code

[minyard]

I believe I've fixed all the issues pointed out by everyone, and I've
tested migration between 2.12 and 3.0 and back on q35 and piix4
(with the fix I posted earlier for piix4).

Changes since v2:

Added proper license headers for the newly created files.

Added myself as a maintainer of the i2c core files.

Removed unneeded code in several places pointed out by Peter Maydell
since i2c_recv() returns a uint8_t instead of an int.  These are
in two separate patches.

Added a patch from Philippe Mathieu-Daudé <philmd@redhat.com> to
replace a magic number with a constant in the smbus_eeprom code and
verify the count so there is no overflow.

Updated the pm_smbus migration to not update the version numbers
in the existing vmstate structures.  The needed field didn't
accomplish what was needed because it is only called on the save
side, not the load side of the migration.  So I'm using
VMSTATE_STRUCT_TEST() to transfer the pm_smbus data now, that
should be all that is required for backwards compatibility, and
will be safer for falling back to an older version.

I found the problem with the SMBus being broken on piix4 after a
migration, but I sent it out as a separate patch since it has
nothing to do with this series, and should probably go into the
current release being worked on.

[Qemu-devel] [PATCH v3 01/16] i2c: Split smbus into parts

[minyard]

From: Corey Minyard <cminyard@mvista.com>

smbus.c and smbus.h had device side code, master side code, and
smbus.h has some smbus_eeprom.c definitions.  Split them into
separate files.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 MAINTAINERS                               |  12 ++
 hw/arm/aspeed.c                           |   2 +-
 hw/i2c/Makefile.objs                      |   2 +-
 hw/i2c/pm_smbus.c                         |   2 +-
 hw/i2c/smbus_eeprom.c                     |   3 +-
 hw/i2c/smbus_ich9.c                       |   2 -
 hw/i2c/smbus_master.c                     | 165 ++++++++++++++++++++++
 hw/i2c/{smbus.c => smbus_slave.c}         | 153 +-------------------
 hw/i386/pc_piix.c                         |   2 +-
 hw/i386/pc_q35.c                          |   2 +-
 hw/isa/vt82c686.c                         |   1 -
 hw/mips/mips_fulong2e.c                   |   2 +-
 hw/mips/mips_malta.c                      |   2 +-
 hw/ppc/sam460ex.c                         |   2 +-
 include/hw/i2c/pm_smbus.h                 |   2 +
 include/hw/i2c/smbus_eeprom.h             |  32 +++++
 include/hw/i2c/smbus_master.h             |  55 ++++++++
 include/hw/i2c/{smbus.h => smbus_slave.h} |  37 +----
 18 files changed, 285 insertions(+), 193 deletions(-)
 create mode 100644 hw/i2c/smbus_master.c
 rename hw/i2c/{smbus.c => smbus_slave.c} (64%)
 create mode 100644 include/hw/i2c/smbus_eeprom.h
 create mode 100644 include/hw/i2c/smbus_master.h
 rename include/hw/i2c/{smbus.h => smbus_slave.h} (65%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0499e11593..62340d32b8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1935,6 +1935,18 @@ M: Viktor Prutyanov <viktor.prutyanov@phystech.edu>
 S: Maintained
 F: contrib/elf2dmp/
 
+I2C
+M: Corey Minyard <cminyard@mvista.com>
+S: Maintained
+F: hw/i2c/core.c
+F: hw/i2c/smbus_slave.c
+F: hw/i2c/smbus_master.c
+F: hw/i2c/smbus_eeprom.c
+F: include/hw/i2c/i2c.h
+F: include/hw/i2c/smbus_master.h
+F: include/hw/i2c/smbus_slave.h
+F: include/hw/i2c/smbus_eeprom.h
+
 Usermode Emulation
 ------------------
 Overall
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 6b33ecd5aa..69a19df00d 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -18,7 +18,7 @@
 #include "hw/arm/aspeed.h"
 #include "hw/arm/aspeed_soc.h"
 #include "hw/boards.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_eeprom.h"
 #include "qemu/log.h"
 #include "sysemu/block-backend.h"
 #include "hw/loader.h"
diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
index 37cacde978..8973edfa22 100644
--- a/hw/i2c/Makefile.objs
+++ b/hw/i2c/Makefile.objs
@@ -1,4 +1,4 @@
-common-obj-$(CONFIG_I2C) += core.o smbus.o smbus_eeprom.o
+common-obj-$(CONFIG_I2C) += core.o smbus_slave.o smbus_master.o smbus_eeprom.o
 common-obj-$(CONFIG_DDC) += i2c-ddc.o
 common-obj-$(CONFIG_VERSATILE_I2C) += versatile_i2c.o
 common-obj-$(CONFIG_ACPI_X86) += smbus_ich9.o
diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 685a2378ed..f3c6cc46f9 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -20,7 +20,7 @@
 #include "qemu/osdep.h"
 #include "hw/hw.h"
 #include "hw/i2c/pm_smbus.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_master.h"
 
 #define SMBHSTSTS       0x00
 #define SMBHSTCNT       0x02
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index f18aa3de35..d82423aa7e 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -25,7 +25,8 @@
 #include "qemu/osdep.h"
 #include "hw/hw.h"
 #include "hw/i2c/i2c.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_slave.h"
+#include "hw/i2c/smbus_eeprom.h"
 
 //#define DEBUG
 
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index 2a8b49e02f..e6d8d28194 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -29,8 +29,6 @@
 #include "hw/i2c/pm_smbus.h"
 #include "hw/pci/pci.h"
 #include "sysemu/sysemu.h"
-#include "hw/i2c/i2c.h"
-#include "hw/i2c/smbus.h"
 
 #include "hw/i386/ich9.h"
 
diff --git a/hw/i2c/smbus_master.c b/hw/i2c/smbus_master.c
new file mode 100644
index 0000000000..0a6223744c
--- /dev/null
+++ b/hw/i2c/smbus_master.c
@@ -0,0 +1,165 @@
+/*
+ * QEMU SMBus host (master) emulation.
+ *
+ * This code emulates SMBus transactions from the master point of view,
+ * it runs the individual I2C transaction to do the SMBus protocol
+ * over I2C.
+ *
+ * Copyright (c) 2007 CodeSourcery.
+ * Written by Paul Brook
+ *
+ * This code is licensed under the LGPL.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/hw.h"
+#include "hw/i2c/i2c.h"
+#include "hw/i2c/smbus_master.h"
+
+/* Master device commands.  */
+int smbus_quick_command(I2CBus *bus, uint8_t addr, int read)
+{
+    if (i2c_start_transfer(bus, addr, read)) {
+        return -1;
+    }
+    i2c_end_transfer(bus);
+    return 0;
+}
+
+int smbus_receive_byte(I2CBus *bus, uint8_t addr)
+{
+    uint8_t data;
+
+    if (i2c_start_transfer(bus, addr, 1)) {
+        return -1;
+    }
+    data = i2c_recv(bus);
+    i2c_nack(bus);
+    i2c_end_transfer(bus);
+    return data;
+}
+
+int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data)
+{
+    if (i2c_start_transfer(bus, addr, 0)) {
+        return -1;
+    }
+    i2c_send(bus, data);
+    i2c_end_transfer(bus);
+    return 0;
+}
+
+int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command)
+{
+    uint8_t data;
+    if (i2c_start_transfer(bus, addr, 0)) {
+        return -1;
+    }
+    i2c_send(bus, command);
+    if (i2c_start_transfer(bus, addr, 1)) {
+        i2c_end_transfer(bus);
+        return -1;
+    }
+    data = i2c_recv(bus);
+    i2c_nack(bus);
+    i2c_end_transfer(bus);
+    return data;
+}
+
+int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data)
+{
+    if (i2c_start_transfer(bus, addr, 0)) {
+        return -1;
+    }
+    i2c_send(bus, command);
+    i2c_send(bus, data);
+    i2c_end_transfer(bus);
+    return 0;
+}
+
+int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command)
+{
+    uint16_t data;
+    if (i2c_start_transfer(bus, addr, 0)) {
+        return -1;
+    }
+    i2c_send(bus, command);
+    if (i2c_start_transfer(bus, addr, 1)) {
+        i2c_end_transfer(bus);
+        return -1;
+    }
+    data = i2c_recv(bus);
+    data |= i2c_recv(bus) << 8;
+    i2c_nack(bus);
+    i2c_end_transfer(bus);
+    return data;
+}
+
+int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data)
+{
+    if (i2c_start_transfer(bus, addr, 0)) {
+        return -1;
+    }
+    i2c_send(bus, command);
+    i2c_send(bus, data & 0xff);
+    i2c_send(bus, data >> 8);
+    i2c_end_transfer(bus);
+    return 0;
+}
+
+int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
+                     int len, bool recv_len, bool send_cmd)
+{
+    int rlen;
+    int i;
+
+    if (send_cmd) {
+        if (i2c_start_transfer(bus, addr, 0)) {
+            return -1;
+        }
+        i2c_send(bus, command);
+    }
+    if (i2c_start_transfer(bus, addr, 1)) {
+        if (send_cmd) {
+            i2c_end_transfer(bus);
+        }
+        return -1;
+    }
+    if (recv_len) {
+        rlen = i2c_recv(bus);
+    } else {
+        rlen = len;
+    }
+    if (rlen > len) {
+        rlen = 0;
+    }
+    for (i = 0; i < rlen; i++) {
+        data[i] = i2c_recv(bus);
+    }
+    i2c_nack(bus);
+    i2c_end_transfer(bus);
+    return rlen;
+}
+
+int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
+                      int len, bool send_len)
+{
+    int i;
+
+    if (len > 32) {
+        len = 32;
+    }
+
+    if (i2c_start_transfer(bus, addr, 0)) {
+        return -1;
+    }
+    i2c_send(bus, command);
+    if (send_len) {
+        i2c_send(bus, len);
+    }
+    for (i = 0; i < len; i++) {
+        i2c_send(bus, data[i]);
+    }
+    i2c_end_transfer(bus);
+    return 0;
+}
diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus_slave.c
similarity index 64%
rename from hw/i2c/smbus.c
rename to hw/i2c/smbus_slave.c
index 6ff77c582f..1e734752d7 100644
--- a/hw/i2c/smbus.c
+++ b/hw/i2c/smbus_slave.c
@@ -1,6 +1,10 @@
 /*
  * QEMU SMBus device emulation.
  *
+ * This code is a helper for SMBus device emulation.  It implements an
+ * I2C device inteface and runs the SMBus protocol from the device
+ * point of view and maps those to simple calls to emulate.
+ *
  * Copyright (c) 2007 CodeSourcery.
  * Written by Paul Brook
  *
@@ -12,7 +16,7 @@
 #include "qemu/osdep.h"
 #include "hw/hw.h"
 #include "hw/i2c/i2c.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_slave.h"
 
 //#define DEBUG_SMBUS 1
 
@@ -202,153 +206,6 @@ static int smbus_i2c_send(I2CSlave *s, uint8_t data)
     return 0;
 }
 
-/* Master device commands.  */
-int smbus_quick_command(I2CBus *bus, uint8_t addr, int read)
-{
-    if (i2c_start_transfer(bus, addr, read)) {
-        return -1;
-    }
-    i2c_end_transfer(bus);
-    return 0;
-}
-
-int smbus_receive_byte(I2CBus *bus, uint8_t addr)
-{
-    uint8_t data;
-
-    if (i2c_start_transfer(bus, addr, 1)) {
-        return -1;
-    }
-    data = i2c_recv(bus);
-    i2c_nack(bus);
-    i2c_end_transfer(bus);
-    return data;
-}
-
-int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data)
-{
-    if (i2c_start_transfer(bus, addr, 0)) {
-        return -1;
-    }
-    i2c_send(bus, data);
-    i2c_end_transfer(bus);
-    return 0;
-}
-
-int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command)
-{
-    uint8_t data;
-    if (i2c_start_transfer(bus, addr, 0)) {
-        return -1;
-    }
-    i2c_send(bus, command);
-    if (i2c_start_transfer(bus, addr, 1)) {
-        i2c_end_transfer(bus);
-        return -1;
-    }
-    data = i2c_recv(bus);
-    i2c_nack(bus);
-    i2c_end_transfer(bus);
-    return data;
-}
-
-int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data)
-{
-    if (i2c_start_transfer(bus, addr, 0)) {
-        return -1;
-    }
-    i2c_send(bus, command);
-    i2c_send(bus, data);
-    i2c_end_transfer(bus);
-    return 0;
-}
-
-int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command)
-{
-    uint16_t data;
-    if (i2c_start_transfer(bus, addr, 0)) {
-        return -1;
-    }
-    i2c_send(bus, command);
-    if (i2c_start_transfer(bus, addr, 1)) {
-        i2c_end_transfer(bus);
-        return -1;
-    }
-    data = i2c_recv(bus);
-    data |= i2c_recv(bus) << 8;
-    i2c_nack(bus);
-    i2c_end_transfer(bus);
-    return data;
-}
-
-int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data)
-{
-    if (i2c_start_transfer(bus, addr, 0)) {
-        return -1;
-    }
-    i2c_send(bus, command);
-    i2c_send(bus, data & 0xff);
-    i2c_send(bus, data >> 8);
-    i2c_end_transfer(bus);
-    return 0;
-}
-
-int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
-                     int len, bool recv_len, bool send_cmd)
-{
-    int rlen;
-    int i;
-
-    if (send_cmd) {
-        if (i2c_start_transfer(bus, addr, 0)) {
-            return -1;
-        }
-        i2c_send(bus, command);
-    }
-    if (i2c_start_transfer(bus, addr, 1)) {
-        if (send_cmd) {
-            i2c_end_transfer(bus);
-        }
-        return -1;
-    }
-    if (recv_len) {
-        rlen = i2c_recv(bus);
-    } else {
-        rlen = len;
-    }
-    if (rlen > len) {
-        rlen = 0;
-    }
-    for (i = 0; i < rlen; i++) {
-        data[i] = i2c_recv(bus);
-    }
-    i2c_nack(bus);
-    i2c_end_transfer(bus);
-    return rlen;
-}
-
-int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
-                      int len, bool send_len)
-{
-    int i;
-
-    if (len > 32)
-        len = 32;
-
-    if (i2c_start_transfer(bus, addr, 0)) {
-        return -1;
-    }
-    i2c_send(bus, command);
-    if (send_len) {
-        i2c_send(bus, len);
-    }
-    for (i = 0; i < len; i++) {
-        i2c_send(bus, data[i]);
-    }
-    i2c_end_transfer(bus);
-    return 0;
-}
-
 static void smbus_device_class_init(ObjectClass *klass, void *data)
 {
     I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index dc09466b3e..cb28227cc3 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -42,7 +42,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/sysbus.h"
 #include "sysemu/arch_init.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_eeprom.h"
 #include "hw/xen/xen.h"
 #include "exec/memory.h"
 #include "exec/address-spaces.h"
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 532241e3f8..90e88c9b28 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -33,7 +33,7 @@
 #include "hw/hw.h"
 #include "hw/loader.h"
 #include "sysemu/arch_init.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_eeprom.h"
 #include "hw/boards.h"
 #include "hw/timer/mc146818rtc.h"
 #include "hw/xen/xen.h"
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 7302f6d74b..85d0532dd5 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -14,7 +14,6 @@
 #include "hw/hw.h"
 #include "hw/isa/vt82c686.h"
 #include "hw/i2c/i2c.h"
-#include "hw/i2c/smbus.h"
 #include "hw/pci/pci.h"
 #include "hw/isa/isa.h"
 #include "hw/isa/superio.h"
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 2fbba32c48..dae8acc108 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -27,7 +27,7 @@
 #include "hw/isa/superio.h"
 #include "net/net.h"
 #include "hw/boards.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_eeprom.h"
 #include "hw/block/flash.h"
 #include "hw/mips/mips.h"
 #include "hw/mips/cpudevs.h"
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index c1cf0fe12e..1fb7170f5e 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -33,7 +33,7 @@
 #include "hw/char/serial.h"
 #include "net/net.h"
 #include "hw/boards.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_eeprom.h"
 #include "hw/block/flash.h"
 #include "hw/mips/mips.h"
 #include "hw/mips/cpudevs.h"
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 5aac58f36e..7136b23f91 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -34,7 +34,7 @@
 #include "hw/sysbus.h"
 #include "hw/char/serial.h"
 #include "hw/i2c/ppc4xx_i2c.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_eeprom.h"
 #include "hw/usb/hcd-ehci.h"
 #include "hw/ppc/fdt.h"
 
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index 060d3c6ac0..6dd5b7040b 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -1,6 +1,8 @@
 #ifndef PM_SMBUS_H
 #define PM_SMBUS_H
 
+#include "hw/i2c/smbus_master.h"
+
 #define PM_SMBUS_MAX_MSG_SIZE 32
 
 typedef struct PMSMBus {
diff --git a/include/hw/i2c/smbus_eeprom.h b/include/hw/i2c/smbus_eeprom.h
new file mode 100644
index 0000000000..46fb1a37d6
--- /dev/null
+++ b/include/hw/i2c/smbus_eeprom.h
@@ -0,0 +1,32 @@
+/*
+ * QEMU SMBus EEPROM API
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef HW_SMBUS_EEPROM_H
+#define HW_SMBUS_EEPROM_H
+
+#include "hw/i2c/i2c.h"
+
+void smbus_eeprom_init_one(I2CBus *bus, uint8_t address, uint8_t *eeprom_buf);
+void smbus_eeprom_init(I2CBus *bus, int nb_eeprom,
+                       const uint8_t *eeprom_spd, int size);
+
+#endif
diff --git a/include/hw/i2c/smbus_master.h b/include/hw/i2c/smbus_master.h
new file mode 100644
index 0000000000..bb13bc423c
--- /dev/null
+++ b/include/hw/i2c/smbus_master.h
@@ -0,0 +1,55 @@
+/*
+ * QEMU SMBus host (master) API
+ *
+ * Copyright (c) 2007 Arastra, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef HW_SMBUS_MASTER_H
+#define HW_SMBUS_MASTER_H
+
+#include "hw/i2c/i2c.h"
+
+/* Master device commands.  */
+int smbus_quick_command(I2CBus *bus, uint8_t addr, int read);
+int smbus_receive_byte(I2CBus *bus, uint8_t addr);
+int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data);
+int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command);
+int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data);
+int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command);
+int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data);
+
+/*
+ * Do a block transfer from an I2C device.  If recv_len is set, then the
+ * first received byte is a length field and is used to know how much data
+ * to receive.  Otherwise receive "len" bytes.  If send_cmd is set, send
+ * the command byte first before receiving the data.
+ */
+int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
+                     int len, bool recv_len, bool send_cmd);
+
+/*
+ * Do a block transfer to an I2C device.  If send_len is set, send the
+ * "len" value before the data.
+ */
+int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
+                      int len, bool send_len);
+
+#endif
diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus_slave.h
similarity index 65%
rename from include/hw/i2c/smbus.h
rename to include/hw/i2c/smbus_slave.h
index d8b1b9ee81..ff07ee005d 100644
--- a/include/hw/i2c/smbus.h
+++ b/include/hw/i2c/smbus_slave.h
@@ -1,8 +1,5 @@
-#ifndef QEMU_SMBUS_H
-#define QEMU_SMBUS_H
-
 /*
- * QEMU SMBus API
+ * QEMU SMBus device (slave) API
  *
  * Copyright (c) 2007 Arastra, Inc.
  *
@@ -25,6 +22,9 @@
  * THE SOFTWARE.
  */
 
+#ifndef HW_SMBUS_SLAVE_H
+#define HW_SMBUS_SLAVE_H
+
 #include "hw/i2c/i2c.h"
 
 #define TYPE_SMBUS_DEVICE "smbus-device"
@@ -64,33 +64,4 @@ struct SMBusDevice {
     uint8_t command;
 };
 
-/* Master device commands.  */
-int smbus_quick_command(I2CBus *bus, uint8_t addr, int read);
-int smbus_receive_byte(I2CBus *bus, uint8_t addr);
-int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data);
-int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command);
-int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data);
-int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command);
-int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data);
-
-/*
- * Do a block transfer from an I2C device.  If recv_len is set, then the
- * first received byte is a length field and is used to know how much data
- * to receive.  Otherwise receive "len" bytes.  If send_cmd is set, send
- * the command byte first before receiving the data.
- */
-int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
-                     int len, bool recv_len, bool send_cmd);
-
-/*
- * Do a block transfer to an I2C device.  If send_len is set, send the
- * "len" value before the data.
- */
-int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
-                      int len, bool send_len);
-
-void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf);
-void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
-                       const uint8_t *eeprom_spd, int size);
-
 #endif
-- 
2.17.1

[Qemu-devel] [PATCH v3 02/16] i2c: have I2C receive operation return uint8_t

[minyard]

From: Corey Minyard <cminyard@mvista.com>

It is never supposed to fail and cannot return an error, so just
have it return the proper type.  Have it return 0xff on nothing
available, since that's what would happen on a real bus.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/pxa2xx.c         |  2 +-
 hw/arm/tosa.c           |  4 ++--
 hw/arm/z2.c             |  2 +-
 hw/audio/wm8750.c       |  2 +-
 hw/display/sii9022.c    |  2 +-
 hw/display/ssd0303.c    |  4 ++--
 hw/gpio/max7310.c       |  2 +-
 hw/i2c/core.c           | 32 +++++++++++++-------------------
 hw/i2c/i2c-ddc.c        |  2 +-
 hw/i2c/smbus_slave.c    |  4 ++--
 hw/input/lm832x.c       |  2 +-
 hw/misc/pca9552.c       |  2 +-
 hw/misc/tmp105.c        |  2 +-
 hw/misc/tmp421.c        |  2 +-
 hw/nvram/eeprom_at24c.c |  4 ++--
 hw/timer/ds1338.c       |  2 +-
 hw/timer/m41t80.c       |  2 +-
 hw/timer/twl92230.c     |  2 +-
 include/hw/i2c/i2c.h    |  7 +++----
 19 files changed, 37 insertions(+), 44 deletions(-)

diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index f598a1c053..3d7c88910e 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1286,7 +1286,7 @@ static int pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
     return 0;
 }
 
-static int pxa2xx_i2c_rx(I2CSlave *i2c)
+static uint8_t pxa2xx_i2c_rx(I2CSlave *i2c)
 {
     PXA2xxI2CSlaveState *slave = PXA2XX_I2C_SLAVE(i2c);
     PXA2xxI2CState *s = slave->host;
diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index 7a925fa5e6..eef9d427e7 100644
--- a/hw/arm/tosa.c
+++ b/hw/arm/tosa.c
@@ -197,10 +197,10 @@ static int tosa_dac_event(I2CSlave *i2c, enum i2c_event event)
     return 0;
 }
 
-static int tosa_dac_recv(I2CSlave *s)
+static uint8_t tosa_dac_recv(I2CSlave *s)
 {
     printf("%s: recv not supported!!!\n", __func__);
-    return -1;
+    return 0xff;
 }
 
 static void tosa_tg_init(PXA2xxState *cpu)
diff --git a/hw/arm/z2.c b/hw/arm/z2.c
index 697a822f1e..6f18d924df 100644
--- a/hw/arm/z2.c
+++ b/hw/arm/z2.c
@@ -243,7 +243,7 @@ static int aer915_event(I2CSlave *i2c, enum i2c_event event)
     return 0;
 }
 
-static int aer915_recv(I2CSlave *slave)
+static uint8_t aer915_recv(I2CSlave *slave)
 {
     AER915State *s = AER915(slave);
     int retval = 0x00;
diff --git a/hw/audio/wm8750.c b/hw/audio/wm8750.c
index f4aa838f62..169b006ade 100644
--- a/hw/audio/wm8750.c
+++ b/hw/audio/wm8750.c
@@ -561,7 +561,7 @@ static int wm8750_tx(I2CSlave *i2c, uint8_t data)
     return 0;
 }
 
-static int wm8750_rx(I2CSlave *i2c)
+static uint8_t wm8750_rx(I2CSlave *i2c)
 {
     return 0x00;
 }
diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
index eaf11a6e7b..9994385c35 100644
--- a/hw/display/sii9022.c
+++ b/hw/display/sii9022.c
@@ -79,7 +79,7 @@ static int sii9022_event(I2CSlave *i2c, enum i2c_event event)
     return 0;
 }
 
-static int sii9022_rx(I2CSlave *i2c)
+static uint8_t sii9022_rx(I2CSlave *i2c)
 {
     sii9022_state *s = SII9022(i2c);
     uint8_t res = 0x00;
diff --git a/hw/display/ssd0303.c b/hw/display/ssd0303.c
index eb90ba26be..8edf34986c 100644
--- a/hw/display/ssd0303.c
+++ b/hw/display/ssd0303.c
@@ -62,10 +62,10 @@ typedef struct {
     uint8_t framebuffer[132*8];
 } ssd0303_state;
 
-static int ssd0303_recv(I2CSlave *i2c)
+static uint8_t ssd0303_recv(I2CSlave *i2c)
 {
     BADF("Reads not implemented\n");
-    return -1;
+    return 0xff;
 }
 
 static int ssd0303_send(I2CSlave *i2c, uint8_t data)
diff --git a/hw/gpio/max7310.c b/hw/gpio/max7310.c
index a560e3afd2..f35a930276 100644
--- a/hw/gpio/max7310.c
+++ b/hw/gpio/max7310.c
@@ -39,7 +39,7 @@ static void max7310_reset(DeviceState *dev)
     s->command = 0x00;
 }
 
-static int max7310_rx(I2CSlave *i2c)
+static uint8_t max7310_rx(I2CSlave *i2c)
 {
     MAX7310State *s = MAX7310(i2c);
 
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index b54725985a..15237ad073 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -191,23 +191,17 @@ int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send)
         }
         return ret ? -1 : 0;
     } else {
-        if ((QLIST_EMPTY(&bus->current_devs)) || (bus->broadcast)) {
-            return -1;
-        }
-
-        sc = I2C_SLAVE_GET_CLASS(QLIST_FIRST(&bus->current_devs)->elt);
-        if (sc->recv) {
-            s = QLIST_FIRST(&bus->current_devs)->elt;
-            ret = sc->recv(s);
-            trace_i2c_recv(s->address, ret);
-            if (ret < 0) {
-                return ret;
-            } else {
-                *data = ret;
-                return 0;
+        ret = 0xff;
+        if (!QLIST_EMPTY(&bus->current_devs) && !bus->broadcast) {
+            sc = I2C_SLAVE_GET_CLASS(QLIST_FIRST(&bus->current_devs)->elt);
+            if (sc->recv) {
+                s = QLIST_FIRST(&bus->current_devs)->elt;
+                ret = sc->recv(s);
+                trace_i2c_recv(s->address, ret);
             }
         }
-        return -1;
+        *data = ret;
+        return 0;
     }
 }
 
@@ -216,12 +210,12 @@ int i2c_send(I2CBus *bus, uint8_t data)
     return i2c_send_recv(bus, &data, true);
 }
 
-int i2c_recv(I2CBus *bus)
+uint8_t i2c_recv(I2CBus *bus)
 {
-    uint8_t data;
-    int ret = i2c_send_recv(bus, &data, false);
+    uint8_t data = 0xff;
 
-    return ret < 0 ? ret : data;
+    i2c_send_recv(bus, &data, false);
+    return data;
 }
 
 void i2c_nack(I2CBus *bus)
diff --git a/hw/i2c/i2c-ddc.c b/hw/i2c/i2c-ddc.c
index be34fe072c..95325358db 100644
--- a/hw/i2c/i2c-ddc.c
+++ b/hw/i2c/i2c-ddc.c
@@ -51,7 +51,7 @@ static int i2c_ddc_event(I2CSlave *i2c, enum i2c_event event)
     return 0;
 }
 
-static int i2c_ddc_rx(I2CSlave *i2c)
+static uint8_t i2c_ddc_rx(I2CSlave *i2c)
 {
     I2CDDCState *s = I2CDDC(i2c);
 
diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
index 1e734752d7..549e7ae933 100644
--- a/hw/i2c/smbus_slave.c
+++ b/hw/i2c/smbus_slave.c
@@ -156,11 +156,11 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
     return 0;
 }
 
-static int smbus_i2c_recv(I2CSlave *s)
+static uint8_t smbus_i2c_recv(I2CSlave *s)
 {
     SMBusDevice *dev = SMBUS_DEVICE(s);
     SMBusDeviceClass *sc = SMBUS_DEVICE_GET_CLASS(dev);
-    int ret;
+    uint8_t ret;
 
     switch (dev->mode) {
     case SMBUS_RECV_BYTE:
diff --git a/hw/input/lm832x.c b/hw/input/lm832x.c
index 74da30d9ca..9ae037953d 100644
--- a/hw/input/lm832x.c
+++ b/hw/input/lm832x.c
@@ -401,7 +401,7 @@ static int lm_i2c_event(I2CSlave *i2c, enum i2c_event event)
     return 0;
 }
 
-static int lm_i2c_rx(I2CSlave *i2c)
+static uint8_t lm_i2c_rx(I2CSlave *i2c)
 {
     LM823KbdState *s = LM8323(i2c);
 
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 9775d5274a..7325d3f287 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -115,7 +115,7 @@ static void pca9552_autoinc(PCA9552State *s)
     }
 }
 
-static int pca9552_recv(I2CSlave *i2c)
+static uint8_t pca9552_recv(I2CSlave *i2c)
 {
     PCA9552State *s = PCA9552(i2c);
     uint8_t ret;
diff --git a/hw/misc/tmp105.c b/hw/misc/tmp105.c
index 0918f3a6ea..a4cae665b7 100644
--- a/hw/misc/tmp105.c
+++ b/hw/misc/tmp105.c
@@ -147,7 +147,7 @@ static void tmp105_write(TMP105State *s)
     }
 }
 
-static int tmp105_rx(I2CSlave *i2c)
+static uint8_t tmp105_rx(I2CSlave *i2c)
 {
     TMP105State *s = TMP105(i2c);
 
diff --git a/hw/misc/tmp421.c b/hw/misc/tmp421.c
index c234044305..a75eb994a8 100644
--- a/hw/misc/tmp421.c
+++ b/hw/misc/tmp421.c
@@ -249,7 +249,7 @@ static void tmp421_write(TMP421State *s)
     }
 }
 
-static int tmp421_rx(I2CSlave *i2c)
+static uint8_t tmp421_rx(I2CSlave *i2c)
 {
     TMP421State *s = TMP421(i2c);
 
diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 27cd01e615..d1456dafbd 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -74,10 +74,10 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
 }
 
 static
-int at24c_eeprom_recv(I2CSlave *s)
+uint8_t at24c_eeprom_recv(I2CSlave *s)
 {
     EEPROMState *ee = AT24C_EE(s);
-    int ret;
+    uint8_t ret;
 
     ret = ee->mem[ee->cur];
 
diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c
index 3849b74a68..03da75486b 100644
--- a/hw/timer/ds1338.c
+++ b/hw/timer/ds1338.c
@@ -117,7 +117,7 @@ static int ds1338_event(I2CSlave *i2c, enum i2c_event event)
     return 0;
 }
 
-static int ds1338_recv(I2CSlave *i2c)
+static uint8_t ds1338_recv(I2CSlave *i2c)
 {
     DS1338State *s = DS1338(i2c);
     uint8_t res;
diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
index 734d7d95fc..c45b9297d8 100644
--- a/hw/timer/m41t80.c
+++ b/hw/timer/m41t80.c
@@ -40,7 +40,7 @@ static int m41t80_send(I2CSlave *i2c, uint8_t data)
     return 0;
 }
 
-static int m41t80_recv(I2CSlave *i2c)
+static uint8_t m41t80_recv(I2CSlave *i2c)
 {
     M41t80State *s = M41T80(i2c);
     struct tm now;
diff --git a/hw/timer/twl92230.c b/hw/timer/twl92230.c
index 3b43b46199..659b216dca 100644
--- a/hw/timer/twl92230.c
+++ b/hw/timer/twl92230.c
@@ -737,7 +737,7 @@ static int menelaus_tx(I2CSlave *i2c, uint8_t data)
     return 0;
 }
 
-static int menelaus_rx(I2CSlave *i2c)
+static uint8_t menelaus_rx(I2CSlave *i2c)
 {
     MenelausState *s = TWL92230(i2c);
 
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 5dc166158b..75c5bd638b 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -33,10 +33,9 @@ typedef struct I2CSlaveClass {
 
     /*
      * Slave to master.  This cannot fail, the device should always
-     * return something here.  Negative values probably result in 0xff
-     * and a possible log from the driver, and shouldn't be used.
+     * return something here.
      */
-    int (*recv)(I2CSlave *s);
+    uint8_t (*recv)(I2CSlave *s);
 
     /*
      * Notify the slave of a bus state change.  For start event,
@@ -78,7 +77,7 @@ void i2c_end_transfer(I2CBus *bus);
 void i2c_nack(I2CBus *bus);
 int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send);
 int i2c_send(I2CBus *bus, uint8_t data);
-int i2c_recv(I2CBus *bus);
+uint8_t i2c_recv(I2CBus *bus);
 
 DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr);
 
-- 
2.17.1

[Qemu-devel] [PATCH v3 03/16] arm:i2c: Don't mask return from i2c_recv()

[minyard]

From: Corey Minyard <cminyard@mvista.com>

It can't fail, and now that it returns a uint8_t a 0xff mask
is unnecessary.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/stellaris.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 6c69ce79b2..638b649911 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -811,7 +811,7 @@ static void stellaris_i2c_write(void *opaque, hwaddr offset,
             /* TODO: Handle errors.  */
             if (s->msa & 1) {
                 /* Recv */
-                s->mdr = i2c_recv(s->bus) & 0xff;
+                s->mdr = i2c_recv(s->bus);
             } else {
                 /* Send */
                 i2c_send(s->bus, s->mdr);
-- 
2.17.1

[Qemu-devel] [PATCH v3 04/16] i2c: Don't check return value from i2c_recv()

[minyard]

From: Corey Minyard <cminyard@mvista.com>

i2c_recv() cannot fail, so there is no need to check the return
value.  It also returns unt8_t, so comparing with < 0 is not
meaningful.

Fix up various I2C controllers to remove the unneeded code.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/i2c/aspeed_i2c.c     |  9 ++-------
 hw/i2c/exynos4210_i2c.c |  4 ++--
 hw/i2c/imx_i2c.c        | 12 ++----------
 3 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index a2dfa82760..a085510cfd 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -189,16 +189,11 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
 
 static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
 {
-    int ret;
+    uint8_t ret;
 
     aspeed_i2c_set_state(bus, I2CD_MRXD);
     ret = i2c_recv(bus->bus);
-    if (ret < 0) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
-        ret = 0xff;
-    } else {
-        bus->intr_status |= I2CD_INTR_RX_DONE;
-    }
+    bus->intr_status |= I2CD_INTR_RX_DONE;
     bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
     if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
         i2c_nack(bus->bus);
diff --git a/hw/i2c/exynos4210_i2c.c b/hw/i2c/exynos4210_i2c.c
index c96fa7d7be..43f284eab7 100644
--- a/hw/i2c/exynos4210_i2c.c
+++ b/hw/i2c/exynos4210_i2c.c
@@ -106,12 +106,12 @@ static inline void exynos4210_i2c_raise_interrupt(Exynos4210I2CState *s)
 static void exynos4210_i2c_data_receive(void *opaque)
 {
     Exynos4210I2CState *s = (Exynos4210I2CState *)opaque;
-    int ret;
+    uint8_t ret;
 
     s->i2cstat &= ~I2CSTAT_LAST_BIT;
     s->scl_free = false;
     ret = i2c_recv(s->bus);
-    if (ret < 0 && (s->i2ccon & I2CCON_ACK_GEN)) {
+    if (s->i2ccon & I2CCON_ACK_GEN) {
         s->i2cstat |= I2CSTAT_LAST_BIT;  /* Data is not acknowledged */
     } else {
         s->i2cds = ret;
diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
index 6c81b98ebd..6da5224e2e 100644
--- a/hw/i2c/imx_i2c.c
+++ b/hw/i2c/imx_i2c.c
@@ -120,7 +120,7 @@ static uint64_t imx_i2c_read(void *opaque, hwaddr offset,
         value = s->i2dr_read;
 
         if (imx_i2c_is_master(s)) {
-            int ret = 0xff;
+            uint8_t ret = 0xff;
 
             if (s->address == ADDR_RESET) {
                 /* something is wrong as the address is not set */
@@ -133,15 +133,7 @@ static uint64_t imx_i2c_read(void *opaque, hwaddr offset,
             } else {
                 /* get the next byte */
                 ret = i2c_recv(s->bus);
-
-                if (ret >= 0) {
-                    imx_i2c_raise_interrupt(s);
-                } else {
-                    qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: read failed "
-                                  "for device 0x%02x\n", TYPE_IMX_I2C,
-                                  __func__, s->address);
-                    ret = 0xff;
-                }
+                imx_i2c_raise_interrupt(s);
             }
 
             s->i2dr_read = ret;
-- 
2.17.1

[Qemu-devel] [PATCH v3 05/16] i2c: Simplify and correct the SMBus state machine

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The SMBus slave code had an unneeded state, unnecessary function
pointers and incorrectly handled quick commands.  Rewrite it
to simplify the code and make it work correctly.

smbus_eeprom is the only user, so no other effects and the eeprom
code also gets a significant simplification.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/i2c/smbus_eeprom.c        | 58 ++++++-----------------
 hw/i2c/smbus_slave.c         | 91 ++++++++++++++++--------------------
 include/hw/i2c/smbus_slave.h | 45 +++++++++++++-----
 3 files changed, 86 insertions(+), 108 deletions(-)

diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index d82423aa7e..4d25222e23 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -36,28 +36,12 @@ typedef struct SMBusEEPROMDevice {
     uint8_t offset;
 } SMBusEEPROMDevice;
 
-static void eeprom_quick_cmd(SMBusDevice *dev, uint8_t read)
-{
-#ifdef DEBUG
-    printf("eeprom_quick_cmd: addr=0x%02x read=%d\n", dev->i2c.address, read);
-#endif
-}
-
-static void eeprom_send_byte(SMBusDevice *dev, uint8_t val)
-{
-    SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
-#ifdef DEBUG
-    printf("eeprom_send_byte: addr=0x%02x val=0x%02x\n",
-           dev->i2c.address, val);
-#endif
-    eeprom->offset = val;
-}
-
 static uint8_t eeprom_receive_byte(SMBusDevice *dev)
 {
     SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
     uint8_t *data = eeprom->data;
     uint8_t val = data[eeprom->offset++];
+
 #ifdef DEBUG
     printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n",
            dev->i2c.address, val);
@@ -65,37 +49,26 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev)
     return val;
 }
 
-static void eeprom_write_data(SMBusDevice *dev, uint8_t cmd, uint8_t *buf, int len)
+static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
 {
     SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
-    int n;
+    uint8_t *data = eeprom->data;
+
 #ifdef DEBUG
     printf("eeprom_write_byte: addr=0x%02x cmd=0x%02x val=0x%02x\n",
            dev->i2c.address, cmd, buf[0]);
 #endif
-    /* A page write operation is not a valid SMBus command.
-       It is a block write without a length byte.  Fortunately we
-       get the full block anyway.  */
-    /* TODO: Should this set the current location?  */
-    if (cmd + len > 256)
-        n = 256 - cmd;
-    else
-        n = len;
-    memcpy(eeprom->data + cmd, buf, n);
-    len -= n;
-    if (len)
-        memcpy(eeprom->data, buf + n, len);
-}
+    /* len is guaranteed to be > 0 */
+    eeprom->offset = buf[0];
+    buf++;
+    len--;
+
+    for (; len > 0; len--) {
+        data[eeprom->offset] = *buf++;
+        eeprom->offset = (eeprom->offset + 1) % 256;
+    }
 
-static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t cmd, int n)
-{
-    SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
-    /* If this is the first byte then set the current position.  */
-    if (n == 0)
-        eeprom->offset = cmd;
-    /* As with writes, we implement block reads without the
-       SMBus length byte.  */
-    return eeprom_receive_byte(dev);
+    return 0;
 }
 
 static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
@@ -116,11 +89,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
     SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
 
     dc->realize = smbus_eeprom_realize;
-    sc->quick_cmd = eeprom_quick_cmd;
-    sc->send_byte = eeprom_send_byte;
     sc->receive_byte = eeprom_receive_byte;
     sc->write_data = eeprom_write_data;
-    sc->read_data = eeprom_read_data;
     dc->props = smbus_eeprom_properties;
     /* Reason: pointer property "data" */
     dc->user_creatable = false;
diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
index 549e7ae933..70ff29c095 100644
--- a/hw/i2c/smbus_slave.c
+++ b/hw/i2c/smbus_slave.c
@@ -34,7 +34,6 @@ do { fprintf(stderr, "smbus: error: " fmt , ## __VA_ARGS__);} while (0)
 enum {
     SMBUS_IDLE,
     SMBUS_WRITE_DATA,
-    SMBUS_RECV_BYTE,
     SMBUS_READ_DATA,
     SMBUS_DONE,
     SMBUS_CONFUSED = -1
@@ -54,20 +53,9 @@ static void smbus_do_write(SMBusDevice *dev)
 {
     SMBusDeviceClass *sc = SMBUS_DEVICE_GET_CLASS(dev);
 
-    if (dev->data_len == 0) {
-        smbus_do_quick_cmd(dev, 0);
-    } else if (dev->data_len == 1) {
-        DPRINTF("Send Byte\n");
-        if (sc->send_byte) {
-            sc->send_byte(dev, dev->data_buf[0]);
-        }
-    } else {
-        dev->command = dev->data_buf[0];
-        DPRINTF("Command %d len %d\n", dev->command, dev->data_len - 1);
-        if (sc->write_data) {
-            sc->write_data(dev, dev->command, dev->data_buf + 1,
-                           dev->data_len - 1);
-        }
+    DPRINTF("Command %d len %d\n", dev->data_buf[0], dev->data_len);
+    if (sc->write_data) {
+        sc->write_data(dev, dev->data_buf, dev->data_len);
     }
 }
 
@@ -82,6 +70,7 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
             DPRINTF("Incoming data\n");
             dev->mode = SMBUS_WRITE_DATA;
             break;
+
         default:
             BADF("Unexpected send start condition in state %d\n", dev->mode);
             dev->mode = SMBUS_CONFUSED;
@@ -93,25 +82,20 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
         switch (dev->mode) {
         case SMBUS_IDLE:
             DPRINTF("Read mode\n");
-            dev->mode = SMBUS_RECV_BYTE;
+            dev->mode = SMBUS_READ_DATA;
             break;
+
         case SMBUS_WRITE_DATA:
             if (dev->data_len == 0) {
                 BADF("Read after write with no data\n");
                 dev->mode = SMBUS_CONFUSED;
             } else {
-                if (dev->data_len > 1) {
-                    smbus_do_write(dev);
-                } else {
-                    dev->command = dev->data_buf[0];
-                    DPRINTF("%02x: Command %d\n", dev->i2c.address,
-                            dev->command);
-                }
+                smbus_do_write(dev);
                 DPRINTF("Read mode\n");
-                dev->data_len = 0;
                 dev->mode = SMBUS_READ_DATA;
             }
             break;
+
         default:
             BADF("Unexpected recv start condition in state %d\n", dev->mode);
             dev->mode = SMBUS_CONFUSED;
@@ -120,19 +104,29 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
         break;
 
     case I2C_FINISH:
-        switch (dev->mode) {
-        case SMBUS_WRITE_DATA:
-            smbus_do_write(dev);
-            break;
-        case SMBUS_RECV_BYTE:
-            smbus_do_quick_cmd(dev, 1);
-            break;
-        case SMBUS_READ_DATA:
-            BADF("Unexpected stop during receive\n");
-            break;
-        default:
-            /* Nothing to do.  */
-            break;
+        if (dev->data_len == 0) {
+            if (dev->mode == SMBUS_WRITE_DATA || dev->mode == SMBUS_READ_DATA) {
+                smbus_do_quick_cmd(dev, dev->mode == SMBUS_READ_DATA);
+            }
+        } else {
+            SMBusDeviceClass *sc = SMBUS_DEVICE_GET_CLASS(dev);
+
+            switch (dev->mode) {
+            case SMBUS_WRITE_DATA:
+                smbus_do_write(dev);
+                break;
+
+            case SMBUS_READ_DATA:
+                BADF("Unexpected stop during receive\n");
+                break;
+
+            default:
+                /* Nothing to do.  */
+                break;
+            }
+            if (sc->transaction_complete) {
+                sc->transaction_complete(dev);
+            }
         }
         dev->mode = SMBUS_IDLE;
         dev->data_len = 0;
@@ -143,9 +137,11 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
         case SMBUS_DONE:
             /* Nothing to do.  */
             break;
+
         case SMBUS_READ_DATA:
             dev->mode = SMBUS_DONE;
             break;
+
         default:
             BADF("Unexpected NACK in state %d\n", dev->mode);
             dev->mode = SMBUS_CONFUSED;
@@ -160,33 +156,22 @@ static uint8_t smbus_i2c_recv(I2CSlave *s)
 {
     SMBusDevice *dev = SMBUS_DEVICE(s);
     SMBusDeviceClass *sc = SMBUS_DEVICE_GET_CLASS(dev);
-    uint8_t ret;
+    uint8_t ret = 0xff;
 
     switch (dev->mode) {
-    case SMBUS_RECV_BYTE:
+    case SMBUS_READ_DATA:
         if (sc->receive_byte) {
             ret = sc->receive_byte(dev);
-        } else {
-            ret = 0;
-        }
-        DPRINTF("Receive Byte %02x\n", ret);
-        dev->mode = SMBUS_DONE;
-        break;
-    case SMBUS_READ_DATA:
-        if (sc->read_data) {
-            ret = sc->read_data(dev, dev->command, dev->data_len);
-            dev->data_len++;
-        } else {
-            ret = 0;
         }
         DPRINTF("Read data %02x\n", ret);
         break;
+
     default:
         BADF("Unexpected read in state %d\n", dev->mode);
         dev->mode = SMBUS_CONFUSED;
-        ret = 0;
         break;
     }
+
     return ret;
 }
 
@@ -199,10 +184,12 @@ static int smbus_i2c_send(I2CSlave *s, uint8_t data)
         DPRINTF("Write data %02x\n", data);
         dev->data_buf[dev->data_len++] = data;
         break;
+
     default:
         BADF("Unexpected write in state %d\n", dev->mode);
         break;
     }
+
     return 0;
 }
 
diff --git a/include/hw/i2c/smbus_slave.h b/include/hw/i2c/smbus_slave.h
index ff07ee005d..fad2db9c76 100644
--- a/include/hw/i2c/smbus_slave.h
+++ b/include/hw/i2c/smbus_slave.h
@@ -38,19 +38,41 @@
 typedef struct SMBusDeviceClass
 {
     I2CSlaveClass parent_class;
+
+    /*
+     * An operation with no data, special in SMBus.
+     * This may be NULL, quick commands are ignore in that case.
+     */
     void (*quick_cmd)(SMBusDevice *dev, uint8_t read);
-    void (*send_byte)(SMBusDevice *dev, uint8_t val);
+
+    /*
+     * We can't distinguish between a word write and a block write with
+     * length 1, so pass the whole data block including the length byte
+     * (if present).  The device is responsible figuring out what type of
+     * command this is.
+     * This may be NULL if no data is written to the device.  Writes
+     * will be ignore in that case.
+     */
+    int (*write_data)(SMBusDevice *dev, uint8_t *buf, uint8_t len);
+
+    /*
+     * Likewise we can't distinguish between different reads, or even know
+     * the length of the read until the read is complete, so read data a
+     * byte at a time.  The device is responsible for adding the length
+     * byte on block reads.  This call cannot fail, it should return
+     * something, preferably 0xff if nothing is available.
+     * This may be NULL if no data is read from the device.  Reads will
+     * return 0xff in that case.
+     */
     uint8_t (*receive_byte)(SMBusDevice *dev);
-    /* We can't distinguish between a word write and a block write with
-       length 1, so pass the whole data block including the length byte
-       (if present).  The device is responsible figuring out what type of
-       command  this is.  */
-    void (*write_data)(SMBusDevice *dev, uint8_t cmd, uint8_t *buf, int len);
-    /* Likewise we can't distinguish between different reads, or even know
-       the length of the read until the read is complete, so read data a
-       byte at a time.  The device is responsible for adding the length
-       byte on block reads.  */
-    uint8_t (*read_data)(SMBusDevice *dev, uint8_t cmd, int n);
+
+    /*
+     * Called whan an SMBus transaction has completed.  This can be used
+     * so the device knows when an operation completes.  This is not
+     * called after quick commands, those are complete by nature.
+     * This may be NULL if the device doesn't need this.
+     */
+    void (*transaction_complete)(SMBusDevice *dev);
 } SMBusDeviceClass;
 
 struct SMBusDevice {
@@ -61,7 +83,6 @@ struct SMBusDevice {
     int mode;
     int data_len;
     uint8_t data_buf[34]; /* command + len + 32 bytes of data.  */
-    uint8_t command;
 };
 
 #endif
-- 
2.17.1

[Qemu-devel] [PATCH v3 06/16] i2c: Add a length check to the SMBus write handling

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Avoid an overflow.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/i2c/smbus_slave.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
index 70ff29c095..d03f714608 100644
--- a/hw/i2c/smbus_slave.c
+++ b/hw/i2c/smbus_slave.c
@@ -182,7 +182,11 @@ static int smbus_i2c_send(I2CSlave *s, uint8_t data)
     switch (dev->mode) {
     case SMBUS_WRITE_DATA:
         DPRINTF("Write data %02x\n", data);
-        dev->data_buf[dev->data_len++] = data;
+        if (dev->data_len >= sizeof(dev->data_buf)) {
+            BADF("Too many bytes sent\n");
+        } else {
+            dev->data_buf[dev->data_len++] = data;
+        }
         break;
 
     default:
-- 
2.17.1

[Qemu-devel] [PATCH v3 07/16] i2c:pm_smbus: Fix pm_smbus handling of I2C block read

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The I2C block read function of pm_smbus was completely broken.  It
required doing some direct I2C handling because it didn't have a
defined size, the OS code just reads bytes until it marks the
transaction finished.

This also required adjusting how the AMIBIOS workaround code worked,
the I2C block mode was setting STS_HOST_BUSY during a transaction,
so that bit could no longer be used to inform the host status read
code to start the transaction.  Create a explicit bool for that
operation.

Also, don't read the next byte from the device in byte-by-byte
mode unless the OS is actually clearing the byte done bit.  Just
assuming that's what the OS is doing is a bad idea.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/i2c/pm_smbus.c         | 86 ++++++++++++++++++++++++++++++---------
 include/hw/i2c/pm_smbus.h |  6 +++
 2 files changed, 73 insertions(+), 19 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index f3c6cc46f9..8793113c25 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -118,19 +118,30 @@ static void smb_transaction(PMSMBus *s)
         }
         break;
     case PROT_I2C_BLOCK_READ:
-        if (read) {
-            int xfersize = s->smb_data0;
-            if (xfersize > sizeof(s->smb_data)) {
-                xfersize = sizeof(s->smb_data);
-            }
-            ret = smbus_read_block(bus, addr, s->smb_data1, s->smb_data,
-                                   xfersize, false, true);
-            goto data8;
-        } else {
-            /* The manual says the behavior is undefined, just set DEV_ERR. */
+        /* According to the Linux i2c-i801 driver:
+         *   NB: page 240 of ICH5 datasheet shows that the R/#W
+         *   bit should be cleared here, even when reading.
+         *   However if SPD Write Disable is set (Lynx Point and later),
+         *   the read will fail if we don't set the R/#W bit.
+         * So at least Linux may or may not set the read bit here.
+         * So just ignore the read bit for this command.
+         */
+        if (i2c_start_transfer(bus, addr, 0)) {
             goto error;
         }
-        break;
+        ret = i2c_send(bus, s->smb_data1);
+        if (ret) {
+            goto error;
+        }
+        if (i2c_start_transfer(bus, addr, 1)) {
+            goto error;
+        }
+        s->in_i2c_block_read = true;
+        s->smb_blkdata = i2c_recv(s->smbus);
+        s->op_done = false;
+        s->smb_stat |= STS_HOST_BUSY | STS_BYTE_DONE;
+        goto out;
+
     case PROT_BLOCK_DATA:
         if (read) {
             ret = smbus_read_block(bus, addr, cmd, s->smb_data,
@@ -208,6 +219,7 @@ static void smb_transaction_start(PMSMBus *s)
 {
     if (s->smb_ctl & CTL_INTREN) {
         smb_transaction(s);
+        s->start_transaction_on_status_read = false;
     } else {
         /* Do not execute immediately the command; it will be
          * executed when guest will read SMB_STAT register.  This
@@ -217,6 +229,7 @@ static void smb_transaction_start(PMSMBus *s)
          * checking for status.  If STS_HOST_BUSY doesn't get
          * set, it gets stuck. */
         s->smb_stat |= STS_HOST_BUSY;
+        s->start_transaction_on_status_read = true;
     }
 }
 
@@ -226,19 +239,38 @@ smb_irq_value(PMSMBus *s)
     return ((s->smb_stat & ~STS_HOST_BUSY) != 0) && (s->smb_ctl & CTL_INTREN);
 }
 
+static bool
+smb_byte_by_byte(PMSMBus *s)
+{
+    if (s->op_done) {
+        return false;
+    }
+    if (s->in_i2c_block_read) {
+        return true;
+    }
+    return !(s->smb_auxctl & AUX_BLK);
+}
+
 static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
                               unsigned width)
 {
     PMSMBus *s = opaque;
+    uint8_t clear_byte_done;
 
     SMBUS_DPRINTF("SMB writeb port=0x%04" HWADDR_PRIx
                   " val=0x%02" PRIx64 "\n", addr, val);
     switch(addr) {
     case SMBHSTSTS:
+        clear_byte_done = s->smb_stat & val & STS_BYTE_DONE;
         s->smb_stat &= ~(val & ~STS_HOST_BUSY);
-        if (!s->op_done && !(s->smb_auxctl & AUX_BLK)) {
+        if (clear_byte_done && smb_byte_by_byte(s)) {
             uint8_t read = s->smb_addr & 0x01;
 
+            if (s->in_i2c_block_read) {
+                /* See comment below PROT_I2C_BLOCK_READ above. */
+                read = 1;
+            }
+
             s->smb_index++;
             if (!read && s->smb_index == s->smb_data0) {
                 uint8_t prot = (s->smb_ctl >> 2) & 0x07;
@@ -265,12 +297,23 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
                 s->smb_stat |= STS_BYTE_DONE;
             } else if (s->smb_ctl & CTL_LAST_BYTE) {
                 s->op_done = true;
-                s->smb_blkdata = s->smb_data[s->smb_index];
+                if (s->in_i2c_block_read) {
+                    s->in_i2c_block_read = false;
+                    s->smb_blkdata = i2c_recv(s->smbus);
+                    i2c_nack(s->smbus);
+                    i2c_end_transfer(s->smbus);
+                } else {
+                    s->smb_blkdata = s->smb_data[s->smb_index];
+                }
                 s->smb_index = 0;
                 s->smb_stat |= STS_INTR;
                 s->smb_stat &= ~STS_HOST_BUSY;
             } else {
-                s->smb_blkdata = s->smb_data[s->smb_index];
+                if (s->in_i2c_block_read) {
+                    s->smb_blkdata = i2c_recv(s->smbus);
+                } else {
+                    s->smb_blkdata = s->smb_data[s->smb_index];
+                }
                 s->smb_stat |= STS_BYTE_DONE;
             }
         }
@@ -281,6 +324,10 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
             if (!s->op_done) {
                 s->smb_index = 0;
                 s->op_done = true;
+                if (s->in_i2c_block_read) {
+                    s->in_i2c_block_read = false;
+                    i2c_end_transfer(s->smbus);
+                }
             }
             smb_transaction_start(s);
         }
@@ -334,8 +381,9 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
     switch(addr) {
     case SMBHSTSTS:
         val = s->smb_stat;
-        if (s->smb_stat & STS_HOST_BUSY) {
+        if (s->start_transaction_on_status_read) {
             /* execute command now */
+            s->start_transaction_on_status_read = false;
             s->smb_stat &= ~STS_HOST_BUSY;
             smb_transaction(s);
         }
@@ -356,10 +404,10 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
         val = s->smb_data1;
         break;
     case SMBBLKDAT:
-        if (s->smb_index >= PM_SMBUS_MAX_MSG_SIZE) {
-            s->smb_index = 0;
-        }
-        if (s->smb_auxctl & AUX_BLK) {
+        if (s->smb_auxctl & AUX_BLK && !s->in_i2c_block_read) {
+            if (s->smb_index >= PM_SMBUS_MAX_MSG_SIZE) {
+                s->smb_index = 0;
+            }
             val = s->smb_data[s->smb_index++];
             if (!s->op_done && s->smb_index == s->smb_data0) {
                 s->op_done = true;
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index 6dd5b7040b..7bcca97672 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -33,6 +33,12 @@ typedef struct PMSMBus {
     /* Set on block transfers after the last byte has been read, so the
        INTR bit can be set at the right time. */
     bool op_done;
+
+    /* Set during an I2C block read, so we know how to handle data. */
+    bool in_i2c_block_read;
+
+    /* Used to work around a bug in AMIBIOS, see smb_transaction_start() */
+    bool start_transaction_on_status_read;
 } PMSMBus;
 
 void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk);
-- 
2.17.1

[Qemu-devel] [PATCH v3 08/16] boards.h: Ignore migration for SMBus devices on older machines

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Migration capability is being added for pm_smbus and SMBus devices.
This change will allow backwards compatibility to be kept when
migrating back to an old qemu version.  Add a bool to the machine
class tho keep smbus migration from happening.  Future changes
will use this.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
---
 hw/i386/pc_piix.c   | 1 +
 hw/i386/pc_q35.c    | 1 +
 include/hw/boards.h | 1 +
 3 files changed, 3 insertions(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index cb28227cc3..3d1ccb1af1 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -443,6 +443,7 @@ static void pc_i440fx_2_12_machine_options(MachineClass *m)
     pc_i440fx_3_0_machine_options(m);
     m->is_default = 0;
     m->alias = NULL;
+    m->smbus_no_migration_support = true;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 90e88c9b28..0c6fca6a40 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -324,6 +324,7 @@ static void pc_q35_2_12_machine_options(MachineClass *m)
 {
     pc_q35_3_0_machine_options(m);
     m->alias = NULL;
+    m->smbus_no_migration_support = true;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
 }
 
diff --git a/include/hw/boards.h b/include/hw/boards.h
index f82f28468b..65314fbe2a 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -207,6 +207,7 @@ struct MachineClass {
     void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
                                  int nb_nodes, ram_addr_t size);
     bool ignore_boot_device_suffixes;
+    bool smbus_no_migration_support;
 
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
-- 
2.17.1

[Qemu-devel] [PATCH v3 09/16] migration: Add a VMSTATE_BOOL_TEST() macro

[minyard]

From: Corey Minyard <cminyard@mvista.com>

This will be needed by coming I2C changes.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 include/migration/vmstate.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 2b501d0466..0861e18138 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -850,6 +850,9 @@ extern const VMStateInfo vmstate_info_qtailq;
 #define VMSTATE_INT32_POSITIVE_LE(_f, _s)                             \
     VMSTATE_SINGLE(_f, _s, 0, vmstate_info_int32_le, int32_t)
 
+#define VMSTATE_BOOL_TEST(_f, _s, _t)                               \
+    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_info_bool, bool)
+
 #define VMSTATE_INT8_TEST(_f, _s, _t)                               \
     VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_info_int8, int8_t)
 
-- 
2.17.1

[Qemu-devel] [PATCH v3 10/16] i2c:pm_smbus: Fix state transfer

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Transfer the state information for the SMBus registers and
internal data so it will work on a VM transfer.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/acpi/piix4.c           |  7 +++++++
 hw/i2c/pm_smbus.c         | 31 +++++++++++++++++++++++++++++++
 hw/i2c/smbus_ich9.c       | 10 +++++++++-
 include/hw/i2c/pm_smbus.h |  9 +++++++++
 4 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 2f4dd03b83..91fe4821d3 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -302,6 +302,11 @@ static const VMStateDescription vmstate_cpuhp_state = {
     }
 };
 
+static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
+{
+    return pm_smbus_vmstate_needed();
+}
+
 /* 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
@@ -321,6 +326,8 @@ static const VMStateDescription vmstate_acpi = {
         VMSTATE_UINT16(ar.pm1.evt.en, PIIX4PMState),
         VMSTATE_UINT16(ar.pm1.cnt.cnt, PIIX4PMState),
         VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
+        VMSTATE_STRUCT_TEST(smb, PIIX4PMState, piix4_vmstate_need_smbus, 3,
+                            pmsmb_vmstate, PMSMBus),
         VMSTATE_TIMER_PTR(ar.tmr.timer, PIIX4PMState),
         VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
         VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 8793113c25..2a9bc6e8c0 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -19,6 +19,7 @@
  */
 #include "qemu/osdep.h"
 #include "hw/hw.h"
+#include "hw/boards.h"
 #include "hw/i2c/pm_smbus.h"
 #include "hw/i2c/smbus_master.h"
 
@@ -450,6 +451,36 @@ static const MemoryRegionOps pm_smbus_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+bool pm_smbus_vmstate_needed(void)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
+
+    return !mc->smbus_no_migration_support;
+}
+
+const VMStateDescription pmsmb_vmstate = {
+    .name = "pmsmb",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(smb_stat, PMSMBus),
+        VMSTATE_UINT8(smb_ctl, PMSMBus),
+        VMSTATE_UINT8(smb_cmd, PMSMBus),
+        VMSTATE_UINT8(smb_addr, PMSMBus),
+        VMSTATE_UINT8(smb_data0, PMSMBus),
+        VMSTATE_UINT8(smb_data1, PMSMBus),
+        VMSTATE_UINT32(smb_index, PMSMBus),
+        VMSTATE_UINT8_ARRAY(smb_data, PMSMBus, PM_SMBUS_MAX_MSG_SIZE),
+        VMSTATE_UINT8(smb_auxctl, PMSMBus),
+        VMSTATE_UINT8(smb_blkdata, PMSMBus),
+        VMSTATE_BOOL(i2c_enable, PMSMBus),
+        VMSTATE_BOOL(op_done, PMSMBus),
+        VMSTATE_BOOL(in_i2c_block_read, PMSMBus),
+        VMSTATE_BOOL(start_transaction_on_status_read, PMSMBus),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk)
 {
     smb->op_done = true;
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index e6d8d28194..7b24be8256 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -43,12 +43,20 @@ typedef struct ICH9SMBState {
     PMSMBus smb;
 } ICH9SMBState;
 
+static bool ich9_vmstate_need_smbus(void *opaque, int version_id)
+{
+    return pm_smbus_vmstate_needed();
+}
+
 static const VMStateDescription vmstate_ich9_smbus = {
     .name = "ich9_smb",
     .version_id = 1,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(dev, struct ICH9SMBState),
+        VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
+        VMSTATE_BOOL_TEST(irq_enabled, ICH9SMBState, ich9_vmstate_need_smbus),
+        VMSTATE_STRUCT_TEST(smb, ICH9SMBState, ich9_vmstate_need_smbus, 1,
+                            pmsmb_vmstate, PMSMBus),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index 7bcca97672..fb55c44444 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -43,4 +43,13 @@ typedef struct PMSMBus {
 
 void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk);
 
+/*
+ * For backwards compatibility on migration, older versions don't have
+ * working migration for pm_smbus, this lets us ignore the migrations
+ * for older machine versions.
+ */
+bool pm_smbus_vmstate_needed(void);
+
+extern const VMStateDescription pmsmb_vmstate;
+
 #endif /* PM_SMBUS_H */
-- 
2.17.1

[Qemu-devel] [PATCH v3 11/16] i2c:smbus_slave: Add an SMBus vmstate structure

[minyard]

From: Corey Minyard <cminyard@mvista.com>

There is no vmstate handling for SMBus, so no device sitting on SMBus
can have a state transfer that works reliably.  So add it.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/i2c/smbus_slave.c         | 18 ++++++++++++++++++
 include/hw/i2c/smbus_slave.h | 24 +++++++++++++++++++++---
 2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
index d03f714608..e839c1dd11 100644
--- a/hw/i2c/smbus_slave.c
+++ b/hw/i2c/smbus_slave.c
@@ -206,6 +206,24 @@ static void smbus_device_class_init(ObjectClass *klass, void *data)
     sc->send = smbus_i2c_send;
 }
 
+bool smbus_vmstate_needed(SMBusDevice *dev)
+{
+    return dev->mode != SMBUS_IDLE;
+}
+
+const VMStateDescription vmstate_smbus_device = {
+    .name = TYPE_SMBUS_DEVICE,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_I2C_SLAVE(i2c, SMBusDevice),
+        VMSTATE_INT32(mode, SMBusDevice),
+        VMSTATE_INT32(data_len, SMBusDevice),
+        VMSTATE_UINT8_ARRAY(data_buf, SMBusDevice, SMBUS_DATA_MAX_LEN),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const TypeInfo smbus_device_type_info = {
     .name = TYPE_SMBUS_DEVICE,
     .parent = TYPE_I2C_SLAVE,
diff --git a/include/hw/i2c/smbus_slave.h b/include/hw/i2c/smbus_slave.h
index fad2db9c76..78fd45cb51 100644
--- a/include/hw/i2c/smbus_slave.h
+++ b/include/hw/i2c/smbus_slave.h
@@ -75,14 +75,32 @@ typedef struct SMBusDeviceClass
     void (*transaction_complete)(SMBusDevice *dev);
 } SMBusDeviceClass;
 
+#define SMBUS_DATA_MAX_LEN 34  /* command + len + 32 bytes of data.  */
+
 struct SMBusDevice {
     /* The SMBus protocol is implemented on top of I2C.  */
     I2CSlave i2c;
 
     /* Remaining fields for internal use only.  */
-    int mode;
-    int data_len;
-    uint8_t data_buf[34]; /* command + len + 32 bytes of data.  */
+    int32_t mode;
+    int32_t data_len;
+    uint8_t data_buf[SMBUS_DATA_MAX_LEN];
 };
 
+extern const VMStateDescription vmstate_smbus_device;
+
+#define VMSTATE_SMBUS_DEVICE(_field, _state) {                       \
+    .name       = (stringify(_field)),                               \
+    .size       = sizeof(SMBusDevice),                               \
+    .vmsd       = &vmstate_smbus_device,                             \
+    .flags      = VMS_STRUCT,                                        \
+    .offset     = vmstate_offset_value(_state, _field, SMBusDevice), \
+}
+
+/*
+ * Users should call this in their .needed functions to know if the
+ * SMBus slave data needs to be transferred.
+ */
+bool smbus_vmstate_needed(SMBusDevice *dev);
+
 #endif
-- 
2.17.1

[Qemu-devel] [PATCH v3 12/16] i2c:smbus_eeprom: Add normal type name and cast to smbus_eeprom.c

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Create a type name and a cast macro and use those through the
code.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/i2c/smbus_eeprom.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 4d25222e23..8d4eed129f 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -30,6 +30,11 @@
 
 //#define DEBUG
 
+#define TYPE_SMBUS_EEPROM "smbus-eeprom"
+
+#define SMBUS_EEPROM(obj) \
+    OBJECT_CHECK(SMBusEEPROMDevice, (obj), TYPE_SMBUS_EEPROM)
+
 typedef struct SMBusEEPROMDevice {
     SMBusDevice smbusdev;
     void *data;
@@ -38,7 +43,7 @@ typedef struct SMBusEEPROMDevice {
 
 static uint8_t eeprom_receive_byte(SMBusDevice *dev)
 {
-    SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
+    SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
     uint8_t *data = eeprom->data;
     uint8_t val = data[eeprom->offset++];
 
@@ -51,7 +56,7 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev)
 
 static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
 {
-    SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
+    SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
     uint8_t *data = eeprom->data;
 
 #ifdef DEBUG
@@ -73,7 +78,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
 
 static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
 {
-    SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
+    SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
 
     eeprom->offset = 0;
 }
@@ -97,7 +102,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo smbus_eeprom_info = {
-    .name          = "smbus-eeprom",
+    .name          = TYPE_SMBUS_EEPROM,
     .parent        = TYPE_SMBUS_DEVICE,
     .instance_size = sizeof(SMBusEEPROMDevice),
     .class_init    = smbus_eeprom_class_initfn,
@@ -114,7 +119,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf)
 {
     DeviceState *dev;
 
-    dev = qdev_create((BusState *) smbus, "smbus-eeprom");
+    dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM);
     qdev_prop_set_uint8(dev, "address", address);
     qdev_prop_set_ptr(dev, "data", eeprom_buf);
     qdev_init_nofail(dev);
-- 
2.17.1

[Qemu-devel] [PATCH v3 13/16] i2c:smbus_eeprom: Add a size constant for the smbus_eeprom size

[minyard]

From: Corey Minyard <cminyard@mvista.com>

It was hard-coded to 256 in a number of places, create a constant
for that.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i2c/smbus_eeprom.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 8d4eed129f..8e9b734c09 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -35,6 +35,8 @@
 #define SMBUS_EEPROM(obj) \
     OBJECT_CHECK(SMBusEEPROMDevice, (obj), TYPE_SMBUS_EEPROM)
 
+#define SMBUS_EEPROM_SIZE 256
+
 typedef struct SMBusEEPROMDevice {
     SMBusDevice smbusdev;
     void *data;
@@ -70,7 +72,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
 
     for (; len > 0; len--) {
         data[eeprom->offset] = *buf++;
-        eeprom->offset = (eeprom->offset + 1) % 256;
+        eeprom->offset = (eeprom->offset + 1) % SMBUS_EEPROM_SIZE;
     }
 
     return 0;
@@ -129,12 +131,14 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
                        const uint8_t *eeprom_spd, int eeprom_spd_size)
 {
     int i;
-    uint8_t *eeprom_buf = g_malloc0(8 * 256); /* XXX: make this persistent */
+     /* XXX: make this persistent */
+    uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE);
     if (eeprom_spd_size > 0) {
         memcpy(eeprom_buf, eeprom_spd, eeprom_spd_size);
     }
 
     for (i = 0; i < nb_eeprom; i++) {
-        smbus_eeprom_init_one(smbus, 0x50 + i, eeprom_buf + (i * 256));
+        smbus_eeprom_init_one(smbus, 0x50 + i,
+                              eeprom_buf + (i * SMBUS_EEPROM_SIZE));
     }
 }
-- 
2.17.1

[Qemu-devel] [PATCH v3 14/16] i2c:smbus_eeprom: Add vmstate handling to the smbus eeprom

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Transfer the state of the EEPROM on a migration.  This way the
data remains consistent on migration.

This required moving the actual data to a separate array and
using the data provided in the init function as a separate
initialization array, since a pointer property has to be a
void * and the array needs to be uint8_t[].

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
---
 hw/i2c/smbus_eeprom.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 8e9b734c09..942057dc10 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -24,6 +24,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/hw.h"
+#include "hw/boards.h"
 #include "hw/i2c/i2c.h"
 #include "hw/i2c/smbus_slave.h"
 #include "hw/i2c/smbus_eeprom.h"
@@ -39,8 +40,10 @@
 
 typedef struct SMBusEEPROMDevice {
     SMBusDevice smbusdev;
-    void *data;
+    uint8_t data[SMBUS_EEPROM_SIZE];
+    void *init_data;
     uint8_t offset;
+    bool accessed;
 } SMBusEEPROMDevice;
 
 static uint8_t eeprom_receive_byte(SMBusDevice *dev)
@@ -49,6 +52,7 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev)
     uint8_t *data = eeprom->data;
     uint8_t val = data[eeprom->offset++];
 
+    eeprom->accessed = true;
 #ifdef DEBUG
     printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n",
            dev->i2c.address, val);
@@ -61,6 +65,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
     SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
     uint8_t *data = eeprom->data;
 
+    eeprom->accessed = true;
 #ifdef DEBUG
     printf("eeprom_write_byte: addr=0x%02x cmd=0x%02x val=0x%02x\n",
            dev->i2c.address, cmd, buf[0]);
@@ -78,15 +83,39 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
     return 0;
 }
 
+static bool smbus_eeprom_vmstate_needed(void *opaque)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
+    SMBusEEPROMDevice *eeprom = opaque;
+
+    return (eeprom->accessed || smbus_vmstate_needed(&eeprom->smbusdev)) &&
+        !mc->smbus_no_migration_support;
+}
+
+static const VMStateDescription vmstate_smbus_eeprom = {
+    .name = "smbus-eeprom",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = smbus_eeprom_vmstate_needed,
+    .fields      = (VMStateField[]) {
+        VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice),
+        VMSTATE_UINT8_ARRAY(data, SMBusEEPROMDevice, SMBUS_EEPROM_SIZE),
+        VMSTATE_UINT8(offset, SMBusEEPROMDevice),
+        VMSTATE_BOOL(accessed, SMBusEEPROMDevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
 {
     SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
 
+    memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
     eeprom->offset = 0;
 }
 
 static Property smbus_eeprom_properties[] = {
-    DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data),
+    DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -99,6 +128,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
     sc->receive_byte = eeprom_receive_byte;
     sc->write_data = eeprom_write_data;
     dc->props = smbus_eeprom_properties;
+    dc->vmsd = &vmstate_smbus_eeprom;
     /* Reason: pointer property "data" */
     dc->user_creatable = false;
 }
-- 
2.17.1

[Qemu-devel] [PATCH v3 15/16] hw/i2c/smbus_eeprom: Create at most SMBUS_EEPROM_MAX EEPROMs on a SMBus

[minyard]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

Calling smbus_eeprom_init() with more than 8 EEPROMs would lead to a
heap overflow.
Replace the '8' magic number by a definition, and check no more than
this number are created.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/i2c/smbus_eeprom.c         | 13 +++++++++++--
 include/hw/i2c/smbus_eeprom.h |  4 +++-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 942057dc10..a0dcadbd60 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -23,6 +23,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "hw/hw.h"
 #include "hw/boards.h"
 #include "hw/i2c/i2c.h"
@@ -157,12 +158,20 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf)
     qdev_init_nofail(dev);
 }
 
-void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
+void smbus_eeprom_init(I2CBus *smbus, unsigned int nb_eeprom,
                        const uint8_t *eeprom_spd, int eeprom_spd_size)
 {
     int i;
+    uint8_t *eeprom_buf;
+
+    if (nb_eeprom > SMBUS_EEPROM_MAX) {
+        error_report("At most %u EEPROM are supported on a SMBus.",
+                     SMBUS_EEPROM_MAX);
+        exit(1);
+    }
+
      /* XXX: make this persistent */
-    uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE);
+    eeprom_buf = g_malloc0(nb_eeprom * SMBUS_EEPROM_SIZE);
     if (eeprom_spd_size > 0) {
         memcpy(eeprom_buf, eeprom_spd, eeprom_spd_size);
     }
diff --git a/include/hw/i2c/smbus_eeprom.h b/include/hw/i2c/smbus_eeprom.h
index 46fb1a37d6..8436200599 100644
--- a/include/hw/i2c/smbus_eeprom.h
+++ b/include/hw/i2c/smbus_eeprom.h
@@ -25,8 +25,10 @@
 
 #include "hw/i2c/i2c.h"
 
+#define SMBUS_EEPROM_MAX 8
+
 void smbus_eeprom_init_one(I2CBus *bus, uint8_t address, uint8_t *eeprom_buf);
-void smbus_eeprom_init(I2CBus *bus, int nb_eeprom,
+void smbus_eeprom_init(I2CBus *bus, unsigned int nb_eeprom,
                        const uint8_t *eeprom_spd, int size);
 
 #endif
-- 
2.17.1

[Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Reset the contents to init data and reset the offset on a machine
reset.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/i2c/smbus_eeprom.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index a0dcadbd60..de3a492df4 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -107,7 +107,7 @@ static const VMStateDescription vmstate_smbus_eeprom = {
     }
 };
 
-static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
+static void smbus_eeprom_reset(DeviceState *dev)
 {
     SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
 
@@ -115,6 +115,11 @@ static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
     eeprom->offset = 0;
 }
 
+static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
+{
+    smbus_eeprom_reset(dev);
+}
+
 static Property smbus_eeprom_properties[] = {
     DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
     DEFINE_PROP_END_OF_LIST(),
@@ -126,6 +131,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
     SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
 
     dc->realize = smbus_eeprom_realize;
+    dc->reset = smbus_eeprom_reset;
     sc->receive_byte = eeprom_receive_byte;
     sc->write_data = eeprom_write_data;
     dc->props = smbus_eeprom_properties;
-- 
2.17.1

Re: [Qemu-devel] [PATCH v3 01/16] i2c: Split smbus into parts

[Philippe Mathieu-Daudé]

On 26/11/18 21:04, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> smbus.c and smbus.h had device side code, master side code, and
> smbus.h has some smbus_eeprom.c definitions.  Split them into
> separate files.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  MAINTAINERS                               |  12 ++
>  hw/arm/aspeed.c                           |   2 +-
>  hw/i2c/Makefile.objs                      |   2 +-
>  hw/i2c/pm_smbus.c                         |   2 +-
>  hw/i2c/smbus_eeprom.c                     |   3 +-
>  hw/i2c/smbus_ich9.c                       |   2 -
>  hw/i2c/smbus_master.c                     | 165 ++++++++++++++++++++++
>  hw/i2c/{smbus.c => smbus_slave.c}         | 153 +-------------------
>  hw/i386/pc_piix.c                         |   2 +-
>  hw/i386/pc_q35.c                          |   2 +-
>  hw/isa/vt82c686.c                         |   1 -
>  hw/mips/mips_fulong2e.c                   |   2 +-
>  hw/mips/mips_malta.c                      |   2 +-
>  hw/ppc/sam460ex.c                         |   2 +-
>  include/hw/i2c/pm_smbus.h                 |   2 +
>  include/hw/i2c/smbus_eeprom.h             |  32 +++++
>  include/hw/i2c/smbus_master.h             |  55 ++++++++
>  include/hw/i2c/{smbus.h => smbus_slave.h} |  37 +----
>  18 files changed, 285 insertions(+), 193 deletions(-)
>  create mode 100644 hw/i2c/smbus_master.c
>  rename hw/i2c/{smbus.c => smbus_slave.c} (64%)
>  create mode 100644 include/hw/i2c/smbus_eeprom.h
>  create mode 100644 include/hw/i2c/smbus_master.h
>  rename include/hw/i2c/{smbus.h => smbus_slave.h} (65%)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0499e11593..62340d32b8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1935,6 +1935,18 @@ M: Viktor Prutyanov <viktor.prutyanov@phystech.edu>
>  S: Maintained
>  F: contrib/elf2dmp/
>  
> +I2C

Maybe "I2C and SMBus"

> +M: Corey Minyard <cminyard@mvista.com>
> +S: Maintained
> +F: hw/i2c/core.c
> +F: hw/i2c/smbus_slave.c
> +F: hw/i2c/smbus_master.c
> +F: hw/i2c/smbus_eeprom.c
> +F: include/hw/i2c/i2c.h
> +F: include/hw/i2c/smbus_master.h
> +F: include/hw/i2c/smbus_slave.h
> +F: include/hw/i2c/smbus_eeprom.h
> +
>  Usermode Emulation
>  ------------------
>  Overall
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 6b33ecd5aa..69a19df00d 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -18,7 +18,7 @@
>  #include "hw/arm/aspeed.h"
>  #include "hw/arm/aspeed_soc.h"
>  #include "hw/boards.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_eeprom.h"
>  #include "qemu/log.h"
>  #include "sysemu/block-backend.h"
>  #include "hw/loader.h"
> diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
> index 37cacde978..8973edfa22 100644
> --- a/hw/i2c/Makefile.objs
> +++ b/hw/i2c/Makefile.objs
> @@ -1,4 +1,4 @@
> -common-obj-$(CONFIG_I2C) += core.o smbus.o smbus_eeprom.o
> +common-obj-$(CONFIG_I2C) += core.o smbus_slave.o smbus_master.o smbus_eeprom.o
>  common-obj-$(CONFIG_DDC) += i2c-ddc.o
>  common-obj-$(CONFIG_VERSATILE_I2C) += versatile_i2c.o
>  common-obj-$(CONFIG_ACPI_X86) += smbus_ich9.o
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 685a2378ed..f3c6cc46f9 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -20,7 +20,7 @@
>  #include "qemu/osdep.h"
>  #include "hw/hw.h"
>  #include "hw/i2c/pm_smbus.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_master.h"
>  
>  #define SMBHSTSTS       0x00
>  #define SMBHSTCNT       0x02
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index f18aa3de35..d82423aa7e 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -25,7 +25,8 @@
>  #include "qemu/osdep.h"
>  #include "hw/hw.h"
>  #include "hw/i2c/i2c.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_slave.h"
> +#include "hw/i2c/smbus_eeprom.h"
>  
>  //#define DEBUG
>  
> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
> index 2a8b49e02f..e6d8d28194 100644
> --- a/hw/i2c/smbus_ich9.c
> +++ b/hw/i2c/smbus_ich9.c
> @@ -29,8 +29,6 @@
>  #include "hw/i2c/pm_smbus.h"
>  #include "hw/pci/pci.h"
>  #include "sysemu/sysemu.h"
> -#include "hw/i2c/i2c.h"
> -#include "hw/i2c/smbus.h"
>  
>  #include "hw/i386/ich9.h"
>  
> diff --git a/hw/i2c/smbus_master.c b/hw/i2c/smbus_master.c
> new file mode 100644
> index 0000000000..0a6223744c
> --- /dev/null
> +++ b/hw/i2c/smbus_master.c
> @@ -0,0 +1,165 @@
> +/*
> + * QEMU SMBus host (master) emulation.
> + *
> + * This code emulates SMBus transactions from the master point of view,
> + * it runs the individual I2C transaction to do the SMBus protocol
> + * over I2C.
> + *
> + * Copyright (c) 2007 CodeSourcery.
> + * Written by Paul Brook
> + *
> + * This code is licensed under the LGPL.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/hw.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/i2c/smbus_master.h"
> +
> +/* Master device commands.  */
> +int smbus_quick_command(I2CBus *bus, uint8_t addr, int read)
> +{
> +    if (i2c_start_transfer(bus, addr, read)) {
> +        return -1;
> +    }
> +    i2c_end_transfer(bus);
> +    return 0;
> +}
> +
> +int smbus_receive_byte(I2CBus *bus, uint8_t addr)
> +{
> +    uint8_t data;
> +
> +    if (i2c_start_transfer(bus, addr, 1)) {
> +        return -1;
> +    }
> +    data = i2c_recv(bus);
> +    i2c_nack(bus);
> +    i2c_end_transfer(bus);
> +    return data;
> +}
> +
> +int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data)
> +{
> +    if (i2c_start_transfer(bus, addr, 0)) {
> +        return -1;
> +    }
> +    i2c_send(bus, data);
> +    i2c_end_transfer(bus);
> +    return 0;
> +}
> +
> +int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command)
> +{
> +    uint8_t data;
> +    if (i2c_start_transfer(bus, addr, 0)) {
> +        return -1;
> +    }
> +    i2c_send(bus, command);
> +    if (i2c_start_transfer(bus, addr, 1)) {
> +        i2c_end_transfer(bus);
> +        return -1;
> +    }
> +    data = i2c_recv(bus);
> +    i2c_nack(bus);
> +    i2c_end_transfer(bus);
> +    return data;
> +}
> +
> +int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data)
> +{
> +    if (i2c_start_transfer(bus, addr, 0)) {
> +        return -1;
> +    }
> +    i2c_send(bus, command);
> +    i2c_send(bus, data);
> +    i2c_end_transfer(bus);
> +    return 0;
> +}
> +
> +int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command)
> +{
> +    uint16_t data;
> +    if (i2c_start_transfer(bus, addr, 0)) {
> +        return -1;
> +    }
> +    i2c_send(bus, command);
> +    if (i2c_start_transfer(bus, addr, 1)) {
> +        i2c_end_transfer(bus);
> +        return -1;
> +    }
> +    data = i2c_recv(bus);
> +    data |= i2c_recv(bus) << 8;
> +    i2c_nack(bus);
> +    i2c_end_transfer(bus);
> +    return data;
> +}
> +
> +int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data)
> +{
> +    if (i2c_start_transfer(bus, addr, 0)) {
> +        return -1;
> +    }
> +    i2c_send(bus, command);
> +    i2c_send(bus, data & 0xff);
> +    i2c_send(bus, data >> 8);
> +    i2c_end_transfer(bus);
> +    return 0;
> +}
> +
> +int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> +                     int len, bool recv_len, bool send_cmd)
> +{
> +    int rlen;
> +    int i;
> +
> +    if (send_cmd) {
> +        if (i2c_start_transfer(bus, addr, 0)) {
> +            return -1;
> +        }
> +        i2c_send(bus, command);
> +    }
> +    if (i2c_start_transfer(bus, addr, 1)) {
> +        if (send_cmd) {
> +            i2c_end_transfer(bus);
> +        }
> +        return -1;
> +    }
> +    if (recv_len) {
> +        rlen = i2c_recv(bus);
> +    } else {
> +        rlen = len;
> +    }
> +    if (rlen > len) {
> +        rlen = 0;
> +    }
> +    for (i = 0; i < rlen; i++) {
> +        data[i] = i2c_recv(bus);
> +    }
> +    i2c_nack(bus);
> +    i2c_end_transfer(bus);
> +    return rlen;
> +}
> +
> +int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> +                      int len, bool send_len)
> +{
> +    int i;
> +
> +    if (len > 32) {
> +        len = 32;
> +    }
> +
> +    if (i2c_start_transfer(bus, addr, 0)) {
> +        return -1;
> +    }
> +    i2c_send(bus, command);
> +    if (send_len) {
> +        i2c_send(bus, len);
> +    }
> +    for (i = 0; i < len; i++) {
> +        i2c_send(bus, data[i]);
> +    }
> +    i2c_end_transfer(bus);
> +    return 0;
> +}
> diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus_slave.c
> similarity index 64%
> rename from hw/i2c/smbus.c
> rename to hw/i2c/smbus_slave.c
> index 6ff77c582f..1e734752d7 100644
> --- a/hw/i2c/smbus.c
> +++ b/hw/i2c/smbus_slave.c
> @@ -1,6 +1,10 @@
>  /*
>   * QEMU SMBus device emulation.
>   *
> + * This code is a helper for SMBus device emulation.  It implements an
> + * I2C device inteface and runs the SMBus protocol from the device
> + * point of view and maps those to simple calls to emulate.
> + *
>   * Copyright (c) 2007 CodeSourcery.
>   * Written by Paul Brook
>   *
> @@ -12,7 +16,7 @@
>  #include "qemu/osdep.h"
>  #include "hw/hw.h"
>  #include "hw/i2c/i2c.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_slave.h"
>  
>  //#define DEBUG_SMBUS 1
>  
> @@ -202,153 +206,6 @@ static int smbus_i2c_send(I2CSlave *s, uint8_t data)
>      return 0;
>  }
>  
> -/* Master device commands.  */
> -int smbus_quick_command(I2CBus *bus, uint8_t addr, int read)
> -{
> -    if (i2c_start_transfer(bus, addr, read)) {
> -        return -1;
> -    }
> -    i2c_end_transfer(bus);
> -    return 0;
> -}
> -
> -int smbus_receive_byte(I2CBus *bus, uint8_t addr)
> -{
> -    uint8_t data;
> -
> -    if (i2c_start_transfer(bus, addr, 1)) {
> -        return -1;
> -    }
> -    data = i2c_recv(bus);
> -    i2c_nack(bus);
> -    i2c_end_transfer(bus);
> -    return data;
> -}
> -
> -int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data)
> -{
> -    if (i2c_start_transfer(bus, addr, 0)) {
> -        return -1;
> -    }
> -    i2c_send(bus, data);
> -    i2c_end_transfer(bus);
> -    return 0;
> -}
> -
> -int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command)
> -{
> -    uint8_t data;
> -    if (i2c_start_transfer(bus, addr, 0)) {
> -        return -1;
> -    }
> -    i2c_send(bus, command);
> -    if (i2c_start_transfer(bus, addr, 1)) {
> -        i2c_end_transfer(bus);
> -        return -1;
> -    }
> -    data = i2c_recv(bus);
> -    i2c_nack(bus);
> -    i2c_end_transfer(bus);
> -    return data;
> -}
> -
> -int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data)
> -{
> -    if (i2c_start_transfer(bus, addr, 0)) {
> -        return -1;
> -    }
> -    i2c_send(bus, command);
> -    i2c_send(bus, data);
> -    i2c_end_transfer(bus);
> -    return 0;
> -}
> -
> -int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command)
> -{
> -    uint16_t data;
> -    if (i2c_start_transfer(bus, addr, 0)) {
> -        return -1;
> -    }
> -    i2c_send(bus, command);
> -    if (i2c_start_transfer(bus, addr, 1)) {
> -        i2c_end_transfer(bus);
> -        return -1;
> -    }
> -    data = i2c_recv(bus);
> -    data |= i2c_recv(bus) << 8;
> -    i2c_nack(bus);
> -    i2c_end_transfer(bus);
> -    return data;
> -}
> -
> -int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data)
> -{
> -    if (i2c_start_transfer(bus, addr, 0)) {
> -        return -1;
> -    }
> -    i2c_send(bus, command);
> -    i2c_send(bus, data & 0xff);
> -    i2c_send(bus, data >> 8);
> -    i2c_end_transfer(bus);
> -    return 0;
> -}
> -
> -int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> -                     int len, bool recv_len, bool send_cmd)
> -{
> -    int rlen;
> -    int i;
> -
> -    if (send_cmd) {
> -        if (i2c_start_transfer(bus, addr, 0)) {
> -            return -1;
> -        }
> -        i2c_send(bus, command);
> -    }
> -    if (i2c_start_transfer(bus, addr, 1)) {
> -        if (send_cmd) {
> -            i2c_end_transfer(bus);
> -        }
> -        return -1;
> -    }
> -    if (recv_len) {
> -        rlen = i2c_recv(bus);
> -    } else {
> -        rlen = len;
> -    }
> -    if (rlen > len) {
> -        rlen = 0;
> -    }
> -    for (i = 0; i < rlen; i++) {
> -        data[i] = i2c_recv(bus);
> -    }
> -    i2c_nack(bus);
> -    i2c_end_transfer(bus);
> -    return rlen;
> -}
> -
> -int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> -                      int len, bool send_len)
> -{
> -    int i;
> -
> -    if (len > 32)
> -        len = 32;
> -
> -    if (i2c_start_transfer(bus, addr, 0)) {
> -        return -1;
> -    }
> -    i2c_send(bus, command);
> -    if (send_len) {
> -        i2c_send(bus, len);
> -    }
> -    for (i = 0; i < len; i++) {
> -        i2c_send(bus, data[i]);
> -    }
> -    i2c_end_transfer(bus);
> -    return 0;
> -}
> -
>  static void smbus_device_class_init(ObjectClass *klass, void *data)
>  {
>      I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index dc09466b3e..cb28227cc3 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -42,7 +42,7 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/sysbus.h"
>  #include "sysemu/arch_init.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_eeprom.h"
>  #include "hw/xen/xen.h"
>  #include "exec/memory.h"
>  #include "exec/address-spaces.h"
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 532241e3f8..90e88c9b28 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -33,7 +33,7 @@
>  #include "hw/hw.h"
>  #include "hw/loader.h"
>  #include "sysemu/arch_init.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_eeprom.h"
>  #include "hw/boards.h"
>  #include "hw/timer/mc146818rtc.h"
>  #include "hw/xen/xen.h"
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 7302f6d74b..85d0532dd5 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -14,7 +14,6 @@
>  #include "hw/hw.h"
>  #include "hw/isa/vt82c686.h"
>  #include "hw/i2c/i2c.h"
> -#include "hw/i2c/smbus.h"
>  #include "hw/pci/pci.h"
>  #include "hw/isa/isa.h"
>  #include "hw/isa/superio.h"
> diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
> index 2fbba32c48..dae8acc108 100644
> --- a/hw/mips/mips_fulong2e.c
> +++ b/hw/mips/mips_fulong2e.c
> @@ -27,7 +27,7 @@
>  #include "hw/isa/superio.h"
>  #include "net/net.h"
>  #include "hw/boards.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_eeprom.h"
>  #include "hw/block/flash.h"
>  #include "hw/mips/mips.h"
>  #include "hw/mips/cpudevs.h"
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index c1cf0fe12e..1fb7170f5e 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -33,7 +33,7 @@
>  #include "hw/char/serial.h"
>  #include "net/net.h"
>  #include "hw/boards.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_eeprom.h"
>  #include "hw/block/flash.h"
>  #include "hw/mips/mips.h"
>  #include "hw/mips/cpudevs.h"
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 5aac58f36e..7136b23f91 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -34,7 +34,7 @@
>  #include "hw/sysbus.h"
>  #include "hw/char/serial.h"
>  #include "hw/i2c/ppc4xx_i2c.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_eeprom.h"
>  #include "hw/usb/hcd-ehci.h"
>  #include "hw/ppc/fdt.h"
>  
> diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
> index 060d3c6ac0..6dd5b7040b 100644
> --- a/include/hw/i2c/pm_smbus.h
> +++ b/include/hw/i2c/pm_smbus.h
> @@ -1,6 +1,8 @@
>  #ifndef PM_SMBUS_H
>  #define PM_SMBUS_H
>  
> +#include "hw/i2c/smbus_master.h"
> +
>  #define PM_SMBUS_MAX_MSG_SIZE 32
>  
>  typedef struct PMSMBus {
> diff --git a/include/hw/i2c/smbus_eeprom.h b/include/hw/i2c/smbus_eeprom.h
> new file mode 100644
> index 0000000000..46fb1a37d6
> --- /dev/null
> +++ b/include/hw/i2c/smbus_eeprom.h
> @@ -0,0 +1,32 @@
> +/*
> + * QEMU SMBus EEPROM API
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef HW_SMBUS_EEPROM_H
> +#define HW_SMBUS_EEPROM_H
> +
> +#include "hw/i2c/i2c.h"
> +
> +void smbus_eeprom_init_one(I2CBus *bus, uint8_t address, uint8_t *eeprom_buf);
> +void smbus_eeprom_init(I2CBus *bus, int nb_eeprom,
> +                       const uint8_t *eeprom_spd, int size);
> +
> +#endif
> diff --git a/include/hw/i2c/smbus_master.h b/include/hw/i2c/smbus_master.h
> new file mode 100644
> index 0000000000..bb13bc423c
> --- /dev/null
> +++ b/include/hw/i2c/smbus_master.h
> @@ -0,0 +1,55 @@
> +/*
> + * QEMU SMBus host (master) API
> + *
> + * Copyright (c) 2007 Arastra, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef HW_SMBUS_MASTER_H
> +#define HW_SMBUS_MASTER_H
> +
> +#include "hw/i2c/i2c.h"
> +
> +/* Master device commands.  */
> +int smbus_quick_command(I2CBus *bus, uint8_t addr, int read);
> +int smbus_receive_byte(I2CBus *bus, uint8_t addr);
> +int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data);
> +int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command);
> +int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data);
> +int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command);
> +int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data);
> +
> +/*
> + * Do a block transfer from an I2C device.  If recv_len is set, then the
> + * first received byte is a length field and is used to know how much data
> + * to receive.  Otherwise receive "len" bytes.  If send_cmd is set, send
> + * the command byte first before receiving the data.
> + */
> +int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> +                     int len, bool recv_len, bool send_cmd);
> +
> +/*
> + * Do a block transfer to an I2C device.  If send_len is set, send the
> + * "len" value before the data.
> + */
> +int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> +                      int len, bool send_len);
> +
> +#endif
> diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus_slave.h
> similarity index 65%
> rename from include/hw/i2c/smbus.h
> rename to include/hw/i2c/smbus_slave.h
> index d8b1b9ee81..ff07ee005d 100644
> --- a/include/hw/i2c/smbus.h
> +++ b/include/hw/i2c/smbus_slave.h
> @@ -1,8 +1,5 @@
> -#ifndef QEMU_SMBUS_H
> -#define QEMU_SMBUS_H
> -
>  /*
> - * QEMU SMBus API
> + * QEMU SMBus device (slave) API
>   *
>   * Copyright (c) 2007 Arastra, Inc.
>   *
> @@ -25,6 +22,9 @@
>   * THE SOFTWARE.
>   */
>  
> +#ifndef HW_SMBUS_SLAVE_H
> +#define HW_SMBUS_SLAVE_H
> +
>  #include "hw/i2c/i2c.h"
>  
>  #define TYPE_SMBUS_DEVICE "smbus-device"
> @@ -64,33 +64,4 @@ struct SMBusDevice {
>      uint8_t command;
>  };
>  
> -/* Master device commands.  */
> -int smbus_quick_command(I2CBus *bus, uint8_t addr, int read);
> -int smbus_receive_byte(I2CBus *bus, uint8_t addr);
> -int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data);
> -int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command);
> -int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data);
> -int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command);
> -int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data);
> -
> -/*
> - * Do a block transfer from an I2C device.  If recv_len is set, then the
> - * first received byte is a length field and is used to know how much data
> - * to receive.  Otherwise receive "len" bytes.  If send_cmd is set, send
> - * the command byte first before receiving the data.
> - */
> -int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> -                     int len, bool recv_len, bool send_cmd);
> -
> -/*
> - * Do a block transfer to an I2C device.  If send_len is set, send the
> - * "len" value before the data.
> - */
> -int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> -                      int len, bool send_len);
> -
> -void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf);
> -void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
> -                       const uint8_t *eeprom_spd, int size);
> -
>  #endif
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH v3 02/16] i2c: have I2C receive operation return uint8_t

[Philippe Mathieu-Daudé]

Hi Corey,

On 26/11/18 21:04, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> It is never supposed to fail and cannot return an error, so just
> have it return the proper type.  Have it return 0xff on nothing
> available, since that's what would happen on a real bus.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/pxa2xx.c         |  2 +-
>  hw/arm/tosa.c           |  4 ++--
>  hw/arm/z2.c             |  2 +-
>  hw/audio/wm8750.c       |  2 +-
>  hw/display/sii9022.c    |  2 +-
>  hw/display/ssd0303.c    |  4 ++--
>  hw/gpio/max7310.c       |  2 +-
>  hw/i2c/core.c           | 32 +++++++++++++-------------------
>  hw/i2c/i2c-ddc.c        |  2 +-
>  hw/i2c/smbus_slave.c    |  4 ++--
>  hw/input/lm832x.c       |  2 +-
>  hw/misc/pca9552.c       |  2 +-
>  hw/misc/tmp105.c        |  2 +-
>  hw/misc/tmp421.c        |  2 +-
>  hw/nvram/eeprom_at24c.c |  4 ++--
>  hw/timer/ds1338.c       |  2 +-
>  hw/timer/m41t80.c       |  2 +-
>  hw/timer/twl92230.c     |  2 +-
>  include/hw/i2c/i2c.h    |  7 +++----
>  19 files changed, 37 insertions(+), 44 deletions(-)
> 
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index f598a1c053..3d7c88910e 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -1286,7 +1286,7 @@ static int pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
>      return 0;
>  }
>  
> -static int pxa2xx_i2c_rx(I2CSlave *i2c)
> +static uint8_t pxa2xx_i2c_rx(I2CSlave *i2c)
>  {
>      PXA2xxI2CSlaveState *slave = PXA2XX_I2C_SLAVE(i2c);
>      PXA2xxI2CState *s = slave->host;
> diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
> index 7a925fa5e6..eef9d427e7 100644
> --- a/hw/arm/tosa.c
> +++ b/hw/arm/tosa.c
> @@ -197,10 +197,10 @@ static int tosa_dac_event(I2CSlave *i2c, enum i2c_event event)
>      return 0;
>  }
>  
> -static int tosa_dac_recv(I2CSlave *s)
> +static uint8_t tosa_dac_recv(I2CSlave *s)
>  {
>      printf("%s: recv not supported!!!\n", __func__);
> -    return -1;
> +    return 0xff;
>  }
>  
>  static void tosa_tg_init(PXA2xxState *cpu)
> diff --git a/hw/arm/z2.c b/hw/arm/z2.c
> index 697a822f1e..6f18d924df 100644
> --- a/hw/arm/z2.c
> +++ b/hw/arm/z2.c
> @@ -243,7 +243,7 @@ static int aer915_event(I2CSlave *i2c, enum i2c_event event)
>      return 0;
>  }
>  
> -static int aer915_recv(I2CSlave *slave)
> +static uint8_t aer915_recv(I2CSlave *slave)
>  {
>      AER915State *s = AER915(slave);
>      int retval = 0x00;
> diff --git a/hw/audio/wm8750.c b/hw/audio/wm8750.c
> index f4aa838f62..169b006ade 100644
> --- a/hw/audio/wm8750.c
> +++ b/hw/audio/wm8750.c
> @@ -561,7 +561,7 @@ static int wm8750_tx(I2CSlave *i2c, uint8_t data)
>      return 0;
>  }
>  
> -static int wm8750_rx(I2CSlave *i2c)
> +static uint8_t wm8750_rx(I2CSlave *i2c)
>  {
>      return 0x00;
>  }
> diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
> index eaf11a6e7b..9994385c35 100644
> --- a/hw/display/sii9022.c
> +++ b/hw/display/sii9022.c
> @@ -79,7 +79,7 @@ static int sii9022_event(I2CSlave *i2c, enum i2c_event event)
>      return 0;
>  }
>  
> -static int sii9022_rx(I2CSlave *i2c)
> +static uint8_t sii9022_rx(I2CSlave *i2c)
>  {
>      sii9022_state *s = SII9022(i2c);
>      uint8_t res = 0x00;
> diff --git a/hw/display/ssd0303.c b/hw/display/ssd0303.c
> index eb90ba26be..8edf34986c 100644
> --- a/hw/display/ssd0303.c
> +++ b/hw/display/ssd0303.c
> @@ -62,10 +62,10 @@ typedef struct {
>      uint8_t framebuffer[132*8];
>  } ssd0303_state;
>  
> -static int ssd0303_recv(I2CSlave *i2c)
> +static uint8_t ssd0303_recv(I2CSlave *i2c)
>  {
>      BADF("Reads not implemented\n");
> -    return -1;
> +    return 0xff;
>  }
>  
>  static int ssd0303_send(I2CSlave *i2c, uint8_t data)
> diff --git a/hw/gpio/max7310.c b/hw/gpio/max7310.c
> index a560e3afd2..f35a930276 100644
> --- a/hw/gpio/max7310.c
> +++ b/hw/gpio/max7310.c
> @@ -39,7 +39,7 @@ static void max7310_reset(DeviceState *dev)
>      s->command = 0x00;
>  }
>  
> -static int max7310_rx(I2CSlave *i2c)
> +static uint8_t max7310_rx(I2CSlave *i2c)
>  {
>      MAX7310State *s = MAX7310(i2c);
>  
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index b54725985a..15237ad073 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -191,23 +191,17 @@ int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send)

i2c_send_recv() could benefit the same improvement.

>          }
>          return ret ? -1 : 0;
>      } else {
> -        if ((QLIST_EMPTY(&bus->current_devs)) || (bus->broadcast)) {
> -            return -1;
> -        }
> -
> -        sc = I2C_SLAVE_GET_CLASS(QLIST_FIRST(&bus->current_devs)->elt);
> -        if (sc->recv) {
> -            s = QLIST_FIRST(&bus->current_devs)->elt;
> -            ret = sc->recv(s);
> -            trace_i2c_recv(s->address, ret);
> -            if (ret < 0) {
> -                return ret;
> -            } else {
> -                *data = ret;
> -                return 0;
> +        ret = 0xff;
> +        if (!QLIST_EMPTY(&bus->current_devs) && !bus->broadcast) {
> +            sc = I2C_SLAVE_GET_CLASS(QLIST_FIRST(&bus->current_devs)->elt);
> +            if (sc->recv) {
> +                s = QLIST_FIRST(&bus->current_devs)->elt;
> +                ret = sc->recv(s);
> +                trace_i2c_recv(s->address, ret);
>              }
>          }
> -        return -1;
> +        *data = ret;
> +        return 0;
>      }
>  }
>  
> @@ -216,12 +210,12 @@ int i2c_send(I2CBus *bus, uint8_t data)
>      return i2c_send_recv(bus, &data, true);
>  }
>  
> -int i2c_recv(I2CBus *bus)
> +uint8_t i2c_recv(I2CBus *bus)
>  {
> -    uint8_t data;
> -    int ret = i2c_send_recv(bus, &data, false);
> +    uint8_t data = 0xff;
>  
> -    return ret < 0 ? ret : data;
> +    i2c_send_recv(bus, &data, false);
> +    return data;
>  }
>  
>  void i2c_nack(I2CBus *bus)
> diff --git a/hw/i2c/i2c-ddc.c b/hw/i2c/i2c-ddc.c
> index be34fe072c..95325358db 100644
> --- a/hw/i2c/i2c-ddc.c
> +++ b/hw/i2c/i2c-ddc.c
> @@ -51,7 +51,7 @@ static int i2c_ddc_event(I2CSlave *i2c, enum i2c_event event)
>      return 0;
>  }
>  
> -static int i2c_ddc_rx(I2CSlave *i2c)
> +static uint8_t i2c_ddc_rx(I2CSlave *i2c)
>  {
>      I2CDDCState *s = I2CDDC(i2c);
>  
> diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
> index 1e734752d7..549e7ae933 100644
> --- a/hw/i2c/smbus_slave.c
> +++ b/hw/i2c/smbus_slave.c
> @@ -156,11 +156,11 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
>      return 0;
>  }
>  
> -static int smbus_i2c_recv(I2CSlave *s)
> +static uint8_t smbus_i2c_recv(I2CSlave *s)
>  {
>      SMBusDevice *dev = SMBUS_DEVICE(s);
>      SMBusDeviceClass *sc = SMBUS_DEVICE_GET_CLASS(dev);
> -    int ret;
> +    uint8_t ret;
>  
>      switch (dev->mode) {
>      case SMBUS_RECV_BYTE:
> diff --git a/hw/input/lm832x.c b/hw/input/lm832x.c
> index 74da30d9ca..9ae037953d 100644
> --- a/hw/input/lm832x.c
> +++ b/hw/input/lm832x.c
> @@ -401,7 +401,7 @@ static int lm_i2c_event(I2CSlave *i2c, enum i2c_event event)
>      return 0;
>  }
>  
> -static int lm_i2c_rx(I2CSlave *i2c)
> +static uint8_t lm_i2c_rx(I2CSlave *i2c)
>  {
>      LM823KbdState *s = LM8323(i2c);
>  
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index 9775d5274a..7325d3f287 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -115,7 +115,7 @@ static void pca9552_autoinc(PCA9552State *s)
>      }
>  }
>  
> -static int pca9552_recv(I2CSlave *i2c)
> +static uint8_t pca9552_recv(I2CSlave *i2c)
>  {
>      PCA9552State *s = PCA9552(i2c);
>      uint8_t ret;
> diff --git a/hw/misc/tmp105.c b/hw/misc/tmp105.c
> index 0918f3a6ea..a4cae665b7 100644
> --- a/hw/misc/tmp105.c
> +++ b/hw/misc/tmp105.c
> @@ -147,7 +147,7 @@ static void tmp105_write(TMP105State *s)
>      }
>  }
>  
> -static int tmp105_rx(I2CSlave *i2c)
> +static uint8_t tmp105_rx(I2CSlave *i2c)
>  {
>      TMP105State *s = TMP105(i2c);
>  
> diff --git a/hw/misc/tmp421.c b/hw/misc/tmp421.c
> index c234044305..a75eb994a8 100644
> --- a/hw/misc/tmp421.c
> +++ b/hw/misc/tmp421.c
> @@ -249,7 +249,7 @@ static void tmp421_write(TMP421State *s)
>      }
>  }
>  
> -static int tmp421_rx(I2CSlave *i2c)
> +static uint8_t tmp421_rx(I2CSlave *i2c)
>  {
>      TMP421State *s = TMP421(i2c);
>  
> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
> index 27cd01e615..d1456dafbd 100644
> --- a/hw/nvram/eeprom_at24c.c
> +++ b/hw/nvram/eeprom_at24c.c
> @@ -74,10 +74,10 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
>  }
>  
>  static
> -int at24c_eeprom_recv(I2CSlave *s)
> +uint8_t at24c_eeprom_recv(I2CSlave *s)
>  {
>      EEPROMState *ee = AT24C_EE(s);
> -    int ret;
> +    uint8_t ret;
>  
>      ret = ee->mem[ee->cur];
>  
> diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c
> index 3849b74a68..03da75486b 100644
> --- a/hw/timer/ds1338.c
> +++ b/hw/timer/ds1338.c
> @@ -117,7 +117,7 @@ static int ds1338_event(I2CSlave *i2c, enum i2c_event event)
>      return 0;
>  }
>  
> -static int ds1338_recv(I2CSlave *i2c)
> +static uint8_t ds1338_recv(I2CSlave *i2c)
>  {
>      DS1338State *s = DS1338(i2c);
>      uint8_t res;
> diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
> index 734d7d95fc..c45b9297d8 100644
> --- a/hw/timer/m41t80.c
> +++ b/hw/timer/m41t80.c
> @@ -40,7 +40,7 @@ static int m41t80_send(I2CSlave *i2c, uint8_t data)
>      return 0;
>  }
>  
> -static int m41t80_recv(I2CSlave *i2c)
> +static uint8_t m41t80_recv(I2CSlave *i2c)
>  {
>      M41t80State *s = M41T80(i2c);
>      struct tm now;
> diff --git a/hw/timer/twl92230.c b/hw/timer/twl92230.c
> index 3b43b46199..659b216dca 100644
> --- a/hw/timer/twl92230.c
> +++ b/hw/timer/twl92230.c
> @@ -737,7 +737,7 @@ static int menelaus_tx(I2CSlave *i2c, uint8_t data)
>      return 0;
>  }
>  
> -static int menelaus_rx(I2CSlave *i2c)
> +static uint8_t menelaus_rx(I2CSlave *i2c)
>  {
>      MenelausState *s = TWL92230(i2c);
>  
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index 5dc166158b..75c5bd638b 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -33,10 +33,9 @@ typedef struct I2CSlaveClass {
>  
>      /*
>       * Slave to master.  This cannot fail, the device should always
> -     * return something here.  Negative values probably result in 0xff
> -     * and a possible log from the driver, and shouldn't be used.
> +     * return something here.

Maybe use simple comment as "Master receives the data sent by the Slave"

>       */
> -    int (*recv)(I2CSlave *s);
> +    uint8_t (*recv)(I2CSlave *s);
>  
>      /*
>       * Notify the slave of a bus state change.  For start event,
> @@ -78,7 +77,7 @@ void i2c_end_transfer(I2CBus *bus);
>  void i2c_nack(I2CBus *bus);
>  int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send);
>  int i2c_send(I2CBus *bus, uint8_t data);
> -int i2c_recv(I2CBus *bus);
> +uint8_t i2c_recv(I2CBus *bus);
>  
>  DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr);
>  

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH v3 03/16] arm:i2c: Don't mask return from i2c_recv()

[Philippe Mathieu-Daudé]

On 26/11/18 21:04, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> It can't fail, and now that it returns a uint8_t a 0xff mask
> is unnecessary.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/stellaris.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 6c69ce79b2..638b649911 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -811,7 +811,7 @@ static void stellaris_i2c_write(void *opaque, hwaddr offset,
>              /* TODO: Handle errors.  */
>              if (s->msa & 1) {
>                  /* Recv */
> -                s->mdr = i2c_recv(s->bus) & 0xff;
> +                s->mdr = i2c_recv(s->bus);
>              } else {
>                  /* Send */
>                  i2c_send(s->bus, s->mdr);
> 

This could be squashed in the previous patch.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH v3 06/16] i2c: Add a length check to the SMBus write handling

[Philippe Mathieu-Daudé]

On 26/11/18 21:04, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Avoid an overflow.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  hw/i2c/smbus_slave.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
> index 70ff29c095..d03f714608 100644
> --- a/hw/i2c/smbus_slave.c
> +++ b/hw/i2c/smbus_slave.c
> @@ -182,7 +182,11 @@ static int smbus_i2c_send(I2CSlave *s, uint8_t data)
>      switch (dev->mode) {
>      case SMBUS_WRITE_DATA:
>          DPRINTF("Write data %02x\n", data);
> -        dev->data_buf[dev->data_len++] = data;
> +        if (dev->data_len >= sizeof(dev->data_buf)) {
> +            BADF("Too many bytes sent\n");
> +        } else {
> +            dev->data_buf[dev->data_len++] = data;
> +        }
>          break;
>  
>      default:
> 

Re: [Qemu-devel] [PATCH v3 12/16] i2c:smbus_eeprom: Add normal type name and cast to smbus_eeprom.c

[Philippe Mathieu-Daudé]

On 26/11/18 21:04, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Create a type name and a cast macro and use those through the
> code.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  hw/i2c/smbus_eeprom.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index 4d25222e23..8d4eed129f 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -30,6 +30,11 @@
>  
>  //#define DEBUG
>  
> +#define TYPE_SMBUS_EEPROM "smbus-eeprom"
> +
> +#define SMBUS_EEPROM(obj) \
> +    OBJECT_CHECK(SMBusEEPROMDevice, (obj), TYPE_SMBUS_EEPROM)
> +
>  typedef struct SMBusEEPROMDevice {
>      SMBusDevice smbusdev;
>      void *data;
> @@ -38,7 +43,7 @@ typedef struct SMBusEEPROMDevice {
>  
>  static uint8_t eeprom_receive_byte(SMBusDevice *dev)
>  {
> -    SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
> +    SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>      uint8_t *data = eeprom->data;
>      uint8_t val = data[eeprom->offset++];
>  
> @@ -51,7 +56,7 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev)
>  
>  static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
>  {
> -    SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
> +    SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>      uint8_t *data = eeprom->data;
>  
>  #ifdef DEBUG
> @@ -73,7 +78,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
>  
>  static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>  {
> -    SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
> +    SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>  
>      eeprom->offset = 0;
>  }
> @@ -97,7 +102,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>  }
>  
>  static const TypeInfo smbus_eeprom_info = {
> -    .name          = "smbus-eeprom",
> +    .name          = TYPE_SMBUS_EEPROM,
>      .parent        = TYPE_SMBUS_DEVICE,
>      .instance_size = sizeof(SMBusEEPROMDevice),
>      .class_init    = smbus_eeprom_class_initfn,
> @@ -114,7 +119,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf)
>  {
>      DeviceState *dev;
>  
> -    dev = qdev_create((BusState *) smbus, "smbus-eeprom");
> +    dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM);
>      qdev_prop_set_uint8(dev, "address", address);
>      qdev_prop_set_ptr(dev, "data", eeprom_buf);
>      qdev_init_nofail(dev);
> 

Re: [Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom

[Philippe Mathieu-Daudé]

Hi Corey,

On 26/11/18 21:04, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Reset the contents to init data and reset the offset on a machine
> reset.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  hw/i2c/smbus_eeprom.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index a0dcadbd60..de3a492df4 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -107,7 +107,7 @@ static const VMStateDescription vmstate_smbus_eeprom = {
>      }
>  };
>  
> -static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
> +static void smbus_eeprom_reset(DeviceState *dev)
>  {
>      SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>  

'git diff -U4' also shows this line:

       memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);

I don't think this is correct.

One test I'd like to have is a machine booting, updating the EPROM then
rebooting calling hw reset() to use the new values (BIOS use this).

With this patch this won't work, you'll restore the EPROM content on
each machine reset.

I'd move the memcpy() call to the realize() function.

What do you think?

>      eeprom->offset = 0;

This is correct, the offset reset belongs to the reset() function.

Regards,

Phil.

>  }
>  
> +static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
> +{
> +    smbus_eeprom_reset(dev);
> +}
> +
>  static Property smbus_eeprom_properties[] = {
>      DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
>      DEFINE_PROP_END_OF_LIST(),
> @@ -126,6 +131,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>      SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
>  
>      dc->realize = smbus_eeprom_realize;
> +    dc->reset = smbus_eeprom_reset;
>      sc->receive_byte = eeprom_receive_byte;
>      sc->write_data = eeprom_write_data;
>      dc->props = smbus_eeprom_properties;
> 

Re: [Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom

[Corey Minyard]

On 11/26/18 2:42 PM, Philippe Mathieu-Daudé wrote:
> Hi Corey,
>
> On 26/11/18 21:04, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> Reset the contents to init data and reset the offset on a machine
>> reset.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
>>   hw/i2c/smbus_eeprom.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>> index a0dcadbd60..de3a492df4 100644
>> --- a/hw/i2c/smbus_eeprom.c
>> +++ b/hw/i2c/smbus_eeprom.c
>> @@ -107,7 +107,7 @@ static const VMStateDescription vmstate_smbus_eeprom = {
>>       }
>>   };
>>   
>> -static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>> +static void smbus_eeprom_reset(DeviceState *dev)
>>   {
>>       SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>>   
> 'git diff -U4' also shows this line:
>
>         memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
>
> I don't think this is correct.
>
> One test I'd like to have is a machine booting, updating the EPROM then
> rebooting calling hw reset() to use the new values (BIOS use this).
>
> With this patch this won't work, you'll restore the EPROM content on
> each machine reset.
>
> I'd move the memcpy() call to the realize() function.
>
> What do you think?

There was some debate on this in the earlier patch set.  The general 
principle
is that a reset is the same as starting up qemu from scratch, so I added 
this
code based on that principle.  But I'm not really sure.

>>       eeprom->offset = 0;
> This is correct, the offset reset belongs to the reset() function.

Actually, on a real system, a hardware reset will generally not affect the
eeprom current offset register.  So if we don't take the above code, then
IMHO this is wrong, too.

-corey


> Regards,
>
> Phil.
>
>>   }
>>   
>> +static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>> +{
>> +    smbus_eeprom_reset(dev);
>> +}
>> +
>>   static Property smbus_eeprom_properties[] = {
>>       DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
>>       DEFINE_PROP_END_OF_LIST(),
>> @@ -126,6 +131,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>>       SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
>>   
>>       dc->realize = smbus_eeprom_realize;
>> +    dc->reset = smbus_eeprom_reset;
>>       sc->receive_byte = eeprom_receive_byte;
>>       sc->write_data = eeprom_write_data;
>>       dc->props = smbus_eeprom_properties;
>>

Re: [Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom

[Philippe Mathieu-Daudé]

On 26/11/18 23:41, Corey Minyard wrote:
> On 11/26/18 2:42 PM, Philippe Mathieu-Daudé wrote:
>> Hi Corey,
>>
>> On 26/11/18 21:04, minyard@acm.org wrote:
>>> From: Corey Minyard <cminyard@mvista.com>
>>>
>>> Reset the contents to init data and reset the offset on a machine
>>> reset.
>>>
>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>> ---
>>>   hw/i2c/smbus_eeprom.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>>> index a0dcadbd60..de3a492df4 100644
>>> --- a/hw/i2c/smbus_eeprom.c
>>> +++ b/hw/i2c/smbus_eeprom.c
>>> @@ -107,7 +107,7 @@ static const VMStateDescription
>>> vmstate_smbus_eeprom = {
>>>       }
>>>   };
>>>   -static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>>> +static void smbus_eeprom_reset(DeviceState *dev)
>>>   {
>>>       SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>>>   
>> 'git diff -U4' also shows this line:
>>
>>         memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
>>
>> I don't think this is correct.
>>
>> One test I'd like to have is a machine booting, updating the EPROM then
>> rebooting calling hw reset() to use the new values (BIOS use this).
>>
>> With this patch this won't work, you'll restore the EPROM content on
>> each machine reset.
>>
>> I'd move the memcpy() call to the realize() function.
>>
>> What do you think?
> 
> There was some debate on this in the earlier patch set.  The general
> principle

Hmm I missed it and can't find it (quick basic search). I only find
references about VMState.

> is that a reset is the same as starting up qemu from scratch, so I added
> this
> code based on that principle.  But I'm not really sure.
> 
>>>       eeprom->offset = 0;
>> This is correct, the offset reset belongs to the reset() function.
> 
> Actually, on a real system, a hardware reset will generally not affect the
> eeprom current offset register.  So if we don't take the above code, then
> IMHO this is wrong, too.

Indeed cpu reset shouldn't affect the EEPROM, but a board powercycle would.

Maybe we can argue QEMU system reset doesn't work correctly yet to use
this feature. Personally I wouldn't expect the EEPROM content be be
reset after a reset, but maybe I should rely on a block backend for a
such feature, and not the current simple approach.

> 
> -corey
> 
> 
>> Regards,
>>
>> Phil.
>>
>>>   }
>>>   +static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    smbus_eeprom_reset(dev);
>>> +}
>>> +
>>>   static Property smbus_eeprom_properties[] = {
>>>       DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
>>>       DEFINE_PROP_END_OF_LIST(),
>>> @@ -126,6 +131,7 @@ static void smbus_eeprom_class_initfn(ObjectClass
>>> *klass, void *data)
>>>       SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
>>>         dc->realize = smbus_eeprom_realize;
>>> +    dc->reset = smbus_eeprom_reset;
>>>       sc->receive_byte = eeprom_receive_byte;
>>>       sc->write_data = eeprom_write_data;
>>>       dc->props = smbus_eeprom_properties;
>>>
> 

Re: [Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom

[Corey Minyard]

On 11/26/18 5:01 PM, Philippe Mathieu-Daudé wrote:
> On 26/11/18 23:41, Corey Minyard wrote:
>> On 11/26/18 2:42 PM, Philippe Mathieu-Daudé wrote:
>>> Hi Corey,
>>>
>>> On 26/11/18 21:04, minyard@acm.org wrote:
>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>
>>>> Reset the contents to init data and reset the offset on a machine
>>>> reset.
>>>>
>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>> ---
>>>>    hw/i2c/smbus_eeprom.c | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>>>> index a0dcadbd60..de3a492df4 100644
>>>> --- a/hw/i2c/smbus_eeprom.c
>>>> +++ b/hw/i2c/smbus_eeprom.c
>>>> @@ -107,7 +107,7 @@ static const VMStateDescription
>>>> vmstate_smbus_eeprom = {
>>>>        }
>>>>    };
>>>>    -static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>>>> +static void smbus_eeprom_reset(DeviceState *dev)
>>>>    {
>>>>        SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>>>>    
>>> 'git diff -U4' also shows this line:
>>>
>>>          memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
>>>
>>> I don't think this is correct.
>>>
>>> One test I'd like to have is a machine booting, updating the EPROM then
>>> rebooting calling hw reset() to use the new values (BIOS use this).
>>>
>>> With this patch this won't work, you'll restore the EPROM content on
>>> each machine reset.
>>>
>>> I'd move the memcpy() call to the realize() function.
>>>
>>> What do you think?
>> There was some debate on this in the earlier patch set.  The general
>> principle
> Hmm I missed it and can't find it (quick basic search). I only find
> references about VMState.


It starts at 
http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg01737.html

The patch set was fairly different at that point.


>> is that a reset is the same as starting up qemu from scratch, so I added
>> this
>> code based on that principle.  But I'm not really sure.
>>
>>>>        eeprom->offset = 0;
>>> This is correct, the offset reset belongs to the reset() function.
>> Actually, on a real system, a hardware reset will generally not affect the
>> eeprom current offset register.  So if we don't take the above code, then
>> IMHO this is wrong, too.
> Indeed cpu reset shouldn't affect the EEPROM, but a board powercycle would.
>
> Maybe we can argue QEMU system reset doesn't work correctly yet to use
> this feature. Personally I wouldn't expect the EEPROM content be be
> reset after a reset, but maybe I should rely on a block backend for a
> such feature, and not the current simple approach.
>
Yeah, it was mentioned that to do this correctly would require a block 
backend.
I'll let others comment on the correctness of this, I guess.  It's a 
separate patch
so it can be easily dropped.

The current code is far too broken for anyone to be using it, so we won't be
breaking any current users, I don't think.

-corey

>> -corey
>>
>>
>>> Regards,
>>>
>>> Phil.
>>>
>>>>    }
>>>>    +static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>>>> +{
>>>> +    smbus_eeprom_reset(dev);
>>>> +}
>>>> +
>>>>    static Property smbus_eeprom_properties[] = {
>>>>        DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
>>>>        DEFINE_PROP_END_OF_LIST(),
>>>> @@ -126,6 +131,7 @@ static void smbus_eeprom_class_initfn(ObjectClass
>>>> *klass, void *data)
>>>>        SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
>>>>          dc->realize = smbus_eeprom_realize;
>>>> +    dc->reset = smbus_eeprom_reset;
>>>>        sc->receive_byte = eeprom_receive_byte;
>>>>        sc->write_data = eeprom_write_data;
>>>>        dc->props = smbus_eeprom_properties;
>>>>

Re: [Qemu-devel] [PATCH v3 02/16] i2c: have I2C receive operation return uint8_t

[Corey Minyard]

On 11/26/18 2:23 PM, Philippe Mathieu-Daudé wrote:
> Hi Corey,
>
> On 26/11/18 21:04, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> It is never supposed to fail and cannot return an error, so just
>> have it return the proper type.  Have it return 0xff on nothing
>> available, since that's what would happen on a real bus.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   hw/arm/pxa2xx.c         |  2 +-
>>   hw/arm/tosa.c           |  4 ++--
>>   hw/arm/z2.c             |  2 +-
>>   hw/audio/wm8750.c       |  2 +-
>>   hw/display/sii9022.c    |  2 +-
>>   hw/display/ssd0303.c    |  4 ++--
>>   hw/gpio/max7310.c       |  2 +-
>>   hw/i2c/core.c           | 32 +++++++++++++-------------------
>>   hw/i2c/i2c-ddc.c        |  2 +-
>>   hw/i2c/smbus_slave.c    |  4 ++--
>>   hw/input/lm832x.c       |  2 +-
>>   hw/misc/pca9552.c       |  2 +-
>>   hw/misc/tmp105.c        |  2 +-
>>   hw/misc/tmp421.c        |  2 +-
>>   hw/nvram/eeprom_at24c.c |  4 ++--
>>   hw/timer/ds1338.c       |  2 +-
>>   hw/timer/m41t80.c       |  2 +-
>>   hw/timer/twl92230.c     |  2 +-
>>   include/hw/i2c/i2c.h    |  7 +++----
>>   19 files changed, 37 insertions(+), 44 deletions(-)
>>
>> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
>> index f598a1c053..3d7c88910e 100644
>> --- a/hw/arm/pxa2xx.c
>> +++ b/hw/arm/pxa2xx.c
>> @@ -1286,7 +1286,7 @@ static int pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
>>       return 0;
>>   }
>>   
>> -static int pxa2xx_i2c_rx(I2CSlave *i2c)
>> +static uint8_t pxa2xx_i2c_rx(I2CSlave *i2c)
>>   {
>>       PXA2xxI2CSlaveState *slave = PXA2XX_I2C_SLAVE(i2c);
>>       PXA2xxI2CState *s = slave->host;
>> diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
>> index 7a925fa5e6..eef9d427e7 100644
>> --- a/hw/arm/tosa.c
>> +++ b/hw/arm/tosa.c
>> @@ -197,10 +197,10 @@ static int tosa_dac_event(I2CSlave *i2c, enum i2c_event event)
>>       return 0;
>>   }
>>   
>> -static int tosa_dac_recv(I2CSlave *s)
>> +static uint8_t tosa_dac_recv(I2CSlave *s)
>>   {
>>       printf("%s: recv not supported!!!\n", __func__);
>> -    return -1;
>> +    return 0xff;
>>   }
>>   
>>   static void tosa_tg_init(PXA2xxState *cpu)
>> diff --git a/hw/arm/z2.c b/hw/arm/z2.c
>> index 697a822f1e..6f18d924df 100644
>> --- a/hw/arm/z2.c
>> +++ b/hw/arm/z2.c
>> @@ -243,7 +243,7 @@ static int aer915_event(I2CSlave *i2c, enum i2c_event event)
>>       return 0;
>>   }
>>   
>> -static int aer915_recv(I2CSlave *slave)
>> +static uint8_t aer915_recv(I2CSlave *slave)
>>   {
>>       AER915State *s = AER915(slave);
>>       int retval = 0x00;
>> diff --git a/hw/audio/wm8750.c b/hw/audio/wm8750.c
>> index f4aa838f62..169b006ade 100644
>> --- a/hw/audio/wm8750.c
>> +++ b/hw/audio/wm8750.c
>> @@ -561,7 +561,7 @@ static int wm8750_tx(I2CSlave *i2c, uint8_t data)
>>       return 0;
>>   }
>>   
>> -static int wm8750_rx(I2CSlave *i2c)
>> +static uint8_t wm8750_rx(I2CSlave *i2c)
>>   {
>>       return 0x00;
>>   }
>> diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
>> index eaf11a6e7b..9994385c35 100644
>> --- a/hw/display/sii9022.c
>> +++ b/hw/display/sii9022.c
>> @@ -79,7 +79,7 @@ static int sii9022_event(I2CSlave *i2c, enum i2c_event event)
>>       return 0;
>>   }
>>   
>> -static int sii9022_rx(I2CSlave *i2c)
>> +static uint8_t sii9022_rx(I2CSlave *i2c)
>>   {
>>       sii9022_state *s = SII9022(i2c);
>>       uint8_t res = 0x00;
>> diff --git a/hw/display/ssd0303.c b/hw/display/ssd0303.c
>> index eb90ba26be..8edf34986c 100644
>> --- a/hw/display/ssd0303.c
>> +++ b/hw/display/ssd0303.c
>> @@ -62,10 +62,10 @@ typedef struct {
>>       uint8_t framebuffer[132*8];
>>   } ssd0303_state;
>>   
>> -static int ssd0303_recv(I2CSlave *i2c)
>> +static uint8_t ssd0303_recv(I2CSlave *i2c)
>>   {
>>       BADF("Reads not implemented\n");
>> -    return -1;
>> +    return 0xff;
>>   }
>>   
>>   static int ssd0303_send(I2CSlave *i2c, uint8_t data)
>> diff --git a/hw/gpio/max7310.c b/hw/gpio/max7310.c
>> index a560e3afd2..f35a930276 100644
>> --- a/hw/gpio/max7310.c
>> +++ b/hw/gpio/max7310.c
>> @@ -39,7 +39,7 @@ static void max7310_reset(DeviceState *dev)
>>       s->command = 0x00;
>>   }
>>   
>> -static int max7310_rx(I2CSlave *i2c)
>> +static uint8_t max7310_rx(I2CSlave *i2c)
>>   {
>>       MAX7310State *s = MAX7310(i2c);
>>   
>> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
>> index b54725985a..15237ad073 100644
>> --- a/hw/i2c/core.c
>> +++ b/hw/i2c/core.c
>> @@ -191,23 +191,17 @@ int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send)
> i2c_send_recv() could benefit the same improvement.
>
I thought some about that, but the send function has to be able to return -1
for a NACK.  It would be nice to have separate send and recv functions, but
several host devices use i2c_send_recv() directly.


>>           }
>>           return ret ? -1 : 0;
>>       } else {
>> -        if ((QLIST_EMPTY(&bus->current_devs)) || (bus->broadcast)) {
>> -            return -1;
>> -        }
>> -
>> -        sc = I2C_SLAVE_GET_CLASS(QLIST_FIRST(&bus->current_devs)->elt);
>> -        if (sc->recv) {
>> -            s = QLIST_FIRST(&bus->current_devs)->elt;
>> -            ret = sc->recv(s);
>> -            trace_i2c_recv(s->address, ret);
>> -            if (ret < 0) {
>> -                return ret;
>> -            } else {
>> -                *data = ret;
>> -                return 0;
>> +        ret = 0xff;
>> +        if (!QLIST_EMPTY(&bus->current_devs) && !bus->broadcast) {
>> +            sc = I2C_SLAVE_GET_CLASS(QLIST_FIRST(&bus->current_devs)->elt);
>> +            if (sc->recv) {
>> +                s = QLIST_FIRST(&bus->current_devs)->elt;
>> +                ret = sc->recv(s);
>> +                trace_i2c_recv(s->address, ret);
>>               }
>>           }
>> -        return -1;
>> +        *data = ret;
>> +        return 0;
>>       }
>>   }
>>   
>> @@ -216,12 +210,12 @@ int i2c_send(I2CBus *bus, uint8_t data)
>>       return i2c_send_recv(bus, &data, true);
>>   }
>>   
>> -int i2c_recv(I2CBus *bus)
>> +uint8_t i2c_recv(I2CBus *bus)
>>   {
>> -    uint8_t data;
>> -    int ret = i2c_send_recv(bus, &data, false);
>> +    uint8_t data = 0xff;
>>   
>> -    return ret < 0 ? ret : data;
>> +    i2c_send_recv(bus, &data, false);
>> +    return data;
>>   }
>>   
>>   void i2c_nack(I2CBus *bus)
>> diff --git a/hw/i2c/i2c-ddc.c b/hw/i2c/i2c-ddc.c
>> index be34fe072c..95325358db 100644
>> --- a/hw/i2c/i2c-ddc.c
>> +++ b/hw/i2c/i2c-ddc.c
>> @@ -51,7 +51,7 @@ static int i2c_ddc_event(I2CSlave *i2c, enum i2c_event event)
>>       return 0;
>>   }
>>   
>> -static int i2c_ddc_rx(I2CSlave *i2c)
>> +static uint8_t i2c_ddc_rx(I2CSlave *i2c)
>>   {
>>       I2CDDCState *s = I2CDDC(i2c);
>>   
>> diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
>> index 1e734752d7..549e7ae933 100644
>> --- a/hw/i2c/smbus_slave.c
>> +++ b/hw/i2c/smbus_slave.c
>> @@ -156,11 +156,11 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
>>       return 0;
>>   }
>>   
>> -static int smbus_i2c_recv(I2CSlave *s)
>> +static uint8_t smbus_i2c_recv(I2CSlave *s)
>>   {
>>       SMBusDevice *dev = SMBUS_DEVICE(s);
>>       SMBusDeviceClass *sc = SMBUS_DEVICE_GET_CLASS(dev);
>> -    int ret;
>> +    uint8_t ret;
>>   
>>       switch (dev->mode) {
>>       case SMBUS_RECV_BYTE:
>> diff --git a/hw/input/lm832x.c b/hw/input/lm832x.c
>> index 74da30d9ca..9ae037953d 100644
>> --- a/hw/input/lm832x.c
>> +++ b/hw/input/lm832x.c
>> @@ -401,7 +401,7 @@ static int lm_i2c_event(I2CSlave *i2c, enum i2c_event event)
>>       return 0;
>>   }
>>   
>> -static int lm_i2c_rx(I2CSlave *i2c)
>> +static uint8_t lm_i2c_rx(I2CSlave *i2c)
>>   {
>>       LM823KbdState *s = LM8323(i2c);
>>   
>> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
>> index 9775d5274a..7325d3f287 100644
>> --- a/hw/misc/pca9552.c
>> +++ b/hw/misc/pca9552.c
>> @@ -115,7 +115,7 @@ static void pca9552_autoinc(PCA9552State *s)
>>       }
>>   }
>>   
>> -static int pca9552_recv(I2CSlave *i2c)
>> +static uint8_t pca9552_recv(I2CSlave *i2c)
>>   {
>>       PCA9552State *s = PCA9552(i2c);
>>       uint8_t ret;
>> diff --git a/hw/misc/tmp105.c b/hw/misc/tmp105.c
>> index 0918f3a6ea..a4cae665b7 100644
>> --- a/hw/misc/tmp105.c
>> +++ b/hw/misc/tmp105.c
>> @@ -147,7 +147,7 @@ static void tmp105_write(TMP105State *s)
>>       }
>>   }
>>   
>> -static int tmp105_rx(I2CSlave *i2c)
>> +static uint8_t tmp105_rx(I2CSlave *i2c)
>>   {
>>       TMP105State *s = TMP105(i2c);
>>   
>> diff --git a/hw/misc/tmp421.c b/hw/misc/tmp421.c
>> index c234044305..a75eb994a8 100644
>> --- a/hw/misc/tmp421.c
>> +++ b/hw/misc/tmp421.c
>> @@ -249,7 +249,7 @@ static void tmp421_write(TMP421State *s)
>>       }
>>   }
>>   
>> -static int tmp421_rx(I2CSlave *i2c)
>> +static uint8_t tmp421_rx(I2CSlave *i2c)
>>   {
>>       TMP421State *s = TMP421(i2c);
>>   
>> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
>> index 27cd01e615..d1456dafbd 100644
>> --- a/hw/nvram/eeprom_at24c.c
>> +++ b/hw/nvram/eeprom_at24c.c
>> @@ -74,10 +74,10 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
>>   }
>>   
>>   static
>> -int at24c_eeprom_recv(I2CSlave *s)
>> +uint8_t at24c_eeprom_recv(I2CSlave *s)
>>   {
>>       EEPROMState *ee = AT24C_EE(s);
>> -    int ret;
>> +    uint8_t ret;
>>   
>>       ret = ee->mem[ee->cur];
>>   
>> diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c
>> index 3849b74a68..03da75486b 100644
>> --- a/hw/timer/ds1338.c
>> +++ b/hw/timer/ds1338.c
>> @@ -117,7 +117,7 @@ static int ds1338_event(I2CSlave *i2c, enum i2c_event event)
>>       return 0;
>>   }
>>   
>> -static int ds1338_recv(I2CSlave *i2c)
>> +static uint8_t ds1338_recv(I2CSlave *i2c)
>>   {
>>       DS1338State *s = DS1338(i2c);
>>       uint8_t res;
>> diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
>> index 734d7d95fc..c45b9297d8 100644
>> --- a/hw/timer/m41t80.c
>> +++ b/hw/timer/m41t80.c
>> @@ -40,7 +40,7 @@ static int m41t80_send(I2CSlave *i2c, uint8_t data)
>>       return 0;
>>   }
>>   
>> -static int m41t80_recv(I2CSlave *i2c)
>> +static uint8_t m41t80_recv(I2CSlave *i2c)
>>   {
>>       M41t80State *s = M41T80(i2c);
>>       struct tm now;
>> diff --git a/hw/timer/twl92230.c b/hw/timer/twl92230.c
>> index 3b43b46199..659b216dca 100644
>> --- a/hw/timer/twl92230.c
>> +++ b/hw/timer/twl92230.c
>> @@ -737,7 +737,7 @@ static int menelaus_tx(I2CSlave *i2c, uint8_t data)
>>       return 0;
>>   }
>>   
>> -static int menelaus_rx(I2CSlave *i2c)
>> +static uint8_t menelaus_rx(I2CSlave *i2c)
>>   {
>>       MenelausState *s = TWL92230(i2c);
>>   
>> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
>> index 5dc166158b..75c5bd638b 100644
>> --- a/include/hw/i2c/i2c.h
>> +++ b/include/hw/i2c/i2c.h
>> @@ -33,10 +33,9 @@ typedef struct I2CSlaveClass {
>>   
>>       /*
>>        * Slave to master.  This cannot fail, the device should always
>> -     * return something here.  Negative values probably result in 0xff
>> -     * and a possible log from the driver, and shouldn't be used.
>> +     * return something here.
> Maybe use simple comment as "Master receives the data sent by the Slave"

I'm not sure I follow.  I think it's important to say that the device 
must return something,
Are you talking about the "Slave to master" part?

Thanks,

-corey


>>        */
>> -    int (*recv)(I2CSlave *s);
>> +    uint8_t (*recv)(I2CSlave *s);
>>   
>>       /*
>>        * Notify the slave of a bus state change.  For start event,
>> @@ -78,7 +77,7 @@ void i2c_end_transfer(I2CBus *bus);
>>   void i2c_nack(I2CBus *bus);
>>   int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send);
>>   int i2c_send(I2CBus *bus, uint8_t data);
>> -int i2c_recv(I2CBus *bus);
>> +uint8_t i2c_recv(I2CBus *bus);
>>   
>>   DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr);
>>   
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>

Re: [Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom

[Philippe Mathieu-Daudé]

On 27/11/18 0:58, Corey Minyard wrote:
> On 11/26/18 5:01 PM, Philippe Mathieu-Daudé wrote:
>> On 26/11/18 23:41, Corey Minyard wrote:
>>> On 11/26/18 2:42 PM, Philippe Mathieu-Daudé wrote:
>>>> Hi Corey,
>>>>
>>>> On 26/11/18 21:04, minyard@acm.org wrote:
>>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>>
>>>>> Reset the contents to init data and reset the offset on a machine
>>>>> reset.
>>>>>
>>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>>> ---
>>>>>    hw/i2c/smbus_eeprom.c | 8 +++++++-
>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>>>>> index a0dcadbd60..de3a492df4 100644
>>>>> --- a/hw/i2c/smbus_eeprom.c
>>>>> +++ b/hw/i2c/smbus_eeprom.c
>>>>> @@ -107,7 +107,7 @@ static const VMStateDescription
>>>>> vmstate_smbus_eeprom = {
>>>>>        }
>>>>>    };
>>>>>    -static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>>>>> +static void smbus_eeprom_reset(DeviceState *dev)
>>>>>    {
>>>>>        SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>>>>>    
>>>> 'git diff -U4' also shows this line:
>>>>
>>>>          memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
>>>>
>>>> I don't think this is correct.
>>>>
>>>> One test I'd like to have is a machine booting, updating the EPROM then
>>>> rebooting calling hw reset() to use the new values (BIOS use this).
>>>>
>>>> With this patch this won't work, you'll restore the EPROM content on
>>>> each machine reset.
>>>>
>>>> I'd move the memcpy() call to the realize() function.
>>>>
>>>> What do you think?
>>> There was some debate on this in the earlier patch set.  The general
>>> principle
>> Hmm I missed it and can't find it (quick basic search). I only find
>> references about VMState.
> 
> 
> It starts at
> http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg01737.html

Thank you, very helpful.

> 
> The patch set was fairly different at that point.
> 
> 
>>> is that a reset is the same as starting up qemu from scratch, so I added
>>> this
>>> code based on that principle.  But I'm not really sure.
>>>
>>>>>        eeprom->offset = 0;
>>>> This is correct, the offset reset belongs to the reset() function.
>>> Actually, on a real system, a hardware reset will generally not
>>> affect the
>>> eeprom current offset register.  So if we don't take the above code,
>>> then
>>> IMHO this is wrong, too.
>> Indeed cpu reset shouldn't affect the EEPROM, but a board powercycle
>> would.
>>
>> Maybe we can argue QEMU system reset doesn't work correctly yet to use
>> this feature. Personally I wouldn't expect the EEPROM content be be
>> reset after a reset, but maybe I should rely on a block backend for a
>> such feature, and not the current simple approach.
>>
> Yeah, it was mentioned that to do this correctly would require a block
> backend.
> I'll let others comment on the correctness of this, I guess.  It's a
> separate patch
> so it can be easily dropped.

Since modelling eeprom data retention on hardware reset isn't the goal
of your series, we can have a consensus, adding a comment explaining why
we choose this simpler way, and eeprom retention simulation requieres
more work with block backend.

> 
> The current code is far too broken for anyone to be using it, so we
> won't be
> breaking any current users, I don't think.
> 
> -corey
> 
>>> -corey
>>>
>>>
>>>> Regards,
>>>>
>>>> Phil.
>>>>
>>>>>    }
>>>>>    +static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>>>>> +{
>>>>> +    smbus_eeprom_reset(dev);
>>>>> +}
>>>>> +
>>>>>    static Property smbus_eeprom_properties[] = {
>>>>>        DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
>>>>>        DEFINE_PROP_END_OF_LIST(),
>>>>> @@ -126,6 +131,7 @@ static void smbus_eeprom_class_initfn(ObjectClass
>>>>> *klass, void *data)
>>>>>        SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
>>>>>          dc->realize = smbus_eeprom_realize;
>>>>> +    dc->reset = smbus_eeprom_reset;
>>>>>        sc->receive_byte = eeprom_receive_byte;
>>>>>        sc->write_data = eeprom_write_data;
>>>>>        dc->props = smbus_eeprom_properties;
>>>>>
> 

Re: [Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom

[Corey Minyard]

On 11/27/18 4:54 AM, Philippe Mathieu-Daudé wrote:
> On 27/11/18 0:58, Corey Minyard wrote:
>> On 11/26/18 5:01 PM, Philippe Mathieu-Daudé wrote:
>>> On 26/11/18 23:41, Corey Minyard wrote:
>>>> On 11/26/18 2:42 PM, Philippe Mathieu-Daudé wrote:
>>>>> Hi Corey,
>>>>>
>>>>> On 26/11/18 21:04, minyard@acm.org wrote:
>>>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>>>
>>>>>> Reset the contents to init data and reset the offset on a machine
>>>>>> reset.
>>>>>>
>>>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>>>> ---
>>>>>>     hw/i2c/smbus_eeprom.c | 8 +++++++-
>>>>>>     1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>>>>>> index a0dcadbd60..de3a492df4 100644
>>>>>> --- a/hw/i2c/smbus_eeprom.c
>>>>>> +++ b/hw/i2c/smbus_eeprom.c
>>>>>> @@ -107,7 +107,7 @@ static const VMStateDescription
>>>>>> vmstate_smbus_eeprom = {
>>>>>>         }
>>>>>>     };
>>>>>>     -static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>>>>>> +static void smbus_eeprom_reset(DeviceState *dev)
>>>>>>     {
>>>>>>         SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>>>>>>     
>>>>> 'git diff -U4' also shows this line:
>>>>>
>>>>>           memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
>>>>>
>>>>> I don't think this is correct.
>>>>>
>>>>> One test I'd like to have is a machine booting, updating the EPROM then
>>>>> rebooting calling hw reset() to use the new values (BIOS use this).
>>>>>
>>>>> With this patch this won't work, you'll restore the EPROM content on
>>>>> each machine reset.
>>>>>
>>>>> I'd move the memcpy() call to the realize() function.
>>>>>
>>>>> What do you think?
>>>> There was some debate on this in the earlier patch set.  The general
>>>> principle
>>> Hmm I missed it and can't find it (quick basic search). I only find
>>> references about VMState.
>>
>> It starts at
>> http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg01737.html
> Thank you, very helpful.
>
>> The patch set was fairly different at that point.
>>
>>
>>>> is that a reset is the same as starting up qemu from scratch, so I added
>>>> this
>>>> code based on that principle.  But I'm not really sure.
>>>>
>>>>>>         eeprom->offset = 0;
>>>>> This is correct, the offset reset belongs to the reset() function.
>>>> Actually, on a real system, a hardware reset will generally not
>>>> affect the
>>>> eeprom current offset register.  So if we don't take the above code,
>>>> then
>>>> IMHO this is wrong, too.
>>> Indeed cpu reset shouldn't affect the EEPROM, but a board powercycle
>>> would.
>>>
>>> Maybe we can argue QEMU system reset doesn't work correctly yet to use
>>> this feature. Personally I wouldn't expect the EEPROM content be be
>>> reset after a reset, but maybe I should rely on a block backend for a
>>> such feature, and not the current simple approach.
>>>
>> Yeah, it was mentioned that to do this correctly would require a block
>> backend.
>> I'll let others comment on the correctness of this, I guess.  It's a
>> separate patch
>> so it can be easily dropped.
> Since modelling eeprom data retention on hardware reset isn't the goal
> of your series, we can have a consensus, adding a comment explaining why
> we choose this simpler way, and eeprom retention simulation requieres
> more work with block backend.

Good idea, I've done that.  Thanks for the reviews.  Is this a 
"reviewed-by"?


-corey

>> The current code is far too broken for anyone to be using it, so we
>> won't be
>> breaking any current users, I don't think.
>>
>> -corey
>>
>>>> -corey
>>>>
>>>>
>>>>> Regards,
>>>>>
>>>>> Phil.
>>>>>
>>>>>>     }
>>>>>>     +static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>>>>>> +{
>>>>>> +    smbus_eeprom_reset(dev);
>>>>>> +}
>>>>>> +
>>>>>>     static Property smbus_eeprom_properties[] = {
>>>>>>         DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
>>>>>>         DEFINE_PROP_END_OF_LIST(),
>>>>>> @@ -126,6 +131,7 @@ static void smbus_eeprom_class_initfn(ObjectClass
>>>>>> *klass, void *data)
>>>>>>         SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
>>>>>>           dc->realize = smbus_eeprom_realize;
>>>>>> +    dc->reset = smbus_eeprom_reset;
>>>>>>         sc->receive_byte = eeprom_receive_byte;
>>>>>>         sc->write_data = eeprom_write_data;
>>>>>>         dc->props = smbus_eeprom_properties;
>>>>>>

Re: [Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom

[Philippe Mathieu-Daudé]

On 27/11/18 13:58, Corey Minyard wrote:
> On 11/27/18 4:54 AM, Philippe Mathieu-Daudé wrote:
>> On 27/11/18 0:58, Corey Minyard wrote:
>>> On 11/26/18 5:01 PM, Philippe Mathieu-Daudé wrote:
>>>> On 26/11/18 23:41, Corey Minyard wrote:
>>>>> On 11/26/18 2:42 PM, Philippe Mathieu-Daudé wrote:
>>>>>> Hi Corey,
>>>>>>
>>>>>> On 26/11/18 21:04, minyard@acm.org wrote:
>>>>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>>>>
>>>>>>> Reset the contents to init data and reset the offset on a machine
>>>>>>> reset.
>>>>>>>
>>>>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>>>>> ---
>>>>>>>     hw/i2c/smbus_eeprom.c | 8 +++++++-
>>>>>>>     1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>>>>>>> index a0dcadbd60..de3a492df4 100644
>>>>>>> --- a/hw/i2c/smbus_eeprom.c
>>>>>>> +++ b/hw/i2c/smbus_eeprom.c
>>>>>>> @@ -107,7 +107,7 @@ static const VMStateDescription
>>>>>>> vmstate_smbus_eeprom = {
>>>>>>>         }
>>>>>>>     };
>>>>>>>     -static void smbus_eeprom_realize(DeviceState *dev, Error
>>>>>>> **errp)
>>>>>>> +static void smbus_eeprom_reset(DeviceState *dev)
>>>>>>>     {
>>>>>>>         SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>>>>>>>     
>>>>>> 'git diff -U4' also shows this line:
>>>>>>
>>>>>>           memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
>>>>>>
>>>>>> I don't think this is correct.
>>>>>>
>>>>>> One test I'd like to have is a machine booting, updating the EPROM
>>>>>> then
>>>>>> rebooting calling hw reset() to use the new values (BIOS use this).
>>>>>>
>>>>>> With this patch this won't work, you'll restore the EPROM content on
>>>>>> each machine reset.
>>>>>>
>>>>>> I'd move the memcpy() call to the realize() function.
>>>>>>
>>>>>> What do you think?
>>>>> There was some debate on this in the earlier patch set.  The general
>>>>> principle
>>>> Hmm I missed it and can't find it (quick basic search). I only find
>>>> references about VMState.
>>>
>>> It starts at
>>> http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg01737.html
>> Thank you, very helpful.
>>
>>> The patch set was fairly different at that point.
>>>
>>>
>>>>> is that a reset is the same as starting up qemu from scratch, so I
>>>>> added
>>>>> this
>>>>> code based on that principle.  But I'm not really sure.
>>>>>
>>>>>>>         eeprom->offset = 0;
>>>>>> This is correct, the offset reset belongs to the reset() function.
>>>>> Actually, on a real system, a hardware reset will generally not
>>>>> affect the
>>>>> eeprom current offset register.  So if we don't take the above code,
>>>>> then
>>>>> IMHO this is wrong, too.
>>>> Indeed cpu reset shouldn't affect the EEPROM, but a board powercycle
>>>> would.
>>>>
>>>> Maybe we can argue QEMU system reset doesn't work correctly yet to use
>>>> this feature. Personally I wouldn't expect the EEPROM content be be
>>>> reset after a reset, but maybe I should rely on a block backend for a
>>>> such feature, and not the current simple approach.
>>>>
>>> Yeah, it was mentioned that to do this correctly would require a block
>>> backend.
>>> I'll let others comment on the correctness of this, I guess.  It's a
>>> separate patch
>>> so it can be easily dropped.
>> Since modelling eeprom data retention on hardware reset isn't the goal
>> of your series, we can have a consensus, adding a comment explaining why
>> we choose this simpler way, and eeprom retention simulation requieres
>> more work with block backend.
> 
> Good idea, I've done that.  Thanks for the reviews.  Is this a
> "reviewed-by"?

Why not ;)

With a comment:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> 
> -corey
> 
>>> The current code is far too broken for anyone to be using it, so we
>>> won't be
>>> breaking any current users, I don't think.
>>>
>>> -corey
>>>
>>>>> -corey
>>>>>
>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Phil.
>>>>>>
>>>>>>>     }
>>>>>>>     +static void smbus_eeprom_realize(DeviceState *dev, Error
>>>>>>> **errp)
>>>>>>> +{
>>>>>>> +    smbus_eeprom_reset(dev);
>>>>>>> +}
>>>>>>> +
>>>>>>>     static Property smbus_eeprom_properties[] = {
>>>>>>>         DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
>>>>>>>         DEFINE_PROP_END_OF_LIST(),
>>>>>>> @@ -126,6 +131,7 @@ static void
>>>>>>> smbus_eeprom_class_initfn(ObjectClass
>>>>>>> *klass, void *data)
>>>>>>>         SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
>>>>>>>           dc->realize = smbus_eeprom_realize;
>>>>>>> +    dc->reset = smbus_eeprom_reset;
>>>>>>>         sc->receive_byte = eeprom_receive_byte;
>>>>>>>         sc->write_data = eeprom_write_data;
>>>>>>>         dc->props = smbus_eeprom_properties;
>>>>>>>
> 

Re: [Qemu-devel] [PATCH v3 08/16] boards.h: Ignore migration for SMBus devices on older machines

[Dr. David Alan Gilbert]

* minyard@acm.org (minyard@acm.org) wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Migration capability is being added for pm_smbus and SMBus devices.
> This change will allow backwards compatibility to be kept when
> migrating back to an old qemu version.  Add a bool to the machine
> class tho keep smbus migration from happening.  Future changes
> will use this.

Still needs the 3.0 and 3.1, but other than that:


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> ---
>  hw/i386/pc_piix.c   | 1 +
>  hw/i386/pc_q35.c    | 1 +
>  include/hw/boards.h | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index cb28227cc3..3d1ccb1af1 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -443,6 +443,7 @@ static void pc_i440fx_2_12_machine_options(MachineClass *m)
>      pc_i440fx_3_0_machine_options(m);
>      m->is_default = 0;
>      m->alias = NULL;
> +    m->smbus_no_migration_support = true;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
>  }
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 90e88c9b28..0c6fca6a40 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -324,6 +324,7 @@ static void pc_q35_2_12_machine_options(MachineClass *m)
>  {
>      pc_q35_3_0_machine_options(m);
>      m->alias = NULL;
> +    m->smbus_no_migration_support = true;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
>  }
>  
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index f82f28468b..65314fbe2a 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -207,6 +207,7 @@ struct MachineClass {
>      void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
>                                   int nb_nodes, ram_addr_t size);
>      bool ignore_boot_device_suffixes;
> +    bool smbus_no_migration_support;
>  
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                             DeviceState *dev);
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 09/16] migration: Add a VMSTATE_BOOL_TEST() macro

[Dr. David Alan Gilbert]

* minyard@acm.org (minyard@acm.org) wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> This will be needed by coming I2C changes.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  include/migration/vmstate.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 2b501d0466..0861e18138 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -850,6 +850,9 @@ extern const VMStateInfo vmstate_info_qtailq;
>  #define VMSTATE_INT32_POSITIVE_LE(_f, _s)                             \
>      VMSTATE_SINGLE(_f, _s, 0, vmstate_info_int32_le, int32_t)
>  
> +#define VMSTATE_BOOL_TEST(_f, _s, _t)                               \
> +    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_info_bool, bool)
> +
>  #define VMSTATE_INT8_TEST(_f, _s, _t)                               \
>      VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_info_int8, int8_t)
>  
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 10/16] i2c:pm_smbus: Fix state transfer

[Dr. David Alan Gilbert]

* minyard@acm.org (minyard@acm.org) wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Transfer the state information for the SMBus registers and
> internal data so it will work on a VM transfer.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hw/acpi/piix4.c           |  7 +++++++
>  hw/i2c/pm_smbus.c         | 31 +++++++++++++++++++++++++++++++
>  hw/i2c/smbus_ich9.c       | 10 +++++++++-
>  include/hw/i2c/pm_smbus.h |  9 +++++++++
>  4 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 2f4dd03b83..91fe4821d3 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -302,6 +302,11 @@ static const VMStateDescription vmstate_cpuhp_state = {
>      }
>  };
>  
> +static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
> +{
> +    return pm_smbus_vmstate_needed();
> +}
> +
>  /* 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
> @@ -321,6 +326,8 @@ static const VMStateDescription vmstate_acpi = {
>          VMSTATE_UINT16(ar.pm1.evt.en, PIIX4PMState),
>          VMSTATE_UINT16(ar.pm1.cnt.cnt, PIIX4PMState),
>          VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
> +        VMSTATE_STRUCT_TEST(smb, PIIX4PMState, piix4_vmstate_need_smbus, 3,
> +                            pmsmb_vmstate, PMSMBus),
>          VMSTATE_TIMER_PTR(ar.tmr.timer, PIIX4PMState),
>          VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
>          VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 8793113c25..2a9bc6e8c0 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -19,6 +19,7 @@
>   */
>  #include "qemu/osdep.h"
>  #include "hw/hw.h"
> +#include "hw/boards.h"
>  #include "hw/i2c/pm_smbus.h"
>  #include "hw/i2c/smbus_master.h"
>  
> @@ -450,6 +451,36 @@ static const MemoryRegionOps pm_smbus_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> +bool pm_smbus_vmstate_needed(void)
> +{
> +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> +
> +    return !mc->smbus_no_migration_support;
> +}
> +
> +const VMStateDescription pmsmb_vmstate = {
> +    .name = "pmsmb",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(smb_stat, PMSMBus),
> +        VMSTATE_UINT8(smb_ctl, PMSMBus),
> +        VMSTATE_UINT8(smb_cmd, PMSMBus),
> +        VMSTATE_UINT8(smb_addr, PMSMBus),
> +        VMSTATE_UINT8(smb_data0, PMSMBus),
> +        VMSTATE_UINT8(smb_data1, PMSMBus),
> +        VMSTATE_UINT32(smb_index, PMSMBus),
> +        VMSTATE_UINT8_ARRAY(smb_data, PMSMBus, PM_SMBUS_MAX_MSG_SIZE),
> +        VMSTATE_UINT8(smb_auxctl, PMSMBus),
> +        VMSTATE_UINT8(smb_blkdata, PMSMBus),
> +        VMSTATE_BOOL(i2c_enable, PMSMBus),
> +        VMSTATE_BOOL(op_done, PMSMBus),
> +        VMSTATE_BOOL(in_i2c_block_read, PMSMBus),
> +        VMSTATE_BOOL(start_transaction_on_status_read, PMSMBus),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk)
>  {
>      smb->op_done = true;
> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
> index e6d8d28194..7b24be8256 100644
> --- a/hw/i2c/smbus_ich9.c
> +++ b/hw/i2c/smbus_ich9.c
> @@ -43,12 +43,20 @@ typedef struct ICH9SMBState {
>      PMSMBus smb;
>  } ICH9SMBState;
>  
> +static bool ich9_vmstate_need_smbus(void *opaque, int version_id)
> +{
> +    return pm_smbus_vmstate_needed();
> +}
> +
>  static const VMStateDescription vmstate_ich9_smbus = {
>      .name = "ich9_smb",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
> -        VMSTATE_PCI_DEVICE(dev, struct ICH9SMBState),
> +        VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
> +        VMSTATE_BOOL_TEST(irq_enabled, ICH9SMBState, ich9_vmstate_need_smbus),
> +        VMSTATE_STRUCT_TEST(smb, ICH9SMBState, ich9_vmstate_need_smbus, 1,
> +                            pmsmb_vmstate, PMSMBus),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
> index 7bcca97672..fb55c44444 100644
> --- a/include/hw/i2c/pm_smbus.h
> +++ b/include/hw/i2c/pm_smbus.h
> @@ -43,4 +43,13 @@ typedef struct PMSMBus {
>  
>  void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk);
>  
> +/*
> + * For backwards compatibility on migration, older versions don't have
> + * working migration for pm_smbus, this lets us ignore the migrations
> + * for older machine versions.
> + */
> +bool pm_smbus_vmstate_needed(void);
> +
> +extern const VMStateDescription pmsmb_vmstate;
> +
>  #endif /* PM_SMBUS_H */
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 11/16] i2c:smbus_slave: Add an SMBus vmstate structure

[Dr. David Alan Gilbert]

* minyard@acm.org (minyard@acm.org) wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> There is no vmstate handling for SMBus, so no device sitting on SMBus
> can have a state transfer that works reliably.  So add it.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/i2c/smbus_slave.c         | 18 ++++++++++++++++++
>  include/hw/i2c/smbus_slave.h | 24 +++++++++++++++++++++---
>  2 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
> index d03f714608..e839c1dd11 100644
> --- a/hw/i2c/smbus_slave.c
> +++ b/hw/i2c/smbus_slave.c
> @@ -206,6 +206,24 @@ static void smbus_device_class_init(ObjectClass *klass, void *data)
>      sc->send = smbus_i2c_send;
>  }
>  
> +bool smbus_vmstate_needed(SMBusDevice *dev)
> +{
> +    return dev->mode != SMBUS_IDLE;
> +}
> +
> +const VMStateDescription vmstate_smbus_device = {
> +    .name = TYPE_SMBUS_DEVICE,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_I2C_SLAVE(i2c, SMBusDevice),
> +        VMSTATE_INT32(mode, SMBusDevice),
> +        VMSTATE_INT32(data_len, SMBusDevice),
> +        VMSTATE_UINT8_ARRAY(data_buf, SMBusDevice, SMBUS_DATA_MAX_LEN),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const TypeInfo smbus_device_type_info = {
>      .name = TYPE_SMBUS_DEVICE,
>      .parent = TYPE_I2C_SLAVE,
> diff --git a/include/hw/i2c/smbus_slave.h b/include/hw/i2c/smbus_slave.h
> index fad2db9c76..78fd45cb51 100644
> --- a/include/hw/i2c/smbus_slave.h
> +++ b/include/hw/i2c/smbus_slave.h
> @@ -75,14 +75,32 @@ typedef struct SMBusDeviceClass
>      void (*transaction_complete)(SMBusDevice *dev);
>  } SMBusDeviceClass;
>  
> +#define SMBUS_DATA_MAX_LEN 34  /* command + len + 32 bytes of data.  */
> +
>  struct SMBusDevice {
>      /* The SMBus protocol is implemented on top of I2C.  */
>      I2CSlave i2c;
>  
>      /* Remaining fields for internal use only.  */
> -    int mode;
> -    int data_len;
> -    uint8_t data_buf[34]; /* command + len + 32 bytes of data.  */
> +    int32_t mode;
> +    int32_t data_len;
> +    uint8_t data_buf[SMBUS_DATA_MAX_LEN];
>  };
>  
> +extern const VMStateDescription vmstate_smbus_device;
> +
> +#define VMSTATE_SMBUS_DEVICE(_field, _state) {                       \
> +    .name       = (stringify(_field)),                               \
> +    .size       = sizeof(SMBusDevice),                               \
> +    .vmsd       = &vmstate_smbus_device,                             \
> +    .flags      = VMS_STRUCT,                                        \
> +    .offset     = vmstate_offset_value(_state, _field, SMBusDevice), \
> +}
> +
> +/*
> + * Users should call this in their .needed functions to know if the
> + * SMBus slave data needs to be transferred.
> + */
> +bool smbus_vmstate_needed(SMBusDevice *dev);
> +
>  #endif
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 14/16] i2c:smbus_eeprom: Add vmstate handling to the smbus eeprom

[Dr. David Alan Gilbert]

* minyard@acm.org (minyard@acm.org) wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Transfer the state of the EEPROM on a migration.  This way the
> data remains consistent on migration.
> 
> This required moving the actual data to a separate array and
> using the data provided in the init function as a separate
> initialization array, since a pointer property has to be a
> void * and the array needs to be uint8_t[].

So I think I'm OK with this, but I'd like other peoples comments as
well; see comments.

> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/i2c/smbus_eeprom.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index 8e9b734c09..942057dc10 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -24,6 +24,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "hw/hw.h"
> +#include "hw/boards.h"
>  #include "hw/i2c/i2c.h"
>  #include "hw/i2c/smbus_slave.h"
>  #include "hw/i2c/smbus_eeprom.h"
> @@ -39,8 +40,10 @@
>  
>  typedef struct SMBusEEPROMDevice {
>      SMBusDevice smbusdev;
> -    void *data;
> +    uint8_t data[SMBUS_EEPROM_SIZE];
> +    void *init_data;
>      uint8_t offset;
> +    bool accessed;
>  } SMBusEEPROMDevice;
>  
>  static uint8_t eeprom_receive_byte(SMBusDevice *dev)
> @@ -49,6 +52,7 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev)
>      uint8_t *data = eeprom->data;
>      uint8_t val = data[eeprom->offset++];
>  
> +    eeprom->accessed = true;
>  #ifdef DEBUG
>      printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n",
>             dev->i2c.address, val);
> @@ -61,6 +65,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
>      SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>      uint8_t *data = eeprom->data;
>  
> +    eeprom->accessed = true;
>  #ifdef DEBUG
>      printf("eeprom_write_byte: addr=0x%02x cmd=0x%02x val=0x%02x\n",
>             dev->i2c.address, cmd, buf[0]);
> @@ -78,15 +83,39 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
>      return 0;
>  }
>  
> +static bool smbus_eeprom_vmstate_needed(void *opaque)
> +{
> +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> +    SMBusEEPROMDevice *eeprom = opaque;
> +
> +    return (eeprom->accessed || smbus_vmstate_needed(&eeprom->smbusdev)) &&
> +        !mc->smbus_no_migration_support;

That's a little complx, but OK.

> +}
> +
> +static const VMStateDescription vmstate_smbus_eeprom = {
> +    .name = "smbus-eeprom",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = smbus_eeprom_vmstate_needed,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice),
> +        VMSTATE_UINT8_ARRAY(data, SMBusEEPROMDevice, SMBUS_EEPROM_SIZE),
> +        VMSTATE_UINT8(offset, SMBusEEPROMDevice),
> +        VMSTATE_BOOL(accessed, SMBusEEPROMDevice),

OK, good - including 'accessed' means that if we migrate a->b->c and
it's not changed while it's on b, then 'c' still gets the updated
version.

> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>  {
>      SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>  
> +    memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
>      eeprom->offset = 0;
>  }
>  
>  static Property smbus_eeprom_properties[] = {
> -    DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data),
> +    DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -99,6 +128,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>      sc->receive_byte = eeprom_receive_byte;
>      sc->write_data = eeprom_write_data;
>      dc->props = smbus_eeprom_properties;
> +    dc->vmsd = &vmstate_smbus_eeprom;
>      /* Reason: pointer property "data" */
>      dc->user_creatable = false;
>  }

so from migration side:


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 04/16] i2c: Don't check return value from i2c_recv()

[Peter Maydell]

On Mon, 26 Nov 2018 at 20:04, <minyard@acm.org> wrote:
>
> From: Corey Minyard <cminyard@mvista.com>
>
> i2c_recv() cannot fail, so there is no need to check the return
> value.  It also returns unt8_t, so comparing with < 0 is not
> meaningful.
>
> Fix up various I2C controllers to remove the unneeded code.
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> ---

> diff --git a/hw/i2c/exynos4210_i2c.c b/hw/i2c/exynos4210_i2c.c
> index c96fa7d7be..43f284eab7 100644
> --- a/hw/i2c/exynos4210_i2c.c
> +++ b/hw/i2c/exynos4210_i2c.c
> @@ -106,12 +106,12 @@ static inline void exynos4210_i2c_raise_interrupt(Exynos4210I2CState *s)
>  static void exynos4210_i2c_data_receive(void *opaque)
>  {
>      Exynos4210I2CState *s = (Exynos4210I2CState *)opaque;
> -    int ret;
> +    uint8_t ret;
>
>      s->i2cstat &= ~I2CSTAT_LAST_BIT;
>      s->scl_free = false;
>      ret = i2c_recv(s->bus);
> -    if (ret < 0 && (s->i2ccon & I2CCON_ACK_GEN)) {
> +    if (s->i2ccon & I2CCON_ACK_GEN) {
>          s->i2cstat |= I2CSTAT_LAST_BIT;  /* Data is not acknowledged */

Previously the code was doing this only if i2c_recv()
failed (returned a negative value) and ACK_GEN was set.
i2c_recv() can never fail, so this if() {} branch should
be deleted entirely ("false && something" simplifies
to "false", not "something").

>      } else {
>          s->i2cds = ret;

The rest of the patch looks good.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 03/16] arm:i2c: Don't mask return from i2c_recv()

[Peter Maydell]

On Mon, 26 Nov 2018 at 20:04, <minyard@acm.org> wrote:
>
> From: Corey Minyard <cminyard@mvista.com>
>
> It can't fail, and now that it returns a uint8_t a 0xff mask
> is unnecessary.
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/stellaris.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 6c69ce79b2..638b649911 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -811,7 +811,7 @@ static void stellaris_i2c_write(void *opaque, hwaddr offset,
>              /* TODO: Handle errors.  */
>              if (s->msa & 1) {
>                  /* Recv */
> -                s->mdr = i2c_recv(s->bus) & 0xff;
> +                s->mdr = i2c_recv(s->bus);
>              } else {
>                  /* Send */

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 15/16] hw/i2c/smbus_eeprom: Create at most SMBUS_EEPROM_MAX EEPROMs on a SMBus

[Peter Maydell]

On Mon, 26 Nov 2018 at 20:04, <minyard@acm.org> wrote:
>
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> Calling smbus_eeprom_init() with more than 8 EEPROMs would lead to a
> heap overflow.
> Replace the '8' magic number by a definition, and check no more than
> this number are created.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  hw/i2c/smbus_eeprom.c         | 13 +++++++++++--
>  include/hw/i2c/smbus_eeprom.h |  4 +++-
>  2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index 942057dc10..a0dcadbd60 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -23,6 +23,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "hw/hw.h"
>  #include "hw/boards.h"
>  #include "hw/i2c/i2c.h"
> @@ -157,12 +158,20 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf)
>      qdev_init_nofail(dev);
>  }
>
> -void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
> +void smbus_eeprom_init(I2CBus *smbus, unsigned int nb_eeprom,
>                         const uint8_t *eeprom_spd, int eeprom_spd_size)
>  {
>      int i;
> +    uint8_t *eeprom_buf;
> +
> +    if (nb_eeprom > SMBUS_EEPROM_MAX) {
> +        error_report("At most %u EEPROM are supported on a SMBus.",
> +                     SMBUS_EEPROM_MAX);
> +        exit(1);

You could also just assert(), since this would be a
QEMU bug (all callers use fixed values for this argument).

> +    }
> +
>       /* XXX: make this persistent */
> -    uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE);
> +    eeprom_buf = g_malloc0(nb_eeprom * SMBUS_EEPROM_SIZE);

So if we allocate N buffers as the caller requests, what
is the thing that means that more than 8 won't work ?

We've now changed from allocating always 8 lots of
the EEPROM size to possibly allocating fewer than that.
How does the code in the device know what the size
of the buffer we're passing as the "data" property is
now? We don't pass it the number of EEPROMs as a property.

>      if (eeprom_spd_size > 0) {
>          memcpy(eeprom_buf, eeprom_spd, eeprom_spd_size);
>      }
> diff --git a/include/hw/i2c/smbus_eeprom.h b/include/hw/i2c/smbus_eeprom.h
> index 46fb1a37d6..8436200599 100644
> --- a/include/hw/i2c/smbus_eeprom.h
> +++ b/include/hw/i2c/smbus_eeprom.h
> @@ -25,8 +25,10 @@
>
>  #include "hw/i2c/i2c.h"
>
> +#define SMBUS_EEPROM_MAX 8
> +
>  void smbus_eeprom_init_one(I2CBus *bus, uint8_t address, uint8_t *eeprom_buf);
> -void smbus_eeprom_init(I2CBus *bus, int nb_eeprom,
> +void smbus_eeprom_init(I2CBus *bus, unsigned int nb_eeprom,
>                         const uint8_t *eeprom_spd, int size);

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 05/16] i2c: Simplify and correct the SMBus state machine

[Peter Maydell]

On Mon, 26 Nov 2018 at 20:04, <minyard@acm.org> wrote:
>
> From: Corey Minyard <cminyard@mvista.com>
>
> The SMBus slave code had an unneeded state, unnecessary function
> pointers and incorrectly handled quick commands.  Rewrite it
> to simplify the code and make it work correctly.
>
> smbus_eeprom is the only user, so no other effects and the eeprom
> code also gets a significant simplification.
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  hw/i2c/smbus_eeprom.c        | 58 ++++++-----------------
>  hw/i2c/smbus_slave.c         | 91 ++++++++++++++++--------------------
>  include/hw/i2c/smbus_slave.h | 45 +++++++++++++-----
>  3 files changed, 86 insertions(+), 108 deletions(-)

I'm finding this patch difficult to understand -- it
feels like it's trying to do too many things at once.
Is it possible to split it? For instance it looks like
we're getting rid of send_byte and just handling
all writes to the device with write_data -- is that
right? That sounds like it should be its own patch.
Whatever the change is that means that we don't pass in
the cmd argument to the various methods is probably its
own patch. And fixing quick commands sounds like something
that goes in its own patch.

Stray whitespace change (there are more of these below too).
If you want to do formatting tidyups please put those in
their own patch.

>  #ifdef DEBUG
>      printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n",
>             dev->i2c.address, val);
> @@ -65,37 +49,26 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev)
>      return val;
>  }
>
> -static void eeprom_write_data(SMBusDevice *dev, uint8_t cmd, uint8_t *buf, int len)
> +static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
>  {
>      SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
> -    int n;
> +    uint8_t *data = eeprom->data;
> +
>  #ifdef DEBUG
>      printf("eeprom_write_byte: addr=0x%02x cmd=0x%02x val=0x%02x\n",
>             dev->i2c.address, cmd, buf[0]);

The "cmd" argument has been removed from the prototype
but is still used in this debug printf.

>  #endif

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 04/16] i2c: Don't check return value from i2c_recv()

[Corey Minyard]

On 11/30/18 11:25 AM, Peter Maydell wrote:
> On Mon, 26 Nov 2018 at 20:04, <minyard@acm.org> wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> i2c_recv() cannot fail, so there is no need to check the return
>> value.  It also returns unt8_t, so comparing with < 0 is not
>> meaningful.
>>
>> Fix up various I2C controllers to remove the unneeded code.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> diff --git a/hw/i2c/exynos4210_i2c.c b/hw/i2c/exynos4210_i2c.c
>> index c96fa7d7be..43f284eab7 100644
>> --- a/hw/i2c/exynos4210_i2c.c
>> +++ b/hw/i2c/exynos4210_i2c.c
>> @@ -106,12 +106,12 @@ static inline void exynos4210_i2c_raise_interrupt(Exynos4210I2CState *s)
>>   static void exynos4210_i2c_data_receive(void *opaque)
>>   {
>>       Exynos4210I2CState *s = (Exynos4210I2CState *)opaque;
>> -    int ret;
>> +    uint8_t ret;
>>
>>       s->i2cstat &= ~I2CSTAT_LAST_BIT;
>>       s->scl_free = false;
>>       ret = i2c_recv(s->bus);
>> -    if (ret < 0 && (s->i2ccon & I2CCON_ACK_GEN)) {
>> +    if (s->i2ccon & I2CCON_ACK_GEN) {
>>           s->i2cstat |= I2CSTAT_LAST_BIT;  /* Data is not acknowledged */
> Previously the code was doing this only if i2c_recv()
> failed (returned a negative value) and ACK_GEN was set.
> i2c_recv() can never fail, so this if() {} branch should
> be deleted entirely ("false && something" simplifies
> to "false", not "something").

Oops, yes.  Plus you can just remove the ret variable and simplify
this a lot.  Thanks for finding this.

-corey


>
>>       } else {
>>           s->i2cds = ret;
> The rest of the patch looks good.
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v3 15/16] hw/i2c/smbus_eeprom: Create at most SMBUS_EEPROM_MAX EEPROMs on a SMBus

[Corey Minyard]

On 11/30/18 11:39 AM, Peter Maydell wrote:
> On Mon, 26 Nov 2018 at 20:04, <minyard@acm.org> wrote:
>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>> Calling smbus_eeprom_init() with more than 8 EEPROMs would lead to a
>> heap overflow.
>> Replace the '8' magic number by a definition, and check no more than
>> this number are created.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
>>   hw/i2c/smbus_eeprom.c         | 13 +++++++++++--
>>   include/hw/i2c/smbus_eeprom.h |  4 +++-
>>   2 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>> index 942057dc10..a0dcadbd60 100644
>> --- a/hw/i2c/smbus_eeprom.c
>> +++ b/hw/i2c/smbus_eeprom.c
>> @@ -23,6 +23,7 @@
>>    */
>>
>>   #include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>>   #include "hw/hw.h"
>>   #include "hw/boards.h"
>>   #include "hw/i2c/i2c.h"
>> @@ -157,12 +158,20 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf)
>>       qdev_init_nofail(dev);
>>   }
>>
>> -void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
>> +void smbus_eeprom_init(I2CBus *smbus, unsigned int nb_eeprom,
>>                          const uint8_t *eeprom_spd, int eeprom_spd_size)
>>   {
>>       int i;
>> +    uint8_t *eeprom_buf;
>> +
>> +    if (nb_eeprom > SMBUS_EEPROM_MAX) {
>> +        error_report("At most %u EEPROM are supported on a SMBus.",
>> +                     SMBUS_EEPROM_MAX);
>> +        exit(1);
> You could also just assert(), since this would be a
> QEMU bug (all callers use fixed values for this argument).

I'm fine either way.  Philippe?


>> +    }
>> +
>>        /* XXX: make this persistent */
>> -    uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE);
>> +    eeprom_buf = g_malloc0(nb_eeprom * SMBUS_EEPROM_SIZE);
> So if we allocate N buffers as the caller requests, what
> is the thing that means that more than 8 won't work ?
>
> We've now changed from allocating always 8 lots of
> the EEPROM size to possibly allocating fewer than that.
> How does the code in the device know what the size
> of the buffer we're passing as the "data" property is
> now? We don't pass it the number of EEPROMs as a property.

It doesn't have to.  Each EEPROM is 256 bytes and that's all the data
it has.

But this whole thing is confusing, I agree.  The more I look at this
particular file, the less I like it.  But the caller is basically saying:
"I need N EEPROMs, here's the initialization data".  That's not
really very flexible, IMHO the EEPROM devices should be created
individually using standard qemu methods.

I'm tempted to rewrite this whole thing to make it cleaner.

-corey

>>       if (eeprom_spd_size > 0) {
>>           memcpy(eeprom_buf, eeprom_spd, eeprom_spd_size);
>>       }
>> diff --git a/include/hw/i2c/smbus_eeprom.h b/include/hw/i2c/smbus_eeprom.h
>> index 46fb1a37d6..8436200599 100644
>> --- a/include/hw/i2c/smbus_eeprom.h
>> +++ b/include/hw/i2c/smbus_eeprom.h
>> @@ -25,8 +25,10 @@
>>
>>   #include "hw/i2c/i2c.h"
>>
>> +#define SMBUS_EEPROM_MAX 8
>> +
>>   void smbus_eeprom_init_one(I2CBus *bus, uint8_t address, uint8_t *eeprom_buf);
>> -void smbus_eeprom_init(I2CBus *bus, int nb_eeprom,
>> +void smbus_eeprom_init(I2CBus *bus, unsigned int nb_eeprom,
>>                          const uint8_t *eeprom_spd, int size);
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v3 05/16] i2c: Simplify and correct the SMBus state machine

[Corey Minyard]

On 11/30/18 12:13 PM, Peter Maydell wrote:
> On Mon, 26 Nov 2018 at 20:04, <minyard@acm.org> wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> The SMBus slave code had an unneeded state, unnecessary function
>> pointers and incorrectly handled quick commands.  Rewrite it
>> to simplify the code and make it work correctly.
>>
>> smbus_eeprom is the only user, so no other effects and the eeprom
>> code also gets a significant simplification.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
>>   hw/i2c/smbus_eeprom.c        | 58 ++++++-----------------
>>   hw/i2c/smbus_slave.c         | 91 ++++++++++++++++--------------------
>>   include/hw/i2c/smbus_slave.h | 45 +++++++++++++-----
>>   3 files changed, 86 insertions(+), 108 deletions(-)
> I'm finding this patch difficult to understand -- it
> feels like it's trying to do too many things at once.
> Is it possible to split it? For instance it looks like
> we're getting rid of send_byte and just handling
> all writes to the device with write_data -- is that
> right? That sounds like it should be its own patch.
> Whatever the change is that means that we don't pass in
> the cmd argument to the various methods is probably its
> own patch. And fixing quick commands sounds like something
> that goes in its own patch.
>
> Stray whitespace change (there are more of these below too).
> If you want to do formatting tidyups please put those in
> their own patch.


Ok, I've split this up into separate patches, as they should have been.

Thanks,

-corey


>
>>   #ifdef DEBUG
>>       printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n",
>>              dev->i2c.address, val);
>> @@ -65,37 +49,26 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev)
>>       return val;
>>   }
>>
>> -static void eeprom_write_data(SMBusDevice *dev, uint8_t cmd, uint8_t *buf, int len)
>> +static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
>>   {
>>       SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
>> -    int n;
>> +    uint8_t *data = eeprom->data;
>> +
>>   #ifdef DEBUG
>>       printf("eeprom_write_byte: addr=0x%02x cmd=0x%02x val=0x%02x\n",
>>              dev->i2c.address, cmd, buf[0]);
> The "cmd" argument has been removed from the prototype
> but is still used in this debug printf.
>
>>   #endif
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v3 15/16] hw/i2c/smbus_eeprom: Create at most SMBUS_EEPROM_MAX EEPROMs on a SMBus

[Peter Maydell]

On Fri, 30 Nov 2018 at 20:47, Corey Minyard <minyard@acm.org> wrote:
>
> On 11/30/18 11:39 AM, Peter Maydell wrote:
> > On Mon, 26 Nov 2018 at 20:04, <minyard@acm.org> wrote:
> >> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> >>        /* XXX: make this persistent */
> >> -    uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE);
> >> +    eeprom_buf = g_malloc0(nb_eeprom * SMBUS_EEPROM_SIZE);
> > So if we allocate N buffers as the caller requests, what
> > is the thing that means that more than 8 won't work ?
> >
> > We've now changed from allocating always 8 lots of
> > the EEPROM size to possibly allocating fewer than that.
> > How does the code in the device know what the size
> > of the buffer we're passing as the "data" property is
> > now? We don't pass it the number of EEPROMs as a property.
>
> It doesn't have to.  Each EEPROM is 256 bytes and that's all the data
> it has.
>
> But this whole thing is confusing, I agree.  The more I look at this
> particular file, the less I like it.  But the caller is basically saying:
> "I need N EEPROMs, here's the initialization data".  That's not
> really very flexible, IMHO the EEPROM devices should be created
> individually using standard qemu methods.

Oh, yes, I see now. We pass in a block of N * 256 bytes, and
the function then hands a pointer to offsets 0, 256, ...
to each device it creates.

I definitely don't see why we need to say "only a maximum of
8" now, though -- where does that limit come from? If we
get passed in an arbitrary number of EEPROMs X, and we allocate
256 * X bytes, and create X devices which each get one slice of
the buffer, what goes wrong when X > 8 ?

> I'm tempted to rewrite this whole thing to make it cleaner.

It's certainly pretty awkward code. I think personally given
this patchset is already pretty big I'd go for getting it into
master first and then doing a followup with further cleanup.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 15/16] hw/i2c/smbus_eeprom: Create at most SMBUS_EEPROM_MAX EEPROMs on a SMBus

[Philippe Mathieu-Daudé]

On 1/12/18 12:57, Peter Maydell wrote:
> On Fri, 30 Nov 2018 at 20:47, Corey Minyard <minyard@acm.org> wrote:
>>
>> On 11/30/18 11:39 AM, Peter Maydell wrote:
>>> On Mon, 26 Nov 2018 at 20:04, <minyard@acm.org> wrote:
>>>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>        /* XXX: make this persistent */
>>>> -    uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE);
>>>> +    eeprom_buf = g_malloc0(nb_eeprom * SMBUS_EEPROM_SIZE);
>>> So if we allocate N buffers as the caller requests, what
>>> is the thing that means that more than 8 won't work ?
>>>
>>> We've now changed from allocating always 8 lots of
>>> the EEPROM size to possibly allocating fewer than that.
>>> How does the code in the device know what the size
>>> of the buffer we're passing as the "data" property is
>>> now? We don't pass it the number of EEPROMs as a property.
>>
>> It doesn't have to.  Each EEPROM is 256 bytes and that's all the data
>> it has.
>>
>> But this whole thing is confusing, I agree.  The more I look at this
>> particular file, the less I like it.  But the caller is basically saying:
>> "I need N EEPROMs, here's the initialization data".  That's not
>> really very flexible, IMHO the EEPROM devices should be created
>> individually using standard qemu methods.
> 
> Oh, yes, I see now. We pass in a block of N * 256 bytes, and
> the function then hands a pointer to offsets 0, 256, ...
> to each device it creates.
> 
> I definitely don't see why we need to say "only a maximum of
> 8" now, though -- where does that limit come from? If we
> get passed in an arbitrary number of EEPROMs X, and we allocate
> 256 * X bytes, and create X devices which each get one slice of
> the buffer, what goes wrong when X > 8 ?
> 
>> I'm tempted to rewrite this whole thing to make it cleaner.
> 
> It's certainly pretty awkward code. I think personally given
> this patchset is already pretty big I'd go for getting it into
> master first and then doing a followup with further cleanup.

As suggested Peter, I'd drop this patch (in favor of a later clean
rewrite) and simply add an assert().

Regards,

Phil.

Re: [Qemu-devel] [PATCH v3 15/16] hw/i2c/smbus_eeprom: Create at most SMBUS_EEPROM_MAX EEPROMs on a SMBus

[Corey Minyard]

On 12/1/18 11:43 AM, Philippe Mathieu-Daudé wrote:
> On 1/12/18 12:57, Peter Maydell wrote:
>> On Fri, 30 Nov 2018 at 20:47, Corey Minyard <minyard@acm.org> wrote:
>>> On 11/30/18 11:39 AM, Peter Maydell wrote:
>>>> On Mon, 26 Nov 2018 at 20:04, <minyard@acm.org> wrote:
>>>>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>>         /* XXX: make this persistent */
>>>>> -    uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE);
>>>>> +    eeprom_buf = g_malloc0(nb_eeprom * SMBUS_EEPROM_SIZE);
>>>> So if we allocate N buffers as the caller requests, what
>>>> is the thing that means that more than 8 won't work ?
>>>>
>>>> We've now changed from allocating always 8 lots of
>>>> the EEPROM size to possibly allocating fewer than that.
>>>> How does the code in the device know what the size
>>>> of the buffer we're passing as the "data" property is
>>>> now? We don't pass it the number of EEPROMs as a property.
>>> It doesn't have to.  Each EEPROM is 256 bytes and that's all the data
>>> it has.
>>>
>>> But this whole thing is confusing, I agree.  The more I look at this
>>> particular file, the less I like it.  But the caller is basically saying:
>>> "I need N EEPROMs, here's the initialization data".  That's not
>>> really very flexible, IMHO the EEPROM devices should be created
>>> individually using standard qemu methods.
>> Oh, yes, I see now. We pass in a block of N * 256 bytes, and
>> the function then hands a pointer to offsets 0, 256, ...
>> to each device it creates.
>>
>> I definitely don't see why we need to say "only a maximum of
>> 8" now, though -- where does that limit come from?

I missed this question.

The limit comes from the chip design.  It has 3 pins that set the bottom 3
bit of the I2C address, the address can range from 0x50 to 0x57.

So it's not arbitrary, I guess, but I don't see a strong reason to put
the limitation there.

-corey

>>   If we
>> get passed in an arbitrary number of EEPROMs X, and we allocate
>> 256 * X bytes, and create X devices which each get one slice of
>> the buffer, what goes wrong when X > 8 ?
>>
>>> I'm tempted to rewrite this whole thing to make it cleaner.
>> It's certainly pretty awkward code. I think personally given
>> this patchset is already pretty big I'd go for getting it into
>> master first and then doing a followup with further cleanup.
> As suggested Peter, I'd drop this patch (in favor of a later clean
> rewrite) and simply add an assert().
>
> Regards,
>
> Phil.

Re: [Qemu-devel] [PATCH v3 14/16] i2c:smbus_eeprom: Add vmstate handling to the smbus eeprom

[Corey Minyard]

On 11/29/18 7:29 AM, Dr. David Alan Gilbert wrote:
> * minyard@acm.org (minyard@acm.org) wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> Transfer the state of the EEPROM on a migration.  This way the
>> data remains consistent on migration.
>>
>> This required moving the actual data to a separate array and
>> using the data provided in the init function as a separate
>> initialization array, since a pointer property has to be a
>> void * and the array needs to be uint8_t[].
> So I think I'm OK with this, but I'd like other peoples comments as
> well; see comments.


Well, no comments so far.

I have a question on this.  If this particular device were modified to 
be able
to support different size eeproms, would this need a new version of the
vmstate structure?  I know the vmstate structure itself would have to
change to have an array size, but would it have to change in such a
way that the array size would need to be transmitted?

I ask because someday, someone might need an eeprom of a different
size, and it would be nice if it could be done now in a way that avoided
creating a new version.

Thanks,

-corey

>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   hw/i2c/smbus_eeprom.c | 34 ++++++++++++++++++++++++++++++++--
>>   1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>> index 8e9b734c09..942057dc10 100644
>> --- a/hw/i2c/smbus_eeprom.c
>> +++ b/hw/i2c/smbus_eeprom.c
>> @@ -24,6 +24,7 @@
>>   
>>   #include "qemu/osdep.h"
>>   #include "hw/hw.h"
>> +#include "hw/boards.h"
>>   #include "hw/i2c/i2c.h"
>>   #include "hw/i2c/smbus_slave.h"
>>   #include "hw/i2c/smbus_eeprom.h"
>> @@ -39,8 +40,10 @@
>>   
>>   typedef struct SMBusEEPROMDevice {
>>       SMBusDevice smbusdev;
>> -    void *data;
>> +    uint8_t data[SMBUS_EEPROM_SIZE];
>> +    void *init_data;
>>       uint8_t offset;
>> +    bool accessed;
>>   } SMBusEEPROMDevice;
>>   
>>   static uint8_t eeprom_receive_byte(SMBusDevice *dev)
>> @@ -49,6 +52,7 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev)
>>       uint8_t *data = eeprom->data;
>>       uint8_t val = data[eeprom->offset++];
>>   
>> +    eeprom->accessed = true;
>>   #ifdef DEBUG
>>       printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n",
>>              dev->i2c.address, val);
>> @@ -61,6 +65,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
>>       SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>>       uint8_t *data = eeprom->data;
>>   
>> +    eeprom->accessed = true;
>>   #ifdef DEBUG
>>       printf("eeprom_write_byte: addr=0x%02x cmd=0x%02x val=0x%02x\n",
>>              dev->i2c.address, cmd, buf[0]);
>> @@ -78,15 +83,39 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
>>       return 0;
>>   }
>>   
>> +static bool smbus_eeprom_vmstate_needed(void *opaque)
>> +{
>> +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>> +    SMBusEEPROMDevice *eeprom = opaque;
>> +
>> +    return (eeprom->accessed || smbus_vmstate_needed(&eeprom->smbusdev)) &&
>> +        !mc->smbus_no_migration_support;
> That's a little complx, but OK.
>
>> +}
>> +
>> +static const VMStateDescription vmstate_smbus_eeprom = {
>> +    .name = "smbus-eeprom",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .needed = smbus_eeprom_vmstate_needed,
>> +    .fields      = (VMStateField[]) {
>> +        VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice),
>> +        VMSTATE_UINT8_ARRAY(data, SMBusEEPROMDevice, SMBUS_EEPROM_SIZE),
>> +        VMSTATE_UINT8(offset, SMBusEEPROMDevice),
>> +        VMSTATE_BOOL(accessed, SMBusEEPROMDevice),
> OK, good - including 'accessed' means that if we migrate a->b->c and
> it's not changed while it's on b, then 'c' still gets the updated
> version.
>
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>   static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>>   {
>>       SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>>   
>> +    memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
>>       eeprom->offset = 0;
>>   }
>>   
>>   static Property smbus_eeprom_properties[] = {
>> -    DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data),
>> +    DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> @@ -99,6 +128,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>>       sc->receive_byte = eeprom_receive_byte;
>>       sc->write_data = eeprom_write_data;
>>       dc->props = smbus_eeprom_properties;
>> +    dc->vmsd = &vmstate_smbus_eeprom;
>>       /* Reason: pointer property "data" */
>>       dc->user_creatable = false;
>>   }
> so from migration side:
>
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
>> -- 
>> 2.17.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 14/16] i2c:smbus_eeprom: Add vmstate handling to the smbus eeprom

[Dr. David Alan Gilbert]

* Corey Minyard (minyard@acm.org) wrote:
> On 11/29/18 7:29 AM, Dr. David Alan Gilbert wrote:
> > * minyard@acm.org (minyard@acm.org) wrote:
> > > From: Corey Minyard <cminyard@mvista.com>
> > > 
> > > Transfer the state of the EEPROM on a migration.  This way the
> > > data remains consistent on migration.
> > > 
> > > This required moving the actual data to a separate array and
> > > using the data provided in the init function as a separate
> > > initialization array, since a pointer property has to be a
> > > void * and the array needs to be uint8_t[].
> > So I think I'm OK with this, but I'd like other peoples comments as
> > well; see comments.
> 
> 
> Well, no comments so far.
> 
> I have a question on this.  If this particular device were modified to be
> able
> to support different size eeproms, would this need a new version of the
> vmstate structure?  I know the vmstate structure itself would have to
> change to have an array size, but would it have to change in such a
> way that the array size would need to be transmitted?
> 
> I ask because someday, someone might need an eeprom of a different
> size, and it would be nice if it could be done now in a way that avoided
> creating a new version.

That depends; for example one way to do it would be to keep the existing
structure for cases where the PROM was the current default size, and
then add the extra data in a subsection that was only transmitted for a
PROM that was a different size.

However, it's probably simpler - the size of the eprom comes from the
configuration from the command line etc - it's not a dynamic thing;
So you'd probably end up going back to a void *data, making sure that's
allocated before thefields are loaded, and then replacing the
VMSTATE_UINT8_ARRAY by a VMSTATE_BUFFER;  note that if the sizes don't
match on source/destination then that would fail rather messily
where as the subsection would fail cleanly.

Dave

> Thanks,
> 
> -corey
> 
> > > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > Cc: Peter Maydell <peter.maydell@linaro.org>
> > > ---
> > >   hw/i2c/smbus_eeprom.c | 34 ++++++++++++++++++++++++++++++++--
> > >   1 file changed, 32 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> > > index 8e9b734c09..942057dc10 100644
> > > --- a/hw/i2c/smbus_eeprom.c
> > > +++ b/hw/i2c/smbus_eeprom.c
> > > @@ -24,6 +24,7 @@
> > >   #include "qemu/osdep.h"
> > >   #include "hw/hw.h"
> > > +#include "hw/boards.h"
> > >   #include "hw/i2c/i2c.h"
> > >   #include "hw/i2c/smbus_slave.h"
> > >   #include "hw/i2c/smbus_eeprom.h"
> > > @@ -39,8 +40,10 @@
> > >   typedef struct SMBusEEPROMDevice {
> > >       SMBusDevice smbusdev;
> > > -    void *data;
> > > +    uint8_t data[SMBUS_EEPROM_SIZE];
> > > +    void *init_data;
> > >       uint8_t offset;
> > > +    bool accessed;
> > >   } SMBusEEPROMDevice;
> > >   static uint8_t eeprom_receive_byte(SMBusDevice *dev)
> > > @@ -49,6 +52,7 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev)
> > >       uint8_t *data = eeprom->data;
> > >       uint8_t val = data[eeprom->offset++];
> > > +    eeprom->accessed = true;
> > >   #ifdef DEBUG
> > >       printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n",
> > >              dev->i2c.address, val);
> > > @@ -61,6 +65,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
> > >       SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
> > >       uint8_t *data = eeprom->data;
> > > +    eeprom->accessed = true;
> > >   #ifdef DEBUG
> > >       printf("eeprom_write_byte: addr=0x%02x cmd=0x%02x val=0x%02x\n",
> > >              dev->i2c.address, cmd, buf[0]);
> > > @@ -78,15 +83,39 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
> > >       return 0;
> > >   }
> > > +static bool smbus_eeprom_vmstate_needed(void *opaque)
> > > +{
> > > +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> > > +    SMBusEEPROMDevice *eeprom = opaque;
> > > +
> > > +    return (eeprom->accessed || smbus_vmstate_needed(&eeprom->smbusdev)) &&
> > > +        !mc->smbus_no_migration_support;
> > That's a little complx, but OK.
> > 
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_smbus_eeprom = {
> > > +    .name = "smbus-eeprom",
> > > +    .version_id = 1,
> > > +    .minimum_version_id = 1,
> > > +    .needed = smbus_eeprom_vmstate_needed,
> > > +    .fields      = (VMStateField[]) {
> > > +        VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice),
> > > +        VMSTATE_UINT8_ARRAY(data, SMBusEEPROMDevice, SMBUS_EEPROM_SIZE),
> > > +        VMSTATE_UINT8(offset, SMBusEEPROMDevice),
> > > +        VMSTATE_BOOL(accessed, SMBusEEPROMDevice),
> > OK, good - including 'accessed' means that if we migrate a->b->c and
> > it's not changed while it's on b, then 'c' still gets the updated
> > version.
> > 
> > > +        VMSTATE_END_OF_LIST()
> > > +    }
> > > +};
> > > +
> > >   static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
> > >   {
> > >       SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
> > > +    memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
> > >       eeprom->offset = 0;
> > >   }
> > >   static Property smbus_eeprom_properties[] = {
> > > -    DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data),
> > > +    DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
> > >       DEFINE_PROP_END_OF_LIST(),
> > >   };
> > > @@ -99,6 +128,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
> > >       sc->receive_byte = eeprom_receive_byte;
> > >       sc->write_data = eeprom_write_data;
> > >       dc->props = smbus_eeprom_properties;
> > > +    dc->vmsd = &vmstate_smbus_eeprom;
> > >       /* Reason: pointer property "data" */
> > >       dc->user_creatable = false;
> > >   }
> > so from migration side:
> > 
> > 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > > -- 
> > > 2.17.1
> > > 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK