Re: [PATCH v3 1/5] virtio: rtc: Add virtio RTC driver

[Kuan-Wei Chiu <visitorckw@gmail.com>]

On Sun, Oct 18, 2026 at 03:13:46AM +0800, Kuan-Wei Chiu wrote:
> On Sun, Oct 18, 2026 at 02:46:18AM +0800, Hung-Chun Tseng wrote:
> > Add a driver for the virtio RTC device (virtio device ID 17), which
> > QEMU provides as virtio-rtc-pci and virtio-rtc-device since v11.1.0.
> > 
> > The device exposes its clocks through a request virtqueue. As the
> > virtqueue must not be used before the virtio uclass sets DRIVER_OK,
> > which happens after probe(), the first read looks for the clock: it
> > asks for the number of clocks with VIRTIO_RTC_REQ_CFG and queries the
> > type of each with VIRTIO_RTC_REQ_CLOCK_CAP. Reading the time sends a
> > VIRTIO_RTC_REQ_READ for the chosen clock and converts the nanoseconds
> > since the epoch with rtc_to_tm(). Each request gives up with
> > -ETIMEDOUT if the device does not answer within a second, as virtio_9p
> > does. The clocks are read-only, so there is no set operation, and the
> > optional alarm feature is not negotiated.
> 
> This is a bit subjective, but my feeling is that this has become
> somewhat too verbose. Ideally, a commit message should explain the
> "why" rather than just stating what was done. What was done can
> basically be seen directly from the code changes themselves.
> 
> Regards,
> Kuan-Wei
> 
> > 
> > Like Linux, the driver prefers a VIRTIO_RTC_CLOCK_UTC_SMEARED clock.
> > Linux registers an RTC class device only for such a clock and skips
> > VIRTIO_RTC_CLOCK_UTC and VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED, which may
> > step on leap seconds. QEMU, however, exposes a single clock of type
> > VIRTIO_RTC_CLOCK_UTC, so following Linux strictly would leave U-Boot
> > without an RTC there. If no clock smears leap seconds, the driver
> > therefore falls back to the first UTC or maybe-smeared UTC clock. A
> > leap second step matters little to U-Boot, which only reads the time.
> > 
> > Like Linux, the driver stops looking for a clock as soon as a request
> > fails. After a timeout the request is still on the virtqueue, so its
> > late response could be taken for that of a later request.
> > 
> > VIRTIO_RTC defaults to y when VIRTIO and DM_RTC are enabled, like
> > VIRTIO_RNG does for DM_RNG.
> > 
> > If none of the clocks is UTC, the driver remembers it and fails later
> > reads with -ENODEV at once, as the set of clocks never changes.
> > 
> > Tested with QEMU v11.1.2 on qemu-riscv64 over both PCI and MMIO.
> > Sandbox tests, which emulate the device, follow later in this series.
> > 
> > Signed-off-by: Hung-Chun Tseng <alan.tseng.cs@gmail.com>
> > ---
> > Changes in v3:
> > - Give up on a request after one second instead of polling forever
> >   (Kuan-Wei)
> > - Remember that no clock is UTC, so that later reads fail at once
> >   (Simon)
> > - Expand the VIRTIO_RTC help text (Simon)
> > - Drop __packed from the message structs, which are naturally aligned
> >   (Kuan-Wei)
> > - Prefer a VIRTIO_RTC_CLOCK_UTC_SMEARED clock as Linux does, fall back
> >   to the first other UTC clock, and explain why in the commit message
> >   (Kuan-Wei)
> > - Stop looking for a clock as soon as a request fails, as Linux does
> >   (Kuan-Wei)
> > - Mention in the commit message that sandbox tests follow (Simon)
> > 
> > Changes in v2:
> > - Look for the UTC clock on the first virtio_rtc_get() instead of in
> >   probe(), as the virtqueue must not be used before DRIVER_OK (Bin)
> > 
> >  drivers/virtio/Kconfig         |  11 ++
> >  drivers/virtio/Makefile        |   1 +
> >  drivers/virtio/virtio-uclass.c |   1 +
> >  drivers/virtio/virtio_rtc.c    | 249 +++++++++++++++++++++++++++++++++
> >  include/virtio.h               |   4 +-
> >  5 files changed, 265 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/virtio/virtio_rtc.c
> > 
> > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > index 55b444280..48ab669f3 100644
> > --- a/drivers/virtio/Kconfig
> > +++ b/drivers/virtio/Kconfig
> > @@ -84,4 +84,15 @@ config VIRTIO_9P
> >  	help
> >  	  This enables the 9P transport driver over virtio.
> >  	  It can be used with QEMU based targets.
> > +
> > +config VIRTIO_RTC
> > +	bool "virtio RTC driver"
> > +	depends on DM_RTC
> > +	depends on VIRTIO
> > +	default y
> > +	help
> > +	  This enables the virtio RTC driver, which reads the UTC clock
> > +	  exposed by a virtio RTC device. QEMU v11.1.0 or later provides
> > +	  it as virtio-rtc-pci and virtio-rtc-device. The clocks are
> > +	  read-only, so the time cannot be set.
> >  endmenu
> > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > index 6e7bcfc8e..ddb5ce4eb 100644
> > --- a/drivers/virtio/Makefile
> > +++ b/drivers/virtio/Makefile
> > @@ -12,3 +12,4 @@ obj-$(CONFIG_VIRTIO_NET) += virtio_net.o
> >  obj-$(CONFIG_VIRTIO_BLK) += virtio_blk.o
> >  obj-$(CONFIG_VIRTIO_RNG) += virtio_rng.o
> >  obj-$(CONFIG_VIRTIO_9P) += virtio_9p.o
> > +obj-$(CONFIG_VIRTIO_RTC) += virtio_rtc.o
> > diff --git a/drivers/virtio/virtio-uclass.c b/drivers/virtio/virtio-uclass.c
> > index c163b2fd7..c6cf55922 100644
> > --- a/drivers/virtio/virtio-uclass.c
> > +++ b/drivers/virtio/virtio-uclass.c
> > @@ -31,6 +31,7 @@ static const char *const virtio_drv_name[VIRTIO_ID_MAX_NUM] = {
> >  	[VIRTIO_ID_BLOCK]	= VIRTIO_BLK_DRV_NAME,
> >  	[VIRTIO_ID_RNG]		= VIRTIO_RNG_DRV_NAME,
> >  	[VIRTIO_ID_9P]		= VIRTIO_9P_DRV_NAME,
> > +	[VIRTIO_ID_CLOCK]	= VIRTIO_RTC_DRV_NAME,
> >  };
> >  
> >  int virtio_get_config(struct udevice *vdev, unsigned int offset,
> > diff --git a/drivers/virtio/virtio_rtc.c b/drivers/virtio/virtio_rtc.c
> > new file mode 100644
> > index 000000000..806abe237
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_rtc.c
> > @@ -0,0 +1,249 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2026, Hung-Chun Tseng <alan.tseng.cs@gmail.com>
> > + *
> > + * Driver for the virtio RTC device (virtio device ID 17). The device
> > + * exposes one or more read-only clocks through a request virtqueue; this
> > + * driver reads a UTC clock, preferring one which smears leap seconds, and
> > + * reports it through the RTC uclass.
> > + */
> > +
> > +#include <div64.h>
> > +#include <dm.h>
> > +#include <log.h>
> > +#include <rtc.h>
> > +#include <time.h>
> > +#include <virtio_types.h>
> > +#include <virtio.h>
> > +#include <virtio_ring.h>
> > +#include <linux/types.h>
> > +
> > +#define VIRTIO_RTC_TIMEOUT_MS		1000
> > +
> > +#define VIRTIO_RTC_REQ_READ		0x0001
> > +#define VIRTIO_RTC_REQ_CFG		0x1000
> > +#define VIRTIO_RTC_REQ_CLOCK_CAP	0x1001
> > +
> > +#define VIRTIO_RTC_S_OK			0
> > +
> > +#define VIRTIO_RTC_CLOCK_UTC			0
> > +#define VIRTIO_RTC_CLOCK_UTC_SMEARED		3
> > +#define VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED	4
> > +
> > +struct virtio_rtc_req_head {
> > +	__le16 msg_type;
> > +	u8 reserved[6];
> > +};
> > +
> > +struct virtio_rtc_resp_head {
> > +	u8 status;
> > +	u8 reserved[7];
> > +};
> > +
> > +/* Request for VIRTIO_RTC_REQ_READ and VIRTIO_RTC_REQ_CLOCK_CAP */
> > +struct virtio_rtc_req_clock {
> > +	struct virtio_rtc_req_head head;
> > +	__le16 clock_id;
> > +	u8 reserved[6];
> > +};
> > +
> > +struct virtio_rtc_resp_read {
> > +	struct virtio_rtc_resp_head head;
> > +	__le64 clock_reading;
> > +};
> > +
> > +struct virtio_rtc_resp_cfg {
> > +	struct virtio_rtc_resp_head head;
> > +	__le16 num_clocks;
> > +	u8 reserved[6];
> > +};
> > +
> > +struct virtio_rtc_resp_clock_cap {
> > +	struct virtio_rtc_resp_head head;
> > +	u8 type;
> > +	u8 leap_second_smearing;
> > +	u8 flags;
> > +	u8 reserved[5];
> > +};
> > +
> > +struct virtio_rtc_priv {
> > +	struct virtqueue *vq;
> > +	u16 clock_id;
> > +	bool clock_found;
> > +	bool no_utc_clock;
> > +};
> > +
> > +static int virtio_rtc_xfer(struct udevice *dev, void *req, size_t req_len,
> > +			   void *resp, size_t resp_len)
> > +{
> > +	struct virtio_rtc_priv *priv = dev_get_priv(dev);
> > +	struct virtio_rtc_resp_head *head = resp;
> > +	struct virtio_sg req_sg = { req, req_len };
> > +	struct virtio_sg resp_sg = { resp, resp_len };
> > +	struct virtio_sg *sgs[] = { &req_sg, &resp_sg };
> > +	unsigned int len;
> > +	ulong start;
> > +	int ret;
> > +
> > +	ret = virtqueue_add(priv->vq, sgs, 1, 1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	virtqueue_kick(priv->vq);
> > +
> > +	start = get_timer(0);
> > +	while (!virtqueue_get_buf(priv->vq, &len)) {
> > +		if (get_timer(start) > VIRTIO_RTC_TIMEOUT_MS)
> > +			return -ETIMEDOUT;
> > +	}
> > +
> > +	if (len < sizeof(*head) || head->status != VIRTIO_RTC_S_OK)
> > +		return -EIO;
> > +
> > +	if (len < resp_len)
> > +		return -EPROTO;
> > +
> > +	return 0;
> > +}
> > +
> > +static bool virtio_rtc_is_utc(u8 type)
> > +{
> > +	return type == VIRTIO_RTC_CLOCK_UTC ||
> > +	       type == VIRTIO_RTC_CLOCK_UTC_SMEARED ||
> > +	       type == VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED;
> > +}
> > +
> > +static int virtio_rtc_find_utc_clock(struct udevice *dev)
> > +{
> > +	struct virtio_rtc_priv *priv = dev_get_priv(dev);
> > +	struct virtio_rtc_req_head cfg_req = {
> > +		.msg_type = cpu_to_le16(VIRTIO_RTC_REQ_CFG),
> > +	};
> > +	struct virtio_rtc_resp_cfg cfg = {};
> > +	u16 num_clocks, id, utc_id = 0;
> > +	bool utc_found = false;
> > +	int ret;
> > +
> > +	ret = virtio_rtc_xfer(dev, &cfg_req, sizeof(cfg_req), &cfg, sizeof(cfg));
> > +	if (ret)
> > +		return ret;
> > +
> > +	num_clocks = le16_to_cpu(cfg.num_clocks);
> > +	for (id = 0; id < num_clocks; id++) {
> > +		struct virtio_rtc_req_clock req = {
> > +			.head.msg_type = cpu_to_le16(VIRTIO_RTC_REQ_CLOCK_CAP),
> > +			.clock_id = cpu_to_le16(id),
> > +		};
> > +		struct virtio_rtc_resp_clock_cap cap = {};
> > +
> > +		/*
> > +		 * Give up on any error: after a timeout the request is still
> > +		 * on the virtqueue, so a later response could be mismatched.
> > +		 */

I guess your LLM added this comment while fixing my review comment?
I don't really see why we need a comment here. Even if we ignore the
virtqueue timeout mismatch issue, we already abort immediately on any
other error anyway. It looks like a standard and normal error handling
to me, so why is it special to need a comment here?

Regards,
Kuan-Wei

> > +		ret = virtio_rtc_xfer(dev, &req, sizeof(req), &cap, sizeof(cap));
> > +		if (ret)
> > +			return ret;
> > +
> > +		/*
> > +		 * Like Linux, prefer a clock which smears leap seconds. Unlike
> > +		 * Linux, fall back to the first other UTC clock, since that is
> > +		 * the only kind of clock that QEMU provides.
> > +		 */
> > +		if (cap.type == VIRTIO_RTC_CLOCK_UTC_SMEARED) {
> > +			utc_id = id;
> > +			utc_found = true;
> > +			break;
> > +		}
> > +
> > +		if (!utc_found && virtio_rtc_is_utc(cap.type)) {
> > +			utc_id = id;
> > +			utc_found = true;
> > +		}
> > +	}
> > +
> > +	/* The set of clocks is fixed, so remember that none of them is UTC */
> > +	if (!utc_found) {
> > +		priv->no_utc_clock = true;
> > +		return -ENODEV;
> > +	}
> > +
> > +	priv->clock_id = utc_id;
> > +	priv->clock_found = true;
> > +
> > +	return 0;
> > +}
> > +
> > +static int virtio_rtc_get(struct udevice *dev, struct rtc_time *time)
> > +{
> > +	struct virtio_rtc_priv *priv = dev_get_priv(dev);
> > +	struct virtio_rtc_req_clock req = {
> > +		.head.msg_type = cpu_to_le16(VIRTIO_RTC_REQ_READ),
> > +	};
> > +	struct virtio_rtc_resp_read resp = {};
> > +	u64 now;
> > +	int ret;
> > +
> > +	/*
> > +	 * The virtqueue must not be used before DRIVER_OK, which the virtio
> > +	 * uclass sets only after probe(), so look for the clock on first use.
> > +	 */
> > +	if (priv->no_utc_clock)
> > +		return -ENODEV;
> > +
> > +	if (!priv->clock_found) {
> > +		ret = virtio_rtc_find_utc_clock(dev);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	req.clock_id = cpu_to_le16(priv->clock_id);
> > +
> > +	ret = virtio_rtc_xfer(dev, &req, sizeof(req), &resp, sizeof(resp));
> > +	if (ret)
> > +		return ret;
> > +
> > +	now = le64_to_cpu(resp.clock_reading);
> > +	do_div(now, 1000000000U);
> > +	rtc_to_tm(now, time);
> > +
> > +	return 0;
> > +}
> > +
> > +static int virtio_rtc_bind(struct udevice *dev)
> > +{
> > +	struct virtio_dev_priv *uc_priv = dev_get_uclass_priv(dev->parent);
> > +
> > +	/* The alarm feature is not supported */
> > +	virtio_driver_features_init(uc_priv, NULL, 0, NULL, 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static int virtio_rtc_probe(struct udevice *dev)
> > +{
> > +	struct virtio_rtc_priv *priv = dev_get_priv(dev);
> > +	int ret;
> > +
> > +	ret = virtio_find_vqs(dev, 1, &priv->vq);
> > +	if (ret < 0) {
> > +		debug("%s: virtio_find_vqs failed\n", __func__);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct rtc_ops virtio_rtc_ops = {
> > +	.get	= virtio_rtc_get,
> > +};
> > +
> > +U_BOOT_DRIVER(virtio_rtc) = {
> > +	.name	= VIRTIO_RTC_DRV_NAME,
> > +	.id	= UCLASS_RTC,
> > +	.bind	= virtio_rtc_bind,
> > +	.probe	= virtio_rtc_probe,
> > +	.remove	= virtio_reset,
> > +	.ops	= &virtio_rtc_ops,
> > +	.priv_auto	= sizeof(struct virtio_rtc_priv),
> > +	.flags	= DM_FLAG_ACTIVE_DMA,
> > +};
> > diff --git a/include/virtio.h b/include/virtio.h
> > index b981eead5..95069a2de 100644
> > --- a/include/virtio.h
> > +++ b/include/virtio.h
> > @@ -32,12 +32,14 @@
> >  #define VIRTIO_ID_BLOCK		2 /* virtio block */
> >  #define VIRTIO_ID_RNG		4 /* virtio rng */
> >  #define VIRTIO_ID_9P		9 /* virtio 9p */
> > -#define VIRTIO_ID_MAX_NUM	10
> > +#define VIRTIO_ID_CLOCK		17 /* virtio clock/timer (rtc) */
> > +#define VIRTIO_ID_MAX_NUM	18
> >  
> >  #define VIRTIO_NET_DRV_NAME	"virtio-net"
> >  #define VIRTIO_BLK_DRV_NAME	"virtio-blk"
> >  #define VIRTIO_RNG_DRV_NAME	"virtio-rng"
> >  #define VIRTIO_9P_DRV_NAME	"virtio-9p"
> > +#define VIRTIO_RTC_DRV_NAME	"virtio-rtc"
> >  
> >  /* Feature bits for virtio 9P */
> >  #define VIRTIO_9P_MOUNT_TAG	0
> > -- 
> > 2.43.0
> >