[Qemu-devel] [PATCH 0/2] Add Atmel I2C TPM AT97SC3204T emulated device

[Qemu-devel] [PATCH 0/2] Add Atmel I2C TPM AT97SC3204T emulated device

[Fabio Urquiza]

### Overview ###

The TPM passthrough feature allow a developer to test TPM functionalities,
like Measure Boot, without the need to tamper with critical parts of the
host machine, ie. bootloader. It has been implemented to the x86 architecture
and have the same interface that is provided to PC machines: TPM TIS.

With the growing of use of the ARM server machines, also comes the need to
reuse the same security features that are present in the in the PC server 
environment. So comes the need to use TPM devices in the ARM architecture.

This patchset provides a new frontend to the QEMU TPM passthrough with an
interface suitable to communicate with an ARM based machine.

### Technical details ###

The TPM devices on the PC architecture are integrated in a way that the
interface to it is made using an abstraction layer provided by the BIOS/UFI
firmware. That interface is not available to ARM machines, therefore a new
QEMU front end with a more suitable interface needed to be developed. The
options based on the available TPM devices in the market were I2C and SPI.
To make the choice, we look into the supported TPM drivers available to
ARM architecture in the Linux Kernel. One of the simplest drivers was the
ATMEL I2C TPM AT97SC3204T, so that was our target for the emulation.

We created a file called tpm_i2c_atmel.c based on the already available
tpm_tis.c, registering as a I2C_SLAVE_CLASS and calling the tpm_backend
functions.

One of the problems we had to address is regarding the behavior of the
ATMEL I2C TPM AT97SC3204T Linux driver. After the driver sends a request
to the TPM, it keeps polling the device with I2C read request. The real
AT97SC3204T hardware ignore those requests while the response is not ready
simply by not ACKing the I2C read on its address. When the response is
ready it will ACK the request and proceed writing the response in the wire.

The QEMU I2C API does not provide a way to not ACK I2C requests when the
device is not ready to transmit. In fact, if the device has been configured
in the virtual machine, QEMU will automatically ACK every request without
asking for the device permission for it. Therefore we created a flag in
the I2CSlave struct that tells the I2C subsystem that the device is busy
and not ready to ACK a I2C transfer. We understand that it could not be
the best solution to the problem, but it appears to be the solution that
have the least impact in the code overall. Suggestions on a different
approach would be welcome.

### Testing ###

We tested the feature in the versatilepb machine running Linux with
TrouSerS and tpm_tools:

qemu-system-arm -M versatilepb -kernel output/images/zImage -dtb output/images/versatile-pb.dtb -drive file=output/images/rootfs.ext2,if=scsi,format=raw -append "root=/dev/sda console=ttyAMA0,115200" -net nic,model=rtl8139 -net user -nographic -device tpm-tis,tpmdev=tpm-tpm0,address=0x20 -tpmdev passthrough,id=tpm-tpm0,path=/dev/tpm0,cancel-path=/sys/devices/pnp0/00:08/tpm/tpm0/cancel

The following device needed to be included on the Device Tree:

        i2c0: i2c@10002000 {
                 #address-cells = <1>;
                 #size-cells = <0>;
                 compatible = "arm,versatile-i2c";
                 reg = <0x10002000 0x1000>;
 
                 rtc@68 {
                         compatible = "dallas,ds1338";
                         reg = <0x68>;
                 };


                 tpm@20 {
                         compatible = "atmel,at97sc3204t";
                         reg = <0x20>;
                 };

The following config needed to be enabled in the Linux Kernel:

I2C_VERSATILE=y
TCG_TPM=y
TCG_TIS_I2C_ATMEL=y

Fabio Urquiza (2):
  i2c: Add flag to NACK I2C transfers when busy
  tpm: Add TPM I2C Atmel frontend

 default-configs/aarch64-softmmu.mak |   1 +
 default-configs/arm-softmmu.mak     |   1 +
 hw/i2c/core.c                       |   3 +
 hw/tpm/Makefile.objs                |   1 +
 hw/tpm/tpm_i2c_atmel.c              | 361 ++++++++++++++++++++++++++++++++++++
 include/hw/i2c/i2c.h                |   1 +
 6 files changed, 368 insertions(+)
 create mode 100644 hw/tpm/tpm_i2c_atmel.c

-- 
2.9.3 (Apple Git-75)

[Qemu-devel] [PATCH 1/2] i2c: Add flag to NACK I2C transfers when busy

[Fabio Urquiza]

Add a busy flag on the I2CSlave struct which the device could set to NACK
I2C transfer requests during the execution of the event handling function.
If the busy flag is set, i2c_start_transfer() shall return 1.

Signed-off-by: Fabio Urquiza <flus@cesar.org.br>
---
 hw/i2c/core.c        | 3 +++
 include/hw/i2c/i2c.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index abd4c4c..438233c 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -142,6 +142,9 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
            start condition.  */
         if (sc->event) {
             sc->event(node->elt, recv ? I2C_START_RECV : I2C_START_SEND);
+            if (node->elt->busy) {
+                return -1;
+            }
         }
     }
     return 0;
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index c4085aa..9c6b1ce 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -48,6 +48,7 @@ struct I2CSlave
 
     /* Remaining fields for internal use by the I2C code.  */
     uint8_t address;
+    uint8_t busy;
 };
 
 I2CBus *i2c_init_bus(DeviceState *parent, const char *name);
-- 
2.9.3 (Apple Git-75)

[Qemu-devel] [PATCH 2/2] tpm: Add TPM I2C Atmel frontend

[Fabio Urquiza]

Add a new frontend to the TPM backend that emulate the Atmel I2C TPM
AT97SC3204T device and make it available to use on ARM machines that have
I2C Bus configured.

Signed-off-by: Fabio Urquiza <flus@cesar.org.br>
---
 default-configs/aarch64-softmmu.mak |   1 +
 default-configs/arm-softmmu.mak     |   1 +
 hw/tpm/Makefile.objs                |   1 +
 hw/tpm/tpm_i2c_atmel.c              | 361 ++++++++++++++++++++++++++++++++++++
 4 files changed, 364 insertions(+)
 create mode 100644 hw/tpm/tpm_i2c_atmel.c

diff --git a/default-configs/aarch64-softmmu.mak b/default-configs/aarch64-softmmu.mak
index 2449483..232957e 100644
--- a/default-configs/aarch64-softmmu.mak
+++ b/default-configs/aarch64-softmmu.mak
@@ -7,3 +7,4 @@ CONFIG_AUX=y
 CONFIG_DDC=y
 CONFIG_DPCD=y
 CONFIG_XLNX_ZYNQMP=y
+CONFIG_TPM_I2C_ATMEL=$(CONFIG_TPM)
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index 6de3e16..ef3c8ac 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -115,3 +115,4 @@ CONFIG_ACPI=y
 CONFIG_SMBIOS=y
 CONFIG_ASPEED_SOC=y
 CONFIG_GPIO_KEY=y
+CONFIG_TPM_I2C_ATMEL=$(CONFIG_TPM)
diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
index 64cecc3..0d0f0b1 100644
--- a/hw/tpm/Makefile.objs
+++ b/hw/tpm/Makefile.objs
@@ -1,2 +1,3 @@
 common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
+common-obj-$(CONFIG_TPM_I2C_ATMEL) += tpm_i2c_atmel.o
 common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o tpm_util.o
