On Wed, 13 Jun 2018, David Gibson wrote:
On Wed, Jun 13, 2018 at 10:50:59AM +0200, BALATON Zoltan wrote:
On Wed, 13 Jun 2018, David Gibson wrote:
On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
On Wed, 6 Jun 2018, Philippe Mathieu-Daudé wrote:
On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
of day is implemented. Setting time and RTC alarm are not supported.
[...]
diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
new file mode 100644
index 0000000..9dbdb1b
--- /dev/null
+++ b/hw/timer/m41t80.c
@@ -0,0 +1,117 @@
+/*
+ * M41T80 serial rtc emulation
+ *
+ * Copyright (c) 2018 BALATON Zoltan
+ *
+ * This work is licensed under the GNU GPL license version 2 or later.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/timer.h"
+#include "qemu/bcd.h"
+#include "hw/i2c/i2c.h"
+
+#define TYPE_M41T80 "m41t80"
+#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
+
+typedef struct M41t80State {
+    I2CSlave parent_obj;
+    int8_t addr;
+} M41t80State;
+
+static void m41t80_realize(DeviceState *dev, Error **errp)
+{
+    M41t80State *s = M41T80(dev);
+
+    s->addr = -1;
+}
+
+static int m41t80_send(I2CSlave *i2c, uint8_t data)
+{
+    M41t80State *s = M41T80(i2c);
+
+    if (s->addr < 0) {
+        s->addr = data;
+    } else {
+        s->addr++;
+    }


What about adding enum i2c_event in M41t80State and use the enum here
rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
expected?

Thanks for the review. I guess we could add enum for device bytes and the
special case -1 meaning no register address selected yet but this is a very
simple device with only 20 bytes and the datasheet also lists them by number
without naming them so I think we can also refer to them by number. Since
the device has only this 20 bytes the case with 127 should also not be a
problem as that's invalid address anyway. Or did you mean something else?

So, I'm not particularly in favour of adding extra state variables.

But is using addr < 0 safe here?  You're assigning the uint8_t data to
addr - could that result in a negative value?

Why wouldn't it be safe with the expected values for register address
between 0-19? If the guest sends garbage values over 127 it will either
result in invalid register or unselected register and lead to an error when
trying to read/write that register so I don't see what other problem this
may cause.

Ok, but where is that enforced?

I don't see the problem. The addr register selects the register to read or write. It is set by the first write when the device is accessed the first time (this is denoted by addr == -1 (or really any negative value). The device has 20 registers and trying to read any register outside addr between 0-19 will result in returning 0 and logging a warning about invalid register in m41t80_recv. What could fail here when guest sends garbage? It will set addr to invalid value and try to read non-exitent register and get an error just like for any other nonexistent value of addr (or start to read from register 0 if it manages to set a negative value). All writes of registers are ignored currently (except setting addr by the first write). What should be enforced here?

Regards,
BALATON Zoltan

Reply via email to