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