diff --git a/hw/tpm/tpm_i2c_atmel.c b/hw/tpm/tpm_i2c_atmel.c
new file mode 100644
index 0000000..07af79b
--- /dev/null
+++ b/hw/tpm/tpm_i2c_atmel.c
@@ -0,0 +1,361 @@
+/*
+ * tpm_i2c_atmel.c - QEMU's TPM I2C interface emulator
+ *
+ * Copyright (C) 2012, HPE Corporation
+ *
+ * Authors:
+ *  Fabio Urquiza <fabio.urquiza@hpe.com>
+ *
+ * Based on tpm_tis.c:
+ *  Stefan Berger <stefanb@us.ibm.com>
+ *  David Safford <safford@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * Implementation of the TIS I2C interface according to specs found at
+ * http://www.trustedcomputinggroup.org. This implementation currently
+ * supports version 1.2 Atmel AT97SC3204T CI, 10 December 2016
+ *
+ * TPM I2C for TPM 2 implementation following TCG TPM I2C Interface
+ * Specification TPM Profile (PTP) Specification, Familiy 2.0, Revision 1.0
+ */
+
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qemu/main-loop.h"
+#include "hw/i2c/i2c.h"
+#include "qemu/bcd.h"
+#include "sysemu/tpm_backend.h"
+#include "tpm_int.h"
+#include "qapi/error.h"
+
+#define DEBUG_TIS 0
+
+#define DPRINTF(fmt, ...) do { \
+    if (DEBUG_TIS) { \
+        printf(fmt, ## __VA_ARGS__); \
+    } \
+} while (0);
+
+/* vendor-specific registers */
+#define TPM_TIS_STS_TPM_FAMILY_MASK         (0x3 << 26)/* TPM 2.0 */
+#define TPM_TIS_STS_TPM_FAMILY1_2           (0 << 26)  /* TPM 2.0 */
+#define TPM_TIS_STS_TPM_FAMILY2_0           (1 << 26)  /* TPM 2.0 */
+
+#define TPM_TIS_STS_VALID                 (1 << 7)
+#define TPM_TIS_STS_DATA_AVAILABLE        (1 << 4)
+#define TPM_TIS_STS_SELFTEST_DONE         (1 << 2)
+
+#define TPM_TIS_ACCESS_TPM_REG_VALID_STS  (1 << 7)
+
+#define TPM_TIS_IFACE_ID_INTERFACE_TIS1_3   (0xf)     /* TPM 2.0 */
+#define TPM_TIS_IFACE_ID_INTERFACE_FIFO     (0x0)     /* TPM 2.0 */
+#define TPM_TIS_IFACE_ID_INTERFACE_VER_FIFO (0 << 4)  /* TPM 2.0 */
+#define TPM_TIS_IFACE_ID_CAP_5_LOCALITIES   (1 << 8)  /* TPM 2.0 */
+#define TPM_TIS_IFACE_ID_CAP_TIS_SUPPORTED  (1 << 13) /* TPM 2.0 */
+#define TPM_TIS_IFACE_ID_INT_SEL_LOCK       (1 << 19) /* TPM 2.0 */
+
+#define TPM_TIS_IFACE_ID_SUPPORTED_FLAGS1_3 \
+    (TPM_TIS_IFACE_ID_INTERFACE_TIS1_3 | \
+     (~0u << 4)/* all of it is don't care */)
+
+/* if backend was a TPM 2.0: */
+#define TPM_TIS_IFACE_ID_SUPPORTED_FLAGS2_0 \
+    (TPM_TIS_IFACE_ID_INTERFACE_FIFO | \
+     TPM_TIS_IFACE_ID_INTERFACE_VER_FIFO | \
+     TPM_TIS_IFACE_ID_CAP_5_LOCALITIES | \
+     TPM_TIS_IFACE_ID_CAP_TIS_SUPPORTED)
+
+#define TPM_TIS_NO_DATA_BYTE  0xff
+
+static const VMStateDescription vmstate_tpm_i2c_atmel = {
+    .name = "tpm",
+    .unmigratable = 1,
+};
+
+static uint32_t tpm_i2c_atmel_get_size_from_buffer(const TPMSizedBuffer *sb)
+{
+    return be32_to_cpu(*(uint32_t *)&sb->buffer[2]);
+}
+
+static void tpm_i2c_atmel_show_buffer(const TPMSizedBuffer *sb, const char *string)
+{
+#ifdef DEBUG_TIS
+    uint32_t len, i;
+
+    len = tpm_i2c_atmel_get_size_from_buffer(sb);
+    DPRINTF("tpm_tis: %s length = %d\n", string, len);
+    for (i = 0; i < len; i++) {
+        if (i && !(i % 16)) {
+            DPRINTF("\n");
+        }
+        DPRINTF("%.2X ", sb->buffer[i]);
+    }
+    DPRINTF("\n");
+#endif
+}
+
+/*
+ * Set the given flags in the STS register by clearing the register but
+ * preserving the SELFTEST_DONE and TPM_FAMILY_MASK flags and then setting
+ * the new flags.
+ *
+ * The SELFTEST_DONE flag is acquired from the backend that determines it by
+ * peeking into TPM commands.
+ *
+ * A VM suspend/resume will preserve the flag by storing it into the VM
+ * device state, but the backend will not remember it when QEMU is started
+ * again. Therefore, we cache the flag here. Once set, it will not be unset
+ * except by a reset.
+ */
+static inline void tpm_i2c_atmel_sts_set(TPMLocality *l, uint32_t flags)
+{
+    l->sts &= TPM_TIS_STS_SELFTEST_DONE | TPM_TIS_STS_TPM_FAMILY_MASK;
+    l->sts |= flags;
+}
+
+static inline uint32_t tpm_i2c_atmel_tpm_start_recv(TPMState *s)
+{
+    TPMTISEmuState *tis = &s->s.tis;
+    tis->loc[0].r_offset = 0;
+
+    return !(tis->loc[0].sts & TPM_TIS_STS_DATA_AVAILABLE);
+}
+
+static inline void tpm_i2c_atmel_tpm_start_send(TPMState *s)
+{
+    TPMTISEmuState *tis = &s->s.tis;
+    tis->loc[0].r_offset = 0;
+    tis->loc[0].w_offset = 0;
+}
+
+/*
+ * Send a request to the TPM.
+ */
+static inline void tpm_i2c_atmel_tpm_send(TPMState *s)
+{
+    TPMTISEmuState *tis = &s->s.tis;
+
+    if (tis->loc[0].w_offset &&
+        tis->loc[0].state != TPM_TIS_STATE_EXECUTION) {
+        tpm_i2c_atmel_show_buffer(&tis->loc[0].w_buffer, "tpm_tis: To TPM");
+
+        s->locty_number = 0;
+        s->locty_data = &tis->loc[0];
+
+        /*
+         * w_offset serves as length indicator for length of data;
+         * it's reset when the response comes back
+         */
+        tis->loc[0].state = TPM_TIS_STATE_EXECUTION;
+
+        tpm_backend_deliver_request(s->be_driver);
+    }
+}
+
+
+static void tpm_i2c_atmel_receive_bh(void *opaque)
+{
+    TPMState *s = opaque;
+    TPMTISEmuState *tis = &s->s.tis;
+
+    tpm_i2c_atmel_sts_set(&tis->loc[0],
+                    TPM_TIS_STS_VALID | TPM_TIS_STS_DATA_AVAILABLE);
+    tis->loc[0].state = TPM_TIS_STATE_COMPLETION;
+    tis->loc[0].r_offset = 0;
+    tis->loc[0].w_offset = 0;
+    DPRINTF("tpm_i2c_atmel: tpm_i2c_atmel_receive_bh");
+
+}
+
+/*
+ * Read a byte of response data
+ */
+static inline uint32_t tpm_i2c_atmel_data_read(TPMState *s)
+{
+    TPMTISEmuState *tis = &s->s.tis;
+    uint32_t ret = TPM_TIS_NO_DATA_BYTE;
+    uint16_t len;
+
+    if ((tis->loc[0].sts & TPM_TIS_STS_DATA_AVAILABLE)) {
+        len = tpm_i2c_atmel_get_size_from_buffer(&tis->loc[0].r_buffer);
+
+        ret = tis->loc[0].r_buffer.buffer[tis->loc[0].r_offset++];
+        if (tis->loc[0].r_offset >= len) {
+            /* got last byte */
+            tpm_i2c_atmel_sts_set(&tis->loc[0], TPM_TIS_STS_VALID);
+        }
+        DPRINTF("tpm_i2c_atmel: tpm_i2c_atmel_data_read byte 0x%02x   [%d]\n",
+                ret, tis->loc[0].r_offset-1);
+    } else {
+        DPRINTF("tpm_i2c_atmel: !TPM_TIS_STS_DATA_AVAILABLE [%d]\n",
+                tis->loc[0].sts);
+    }
+
+    return ret;
+}
+
+static void tpm_i2c_atmel_event(I2CSlave *i2c, enum i2c_event event)
+{
+    TPMState *s = TPM(&(i2c->qdev));
+    i2c->busy = 0;
+
+    switch (event) {
+    case I2C_START_RECV:
+        i2c->busy = tpm_i2c_atmel_tpm_start_recv(s);
+        break;
+    case I2C_START_SEND:
+        tpm_i2c_atmel_tpm_start_send(s);
+        break;
+    case I2C_FINISH:
+        tpm_i2c_atmel_tpm_send(s);
+        break;
+    default:
+        break;
+    }
+}
+
+static int tpm_i2c_atmel_recv(I2CSlave *i2c)
+{
+    TPMState *s = TPM(&(i2c->qdev));
+    return tpm_i2c_atmel_data_read(s);
+}
+
+static int tpm_i2c_atmel_send(I2CSlave *i2c, uint8_t data)
+{
+    TPMState *s = TPM(&(i2c->qdev));
+    TPMTISEmuState *tis = &s->s.tis;
+    tis->loc[0].w_buffer.buffer[tis->loc[0].w_offset++] = data;
+    return 0;
+}
+
+static void tpm_i2c_atmel_receive_cb(TPMState *s, uint8_t locty,
+                               bool is_selftest_done)
+{
+    TPMTISEmuState *tis = &s->s.tis;
+
+    assert(locty == 0);
+
+    if (is_selftest_done) {
+        tis->loc[0].sts |= TPM_TIS_STS_SELFTEST_DONE;
+    }
+
+    qemu_bh_schedule(tis->bh);
+}
+
+
+static void tpm_i2c_atmel_realizefn(DeviceState *dev, Error **errp)
+{
+    TPMState *s = TPM(dev);
+    TPMTISEmuState *tis = &s->s.tis;
+
+    DPRINTF("backend %s\n", s->backend);
+    s->be_driver = qemu_find_tpm(s->backend);
+    if (!s->be_driver) {
+        error_setg(errp, "tpm_i2c_atmel: backend driver with id %s could not be "
+                   "found", s->backend);
+        return;
+    }
+
+    s->be_driver->fe_model = TPM_MODEL_TPM_TIS;
+
+    if (tpm_backend_init(s->be_driver, s, tpm_i2c_atmel_receive_cb)) {
+        error_setg(errp, "tpm_i2c_atmel: backend driver with id %s could not be "
+                   "initialized", s->backend);
+        return;
+    }
+
+    if (tis->irq_num > 15) {
+        error_setg(errp, "tpm_i2c_atmel: IRQ %d for TPM TIS is outside valid range "
+                   "of 0 to 15", tis->irq_num);
+        return;
+    }
+
+    tis->bh = qemu_bh_new(tpm_i2c_atmel_receive_bh, s);
+}
+
+static int tpm_i2c_atmel_do_startup_tpm(TPMState *s)
+{
+    return tpm_backend_startup_tpm(s->be_driver);
+}
+
+
+static int tpm_i2c_atmel_init(I2CSlave *i2c)
+{
+    return 0;
+}
+
+static void tpm_i2c_atmel_reset(DeviceState *dev)
+{
+    TPMState *s = TPM(dev);
+    TPMTISEmuState *tis = &s->s.tis;
+
+    s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver);
+
+    tpm_backend_reset(s->be_driver);
+
+    tis->active_locty = TPM_TIS_NO_LOCALITY;
+    tis->next_locty = TPM_TIS_NO_LOCALITY;
+    tis->aborting_locty = TPM_TIS_NO_LOCALITY;
+
+    /* ATMEL AT97SC3204T only uses locality 0 */
+    memset(tis->loc, 0, sizeof(tis->loc));
+    tis->loc[0].access = TPM_TIS_ACCESS_TPM_REG_VALID_STS;
+    switch (s->be_tpm_version) {
+    case TPM_VERSION_UNSPEC:
+        break;
+    case TPM_VERSION_1_2:
+        tis->loc[0].sts = TPM_TIS_STS_TPM_FAMILY1_2;
+        tis->loc[0].iface_id = TPM_TIS_IFACE_ID_SUPPORTED_FLAGS1_3;
+        break;
+    case TPM_VERSION_2_0:
+        tis->loc[0].sts = TPM_TIS_STS_TPM_FAMILY2_0;
+        tis->loc[0].iface_id = TPM_TIS_IFACE_ID_SUPPORTED_FLAGS2_0;
+        break;
+    }
+        tis->loc[0].state = TPM_TIS_STATE_IDLE;
+
+    tpm_backend_realloc_buffer(s->be_driver, &tis->loc[0].w_buffer);
+    tpm_backend_realloc_buffer(s->be_driver, &tis->loc[0].r_buffer);
+
+    tpm_i2c_atmel_do_startup_tpm(s);
+}
+
+static Property tpm_tis_properties[] = {
+    DEFINE_PROP_UINT32("irq", TPMState,
+                       s.tis.irq_num, TPM_TIS_IRQ),
+    DEFINE_PROP_STRING("tpmdev", TPMState, backend),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void tpm_i2c_atmel_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
+
+    k->init = tpm_i2c_atmel_init;
+    k->event = tpm_i2c_atmel_event;
+    k->recv = tpm_i2c_atmel_recv;
+    k->send = tpm_i2c_atmel_send;
+    dc->realize = tpm_i2c_atmel_realizefn;
+    dc->props = tpm_tis_properties;
+    dc->reset = tpm_i2c_atmel_reset;
+    dc->vmsd = &vmstate_tpm_i2c_atmel;
+}
+
+static const TypeInfo tpm_i2c_atmel_info = {
+    .name          = TYPE_TPM_TIS,
+    .parent        = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(TPMState),
+    .class_init    = tpm_i2c_atmel_class_init,
+};
+
+static void tpm_i2c_atmel_register_types(void)
+{
+    type_register_static(&tpm_i2c_atmel_info);
+    tpm_register_model(TPM_MODEL_TPM_TIS);
+}
+
+type_init(tpm_i2c_atmel_register_types)
-- 
2.9.3 (Apple Git-75)

