From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Anup Patel <Anup.Patel@wdc.com>,
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>
Cc: Atish Patra <Atish.Patra@wdc.com>,
"qemu-riscv@nongnu.org" <qemu-riscv@nongnu.org>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
Anup Patel <anup@brainfault.org>
Subject: Re: [PATCH v6 1/3] hw: rtc: Add Goldfish RTC device
Date: Wed, 6 Nov 2019 00:24:22 +0100 [thread overview]
Message-ID: <c095f5d9-372e-8b3c-9bfb-ca7569e31830@redhat.com> (raw)
In-Reply-To: <20191103075353.86497-2-anup.patel@wdc.com>
Hi Anup,
On 11/3/19 8:55 AM, Anup Patel 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 <anup.patel@wdc.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> hw/rtc/Kconfig | 3 +
> hw/rtc/Makefile.objs | 1 +
> hw/rtc/goldfish_rtc.c | 288 ++++++++++++++++++++++++++++++++++
> hw/rtc/trace-events | 4 +
> include/hw/rtc/goldfish_rtc.h | 46 ++++++
Correct path, thanks :)
> 5 files changed, 342 insertions(+)
> create mode 100644 hw/rtc/goldfish_rtc.c
> create mode 100644 include/hw/rtc/goldfish_rtc.h
>
> 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..f71f6eaab0
> --- /dev/null
> +++ b/hw/rtc/goldfish_rtc.c
> @@ -0,0 +1,288 @@
> +/*
> + * 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
I'd use a (fixed) release tag, and not the (unstable) master branch:
https://android.googlesource.com/platform/external/qemu/+/refs/heads/emu-2.0-release/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/rtc/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"
> +
> +#include "trace.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 {
> + /*
> + * We should be setting timer expiry to:
> + * qemu_clock_get_ns(rtc_clock) + (event - ticks)
> + * but this is equivalent to:
> + * event - s->tick_offset
> + */
> + timer_mod(s->timer, event - s->tick_offset);
> + s->alarm_running = 1;
> + }
> +}
> +
> +static uint64_t goldfish_rtc_read(void *opaque, hwaddr offset,
> + unsigned size)
> +{
> + GoldfishRTCState *s = opaque;
> + uint64_t r = 0;
> +
> + 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,
> + "%s: Bad offset 0x%x\n", __func__, (uint32_t)offset);
Hmm 0x20 is UNIMP.
> + break;
> + }
> +
> + trace_goldfish_rtc_read(offset, r);
> +
> + return r;
> +}
> +
> +static void goldfish_rtc_write(void *opaque, hwaddr offset,
> + uint64_t value, unsigned size)
> +{
> + GoldfishRTCState *s = opaque;
> + uint64_t current_tick, new_tick;
> +
> + switch (offset) {
> + case RTC_TIME_LOW:
> + current_tick = goldfish_rtc_get_count(s);
FYI, this 2 lines ...:
> + new_tick = current_tick & (0xffffffffULL << 32);
> + new_tick |= value;
... can also be written as:
new_tick = deposit64(current_tick, 0, 32, value);
(I find this notation easier to review).
> + 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:
Ditto these ...
> + s->alarm_next &= 0xffffffffULL;
> + s->alarm_next |= (value << 32);
... as:
s->alarm_next = deposit64(s->alarm_next, 32, 32, value);
> + 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,
> + "%s: Bad offset 0x%x\n", __func__, (uint32_t)offset);
0x20 is UNIMP.
> + break;
> + }
> +
> + trace_goldfish_rtc_write(offset, value);
> +}
> +
> +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 destination 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 destination Host.
Hmm this is the fix from commit 032cfe6a79c.
> + */
> + 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
OK.
> + }
> +};
> +
> +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);
The spec says this device's io region is 0x24 bytes. 0x1000 might be
your SoC mapping. Restricting it to the spec allow other targets to use
this device.
> + 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/hw/rtc/trace-events b/hw/rtc/trace-events
> index d6749f4616..0bfaa26cb8 100644
> --- a/hw/rtc/trace-events
> +++ b/hw/rtc/trace-events
> @@ -17,3 +17,7 @@ pl031_set_alarm(uint32_t ticks) "alarm set for %u ticks"
> # aspeed-rtc.c
> aspeed_rtc_read(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64
> aspeed_rtc_write(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64
> +
> +# goldfish_rtc.c
> +goldfish_rtc_read(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64
> +goldfish_rtc_write(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64
You declared a region of 0x1000 bytes, shouldn't the address format be
0x%03? I'd rather use the correct region size and keep 0x02 which is enough.
> diff --git a/include/hw/rtc/goldfish_rtc.h b/include/hw/rtc/goldfish_rtc.h
> new file mode 100644
> index 0000000000..3be586bdcb
> --- /dev/null
> +++ b/include/hw/rtc/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_RTC_GOLDFISH_RTC_H
> +#define HW_RTC_GOLDFISH_RTC_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_GOLDFISH_RTC "goldfish_rtc"
> +#define GOLDFISH_RTC(obj) \
> +OBJECT_CHECK(GoldfishRTCState, (obj), TYPE_GOLDFISH_RTC)
Maybe insert some spaces before the macro to improve readability.
> +
> +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
>
Rest of the patch looks good.
Regards,
Phil.
next prev parent reply other threads:[~2019-11-05 23:26 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-03 7:55 [PATCH v6 0/3] RTC support for QEMU RISC-V virt machine Anup Patel
2019-11-03 7:55 ` [PATCH v6 1/3] hw: rtc: Add Goldfish RTC device Anup Patel
2019-11-05 23:24 ` Philippe Mathieu-Daudé [this message]
2019-11-06 6:39 ` Anup Patel
2019-11-03 7:55 ` [PATCH v6 2/3] riscv: virt: Use " Anup Patel
2019-11-03 7:55 ` [PATCH v6 3/3] MAINTAINERS: Add maintainer entry for Goldfish RTC Anup Patel
2019-11-04 22:34 ` Alistair Francis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c095f5d9-372e-8b3c-9bfb-ca7569e31830@redhat.com \
--to=philmd@redhat.com \
--cc=Alistair.Francis@wdc.com \
--cc=Anup.Patel@wdc.com \
--cc=Atish.Patra@wdc.com \
--cc=anup@brainfault.org \
--cc=kbastian@mail.uni-paderborn.de \
--cc=palmer@sifive.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=sagark@eecs.berkeley.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).