Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4755 bytes --]

On Wed, Jun 13, 2018 at 04:13:57PM +0200, BALATON Zoltan wrote:
> 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?

Ah, I see your point.  I mean strictly we should match the hardware
behaviour if you write garbage addresses here, but really I don't
think it matters much.

Ok, it should be fine.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]