Re: [PATCH 1/2] hw: timer: Add Goldfish RTC device

[Peter Maydell]

On Tue, 24 Sep 2019 at 12:17, Anup Patel <address@hidden> wrote:
>
>
>
> > -----Original Message-----
> > From: Peter Maydell <address@hidden>
> > Sent: Tuesday, September 24, 2019 3:21 PM
> > To: Anup Patel <address@hidden>
> > Cc: Palmer Dabbelt <address@hidden>; Alistair Francis
> > <address@hidden>; Sagar Karandikar <address@hidden>;
> > Bastian Koppelmann <address@hidden>; Atish Patra
> > <address@hidden>; address@hidden; qemu-
> > address@hidden; Anup Patel <address@hidden>
> > Subject: Re: [PATCH 1/2] hw: timer: Add Goldfish RTC device
> >
> > On Tue, 24 Sep 2019 at 09:45, Anup Patel <address@hidden> wrote:
> > >
> > > This patch adds model for Google Goldfish virtual platform RTC device.
> > >
> > > We will be adding Goldfish RTC device to the QEMU RISC-V virt machine
> > > for providing real date-time to Guest Linux. The corresponding Linux
> > > driver for Goldfish RTC device is already available in upstream Linux.
> > >
> > > For now, VM migration support is not available for Goldfish RTC device
> > > but it will be added later when we implement VM migration for KVM RISC-
> > V.
> > >
> > > Signed-off-by: Anup Patel <address@hidden>

> > > ---
> > > +
> > > +static Property goldfish_rtc_properties[] = {
> > > +    DEFINE_PROP_UINT64("tick-offset", GoldfishRTCState, tick_offset, 0),
> > > +    DEFINE_PROP_UINT64("alarm-next", GoldfishRTCState, alarm_next, 0),
> > > +    DEFINE_PROP_UINT32("alarm-running", GoldfishRTCState,
> > alarm_running, 0),
> > > +    DEFINE_PROP_UINT32("irq-pending", GoldfishRTCState, irq_pending,
> > 0),
> > > +    DEFINE_PROP_UINT32("irq-enabled", GoldfishRTCState, irq_enabled,
> > 0),
> > > +    DEFINE_PROP_END_OF_LIST(),
> > > +};
> >
> > What are all these properties trying to do ?
>
> Argh, I forgot to remove these before sending.
>
> I will drop these in next revision.
>
> >
> > > +
> > > +static void goldfish_rtc_class_init(ObjectClass *klass, void *data) {
> > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > +    dc->props = goldfish_rtc_properties;
> >
> > Missing reset function.
>
> Sure, I will add it. I got confused because I saw reset function in HPET
> but not in PL031.

Yeah, the lack of reset is a bug in the pl031. I did have
a draft set of patches where I tried to fix that, but I
got stuck because it was a bit unclear what I ought to be
resetting. In a real hardware pl031 there is no persistent
storage of the RTC value -- it's just a 1Hz counter, really,
and guest firmware will write to it on startup (from some
other real RTC). In QEMU we use it as a sort of "RTC for a
VM", and back it with the QEMU RTC clock, so it doesn't
behave quite as the hardware does but more like "view that
you'd see from a combination of h/w and firmware behaviour".

TLDR: don't use the pl031 code as a good model of how to do an RTC,
it has some definite flaws. x86 or ppc RTC handling is
probably a better place to look.

Another random question -- is there an existing RTC device
we could put in the riscv board rather than adding a new
model of this goldfish device? Put another way, what are
the merits of using goldfish in particular?

We should probably document this device in docs/specs since it's
a pure-virtual device. (You have a link to the URL for the
GOLDFISH-VIRTUAL-HARDWARE.TXT for the android emulator, but that
doesn't seem to match the implementation here -- the doc says the
alarm registers are non-functional but this code does something with
them.)

> > If you don't want to implement migration support now you should at least
> > put in something that block migration.
> > (Personally I prefer to just write the vmstate, it's as easy as writing the 
> > code
> > to block migrations.)
>
> Okay, I will add vmstate.
>
> Is there a way to unit test the vmstate part ?
> OR
> Are you fine if we test-n-fix it later ?

Test and fix later is fine. That said, I've just remembered
that for an RTC migration is potentially a tricky area
because you typically don't want a vmsave-vmload to
give you the same RTC value after load that there was on
store, you want it to still be following the host RTC.
We got that wrong on the PL031 and had to put in some ugly
workarounds to maintain migration compatibility when we fixed it.
So maybe it would be better to mark as non-migratable for now.

thanks
-- PMM