Re: [Qemu-devel] [PATCH 0/2] Add Atmel I2C TPM AT97SC3204T emulated device

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH 0/2] Add Atmel I2C TPM AT97SC3204T emulated device
Type: series
Message-id: 20161123194605.94717-1-flus@cesar.org.br

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
46ef6f2 tpm: Add TPM I2C Atmel frontend
24c07aa i2c: Add flag to NACK I2C transfers when busy

=== OUTPUT BEGIN ===
Checking PATCH 1/2: i2c: Add flag to NACK I2C transfers when busy...
Checking PATCH 2/2: tpm: Add TPM I2C Atmel frontend...
WARNING: line over 80 characters
#127: FILE: hw/tpm/tpm_i2c_atmel.c:83:
+static void tpm_i2c_atmel_show_buffer(const TPMSizedBuffer *sb, const char *string)

ERROR: spaces required around that '-' (ctx:VxV)
#235: FILE: hw/tpm/tpm_i2c_atmel.c:191:
+                ret, tis->loc[0].r_offset-1);
                                          ^

WARNING: line over 80 characters
#301: FILE: hw/tpm/tpm_i2c_atmel.c:257:
+        error_setg(errp, "tpm_i2c_atmel: backend driver with id %s could not be "

WARNING: line over 80 characters
#309: FILE: hw/tpm/tpm_i2c_atmel.c:265:
+        error_setg(errp, "tpm_i2c_atmel: backend driver with id %s could not be "

WARNING: line over 80 characters
#315: FILE: hw/tpm/tpm_i2c_atmel.c:271:
+        error_setg(errp, "tpm_i2c_atmel: IRQ %d for TPM TIS is outside valid range "

total: 1 errors, 4 warnings, 372 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

[Qemu-devel] [PATCH v1 0/2] Add Atmel I2C TPM AT97SC3204T emulated device

[Fabio Urquiza]

### Overview ###

The TPM passthrough feature allow a developer to test TPM functionalities,
like Measure Boot, without the need to tamper with critical parts of the
host machine, ie. bootloader. It has been implemented to the x86 architecture
and have the same interface that is provided to PC machines: TPM TIS.

With the growing of use of the ARM server machines, also comes the need to
reuse the same security features that are present in the in the PC server 
environment. So comes the need to use TPM devices in the ARM architecture.

This patchset provides a new frontend to the QEMU TPM passthrough with an
interface suitable to communicate with an ARM based machine.

### Technical details ###

The TPM devices on the PC architecture are integrated in a way that the
interface to it is made using an abstraction layer provided by the BIOS/UFI
firmware. That interface is not available to ARM machines, therefore a new
QEMU front end with a more suitable interface needed to be developed. The
options based on the available TPM devices in the market were I2C and SPI.
To make the choice, we look into the supported TPM drivers available to
ARM architecture in the Linux Kernel. One of the simplest drivers was the
ATMEL I2C TPM AT97SC3204T, so that was our target for the emulation.

We created a file called tpm_i2c_atmel.c based on the already available
tpm_tis.c, registering as a I2C_SLAVE_CLASS and calling the tpm_backend
functions.

One of the problems we had to address is regarding the behavior of the
ATMEL I2C TPM AT97SC3204T Linux driver. After the driver sends a request
to the TPM, it keeps polling the device with I2C read request. The real
AT97SC3204T hardware ignore those requests while the response is not ready
simply by not ACKing the I2C read on its address. When the response is
ready it will ACK the request and proceed writing the response in the wire.

The QEMU I2C API does not provide a way to not ACK I2C requests when the
device is not ready to transmit. In fact, if the device has been configured
in the virtual machine, QEMU will automatically ACK every request without
asking for the device permission for it. Therefore we created a flag in
the I2CSlave struct that tells the I2C subsystem that the device is busy
and not ready to ACK a I2C transfer. We understand that it could not be
the best solution to the problem, but it appears to be the solution that
have the least impact in the code overall. Suggestions on a different
approach would be welcome.

### Testing ###

We tested the feature in the versatilepb machine running Linux with
TrouSerS and tpm_tools:

qemu-system-arm -M versatilepb -kernel output/images/zImage \
	-dtb output/images/versatile-pb.dtb \
	-drive file=output/images/rootfs.ext2,if=scsi,format=raw \
	-append "root=/dev/sda console=ttyAMA0,115200" -net nic,model=rtl8139 \
	-net user -nographic -device tpm-tis,tpmdev=tpm-tpm0,address=0x20 \
	-tpmdev passthrough,id=tpm-tpm0,path=/dev/tpm0,cancel-path=/sys/devices/pnp0/00:08/tpm/tpm0/cancel

The following device needed to be included on the Device Tree:

        i2c0: i2c@10002000 {
                 #address-cells = <1>;
                 #size-cells = <0>;
                 compatible = "arm,versatile-i2c";
                 reg = <0x10002000 0x1000>;
 
                 rtc@68 {
                         compatible = "dallas,ds1338";
                         reg = <0x68>;
                 };


                 tpm@20 {
                         compatible = "atmel,at97sc3204t";
                         reg = <0x20>;
                 };

The following config needed to be enabled in the Linux Kernel:

I2C_VERSATILE=y
TCG_TPM=y
TCG_TIS_I2C_ATMEL=y

### Changes from V0 to v1 ###

 - Fixed coding style problems found by Patchew.

Fabio Urquiza (2):
  i2c: Add flag to NACK I2C transfers when busy
  tpm: Add TPM I2C Atmel frontend

 default-configs/aarch64-softmmu.mak |   1 +
 default-configs/arm-softmmu.mak     |   1 +
 hw/i2c/core.c                       |   3 +
 hw/tpm/Makefile.objs                |   1 +
 hw/tpm/tpm_i2c_atmel.c              | 362 ++++++++++++++++++++++++++++++++++++
 include/hw/i2c/i2c.h                |   1 +
 6 files changed, 369 insertions(+)
 create mode 100644 hw/tpm/tpm_i2c_atmel.c

-- 
2.8.3

[Qemu-devel] [PATCH v1 1/2] i2c: Add flag to NACK I2C transfers when busy

[Fabio Urquiza]

Add a busy flag on the I2CSlave struct which the device could set to NACK
I2C transfer requests during the execution of the event handling function.
If the busy flag is set, i2c_start_transfer() shall return 1.

Signed-off-by: Fabio Urquiza <flus@cesar.org.br>
---
 hw/i2c/core.c        | 3 +++
 include/hw/i2c/i2c.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index abd4c4c..f36020f 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -142,6 +142,9 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
            start condition.  */
         if (sc->event) {
             sc->event(node->elt, recv ? I2C_START_RECV : I2C_START_SEND);
+            if (node->elt->busy) {
+                return 1;
+            }
         }
     }
     return 0;
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index c4085aa..9c6b1ce 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -48,6 +48,7 @@ struct I2CSlave
 
     /* Remaining fields for internal use by the I2C code.  */
     uint8_t address;
+    uint8_t busy;
 };
 
 I2CBus *i2c_init_bus(DeviceState *parent, const char *name);
-- 
2.8.3

[Qemu-devel] [PATCH v1 2/2] tpm: Add TPM I2C Atmel frontend

[Fabio Urquiza]

Add a new frontend to the TPM backend that emulate the Atmel I2C TPM
AT97SC3204T device and make it available to use on ARM machines that have
I2C Bus configured.

Signed-off-by: Fabio Urquiza <flus@cesar.org.br>
---
 default-configs/aarch64-softmmu.mak |   1 +
 default-configs/arm-softmmu.mak     |   1 +
 hw/tpm/Makefile.objs                |   1 +
 hw/tpm/tpm_i2c_atmel.c              | 362 ++++++++++++++++++++++++++++++++++++
 4 files changed, 365 insertions(+)
 create mode 100644 hw/tpm/tpm_i2c_atmel.c

diff --git a/default-configs/aarch64-softmmu.mak b/default-configs/aarch64-softmmu.mak
index 2449483..232957e 100644
--- a/default-configs/aarch64-softmmu.mak
+++ b/default-configs/aarch64-softmmu.mak
@@ -7,3 +7,4 @@ CONFIG_AUX=y
 CONFIG_DDC=y
 CONFIG_DPCD=y
 CONFIG_XLNX_ZYNQMP=y
+CONFIG_TPM_I2C_ATMEL=$(CONFIG_TPM)
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index 6de3e16..ef3c8ac 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -115,3 +115,4 @@ CONFIG_ACPI=y
 CONFIG_SMBIOS=y
 CONFIG_ASPEED_SOC=y
 CONFIG_GPIO_KEY=y
+CONFIG_TPM_I2C_ATMEL=$(CONFIG_TPM)
diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
index 64cecc3..0d0f0b1 100644
--- a/hw/tpm/Makefile.objs
+++ b/hw/tpm/Makefile.objs
@@ -1,2 +1,3 @@
 common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
+common-obj-$(CONFIG_TPM_I2C_ATMEL) += tpm_i2c_atmel.o
 common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o tpm_util.o
diff --git a/hw/tpm/tpm_i2c_atmel.c b/hw/tpm/tpm_i2c_atmel.c
new file mode 100644
index 0000000..774b3c3
--- /dev/null
+++ b/hw/tpm/tpm_i2c_atmel.c
@@ -0,0 +1,362 @@
+/*
+ * tpm_i2c_atmel.c - QEMU's TPM I2C interface emulator
+ *
+ * Copyright (C) 2012, HPE Corporation
+ *
+ * Authors:
+ *  Fabio Urquiza <flus@cesar.org.br>
+ *
+ * Based on tpm_tis.c:
+ *  Stefan Berger <stefanb@us.ibm.com>
+ *  David Safford <safford@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * Implementation of the TIS I2C interface according to specs found at
+ * http://www.trustedcomputinggroup.org. This implementation currently
+ * supports version 1.2 Atmel AT97SC3204T CI, 10 December 2016
+ *
+ * TPM I2C for TPM 2 implementation following TCG TPM I2C Interface
+ * Specification TPM Profile (PTP) Specification, Familiy 2.0, Revision 1.0
+ */
+
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qemu/main-loop.h"
+#include "hw/i2c/i2c.h"
+#include "qemu/bcd.h"
+#include "sysemu/tpm_backend.h"
+#include "tpm_int.h"
+#include "qapi/error.h"
+
+#define DEBUG_TIS 0
+
+#define DPRINTF(fmt, ...) do { \
+    if (DEBUG_TIS) { \
+        printf(fmt, ## __VA_ARGS__); \
+    } \
+} while (0);
+
+/* vendor-specific registers */
+#define TPM_TIS_STS_TPM_FAMILY_MASK         (0x3 << 26)/* TPM 2.0 */
+#define TPM_TIS_STS_TPM_FAMILY1_2           (0 << 26)  /* TPM 2.0 */
+#define TPM_TIS_STS_TPM_FAMILY2_0           (1 << 26)  /* TPM 2.0 */
+
+#define TPM_TIS_STS_VALID                 (1 << 7)
+#define TPM_TIS_STS_DATA_AVAILABLE        (1 << 4)
+#define TPM_TIS_STS_SELFTEST_DONE         (1 << 2)
+
+#define TPM_TIS_ACCESS_TPM_REG_VALID_STS  (1 << 7)
+
+#define TPM_TIS_IFACE_ID_INTERFACE_TIS1_3   (0xf)     /* TPM 2.0 */
+#define TPM_TIS_IFACE_ID_INTERFACE_FIFO     (0x0)     /* TPM 2.0 */
+#define TPM_TIS_IFACE_ID_INTERFACE_VER_FIFO (0 << 4)  /* TPM 2.0 */
+#define TPM_TIS_IFACE_ID_CAP_5_LOCALITIES   (1 << 8)  /* TPM 2.0 */
+#define TPM_TIS_IFACE_ID_CAP_TIS_SUPPORTED  (1 << 13) /* TPM 2.0 */
+#define TPM_TIS_IFACE_ID_INT_SEL_LOCK       (1 << 19) /* TPM 2.0 */
+
+#define TPM_TIS_IFACE_ID_SUPPORTED_FLAGS1_3 \
+    (TPM_TIS_IFACE_ID_INTERFACE_TIS1_3 | \
+     (~0u << 4)/* all of it is don't care */)
+
+/* if backend was a TPM 2.0: */
+#define TPM_TIS_IFACE_ID_SUPPORTED_FLAGS2_0 \
+    (TPM_TIS_IFACE_ID_INTERFACE_FIFO | \
+     TPM_TIS_IFACE_ID_INTERFACE_VER_FIFO | \
+     TPM_TIS_IFACE_ID_CAP_5_LOCALITIES | \
+     TPM_TIS_IFACE_ID_CAP_TIS_SUPPORTED)
+
+#define TPM_TIS_NO_DATA_BYTE  0xff
+
+static const VMStateDescription vmstate_tpm_i2c_atmel = {
+    .name = "tpm",
+    .unmigratable = 1,
+};
+
+static uint32_t tpm_i2c_atmel_get_size_from_buffer(const TPMSizedBuffer *sb)
+{
+    return be32_to_cpu(*(uint32_t *)&sb->buffer[2]);
+}
+
+static void tpm_i2c_atmel_show_buffer(const TPMSizedBuffer *sb,
+    const char *string)
+{
+#ifdef DEBUG_TIS
+    uint32_t len, i;
+
+    len = tpm_i2c_atmel_get_size_from_buffer(sb);
+    DPRINTF("tpm_tis: %s length = %d\n", string, len);
+    for (i = 0; i < len; i++) {
+        if (i && !(i % 16)) {
+            DPRINTF("\n");
+        }
+        DPRINTF("%.2X ", sb->buffer[i]);
+    }
+    DPRINTF("\n");
+#endif
+}
+
+/*
+ * Set the given flags in the STS register by clearing the register but
+ * preserving the SELFTEST_DONE and TPM_FAMILY_MASK flags and then setting
+ * the new flags.
+ *
+ * The SELFTEST_DONE flag is acquired from the backend that determines it by
+ * peeking into TPM commands.
+ *
+ * A VM suspend/resume will preserve the flag by storing it into the VM
+ * device state, but the backend will not remember it when QEMU is started
+ * again. Therefore, we cache the flag here. Once set, it will not be unset
+ * except by a reset.
+ */
+static inline void tpm_i2c_atmel_sts_set(TPMLocality *l, uint32_t flags)
+{
+    l->sts &= TPM_TIS_STS_SELFTEST_DONE | TPM_TIS_STS_TPM_FAMILY_MASK;
+    l->sts |= flags;
+}
+
+static inline uint32_t tpm_i2c_atmel_tpm_start_recv(TPMState *s)
+{
+    TPMTISEmuState *tis = &s->s.tis;
+    tis->loc[0].r_offset = 0;
+
+    return !(tis->loc[0].sts & TPM_TIS_STS_DATA_AVAILABLE);
+}
+
+static inline void tpm_i2c_atmel_tpm_start_send(TPMState *s)
+{
+    TPMTISEmuState *tis = &s->s.tis;
+    tis->loc[0].r_offset = 0;
+    tis->loc[0].w_offset = 0;
+}
+
+/*
+ * Send a request to the TPM.
+ */
+static inline void tpm_i2c_atmel_tpm_send(TPMState *s)
+{
+    TPMTISEmuState *tis = &s->s.tis;
+
+    if (tis->loc[0].w_offset &&
+        tis->loc[0].state != TPM_TIS_STATE_EXECUTION) {
+        tpm_i2c_atmel_show_buffer(&tis->loc[0].w_buffer, "tpm_tis: To TPM");
+
+        s->locty_number = 0;
+        s->locty_data = &tis->loc[0];
+
+        /*
+         * w_offset serves as length indicator for length of data;
+         * it's reset when the response comes back
+         */
+        tis->loc[0].state = TPM_TIS_STATE_EXECUTION;
+
+        tpm_backend_deliver_request(s->be_driver);
+    }
+}
+
+
+static void tpm_i2c_atmel_receive_bh(void *opaque)
+{
+    TPMState *s = opaque;
+    TPMTISEmuState *tis = &s->s.tis;
+
+    tpm_i2c_atmel_sts_set(&tis->loc[0],
+                    TPM_TIS_STS_VALID | TPM_TIS_STS_DATA_AVAILABLE);
+    tis->loc[0].state = TPM_TIS_STATE_COMPLETION;
+    tis->loc[0].r_offset = 0;
+    tis->loc[0].w_offset = 0;
+    DPRINTF("tpm_i2c_atmel: tpm_i2c_atmel_receive_bh");
+
+}
+
+/*
+ * Read a byte of response data
+ */
+static inline uint32_t tpm_i2c_atmel_data_read(TPMState *s)
+{
+    TPMTISEmuState *tis = &s->s.tis;
+    uint32_t ret = TPM_TIS_NO_DATA_BYTE;
+    uint16_t len;
+
+    if ((tis->loc[0].sts & TPM_TIS_STS_DATA_AVAILABLE)) {
+        len = tpm_i2c_atmel_get_size_from_buffer(&tis->loc[0].r_buffer);
+
+        ret = tis->loc[0].r_buffer.buffer[tis->loc[0].r_offset++];
+        if (tis->loc[0].r_offset >= len) {
+            /* got last byte */
+            tpm_i2c_atmel_sts_set(&tis->loc[0], TPM_TIS_STS_VALID);
+        }
+        DPRINTF("tpm_i2c_atmel: tpm_i2c_atmel_data_read byte 0x%02x   [%d]\n",
+                ret, tis->loc[0].r_offset - 1);
+    } else {
+        DPRINTF("tpm_i2c_atmel: !TPM_TIS_STS_DATA_AVAILABLE [%d]\n",
+                tis->loc[0].sts);
+    }
+
+    return ret;
+}
+
+static void tpm_i2c_atmel_event(I2CSlave *i2c, enum i2c_event event)
+{
+    TPMState *s = TPM(&(i2c->qdev));
+    i2c->busy = 0;
+
+    switch (event) {
+    case I2C_START_RECV:
+        i2c->busy = tpm_i2c_atmel_tpm_start_recv(s);
+        break;
+    case I2C_START_SEND:
+        tpm_i2c_atmel_tpm_start_send(s);
+        break;
+    case I2C_FINISH:
+        tpm_i2c_atmel_tpm_send(s);
+        break;
+    default:
+        break;
+    }
+}
+
+static int tpm_i2c_atmel_recv(I2CSlave *i2c)
+{
+    TPMState *s = TPM(&(i2c->qdev));
+    return tpm_i2c_atmel_data_read(s);
+}
+
+static int tpm_i2c_atmel_send(I2CSlave *i2c, uint8_t data)
+{
+    TPMState *s = TPM(&(i2c->qdev));
+    TPMTISEmuState *tis = &s->s.tis;
+    tis->loc[0].w_buffer.buffer[tis->loc[0].w_offset++] = data;
+    return 0;
+}
+
+static void tpm_i2c_atmel_receive_cb(TPMState *s, uint8_t locty,
+                               bool is_selftest_done)
+{
+    TPMTISEmuState *tis = &s->s.tis;
+
+    assert(locty == 0);
+
+    if (is_selftest_done) {
+        tis->loc[0].sts |= TPM_TIS_STS_SELFTEST_DONE;
+    }
+
+    qemu_bh_schedule(tis->bh);
+}
+
+
+static void tpm_i2c_atmel_realizefn(DeviceState *dev, Error **errp)
+{
+    TPMState *s = TPM(dev);
+    TPMTISEmuState *tis = &s->s.tis;
+
+    DPRINTF("backend %s\n", s->backend);
+    s->be_driver = qemu_find_tpm(s->backend);
+    if (!s->be_driver) {
+        error_setg(errp, "tpm_i2c_atmel: backend driver with id %s could not "
+                   "be found", s->backend);
+        return;
+    }
+
+    s->be_driver->fe_model = TPM_MODEL_TPM_TIS;
+
+    if (tpm_backend_init(s->be_driver, s, tpm_i2c_atmel_receive_cb)) {
+        error_setg(errp, "tpm_i2c_atmel: backend driver with id %s could not "
+                   "be initialized", s->backend);
+        return;
+    }
+
+    if (tis->irq_num > 15) {
+        error_setg(errp, "tpm_i2c_atmel: IRQ %d for TPM TIS is outside valid "
+                   "range of 0 to 15", tis->irq_num);
+        return;
+    }
+
+    tis->bh = qemu_bh_new(tpm_i2c_atmel_receive_bh, s);
+}
+
+static int tpm_i2c_atmel_do_startup_tpm(TPMState *s)
+{
+    return tpm_backend_startup_tpm(s->be_driver);
+}
+
+
+static int tpm_i2c_atmel_init(I2CSlave *i2c)
+{
+    return 0;
+}
+
+static void tpm_i2c_atmel_reset(DeviceState *dev)
+{
+    TPMState *s = TPM(dev);
+    TPMTISEmuState *tis = &s->s.tis;
+
+    s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver);
+
+    tpm_backend_reset(s->be_driver);
+
+    tis->active_locty = TPM_TIS_NO_LOCALITY;
+    tis->next_locty = TPM_TIS_NO_LOCALITY;
+    tis->aborting_locty = TPM_TIS_NO_LOCALITY;
+
+    /* ATMEL AT97SC3204T only uses locality 0 */
+    memset(tis->loc, 0, sizeof(tis->loc));
+    tis->loc[0].access = TPM_TIS_ACCESS_TPM_REG_VALID_STS;
+    switch (s->be_tpm_version) {
+    case TPM_VERSION_UNSPEC:
+        break;
+    case TPM_VERSION_1_2:
+        tis->loc[0].sts = TPM_TIS_STS_TPM_FAMILY1_2;
+        tis->loc[0].iface_id = TPM_TIS_IFACE_ID_SUPPORTED_FLAGS1_3;
+        break;
+    case TPM_VERSION_2_0:
+        tis->loc[0].sts = TPM_TIS_STS_TPM_FAMILY2_0;
+        tis->loc[0].iface_id = TPM_TIS_IFACE_ID_SUPPORTED_FLAGS2_0;
+        break;
+    }
+        tis->loc[0].state = TPM_TIS_STATE_IDLE;
+
+    tpm_backend_realloc_buffer(s->be_driver, &tis->loc[0].w_buffer);
+    tpm_backend_realloc_buffer(s->be_driver, &tis->loc[0].r_buffer);
+
+    tpm_i2c_atmel_do_startup_tpm(s);
+}
+
+static Property tpm_tis_properties[] = {
+    DEFINE_PROP_UINT32("irq", TPMState,
+                       s.tis.irq_num, TPM_TIS_IRQ),
+    DEFINE_PROP_STRING("tpmdev", TPMState, backend),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void tpm_i2c_atmel_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
+
+    k->init = tpm_i2c_atmel_init;
+    k->event = tpm_i2c_atmel_event;
+    k->recv = tpm_i2c_atmel_recv;
+    k->send = tpm_i2c_atmel_send;
+    dc->realize = tpm_i2c_atmel_realizefn;
+    dc->props = tpm_tis_properties;
+    dc->reset = tpm_i2c_atmel_reset;
+    dc->vmsd = &vmstate_tpm_i2c_atmel;
+}
+
+static const TypeInfo tpm_i2c_atmel_info = {
+    .name          = TYPE_TPM_TIS,
+    .parent        = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(TPMState),
+    .class_init    = tpm_i2c_atmel_class_init,
+};
+
+static void tpm_i2c_atmel_register_types(void)
+{
+    type_register_static(&tpm_i2c_atmel_info);
+    tpm_register_model(TPM_MODEL_TPM_TIS);
+}
+
+type_init(tpm_i2c_atmel_register_types)
-- 
2.8.3

Re: [Qemu-devel] [PATCH v1 0/2] Add Atmel I2C TPM AT97SC3204T emulated device

[Peter Maydell]

(added a couple of people to cc who might have an opinion on the i2c
protocol questions below)

On 29 November 2016 at 19:30, Fabio Urquiza <flus@cesar.org.br> wrote:
> ### Overview ###
>
> The TPM passthrough feature allow a developer to test TPM functionalities,
> like Measure Boot, without the need to tamper with critical parts of the
> host machine, ie. bootloader. It has been implemented to the x86 architecture
> and have the same interface that is provided to PC machines: TPM TIS.
>
> With the growing of use of the ARM server machines, also comes the need to
> reuse the same security features that are present in the in the PC server
> environment. So comes the need to use TPM devices in the ARM architecture.
>
> This patchset provides a new frontend to the QEMU TPM passthrough with an
> interface suitable to communicate with an ARM based machine.

Thanks for this patchset (and apologies for not getting to it
earlier). Unfortunately I'm completely unfamiliar with TPM, so
reviewing this is going to be difficult.

> ### Technical details ###
>
> The TPM devices on the PC architecture are integrated in a way that the
> interface to it is made using an abstraction layer provided by the BIOS/UFI
> firmware. That interface is not available to ARM machines, therefore a new
> QEMU front end with a more suitable interface needed to be developed. The
> options based on the available TPM devices in the market were I2C and SPI.
> To make the choice, we look into the supported TPM drivers available to
> ARM architecture in the Linux Kernel. One of the simplest drivers was the
> ATMEL I2C TPM AT97SC3204T, so that was our target for the emulation.
>
> We created a file called tpm_i2c_atmel.c based on the already available
> tpm_tis.c, registering as a I2C_SLAVE_CLASS and calling the tpm_backend
> functions.

I guess my primary question here is: is an I2C device going to be
the way that TPM is exposed on ARM hardware? If actual servers are
going to do something else then there's limited benefit to
implementing this in QEMU. (On the other hand, having this be
just-another-i2c-slave means that adding it to QEMU isn't going
to add maintenance complication in cases where it isn't used.)

> One of the problems we had to address is regarding the behavior of the
> ATMEL I2C TPM AT97SC3204T Linux driver. After the driver sends a request
> to the TPM, it keeps polling the device with I2C read request. The real
> AT97SC3204T hardware ignore those requests while the response is not ready
> simply by not ACKing the I2C read on its address. When the response is
> ready it will ACK the request and proceed writing the response in the wire.
>
> The QEMU I2C API does not provide a way to not ACK I2C requests when the
> device is not ready to transmit. In fact, if the device has been configured
> in the virtual machine, QEMU will automatically ACK every request without
> asking for the device permission for it. Therefore we created a flag in
> the I2CSlave struct that tells the I2C subsystem that the device is busy
> and not ready to ACK a I2C transfer. We understand that it could not be
> the best solution to the problem, but it appears to be the solution that
> have the least impact in the code overall. Suggestions on a different
> approach would be welcome.

I2C slaves definitely ought to be able to NAK I2C requests. This
is possible for sends, ie data sent from the master to the slave
(the slave just returns non-zero from its I2CSlaveClass::send function).
In i2c protocol terms, this corresponds to the slave generating a NAK
(by not taking SDA low) after the master has sent a byte of data.
The bitbang_i2c code used by versatile was buggy in handling NAKs
for sends until commit 9706e0162d24.

For recv I'm less sure how it ought to work, so if you can explain
in terms of the i2c protocol what slave h/w behaviour we're trying
to emulate that would help. At what points in the protocol can
the slave return a NAK?

Our current API seems to envisage that the slave can return a
negative value from I2CSlaveClass::recv instead of a data byte,
but I'm not sure what this means in the i2c protocol.

If I understand your patch correctly, this is adding support
for the slave refusing to ACK when the master sends out the
slave address and r/w bit. I think that makes sense, but rather
than having a state flag in the I2CSlave struct, we should
change the prototype of the I2CSlaveClass event method so that
it can return a value indicating ack or nak.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 0/2] Add Atmel I2C TPM AT97SC3204T emulated device

[Alastair D'Silva]

On Fri, 2016-12-16 at 17:35 +0000, Peter Maydell wrote:
> (added a couple of people to cc who might have an opinion on the i2c
> protocol questions below)

I'm certainly no expert, but I'll try :)

> On 29 November 2016 at 19:30, Fabio Urquiza <flus@cesar.org.br>
> wrote:
<snip>
> > 
> > One of the problems we had to address is regarding the behavior of
> > the
> > ATMEL I2C TPM AT97SC3204T Linux driver. After the driver sends a
> > request
> > to the TPM, it keeps polling the device with I2C read request. The
> > real
> > AT97SC3204T hardware ignore those requests while the response is
> > not ready
> > simply by not ACKing the I2C read on its address. When the response
> > is
> > ready it will ACK the request and proceed writing the response in
> > the wire.
> > 
> > The QEMU I2C API does not provide a way to not ACK I2C requests
> > when the
> > device is not ready to transmit. In fact, if the device has been
> > configured
> > in the virtual machine, QEMU will automatically ACK every request
> > without
> > asking for the device permission for it. Therefore we created a
> > flag in
> > the I2CSlave struct that tells the I2C subsystem that the device is
> > busy
> > and not ready to ACK a I2C transfer. We understand that it could
> > not be
> > the best solution to the problem, but it appears to be the solution
> > that
> > have the least impact in the code overall. Suggestions on a
> > different
> > approach would be welcome.
> 
> I2C slaves definitely ought to be able to NAK I2C requests. This
> is possible for sends, ie data sent from the master to the slave
> (the slave just returns non-zero from its I2CSlaveClass::send
> function).
> In i2c protocol terms, this corresponds to the slave generating a NAK
> (by not taking SDA low) after the master has sent a byte of data.
> The bitbang_i2c code used by versatile was buggy in handling NAKs
> for sends until commit 9706e0162d24.
> 

I agree that they should be able to NAK, it seems that this may be the
first device that actually needs that functionality (or at least, has
implemented it).

The change looks about as minimal as one could make it, so I'm mostly
happy with it.

I think we may need to add the busy field to hw/i2c/core.c:
VMStateDescription vmstate_i2c_slave, if this state should be
persisted.

Also, it looks like the busy element is only ever used in a boolean
context, so a boolean type may be more appropriate.

> For recv I'm less sure how it ought to work, so if you can explain
> in terms of the i2c protocol what slave h/w behaviour we're trying
> to emulate that would help. At what points in the protocol can
> the slave return a NAK?
> 
> Our current API seems to envisage that the slave can return a
> negative value from I2CSlaveClass::recv instead of a data byte,
> but I'm not sure what this means in the i2c protocol.

Negative values are propagated upwards, where they are treated as
errors, eg, in hw/i2c/aspeed_i2c.c:aspeed_i2c_bus_handle_cmd():

int ret = i2c_recv(bus->bus);
if (ret < 0) {
    qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
    ret = 0xff;
}

The call to i2c_recv is too late to issue the NAK, I believe they occur
during the start_transfer() call.


> If I understand your patch correctly, this is adding support
> for the slave refusing to ACK when the master sends out the
> slave address and r/w bit. I think that makes sense, but rather
> than having a state flag in the I2CSlave struct, we should
> change the prototype of the I2CSlaveClass event method so that
> it can return a value indicating ack or nak.
> 

Hmm, this could end up being quite an invasive change, but ultimately
more elegant. I'm not sure which way the community prefers.

> thanks
> -- PMM

-- 
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819

Re: [Qemu-devel] [PATCH v1 0/2] Add Atmel I2C TPM AT97SC3204T emulated device

[Corey Minyard]

On 12/18/2016 07:47 PM, Alastair D'Silva wrote:
> On Fri, 2016-12-16 at 17:35 +0000, Peter Maydell wrote:
>> (added a couple of people to cc who might have an opinion on the i2c
>> protocol questions below)
> I'm certainly no expert, but I'll try :)

