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

[Aleksandar Markovic <aleksandar.m.mail@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 15026 bytes --]

On Saturday, October 19, 2019, Anup Patel <Anup.Patel@wdc.com> wrote:

> Hi,
>
> From: Aleksandar Markovic <aleksandar.m.mail@gmail.com>
> Sent: Saturday, October 19, 2019 2:45 AM
> To: Anup Patel <Anup.Patel@wdc.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>; Palmer Dabbelt <
> palmer@sifive.com>; Alistair Francis <Alistair.Francis@wdc.com>; Sagar
> Karandikar <sagark@eecs.berkeley.edu>; Bastian Koppelmann <
> kbastian@mail.uni-paderborn.de>; Atish Patra <Atish.Patra@wdc.com>;
> qemu-riscv@nongnu.org; qemu-devel@nongnu.org; Anup Patel <
> anup@brainfault.org>
> Subject: Re: [PATCH v3 1/2] hw: timer: Add Goldfish RTC device
>
> ū
>
> On Tuesday, October 15, 2019, Anup Patel <mailto:Anup.Patel@wdc.com>
> 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 available but untested for Goldfish RTC
> device. It will be hardened in-future when we implement VM migration for
> KVM RISC-V.
>
> Signed-off-by: Anup Patel <mailto:anup.patel@wdc.com>
> ---
>  hw/rtc/Kconfig                  |   3 +
>  hw/rtc/Makefile.objs            |   1 +
>  hw/rtc/goldfish_rtc.c           | 278 ++++++++++++++++++++++++++++++++
>  include/hw/timer/goldfish_rtc.h |  46 ++++++
>  4 files changed, 328 insertions(+)
>  create mode 100644 hw/rtc/goldfish_rtc.c
>  create mode 100644 include/hw/timer/goldfish_rtc.h
>
> Do you plan to add some other devices from Goldfish platform?
>
> [Anup] Nopes, we just needed a simple RTC device for RISC-V
> [Anup] virt machine hence this patch.
>
> [Anup] Most of the required devices are already there in
> [Anup] the VirtIO spec. The RTC device was missing in
> [Anup] VirtIO spec so instead of creating new virtual RTC
> [Anup] device we just re-use Goldfish RTC. This help us
> [Anup] re-use the Linux Goldfish RTC driver.
>
> Did you base your code on Android emulator code?
>
> [Anup] Nopes, the android emulator code seemed too simple
> [Anup] and lacking. In fact, the Linux Goldfish RTC driver does
> [Anup] much more.
>
> [Anup] Based on the documentation and Linux Goldfish RTC
> [Anup] driver, I wrote it from scratch.
>
>
Ok, that is great, nice work. I just wanted to doublecheck if we are OK
from the aspect of authorship. "Western Digital" in copyright preamble is
certainly fine in this case.

A.

