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

[Anup Patel]

> -----Original Message-----
> From: Peter Maydell <address@hidden>
> Sent: Tuesday, September 24, 2019 5:02 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 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?

As of now, there is no RTC device used across SOCs. Atleast nothing that I
am aware of.

I just wanted a very simple RTC for RISC-V virt machine and found goldfish
RTC to be really useful. Although other Goldfish devices are not very
attractive and we generally have an equivalent VirtIO device.

Ideally, we should have a VirtIO RTC thingy but not sure if RTC fits
in VirtIO ring programming model.

> 
> 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.)

The implementation matches corresponding driver available in upstream
Linux. We should certainly have a more complete document under
docs/sepcs (like you suggested) for this device.

> 
> > > 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.

Yes, I saw the workaround in PL031 and the backward compatibility bits.

This is first version of Goldfish RTC vmstate so we don't need the PL031
backward compatibility bits.

I will try implement this in v2 and you can have a look.

Regards,
Anup