I know a little bit and I've implemented some stuff, so I'll try, too :).

>> On 29 November 2016 at 19:30, Fabio Urquiza <flus@cesar.org.br>
>> wrote:
> <snip>
<snip> some more
>> For recv I'm less sure how it ought to work, so if you can explain
>> in terms of the i2c protocol what slave h/w behaviour we're trying
>> to emulate that would help. At what points in the protocol can
>> the slave return a NAK?
>>
>> Our current API seems to envisage that the slave can return a
>> negative value from I2CSlaveClass::recv instead of a data byte,
>> but I'm not sure what this means in the i2c protocol.
> Negative values are propagated upwards, where they are treated as
> errors, eg, in hw/i2c/aspeed_i2c.c:aspeed_i2c_bus_handle_cmd():
>
> int ret = i2c_recv(bus->bus);
> if (ret < 0) {
>      qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>      ret = 0xff;
> }
>
> The call to i2c_recv is too late to issue the NAK, I believe they occur
> during the start_transfer() call.

Indeed, it is.  On the wire, a NAK after the address bits have been sent 
terminates
the transaction normally.

>
>> If I understand your patch correctly, this is adding support
>> for the slave refusing to ACK when the master sends out the
>> slave address and r/w bit. I think that makes sense, but rather
>> than having a state flag in the I2CSlave struct, we should
>> change the prototype of the I2CSlaveClass event method so that
>> it can return a value indicating ack or nak.
>>
> Hmm, this could end up being quite an invasive change, but ultimately
> more elegant. I'm not sure which way the community prefers.

