Hi Philippe,

On Sat, Jan 18, 2020 at 4:05 PM Philippe Mathieu-Daudé <phi...@redhat.com>
wrote:

> Hi Niek,
>
> On 1/14/20 11:57 PM, Niek Linnenbank wrote:
> > On Tue, Jan 14, 2020 at 11:52 PM Niek Linnenbank
> > <nieklinnenb...@gmail.com <mailto:nieklinnenb...@gmail.com>> wrote:
> >
> >     Hi Philippe,
> >
> >     On Mon, Jan 13, 2020 at 11:57 PM Philippe Mathieu-Daudé
> >     <phi...@redhat.com <mailto:phi...@redhat.com>> wrote:
> >
> >         On 1/8/20 9:00 PM, Niek Linnenbank wrote:
> >          > Allwinner System-on-Chips usually contain a Real Time Clock
> (RTC)
> >          > for non-volatile system date and time keeping. This commit
> >         adds a generic
> >          > Allwinner RTC device that supports the RTC devices found in
> >         Allwinner SoC
> >          > family sun4i (A10), sun7i (A20) and sun6i and newer (A31,
> >         H2+, H3, etc).
> [...]
> >          > +static uint64_t allwinner_rtc_read(void *opaque, hwaddr
> offset,
> >          > +                                   unsigned size)
> >          > +{
> >          > +    AwRtcState *s = AW_RTC(opaque);
> >          > +    const AwRtcClass *c = AW_RTC_GET_CLASS(s);
> >          > +    uint64_t val = 0;
> >          > +
> >          > +    if (offset >= AW_RTC_REGS_MAXADDR) {
> >          > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: out-of-bounds
> >         offset 0x%04x\n",
> >          > +                      __func__, (uint32_t)offset);
> >          > +        return 0;
> >          > +    }
> >          > +
> >          > +    if (!c->regmap[offset]) {
> >          > +            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid
> >         register 0x%04x\n",
> >          > +                          __func__, (uint32_t)offset);
> >          > +        return 0;
> >          > +    }
> >          > +
> >          > +    switch (c->regmap[offset]) {
> >          > +    case REG_LOSC:       /* Low Oscillator Control */
> >          > +        val = s->regs[REG_LOSC];
> >          > +        s->regs[REG_LOSC] &= ~(REG_LOSC_YMD | REG_LOSC_HMS);
> >          > +        break;
> >          > +    case REG_YYMMDD:     /* RTC Year-Month-Day */
> >          > +    case REG_HHMMSS:     /* RTC Hour-Minute-Second */
> >          > +    case REG_GP0:        /* General Purpose Register 0 */
> >          > +    case REG_GP1:        /* General Purpose Register 1 */
> >          > +    case REG_GP2:        /* General Purpose Register 2 */
> >          > +    case REG_GP3:        /* General Purpose Register 3 */
> >          > +        val = s->regs[c->regmap[offset]];
> >          > +        break;
> >          > +    default:
> >          > +        if (!c->read(s, offset)) {
> >          > +            qemu_log_mask(LOG_UNIMP, "%s: unimplemented
> >         register 0x%04x\n",
> >          > +                          __func__, (uint32_t)offset);
> >          > +        }
> >          > +        val = s->regs[c->regmap[offset]];
> >          > +        break;
> >          > +    }
> >          > +
> >          > +    trace_allwinner_rtc_read(offset, val);
> >          > +    return val;
> >          > +}
> >          > +
> >          > +static void allwinner_rtc_write(void *opaque, hwaddr offset,
> >          > +                                uint64_t val, unsigned size)
> >          > +{
> >          > +    AwRtcState *s = AW_RTC(opaque);
> >          > +    const AwRtcClass *c = AW_RTC_GET_CLASS(s);
> >          > +
> >          > +    if (offset >= AW_RTC_REGS_MAXADDR) {
> >          > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: out-of-bounds
> >         offset 0x%04x\n",
> >          > +                      __func__, (uint32_t)offset);
> >          > +        return;
> >          > +    }
> >          > +
> >          > +    if (!c->regmap[offset]) {
> >          > +            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid
> >         register 0x%04x\n",
> >          > +                          __func__, (uint32_t)offset);
> >          > +        return;
> >          > +    }
> >          > +
> >          > +    trace_allwinner_rtc_write(offset, val);
> >          > +
> >          > +    switch (c->regmap[offset]) {
> >          > +    case REG_YYMMDD:     /* RTC Year-Month-Day */
> >          > +        s->regs[REG_YYMMDD] = val;
> >          > +        s->regs[REG_LOSC]  |= REG_LOSC_YMD;
> >          > +        break;
> >          > +    case REG_HHMMSS:     /* RTC Hour-Minute-Second */
> >          > +        s->regs[REG_HHMMSS] = val;
> >          > +        s->regs[REG_LOSC]  |= REG_LOSC_HMS;
> >          > +        break;
> >          > +    case REG_GP0:        /* General Purpose Register 0 */
> >          > +    case REG_GP1:        /* General Purpose Register 1 */
> >          > +    case REG_GP2:        /* General Purpose Register 2 */
> >          > +    case REG_GP3:        /* General Purpose Register 3 */
> >          > +        s->regs[c->regmap[offset]] = val;
> >          > +        break;
> >          > +    default:
> >          > +        if (!c->write(s, offset, val)) {
> >          > +            qemu_log_mask(LOG_UNIMP, "%s: unimplemented
> >         register 0x%04x\n",
> >          > +                          __func__, (uint32_t)offset);
> >          > +        }
> >          > +        break;
> >          > +    }
> >          > +}
> >          > +
> >          > +static const MemoryRegionOps allwinner_rtc_ops = {
> >          > +    .read = allwinner_rtc_read,
> >          > +    .write = allwinner_rtc_write,
> >          > +    .endianness = DEVICE_NATIVE_ENDIAN,
> >          > +    .valid = {
> >          > +        .min_access_size = 4,
> >          > +        .max_access_size = 4,
> >          > +    },
> >          > +    .impl.min_access_size = 4,
> >          > +};
> >          > +
> >          > +static void allwinner_rtc_reset(DeviceState *dev)
> >          > +{
> >          > +    AwRtcState *s = AW_RTC(dev);
> >          > +    const AwRtcClass *c = AW_RTC_GET_CLASS(dev);
> >          > +    struct tm now;
> >          > +
> >          > +    /* Clear registers */
> >          > +    memset(s->regs, 0, sizeof(s->regs));
> >          > +
> >          > +    /* Get current datetime */
> >          > +    qemu_get_timedate(&now, 0);
> >          > +
> >          > +    /* Set RTC with current datetime */
> >          > +    s->regs[REG_YYMMDD] =  ((now.tm_year - c->year_offset)
> >         << 16) |
> >          > +                           ((now.tm_mon + 1) << 8) |
> >          > +                             now.tm_mday;
> >          > +    s->regs[REG_HHMMSS] = (((now.tm_wday + 6) % 7) << 29) |
> >          > +                              (now.tm_hour << 16) |
> >          > +                              (now.tm_min << 8) |
> >          > +                               now.tm_sec;
> >
> >         This doesn't look correct.
> >
> >           From H3 Datasheet (Rev1.2):
> >             4.8.3.4. RTC YY-MM-DD Register (Default Value: 0x00000000)
> >             4.8.3.5. RTC HH-MM-SS Register (Default Value: 0x00000000)
> >
> >
> >     I don't yet fully understand what you mean. Could you please explain
> >     a bit more what should be changed?
>
> I was thinking about:
>
> - Start a VM on Monday at 12:34
>
> - Pause the VM on Tuesday at 12:34
>    rtc_time=Tuesday,12:34
>
> - [Eventually savevm/migrate/loadvm]
>    rtc_time=Tuesday,12:34
>
> - Resume the VM on Wednesday 12:34
>    (time should be Tuesday at 12:34)
>    so rtc_time=Tuesday,12:34
>
> - Check time on Thursday at 12:33
>    (time should be Wednesday at 12:33)
>    rtc_time=Wednesday,12:34
>
> - Reset the VM on Thursday at 12:34
>    (time was Wednesday at 12:34)
>
> - Check time on Thursday at 12:35
>    (time should be Wednesday at 12:35)
>
> However due to reset() calling qemu_get_timedate(), the rtc_time will be
> Thursday at 12:35 instead of Wednesday.
>
>
Ah now I see what you mean. So indeed this means that the RTC date & time
is currently
not persistent in the emulated device, while on hardware it is.


> I don't know how the hardware keep these 2*32-bit safe when powered off.
>
> Laurent Vivier posted a patch where he uses a block backend for his
> machine NVRAM (used by RTC). This seems to me the clever way to do this:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg666837.html
>
> I'd use a block of 8 bytes for your RTC.
> If we start a machine without rtc block backend, the 2*32-bit are
> initialized as zero as the datasheet. If we provide a block, the machine
> will power-on from that stored time.
>
> You might want to look at the global -rtc command line option:
>
>    -rtc
>    [base=utc|localtime|datetime][,clock=host|rt|vm][,driftfix=none|slew]
>          Specify base as "utc" or "localtime" to let the RTC start
>          at the current UTC or local time, respectively. "localtime"
>          is required for correct date in MS-DOS or Windows. To start
>          at a specific point in time, provide datetime in the format
>          "2006-06-17T16:01:21" or "2006-06-17". The default base is
>          UTC.
>
> But it might be specific to the MC146818 RTC.
>

Thanks for these suggestions. I'll need to look into it. I don't think I'll
have this ready
in the v4 update. Meanwhile I'll add it as a limitation in the cover letter.

Regards,
Niek

>
> >     For filling the YYMMDD and HHMMSS register fields, I simply looked
> >     at the 4.8.3.4 and 4.8.3.5 sections
> >     and filled it with the time retrieved from qemu_get_timedate. The
> >     shifts are done so the values are set in the proper bits.
> >     If I read it with the hwclock -r command under Linux, this reads out
> OK.
> >     On NetBSD, this works as well, except for the base year mismatch
> >     which I explained above.
> >
> >
> >         I'm not sure what is the proper to model this, maybe set this
> >         value in
> >         init()? If we suspend a machine, migrate it, and resume it, what
> >         RTC are
> >         we expecting?
> >
> > I forgot to reply on this one.
> >
> > I have not used migration very often, but I did manage to test it a
> > couple of times
> > using the 'migrate' command on the Qemu monitor console before I
> > submitted each
> > new version of the patch series. My expectation would be that the RTC
> > time is suspended
> > along with all the other state of the machine such as memory, I/O
> > devices etc. So that would mean
> > the time is 'frozen' until resumed. I think that is what we currently
> > have here.
> >
> > Do you think that is correct or should it work differently?
>
> Yes, this is the behavior I'd expect. Maybe other would disagree.
>
>

-- 
Niek Linnenbank

Reply via email to