Received-SPF: pass (google.com: domain of linux-kernel+bounces-97349-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 8 Mar 2024 18:03:00 +0100
From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Peter Hilber <peter.hilber@opensynergy.com>
Cc: linux-kernel@vger.kernel.org, virtualization@lists.linux.dev,
	virtio-dev@lists.oasis-open.org, linux-rtc@vger.kernel.org,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Jason Wang <jasowang@redhat.com>,
	Xuan Zhuo <xuanzhuo@linux.alibaba.com>,
	Alessandro Zummo <a.zummo@towertech.it>
Subject: Re: [RFC PATCH v3 7/7] virtio_rtc: Add RTC class driver
Message-ID: <2024030817030063a909f0@mail.local>
References: <20231218073849.35294-1-peter.hilber@opensynergy.com>
 <20231218073849.35294-8-peter.hilber@opensynergy.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231218073849.35294-8-peter.hilber@opensynergy.com>

Hello,

I'll start by saying that I'm sorry, I have a very very high level
knowledge about what virtio is.

On 18/12/2023 08:38:45+0100, Peter Hilber wrote:
> Expose the virtio-rtc UTC clock as an RTC clock to userspace, if it is
> present. Support RTC alarm if the virtio-rtc alarm feature is present. The
> virtio-rtc device signals an alarm by marking an alarmq buffer as used.
> 
> Peculiarities
> -------------
> 
> A virtio-rtc clock is a bit special for an RTC clock in that
> 
> - the clock may step (also backwards) autonomously at any time and
> 
> - the device, and its notification mechanism, will be reset during boot or
>   resume from sleep.
> 
> The virtio-rtc device avoids that the driver might miss an alarm. The
> device signals an alarm whenever the clock has reached or passed the alarm
> time, and also when the device is reset (on boot or resume from sleep), if
> the alarm time is in the past.
> 
> Open Issue
> ----------
> 
> The CLOCK_BOOTTIME_ALARM will use the RTC clock to wake up from sleep, and
> implicitly assumes that no RTC clock steps will occur during sleep. The RTC
> class driver does not know whether the current alarm is a real-time alarm
> or a boot-time alarm.
> 
> Perhaps this might be handled by the driver also setting a virtio-rtc
> monotonic alarm (which uses a clock similar to CLOCK_BOOTTIME_ALARM). The
> virtio-rtc monotonic alarm would just be used to wake up in case it was a
> CLOCK_BOOTTIME_ALARM alarm.
> 
> Otherwise, the behavior should not differ from other RTC class drivers.
> 

What I don't quite get is how this is actually related to RTCs. This
would be a super imprecise mechanism to get the current time and date
from the host to the guest which is what I think your are trying to do,
especially since this is not supporting UIE.
The host system clock may come from reading the RTC at some point in
time but more likely from another source so is it really the best
synchronization mechanism?

The other thing is that I don't quite get the point of the RTC alarm
versus a regular timer in this context.


[...]

> +static const struct rtc_class_ops viortc_class_with_alarm_ops = {
> +	.read_time = viortc_class_read_time,
> +	.read_alarm = viortc_class_read_alarm,
> +	.set_alarm = viortc_class_set_alarm,
> +	.alarm_irq_enable = viortc_class_alarm_irq_enable,
> +};
> +
> +static const struct rtc_class_ops viortc_class_no_alarm_ops = {
> +	.read_time = viortc_class_read_time,
> +};
> +

[...]

> +/**
> +/**
> + * viortc_class_init() - init RTC class wrapper and device
> + * @viortc: device data
> + * @vio_clk_id: virtio_rtc clock id
> + * @have_alarm: expose alarm ops
> + * @parent_dev: virtio device
> + *
> + * Context: Process context.
> + * Return: RTC class wrapper on success, ERR_PTR otherwise.
> + */
> +struct viortc_class *viortc_class_init(struct viortc_dev *viortc,
> +				       u16 vio_clk_id, bool have_alarm,
> +				       struct device *parent_dev)
> +{
> +	struct viortc_class *viortc_class;
> +	struct rtc_device *rtc;
> +
> +	viortc_class =
> +		devm_kzalloc(parent_dev, sizeof(*viortc_class), GFP_KERNEL);
> +	if (!viortc_class)
> +		return ERR_PTR(-ENOMEM);
> +
> +	viortc_class->viortc = viortc;
> +
> +	rtc = devm_rtc_allocate_device(parent_dev);
> +	if (IS_ERR(rtc))
> +		return ERR_PTR(PTR_ERR(rtc));
> +
> +	viortc_class->rtc = rtc;
> +
> +	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->features);
> +
> +	rtc->ops = have_alarm ? &viortc_class_with_alarm_ops :
> +				&viortc_class_no_alarm_ops;

Don't do this, simply clear the alarm feature.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com