I have a patch that adds a check_event() handler along side the event() 
handler.
If a device wants to send a NAK, it can implement check_event() instead of
event() and return non-zero to NAK.

I toyed with just changing all the event() calls, but there are a 
bunch.  This seemed
like the better approach.  I can send if you like.

-corey

>> thanks
>> -- PMM

Re: [Qemu-devel] [PATCH v1 0/2] Add Atmel I2C TPM AT97SC3204T emulated device

[Peter Maydell]

On 19 December 2016 at 13:55, Corey Minyard <cminyard@mvista.com> wrote:
> On 12/18/2016 07:47 PM, Alastair D'Silva wrote:
>>
>> On Fri, 2016-12-16 at 17:35 +0000, Peter Maydell wrote:
>>> Our current API seems to envisage that the slave can return a
>>> negative value from I2CSlaveClass::recv instead of a data byte,
>>> but I'm not sure what this means in the i2c protocol.
>>
>> Negative values are propagated upwards, where they are treated as
>> errors, eg, in hw/i2c/aspeed_i2c.c:aspeed_i2c_bus_handle_cmd():
>>
>> int ret = i2c_recv(bus->bus);
>> if (ret < 0) {
>>      qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>>      ret = 0xff;
>> }
>>
>> The call to i2c_recv is too late to issue the NAK, I believe they occur
>> during the start_transfer() call.