[Anup] I first added Goldfish RTC emulator to Xvisor RISC-V
> [Anup] virt machine. Got it working over there and then
> [Anup] ported it to QEMU. For qemu emulation, I looked
> [Anup] at existing RTC emulators (such as PL031) for QEMU
> [Anup] API usage.
>
> Related to the previous question, are you sure the copyright line should
> go to Western Digital only?
>
> [Anup] I thought copyright will be of implementer hence I
> [Anup] put Western Digital only. The spec and Linux driver
> [Anup] can be written by totally different folks.
>
> Regards,
> Anup
>
> diff --git a/hw/rtc/Kconfig b/hw/rtc/Kconfig
> index 45daa8d655..bafe6ac2c9 100644
> --- a/hw/rtc/Kconfig
> +++ b/hw/rtc/Kconfig
> @@ -21,3 +21,6 @@ config MC146818RTC
>
>  config SUN4V_RTC
>      bool
> +
> +config GOLDFISH_RTC
> +    bool
> diff --git a/hw/rtc/Makefile.objs b/hw/rtc/Makefile.objs
> index 8dc9fcd3a9..aa208d0d10 100644
> --- a/hw/rtc/Makefile.objs
> +++ b/hw/rtc/Makefile.objs
> @@ -11,3 +11,4 @@ common-obj-$(CONFIG_EXYNOS4) += exynos4210_rtc.o
>  obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
>  common-obj-$(CONFIG_SUN4V_RTC) += sun4v-rtc.o
>  common-obj-$(CONFIG_ASPEED_SOC) += aspeed_rtc.o
> +common-obj-$(CONFIG_GOLDFISH_RTC) += goldfish_rtc.o
> diff --git a/hw/rtc/goldfish_rtc.c b/hw/rtc/goldfish_rtc.c
> new file mode 100644
> index 0000000000..223616ed75
> --- /dev/null
> +++ b/hw/rtc/goldfish_rtc.c
> @@ -0,0 +1,278 @@
> +/*
> + * Goldfish virtual platform RTC
> + *
> + * Copyright (C) 2019 Western Digital Corporation or its affiliates.
> + *
> + * For more details on Google Goldfish virtual platform refer:
> + * https://android.googlesource.com/platform/external/qemu/+/
> master/docs/GOLDFISH-VIRTUAL-HARDWARE.TXT
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "hw/timer/goldfish_rtc.h"
> +#include "migration/vmstate.h"
> +#include "hw/irq.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/sysbus.h"
> +#include "qemu/timer.h"
> +#include "sysemu/sysemu.h"
> +#include "qemu/cutils.h"
> +#include "qemu/log.h"
> +
> +#define RTC_TIME_LOW            0x00
> +#define RTC_TIME_HIGH           0x04
> +#define RTC_ALARM_LOW           0x08
> +#define RTC_ALARM_HIGH          0x0c
> +#define RTC_IRQ_ENABLED         0x10
> +#define RTC_CLEAR_ALARM         0x14
> +#define RTC_ALARM_STATUS        0x18
> +#define RTC_CLEAR_INTERRUPT     0x1c
> +
> +static void goldfish_rtc_update(GoldfishRTCState *s)
> +{
> +    qemu_set_irq(s->irq, (s->irq_pending & s->irq_enabled) ? 1 : 0);
> +}
> +
> +static void goldfish_rtc_interrupt(void *opaque)
> +{
> +    GoldfishRTCState *s = (GoldfishRTCState *)opaque;
> +
> +    s->alarm_running = 0;
> +    s->irq_pending = 1;
> +    goldfish_rtc_update(s);
> +}
> +
> +static uint64_t goldfish_rtc_get_count(GoldfishRTCState *s)
> +{
> +    return s->tick_offset + (uint64_t)qemu_clock_get_ns(rtc_clock);
> +}
> +
> +static void goldfish_rtc_clear_alarm(GoldfishRTCState *s)
> +{
> +    timer_del(s->timer);
> +    s->alarm_running = 0;
> +}
> +
> +static void goldfish_rtc_set_alarm(GoldfishRTCState *s)
> +{
> +    uint64_t ticks = goldfish_rtc_get_count(s);
> +    uint64_t event = s->alarm_next;
> +
> +    if (event <= ticks) {
> +        goldfish_rtc_clear_alarm(s);
> +        goldfish_rtc_interrupt(s);
> +    } else {
> +        int64_t now = qemu_clock_get_ns(rtc_clock);
> +        timer_mod(s->timer, now + (event - ticks));
> +        s->alarm_running = 1;
> +    }
> +}
> +
> +static uint64_t goldfish_rtc_read(void *opaque, hwaddr offset,
> +                                  unsigned size)
> +{
> +    GoldfishRTCState *s = (GoldfishRTCState *)opaque;
> +    uint64_t r;
> +
> +    switch (offset) {
> +    case RTC_TIME_LOW:
> +        r = goldfish_rtc_get_count(s) & 0xffffffff;
> +        break;
> +    case RTC_TIME_HIGH:
> +        r = goldfish_rtc_get_count(s) >> 32;
> +        break;
> +    case RTC_ALARM_LOW:
> +        r = s->alarm_next & 0xffffffff;
> +        break;
> +    case RTC_ALARM_HIGH:
> +        r = s->alarm_next >> 32;
> +        break;
> +    case RTC_IRQ_ENABLED:
> +        r = s->irq_enabled;
> +        break;
> +    case RTC_ALARM_STATUS:
> +        r = s->alarm_running;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "goldfish_rtc_read: Bad offset 0x%x\n",
> (int)offset);
> +        r = 0;
> +        break;
> +    }
> +
> +    return r;
> +}
> +
> +static void goldfish_rtc_write(void *opaque, hwaddr offset,
> +                               uint64_t value, unsigned size)
> +{
> +    uint64_t current_tick, new_tick;
> +    GoldfishRTCState *s = (GoldfishRTCState *)opaque;
> +
> +    switch (offset) {
> +    case RTC_TIME_LOW:
> +        current_tick = goldfish_rtc_get_count(s);
> +        new_tick = current_tick & (0xffffffffULL << 32);
> +        new_tick |= value;
> +        s->tick_offset += new_tick - current_tick;
> +        break;
> +    case RTC_TIME_HIGH:
> +        current_tick = goldfish_rtc_get_count(s);
> +        new_tick = current_tick & 0xffffffffULL;
> +        new_tick |= (value << 32);
> +        s->tick_offset += new_tick - current_tick;
> +        break;
> +    case RTC_ALARM_LOW:
> +        s->alarm_next &= (0xffffffffULL << 32);
> +        s->alarm_next |= value;
> +        goldfish_rtc_set_alarm(s);
> +        break;
> +    case RTC_ALARM_HIGH:
> +        s->alarm_next &= 0xffffffffULL;
> +        s->alarm_next |= (value << 32);
> +        break;
> +    case RTC_IRQ_ENABLED:
> +        s->irq_enabled = (uint32_t)(value & 0x1);
> +        goldfish_rtc_update(s);
> +        break;
> +    case RTC_CLEAR_ALARM:
> +        goldfish_rtc_clear_alarm(s);
> +        break;
> +    case RTC_CLEAR_INTERRUPT:
> +        s->irq_pending = 0;
> +        goldfish_rtc_update(s);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "goldfish_rtc_write: Bad offset 0x%x\n",
> (int)offset);
> +        break;
> +    }
> +}
> +
> +static int goldfish_rtc_pre_save(void *opaque)
> +{
> +    uint64_t delta;
> +    GoldfishRTCState *s = opaque;
> +
> +    /*
> +     * We want to migrate this offset, which sounds straightforward.
> +     * Unfortunately, we cannot directly pass tick_offset because
> +     * rtc_clock on destinetion Host might not be same source Host.
> +     *
> +     * To tackle, this we pass tick_offset relative to vm_clock from
> +     * source Host and make it relative to rtc_clock at destinetion Host.
> +     */
> +    delta = qemu_clock_get_ns(rtc_clock) -
> +            qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    s->tick_offset_vmstate = s->tick_offset + delta;
> +
> +    return 0;
> +}
> +
> +static int goldfish_rtc_post_load(void *opaque, int version_id)
> +{
> +    uint64_t delta;
> +    GoldfishRTCState *s = opaque;
> +
> +    /*
> +     * We extract tick_offset from tick_offset_vmstate by doing
> +     * reverse math compared to pre_save() function.
> +     */
> +    delta = qemu_clock_get_ns(rtc_clock) -
> +            qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    s->tick_offset = s->tick_offset_vmstate - delta;
> +
> +    return 0;
> +}
> +
> +static const MemoryRegionOps goldfish_rtc_ops = {
> +    .read = goldfish_rtc_read,
> +    .write = goldfish_rtc_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4
> +    }
> +};
> +
> +static const VMStateDescription goldfish_rtc_vmstate = {
> +    .name = TYPE_GOLDFISH_RTC,
> +    .version_id = 1,
> +    .pre_save = goldfish_rtc_pre_save,
> +    .post_load = goldfish_rtc_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(tick_offset_vmstate, GoldfishRTCState),
> +        VMSTATE_UINT64(alarm_next, GoldfishRTCState),
> +        VMSTATE_UINT32(alarm_running, GoldfishRTCState),
> +        VMSTATE_UINT32(irq_pending, GoldfishRTCState),
> +        VMSTATE_UINT32(irq_enabled, GoldfishRTCState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void goldfish_rtc_reset(DeviceState *dev)
> +{
> +    GoldfishRTCState *s = GOLDFISH_RTC(dev);
> +    struct tm tm;
> +
> +    timer_del(s->timer);
> +
> +    qemu_get_timedate(&tm, 0);
> +    s->tick_offset = mktimegm(&tm);
> +    s->tick_offset *= NANOSECONDS_PER_SECOND;
> +    s->tick_offset -= qemu_clock_get_ns(rtc_clock);
> +    s->tick_offset_vmstate = 0;
> +    s->alarm_next = 0;
> +    s->alarm_running = 0;
> +    s->irq_pending = 0;
> +    s->irq_enabled = 0;
> +}
> +
> +static void goldfish_rtc_realize(DeviceState *d, Error **errp)
> +{
> +    SysBusDevice *dev = SYS_BUS_DEVICE(d);
> +    GoldfishRTCState *s = GOLDFISH_RTC(d);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &goldfish_rtc_ops, s,
> +                          "goldfish_rtc", 0x1000);
> +    sysbus_init_mmio(dev, &s->iomem);
> +
> +    sysbus_init_irq(dev, &s->irq);
> +
> +    s->timer = timer_new_ns(rtc_clock, goldfish_rtc_interrupt, s);
> +}
> +
> +static void goldfish_rtc_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = goldfish_rtc_realize;
> +    dc->reset = goldfish_rtc_reset;
> +    dc->vmsd = &goldfish_rtc_vmstate;
> +}
> +
> +static const TypeInfo goldfish_rtc_info = {
> +    .name          = TYPE_GOLDFISH_RTC,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(GoldfishRTCState),
> +    .class_init    = goldfish_rtc_class_init,
> +};
> +
> +static void goldfish_rtc_register_types(void)
> +{
> +    type_register_static(&goldfish_rtc_info);
> +}
> +
> +type_init(goldfish_rtc_register_types)
> diff --git a/include/hw/timer/goldfish_rtc.h b/include/hw/timer/goldfish_
> rtc.h
> new file mode 100644
> index 0000000000..dfb6d70b86
> --- /dev/null
> +++ b/include/hw/timer/goldfish_rtc.h
> @@ -0,0 +1,46 @@
> +/*
> + * Goldfish virtual platform RTC
> + *
> + * Copyright (C) 2019 Western Digital Corporation or its affiliates.
> + *
> + * For more details on Google Goldfish virtual platform refer:
> + * https://android.googlesource.com/platform/external/qemu/+/
> master/docs/GOLDFISH-VIRTUAL-HARDWARE.TXT
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef HW_TIMER_GOLDFISH_RTC_H
> +#define HW_TIMER_GOLDFISH_RTC_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_GOLDFISH_RTC "goldfish_rtc"
> +#define GOLDFISH_RTC(obj) \
> +OBJECT_CHECK(GoldfishRTCState, (obj), TYPE_GOLDFISH_RTC)
> +
> +typedef struct GoldfishRTCState {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion iomem;
> +    QEMUTimer *timer;
> +    qemu_irq irq;
> +
> +    uint64_t tick_offset;
> +    uint64_t tick_offset_vmstate;
> +    uint64_t alarm_next;
> +    uint32_t alarm_running;
> +    uint32_t irq_pending;
> +    uint32_t irq_enabled;
> +} GoldfishRTCState;
> +
> +#endif
> --
> 2.17.1
>
>

[-- Attachment #2: Type: text/html, Size: 18390 bytes --]