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

[Anup Patel]

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

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

> 
> thanks
> -- PMM

Regards,
Anup