OK, so if returning negative values from i2c_recv() isn't
the device saying "I am NAKing this", what *do* they mean?

>>> If I understand your patch correctly, this is adding support
>>> for the slave refusing to ACK when the master sends out the
>>> slave address and r/w bit. I think that makes sense, but rather
>>> than having a state flag in the I2CSlave struct, we should
>>> change the prototype of the I2CSlaveClass event method so that
>>> it can return a value indicating ack or nak.
>>>
>> Hmm, this could end up being quite an invasive change, but ultimately
>> more elegant. I'm not sure which way the community prefers.
>
>
> I have a patch that adds a check_event() handler along side the event()
> handler.
> If a device wants to send a NAK, it can implement check_event() instead of
> event() and return non-zero to NAK.
>
> I toyed with just changing all the event() calls, but there are a bunch.
> This seemed
> like the better approach.  I can send if you like.

It looks like there are only a dozen or so. I think it would
be better for the long term just to change the event calls.
We should also document in the comments in the I2CSlaveClass
struct definition exactly what the semantics of the various
functions are.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 0/2] Add Atmel I2C TPM AT97SC3204T emulated device

[Corey Minyard]

On 12/19/2016 09:31 AM, Peter Maydell wrote:
> On 19 December 2016 at 13:55, Corey Minyard <cminyard@mvista.com> wrote:
>> On 12/18/2016 07:47 PM, Alastair D'Silva wrote:
>>> On Fri, 2016-12-16 at 17:35 +0000, Peter Maydell wrote:
>>>> Our current API seems to envisage that the slave can return a
>>>> negative value from I2CSlaveClass::recv instead of a data byte,
>>>> but I'm not sure what this means in the i2c protocol.
>>> Negative values are propagated upwards, where they are treated as
>>> errors, eg, in hw/i2c/aspeed_i2c.c:aspeed_i2c_bus_handle_cmd():
>>>
>>> int ret = i2c_recv(bus->bus);
>>> if (ret < 0) {
>>>       qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>>>       ret = 0xff;
>>> }
>>>
>>> The call to i2c_recv is too late to issue the NAK, I believe they occur
>>> during the start_transfer() call.
> OK, so if returning negative values from i2c_recv() isn't
> the device saying "I am NAKing this", what *do* they mean?

It actually makes no sense.  In real I2C hardware, the receiver of the 
byte always
does the ACK/NAK.  The NAK is sent by the receiver of the data to signal 
that it
has finished the transfer.

So when i2c_recv() is called, it's actually the I2c device doing a 
transmit and the
i2c master receiving the data.  So the device cannot send a NAK in that 
scenario.

The start conditions and address are always send by the master to the 
device, so
it makes sense for the start events to be able to return a NAK.

And for i2c_send(), the device should respond with a NAK to terminate the
transfer.  So it makes sense for i2c_send() to be able to return a NAK.  
This
doesn't appear to be properly implemented in a number of places.

>>>> If I understand your patch correctly, this is adding support
>>>> for the slave refusing to ACK when the master sends out the
>>>> slave address and r/w bit. I think that makes sense, but rather
>>>> than having a state flag in the I2CSlave struct, we should
>>>> change the prototype of the I2CSlaveClass event method so that
>>>> it can return a value indicating ack or nak.
>>>>
>>> Hmm, this could end up being quite an invasive change, but ultimately
>>> more elegant. I'm not sure which way the community prefers.
>>
>> I have a patch that adds a check_event() handler along side the event()
>> handler.
>> If a device wants to send a NAK, it can implement check_event() instead of
>> event() and return non-zero to NAK.
>>
>> I toyed with just changing all the event() calls, but there are a bunch.
>> This seemed
>> like the better approach.  I can send if you like.
> It looks like there are only a dozen or so. I think it would
> be better for the long term just to change the event calls.
> We should also document in the comments in the I2CSlaveClass
> struct definition exactly what the semantics of the various
> functions are.

Ok, my memory didn't serve me correctly.  I'll rework my patch for that.

Thanks,

-corey

> thanks
> -- PMM

[Qemu-devel] [PATCH] i2c: Allow I2C devices to NAK start events

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Add a return value to the event handler.  Some I2C devices will
NAK if they have no data, so allow them to do this.  This required
the following changes:

Go through all the event handlers and change them to return int
and return 0.

Modify i2c_start_transfer to terminate the transaction on a NAK.

Modify smbus handing to not assert if a NAK occurs on a second
operation, and terminate the transaction and return -1 instead.

Add some information on semantics to I2CSlaveClass.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/arm/pxa2xx.c      |  4 +++-
 hw/arm/tosa.c        |  4 +++-
 hw/arm/z2.c          |  4 +++-
 hw/audio/wm8750.c    |  4 +++-
 hw/display/ssd0303.c |  4 +++-
 hw/gpio/max7310.c    |  4 +++-
 hw/i2c/core.c        | 31 +++++++++++++++++++++++++------
 hw/i2c/i2c-ddc.c     |  4 +++-
 hw/i2c/smbus.c       | 13 +++++++++----
 hw/input/lm832x.c    |  4 +++-
 hw/misc/tmp105.c     |  3 ++-
 hw/timer/ds1338.c    |  4 +++-
 hw/timer/twl92230.c  |  4 +++-
 include/hw/i2c/i2c.h | 16 ++++++++++++----
 14 files changed, 78 insertions(+), 25 deletions(-)

diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index 21ea1d6..4d3179e 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1258,7 +1258,7 @@ static void pxa2xx_i2c_update(PXA2xxI2CState *s)
 }
 
 /* These are only stubs now.  */
-static void pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
+static int pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
 {
     PXA2xxI2CSlaveState *slave = PXA2XX_I2C_SLAVE(i2c);
     PXA2xxI2CState *s = slave->host;
@@ -1280,6 +1280,8 @@ static void pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
         break;
     }
     pxa2xx_i2c_update(s);
+
+    return 0;
 }
 
 static int pxa2xx_i2c_rx(I2CSlave *i2c)
diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index 1ee12f4..1165303 100644
--- a/hw/arm/tosa.c
+++ b/hw/arm/tosa.c
@@ -172,7 +172,7 @@ static int tosa_dac_send(I2CSlave *i2c, uint8_t data)
     return 0;
 }
 
-static void tosa_dac_event(I2CSlave *i2c, enum i2c_event event)
+static int tosa_dac_event(I2CSlave *i2c, enum i2c_event event)
 {
     TosaDACState *s = TOSA_DAC(i2c);
 
@@ -194,6 +194,8 @@ static void tosa_dac_event(I2CSlave *i2c, enum i2c_event event)
     default:
         break;
     }
+
+    return 0;
 }
 
 static int tosa_dac_recv(I2CSlave *s)
diff --git a/hw/arm/z2.c b/hw/arm/z2.c
index 68a92f3..e2d8160 100644
--- a/hw/arm/z2.c
+++ b/hw/arm/z2.c
@@ -220,7 +220,7 @@ static int aer915_send(I2CSlave *i2c, uint8_t data)
     return 0;
 }
 
-static void aer915_event(I2CSlave *i2c, enum i2c_event event)
+static int aer915_event(I2CSlave *i2c, enum i2c_event event)
 {
     AER915State *s = AER915(i2c);
 
@@ -238,6 +238,8 @@ static void aer915_event(I2CSlave *i2c, enum i2c_event event)
     default:
         break;
     }
+
+    return 0;
 }
 
 static int aer915_recv(I2CSlave *slave)
diff --git a/hw/audio/wm8750.c b/hw/audio/wm8750.c
index 0c6500e..f8b5beb 100644
--- a/hw/audio/wm8750.c
+++ b/hw/audio/wm8750.c
@@ -303,7 +303,7 @@ static void wm8750_reset(I2CSlave *i2c)
     s->i2c_len = 0;
 }
 
-static void wm8750_event(I2CSlave *i2c, enum i2c_event event)
+static int wm8750_event(I2CSlave *i2c, enum i2c_event event)
 {
     WM8750State *s = WM8750(i2c);
 
@@ -321,6 +321,8 @@ static void wm8750_event(I2CSlave *i2c, enum i2c_event event)
     default:
         break;
     }
+
+    return 0;
 }
 
 #define WM8750_LINVOL	0x00
diff --git a/hw/display/ssd0303.c b/hw/display/ssd0303.c
index d301756..68a80b9 100644
--- a/hw/display/ssd0303.c
+++ b/hw/display/ssd0303.c
@@ -179,7 +179,7 @@ static int ssd0303_send(I2CSlave *i2c, uint8_t data)
     return 0;
 }
 
-static void ssd0303_event(I2CSlave *i2c, enum i2c_event event)
+static int ssd0303_event(I2CSlave *i2c, enum i2c_event event)
 {
     ssd0303_state *s = SSD0303(i2c);
 
@@ -193,6 +193,8 @@ static void ssd0303_event(I2CSlave *i2c, enum i2c_event event)
         /* Nothing to do.  */
         break;
     }
+
+    return 0;
 }
 
 static void ssd0303_update_display(void *opaque)
diff --git a/hw/gpio/max7310.c b/hw/gpio/max7310.c
index 1bd5eaf..f82e3e6 100644
--- a/hw/gpio/max7310.c
+++ b/hw/gpio/max7310.c
@@ -129,7 +129,7 @@ static int max7310_tx(I2CSlave *i2c, uint8_t data)
     return 0;
 }
 
-static void max7310_event(I2CSlave *i2c, enum i2c_event event)
+static int max7310_event(I2CSlave *i2c, enum i2c_event event)
 {
     MAX7310State *s = MAX7310(i2c);
     s->len = 0;
@@ -147,6 +147,8 @@ static void max7310_event(I2CSlave *i2c, enum i2c_event event)
     default:
         break;
     }
+
+    return 0;
 }
 
 static const VMStateDescription vmstate_max7310 = {
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index abd4c4c..584b76d 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -88,18 +88,26 @@ int i2c_bus_busy(I2CBus *bus)
     return !QLIST_EMPTY(&bus->current_devs);
 }
 
+/* TODO: Make this handle multiple masters.  */
 /*
- * Returns non-zero if the address is not valid.  If this is called
- * again without an intervening i2c_end_transfer(), like in the SMBus
- * case where the operation is switched from write to read, this
- * function will not rescan the bus and thus cannot fail.
+ * Start or continue an i2c transaction.  When this is called for the
+ * first time or after an i2c_end_transfer(), if it returns an error
+ * the bus transaction is terminated (or really never started).  If
+ * this is called after another i2c_start_transfer() without an
+ * intervening i2c_end_transfer(), and it returns an error, the
+ * transaction will not be terminated.  The caller must do it.
+ *
+ * This corresponds with the way real hardware works.  The SMBus
+ * protocol uses a start transfer to switch from write to read mode
+ * without releasing the bus.  If that fails, the bus is still
+ * in a transaction.
  */
-/* TODO: Make this handle multiple masters.  */
 int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
 {
     BusChild *kid;
     I2CSlaveClass *sc;
     I2CNode *node;
+    bool bus_scanned = false;
 
     if (address == I2C_BROADCAST) {
         /*
@@ -130,6 +138,7 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
                 }
             }
         }
+        bus_scanned = true;
     }
 
     if (QLIST_EMPTY(&bus->current_devs)) {
@@ -137,11 +146,21 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
     }
 
     QLIST_FOREACH(node, &bus->current_devs, next) {
+        int rv;
+
         sc = I2C_SLAVE_GET_CLASS(node->elt);
         /* If the bus is already busy, assume this is a repeated
            start condition.  */
+
         if (sc->event) {
-            sc->event(node->elt, recv ? I2C_START_RECV : I2C_START_SEND);
+            rv = sc->event(node->elt, recv ? I2C_START_RECV : I2C_START_SEND);
+            if (rv && !bus->broadcast) {
+                if (bus_scanned) {
+                    /* First call, terminate the transfer. */
+                    i2c_end_transfer(bus);
+                }
+                return rv;
+            }
         }
     }
     return 0;
diff --git a/hw/i2c/i2c-ddc.c b/hw/i2c/i2c-ddc.c
index 1227212..66899d7 100644
--- a/hw/i2c/i2c-ddc.c
+++ b/hw/i2c/i2c-ddc.c
@@ -230,13 +230,15 @@ static void i2c_ddc_reset(DeviceState *ds)
     s->reg = 0;
 }
 
-static void i2c_ddc_event(I2CSlave *i2c, enum i2c_event event)
+static int i2c_ddc_event(I2CSlave *i2c, enum i2c_event event)
 {
     I2CDDCState *s = I2CDDC(i2c);
 
     if (event == I2C_START_SEND) {
         s->firstbyte = true;
     }
+
+    return 0;
 }
 
 static int i2c_ddc_rx(I2CSlave *i2c)
diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
index 5b4dd3e..2d1b79a 100644
--- a/hw/i2c/smbus.c
+++ b/hw/i2c/smbus.c
@@ -67,7 +67,7 @@ static void smbus_do_write(SMBusDevice *dev)
     }
 }
 
-static void smbus_i2c_event(I2CSlave *s, enum i2c_event event)
+static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
 {
     SMBusDevice *dev = SMBUS_DEVICE(s);
 
@@ -148,6 +148,8 @@ static void smbus_i2c_event(I2CSlave *s, enum i2c_event event)
             break;
         }
     }
+
+    return 0;
 }
 
 static int smbus_i2c_recv(I2CSlave *s)
@@ -249,7 +251,8 @@ int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command)
     }
     i2c_send(bus, command);
     if (i2c_start_transfer(bus, addr, 1)) {
-        assert(0);
+        i2c_end_transfer(bus);
+        return -1;
     }
     data = i2c_recv(bus);
     i2c_nack(bus);
@@ -276,7 +279,8 @@ int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command)
     }
     i2c_send(bus, command);
     if (i2c_start_transfer(bus, addr, 1)) {
-        assert(0);
+        i2c_end_transfer(bus);
+        return -1;
     }
     data = i2c_recv(bus);
     data |= i2c_recv(bus) << 8;
@@ -307,7 +311,8 @@ int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data)
     }
     i2c_send(bus, command);
     if (i2c_start_transfer(bus, addr, 1)) {
-        assert(0);
+        i2c_end_transfer(bus);
+        return -1;
     }
     len = i2c_recv(bus);
     if (len > 32) {
diff --git a/hw/input/lm832x.c b/hw/input/lm832x.c
index 539682c..2340523 100644
--- a/hw/input/lm832x.c
+++ b/hw/input/lm832x.c
@@ -383,7 +383,7 @@ static void lm_kbd_write(LM823KbdState *s, int reg, int byte, uint8_t value)
     }
 }
 
-static void lm_i2c_event(I2CSlave *i2c, enum i2c_event event)
+static int lm_i2c_event(I2CSlave *i2c, enum i2c_event event)
 {
     LM823KbdState *s = LM8323(i2c);
 
@@ -397,6 +397,8 @@ static void lm_i2c_event(I2CSlave *i2c, enum i2c_event event)
     default:
         break;
     }
+
+    return 0;
 }
 
 static int lm_i2c_rx(I2CSlave *i2c)
diff --git a/hw/misc/tmp105.c b/hw/misc/tmp105.c
index f5c2472..04e8378 100644
--- a/hw/misc/tmp105.c
+++ b/hw/misc/tmp105.c
@@ -176,7 +176,7 @@ static int tmp105_tx(I2CSlave *i2c, uint8_t data)
     return 0;
 }
 
-static void tmp105_event(I2CSlave *i2c, enum i2c_event event)
+static int tmp105_event(I2CSlave *i2c, enum i2c_event event)
 {
     TMP105State *s = TMP105(i2c);
 
@@ -185,6 +185,7 @@ static void tmp105_event(I2CSlave *i2c, enum i2c_event event)
     }
 
     s->len = 0;
+    return 0;
 }
 
 static int tmp105_post_load(void *opaque, int version_id)
diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c
index 0112949..18c8e38 100644
--- a/hw/timer/ds1338.c
+++ b/hw/timer/ds1338.c
@@ -94,7 +94,7 @@ static void inc_regptr(DS1338State *s)
     }
 }
 
-static void ds1338_event(I2CSlave *i2c, enum i2c_event event)
+static int ds1338_event(I2CSlave *i2c, enum i2c_event event)
 {
     DS1338State *s = DS1338(i2c);
 
@@ -113,6 +113,8 @@ static void ds1338_event(I2CSlave *i2c, enum i2c_event event)
     default:
         break;
     }
+
+    return 0;
 }
 
 static int ds1338_recv(I2CSlave *i2c)
diff --git a/hw/timer/twl92230.c b/hw/timer/twl92230.c
index 7ba4e9a..b8d914e 100644
--- a/hw/timer/twl92230.c
+++ b/hw/timer/twl92230.c
@@ -713,12 +713,14 @@ static void menelaus_write(void *opaque, uint8_t addr, uint8_t value)
     }
 }
 
-static void menelaus_event(I2CSlave *i2c, enum i2c_event event)
+static int menelaus_event(I2CSlave *i2c, enum i2c_event event)
 {
     MenelausState *s = TWL92230(i2c);
 
     if (event == I2C_START_SEND)
         s->firstbyte = 1;
+
+    return 0;
 }
 
 static int menelaus_tx(I2CSlave *i2c, uint8_t data)
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index c4085aa..2ce611d 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -32,14 +32,22 @@ typedef struct I2CSlaveClass
     /* Callbacks provided by the device.  */
     int (*init)(I2CSlave *dev);
 
-    /* Master to slave.  */
+    /* Master to slave. Returns non-zero for a NAK, 0 for success. */
     int (*send)(I2CSlave *s, uint8_t data);
 
-    /* Slave to master.  */
+    /*
+     * 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.
+     */
     int (*recv)(I2CSlave *s);
 
-    /* Notify the slave of a bus state change.  */
-    void (*event)(I2CSlave *s, enum i2c_event event);
+    /*
+     * Notify the slave of a bus state change.  For start event,
+     * returns non-zero to NAK an operation.  For other events the
+     * return code is not used and should be zero.
+     */
+    int (*event)(I2CSlave *s, enum i2c_event event);
 } I2CSlaveClass;
 
 struct I2CSlave
-- 
2.7.4

Re: [Qemu-devel] [PATCH] i2c: Allow I2C devices to NAK start events

[Peter Maydell]

On 20 December 2016 at 21:26,  <minyard@acm.org> wrote:
> From: Corey Minyard <cminyard@mvista.com>
>
> Add a return value to the event handler.  Some I2C devices will
> NAK if they have no data, so allow them to do this.  This required
> the following changes:
>
> Go through all the event handlers and change them to return int
> and return 0.
>
> Modify i2c_start_transfer to terminate the transaction on a NAK.
>
> Modify smbus handing to not assert if a NAK occurs on a second
> operation, and terminate the transaction and return -1 instead.
>
> Add some information on semantics to I2CSlaveClass.
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>

Thanks; added to target-arm.next.

-- PMM