[RFC PATCH 0/7] Add virtio_rtc module and related changes

[RFC PATCH 0/7] Add virtio_rtc module and related changes

[Peter Hilber]

This patch series adds the virtio_rtc module, and related bugfixes and
small interface extensions. The virtio_rtc module implements a driver
compatible with the proposed Virtio RTC device specification [1]. The
Virtio RTC (Real Time Clock) device provides information about current
time. The device can provide different clocks, e.g. for the UTC or TAI time
standards, or for physical time elapsed since some past epoch. The driver
can read the clocks with simple or more accurate methods.

This patch series depends, through its arm_arch_timer patch, on patch
"arm64/arch_timer: Provide noinstr sched_clock_read() functions" [3] which
seems not to be available with all development branches yet. Pull [2] for
an equivalent series on top of mst/linux-next, which should apply without
the above patch. Pull [5] for an equivalent series on top of
tip/timers/core. Pull [6] for an equivalent series on top of
tip/sched/core.

The series first fixes some bugs in the get_device_system_crosststamp()
interpolation code, which is required for reliable virtio_rtc operation.
Next, expose some Arm Generic Timer clocksource details for virtio_rtc.
Last, add the virtio_rtc implementation.

For the Virtio RTC device, there is currently a proprietary implementation,
which has been used for provisional testing.

virtio_rtc exposes clocks as PTP clocks to userspace, similar to ptp_kvm.
If both the Virtio RTC device and this driver have special support for the
current clocksource, time synchronization programs can use
cross-timestamping using ioctl PTP_SYS_OFFSET_PRECISE2 aka
PTP_SYS_OFFSET_PRECISE. Similar to ptp_kvm, system time synchronization
with single-digit ns precision is possible with a quiescent reference clock
(from the Virtio RTC device). This works even when the Virtio device
response is slow compared to ptp_kvm hypercalls.

The following illustrates a test using PTP_SYS_OFFSET_PRECISE, with
interspersed strace log and chrony [4] refclocks log, on arm64. In the
example, chrony tracks a virtio_rtc PTP clock ("PHCV", /dev/ptp0). The raw
offset between the virtio_rtc clock and CLOCK_REALTIME is 0 to 1 ns. At the
device side, the Virtio RTC device artificially delays both the clock read
request, and the response, by 50 ms. Cross-timestamp interpolation still
works with this delay. chrony also monitors a ptp_kvm clock ("PHCK",
/dev/ptp3) for comparison, which yields a similar offset.

	ioctl(5</dev/ptp3>, PTP_SYS_OFFSET_PRECISE, 0xffffe86691c8) = 0 <0.000329>
	===============================================================================
	   Date (UTC) Time         Refid  DP L P  Raw offset   Cooked offset      Disp.
	===============================================================================
	2023-06-29 18:49:55.595742 PHCK    0 N 0  1.000000e-09  8.717931e-10  5.500e-08
	2023-06-29 18:49:55.595742 PHCK    - N -       -        8.717931e-10  5.500e-08
	ioctl(6</dev/ptp0>, PTP_SYS_OFFSET_PRECISE, 0xffffe86691c8) = 0 <0.101545>
	2023-06-29 18:49:56.147766 PHCV    0 N 0  1.000000e-09  8.801870e-10  5.500e-08
	2023-06-29 18:49:56.147766 PHCV    - N -       -        8.801870e-10  5.500e-08
	ioctl(5</dev/ptp3>, PTP_SYS_OFFSET_PRECISE, 0xffffe86691c8) = 0 <0.000195>
	2023-06-29 18:49:56.202446 PHCK    0 N 0  1.000000e-09  7.364180e-10  5.500e-08
	2023-06-29 18:49:56.202446 PHCK    - N -       -        7.364180e-10  5.500e-08
	ioctl(6</dev/ptp0>, PTP_SYS_OFFSET_PRECISE, 0xffffe86691c8) = 0 <0.101484>
	2023-06-29 18:49:56.754641 PHCV    0 N 0  0.000000e+00 -2.617368e-10  5.500e-08
	2023-06-29 18:49:56.754641 PHCV    - N -       -       -2.617368e-10  5.500e-08
	ioctl(5</dev/ptp3>, PTP_SYS_OFFSET_PRECISE, 0xffffe86691c8) = 0 <0.000270>
	2023-06-29 18:49:56.809282 PHCK    0 N 0  1.000000e-09  7.779321e-10  5.500e-08
	2023-06-29 18:49:56.809282 PHCK    - N -       -        7.779321e-10  5.500e-08
	ioctl(6</dev/ptp0>, PTP_SYS_OFFSET_PRECISE, 0xffffe86691c8) = 0 <0.101510>
	2023-06-29 18:49:57.361376 PHCV    0 N 0  0.000000e+00 -2.198794e-10  5.500e-08
	2023-06-29 18:49:57.361376 PHCV    - N -       -       -2.198794e-10  5.500e-08

This patch series only adds special support for the Arm Generic Timer
clocksource. At the driver side, it should be easy to support more
clocksources.

Without special support for the current clocksource, time synchronization
programs can still use ioctl PTP_SYS_OFFSET_EXTENDED2 aka
PTP_SYS_OFFSET_EXTENDED. In this case, precision will generally be worse
and will depend on the Virtio device response characteristics.

The following illustrates a test using PTP_SYS_OFFSET_EXTENDED, with
interspersed strace log and chrony refclocks log, on x86-64 (with `ts'
values omitted):

	ioctl(5, PTP_SYS_OFFSET_EXTENDED, {n_samples=10, ts=OMITTED}) = 0
	===============================================================================
	   Date (UTC) Time         Refid  DP L P  Raw offset   Cooked offset      Disp.
	===============================================================================
	2023-06-28 14:11:26.697782 PHCV    0 N 0  3.318200e-05  3.450891e-05  4.611e-06
	2023-06-28 14:11:26.697782 PHCV    - N -       -        3.450891e-05  4.611e-06
	ioctl(5, PTP_SYS_OFFSET_EXTENDED, {n_samples=10, ts=OMITTED}) = 0
	2023-06-28 14:11:27.208763 PHCV    0 N 0 -3.792800e-05 -4.023965e-05  4.611e-06
	2023-06-28 14:11:27.208763 PHCV    - N -       -       -4.023965e-05  4.611e-06
	ioctl(5, PTP_SYS_OFFSET_EXTENDED, {n_samples=10, ts=OMITTED}) = 0
	2023-06-28 14:11:27.722818 PHCV    0 N 0 -3.328600e-05 -3.134404e-05  4.611e-06
	2023-06-28 14:11:27.722818 PHCV    - N -       -       -3.134404e-05  4.611e-06
	ioctl(5, PTP_SYS_OFFSET_EXTENDED, {n_samples=10, ts=OMITTED}) = 0
	2023-06-28 14:11:28.233572 PHCV    0 N 0 -4.966900e-05 -4.584331e-05  4.611e-06
	2023-06-28 14:11:28.233572 PHCV    - N -       -       -4.584331e-05  4.611e-06
	ioctl(5, PTP_SYS_OFFSET_EXTENDED, {n_samples=10, ts=OMITTED}) = 0
	2023-06-28 14:11:28.742737 PHCV    0 N 0  4.902700e-05  5.361388e-05  4.611e-06
	2023-06-28 14:11:28.742737 PHCV    - N -       -        5.361388e-05  4.611e-06

The following udev rule can be used to get a symlink /dev/ptp_virtio to the
UTC clock:

	SUBSYSTEM=="ptp", ATTR{clock_name}=="Virtio PTP UTC", SYMLINK += "ptp_virtio"

The following chrony configuration directive can then be added in
/etc/chrony/chrony.conf to synchronize to the Virtio UTC clock:

	refclock PHC /dev/ptp_virtio refid PHCV poll -1 dpoll -1

This patch series adds virtio_rtc not in drivers/ptp, but as a generic
Virtio driver. In the near future, virtio_rtc should be extended with an
RTC Class driver, along with extensions to the Virtio RTC device draft spec
to support RTC alarms.

Feedback is greatly appreciated.

[1] https://lists.oasis-open.org/archives/virtio-comment/202306/msg00592.html
[2] https://github.com/OpenSynergy/linux virtio-rtc-v1-on-mst-vhost-linux-next
[3] https://lore.kernel.org/r/20230519102715.435618812@infradead.org
[4] https://chrony.tuxfamily.org/
[5] https://github.com/OpenSynergy/linux virtio-rtc-v1-on-tip-timers-core
[6] https://github.com/OpenSynergy/linux virtio-rtc-v1-on-tip-sched-core


Peter Hilber (7):
  timekeeping: Fix cross-timestamp interpolation on counter wrap
  timekeeping: Fix cross-timestamp interpolation corner case decision
  timekeeping: Fix cross-timestamp interpolation for non-x86
  clocksource: arm_arch_timer: Export counter type, clocksource
  virtio_rtc: Add module and driver core
  virtio_rtc: Add PTP clocks
  virtio_rtc: Add Arm Generic Timer cross-timestamping

 MAINTAINERS                          |   7 +
 drivers/clocksource/arm_arch_timer.c |  16 +
 drivers/virtio/Kconfig               |  43 ++
 drivers/virtio/Makefile              |   4 +
 drivers/virtio/virtio_rtc_arm.c      |  44 ++
 drivers/virtio/virtio_rtc_driver.c   | 841 +++++++++++++++++++++++++++
 drivers/virtio/virtio_rtc_internal.h |  85 +++
 drivers/virtio/virtio_rtc_ptp.c      | 384 ++++++++++++
 include/clocksource/arm_arch_timer.h |  19 +
 include/uapi/linux/virtio_rtc.h      | 159 +++++
 kernel/time/timekeeping.c            |  20 +-
 11 files changed, 1615 insertions(+), 7 deletions(-)
 create mode 100644 drivers/virtio/virtio_rtc_arm.c
 create mode 100644 drivers/virtio/virtio_rtc_driver.c
 create mode 100644 drivers/virtio/virtio_rtc_internal.h
 create mode 100644 drivers/virtio/virtio_rtc_ptp.c
 create mode 100644 include/uapi/linux/virtio_rtc.h


base-commit: 6352a698ca5bf26a9199202666b16cf741f579f6
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH 4/7] clocksource: arm_arch_timer: Export counter type, clocksource

[Peter Hilber]

Export helper functions to allow other code to

- determine the counter type in use (virtual or physical, CP15 or memory),

- get a pointer to the arm_arch_timer clocksource, which can be compared
  with the current clocksource.

The virtio_rtc driver will require the clocksource pointer when using
get_device_system_crosststamp(), and should communicate the actual Arm
counter type to the Virtio RTC device (cf. spec draft [1]).

[1] https://lists.oasis-open.org/archives/virtio-comment/202306/msg00592.html

Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
---
 drivers/clocksource/arm_arch_timer.c | 16 ++++++++++++++++
 include/clocksource/arm_arch_timer.h | 19 +++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index e733a2a1927a..cebdc1b2db4c 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -92,6 +92,7 @@ static enum vdso_clock_mode vdso_default = VDSO_CLOCKMODE_ARCHTIMER;
 #else
 static enum vdso_clock_mode vdso_default = VDSO_CLOCKMODE_NONE;
 #endif /* CONFIG_GENERIC_GETTIMEOFDAY */
+static enum arch_timer_counter_type arch_counter_type __ro_after_init = ARCH_COUNTER_CP15_VIRT;
 
 static cpumask_t evtstrm_available = CPU_MASK_NONE;
 static bool evtstrm_enable __ro_after_init = IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM);
@@ -1109,6 +1110,7 @@ static void __init arch_counter_register(unsigned type)
 				rd = arch_counter_get_cntvct;
 				scr = arch_counter_get_cntvct;
 			}
+			arch_counter_type = ARCH_COUNTER_CP15_VIRT;
 		} else {
 			if (arch_timer_counter_has_wa()) {
 				rd = arch_counter_get_cntpct_stable;
@@ -1117,6 +1119,7 @@ static void __init arch_counter_register(unsigned type)
 				rd = arch_counter_get_cntpct;
 				scr = arch_counter_get_cntpct;
 			}
+			arch_counter_type = ARCH_COUNTER_CP15_PHYS;
 		}
 
 		arch_timer_read_counter = rd;
@@ -1124,6 +1127,7 @@ static void __init arch_counter_register(unsigned type)
 	} else {
 		arch_timer_read_counter = arch_counter_get_cntvct_mem;
 		scr = arch_counter_get_cntvct_mem;
+		arch_counter_type = ARCH_COUNTER_MEM_VIRT;
 	}
 
 	width = arch_counter_get_width();
@@ -1777,6 +1781,18 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
 TIMER_ACPI_DECLARE(arch_timer, ACPI_SIG_GTDT, arch_timer_acpi_init);
 #endif
 
+enum arch_timer_counter_type arch_timer_counter_get_type(void)
+{
+	return arch_counter_type;
+}
+EXPORT_SYMBOL_GPL(arch_timer_counter_get_type);
+
+struct clocksource *arch_timer_get_cs(void)
+{
+	return &clocksource_counter;
+}
+EXPORT_SYMBOL_GPL(arch_timer_get_cs);
+
 int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *ts,
 				 struct clocksource **cs)
 {
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index cbbc9a6dc571..b442db0b5ca0 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -43,6 +43,13 @@ enum arch_timer_spi_nr {
 	ARCH_TIMER_MAX_TIMER_SPI
 };
 
+enum arch_timer_counter_type {
+	ARCH_COUNTER_CP15_VIRT,
+	ARCH_COUNTER_CP15_PHYS,
+	ARCH_COUNTER_MEM_VIRT,
+	ARCH_COUNTER_MEM_PHYS,
+};
+
 #define ARCH_TIMER_PHYS_ACCESS		0
 #define ARCH_TIMER_VIRT_ACCESS		1
 #define ARCH_TIMER_MEM_PHYS_ACCESS	2
@@ -89,6 +96,8 @@ extern u32 arch_timer_get_rate(void);
 extern u64 (*arch_timer_read_counter)(void);
 extern struct arch_timer_kvm_info *arch_timer_get_kvm_info(void);
 extern bool arch_timer_evtstrm_available(void);
+extern enum arch_timer_counter_type arch_timer_counter_get_type(void);
+extern struct clocksource *arch_timer_get_cs(void);
 
 #else
 
@@ -107,6 +116,16 @@ static inline bool arch_timer_evtstrm_available(void)
 	return false;
 }
 
+static inline enum arch_timer_counter_type arch_timer_counter_get_type(void)
+{
+	return ARCH_COUNTER_CP15_VIRT;
+}
+
+static inline struct clocksource *arch_timer_get_cs(void)
+{
+	return NULL;
+}
+
 #endif
 
 #endif
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 4/7] clocksource: arm_arch_timer: Export counter type, clocksource

[Marc Zyngier]

On Fri, 30 Jun 2023 18:10:47 +0100,
Peter Hilber <peter.hilber@opensynergy.com> wrote:
> 
> Export helper functions to allow other code to
> 
> - determine the counter type in use (virtual or physical, CP15 or memory),
> 
> - get a pointer to the arm_arch_timer clocksource, which can be compared
>   with the current clocksource.
> 
> The virtio_rtc driver will require the clocksource pointer when using
> get_device_system_crosststamp(), and should communicate the actual Arm
> counter type to the Virtio RTC device (cf. spec draft [1]).

I really don't see why you should poke at the clocksource backend:

- the MMIO clocksource is only used in PM situations, which a virtio
  driver has no business being involved with

- only the virtual counter is relevant -- it is always at a 0-offset
  from the physical one when userspace has an opportunity to run

So it really looks that out of the four combinations, only one is
relevant.

I'm not Cc'd on the rest of the series, so I can't even see in which
context this is used. But as it is, the approach looks wrong.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 4/7] clocksource: arm_arch_timer: Export counter type, clocksource

[Peter Hilber]

On 03.07.23 10:13, Marc Zyngier wrote:
> On Fri, 30 Jun 2023 18:10:47 +0100,
> Peter Hilber <peter.hilber@opensynergy.com> wrote:
>>
>> Export helper functions to allow other code to
>>
>> - determine the counter type in use (virtual or physical, CP15 or memory),
>>
>> - get a pointer to the arm_arch_timer clocksource, which can be compared
>>   with the current clocksource.
>>
>> The virtio_rtc driver will require the clocksource pointer when using
>> get_device_system_crosststamp(), and should communicate the actual Arm
>> counter type to the Virtio RTC device (cf. spec draft [1]).
> 
> I really don't see why you should poke at the clocksource backend:
> 
> - the MMIO clocksource is only used in PM situations, which a virtio
>   driver has no business being involved with
> 
> - only the virtual counter is relevant -- it is always at a 0-offset
>   from the physical one when userspace has an opportunity to run
> 
> So it really looks that out of the four combinations, only one is
> relevant.

Thanks for the explanation. Dropping arch_timer_counter_get_type() and
assuming that the CP15 virtual counter is in use should also work for
now. With the physical/virtual counter distinction, I tried to be
future-proof, as per the following considerations:

The intended consumer of arch_timer_counter_get_type() is the Virtio RTC
device (draft spec [2], patch series [1]). If the Virtio device has
optional cross-timestamp support, it must know the current Linux kernel
view of the current clocksource counter. The Virtio driver tells the
Virtio device the current counter type (in the Arm case, CNTVCT_EL0 or
CNTPCT_EL0). It is intentionally left unspecified how the Virtio device
would know the counter value. AFAIU, if the Linux kernel were a
virtualization host itself, it would be better for the Virtio device to
look at the physical counter, since the virtual counter could be set for
a guest. And in other cases, the guest OSes use a virtual counter with
an offset.

This was the rationale to come up with the physical/virtual counter
distinction for the Virtio RTC device. Looking at extensions such as
FEAT_ECV, where the CNTPCT_EL0 value can depend on the EL, or FEAT_NV*,
it might be a bit simplistic.

Does this physical/virtual counter distinction sound like a good idea?
Otherwise I would drop the arch_timer_counter_get_type() in the next
iteration.

> 
> I'm not Cc'd on the rest of the series, so I can't even see in which
> context this is used. But as it is, the approach looks wrong.
> 

Sorry, I will Cc you on all relevant patches in the next iteration,
which I will send out soon.

The first patch series can be found at [1]. I think the second helper
function in this patch, arch_timer_get_cs(), would still be needed, in
order to supply the clocksource to get_device_system_crosststamp().

Thanks for the comments,

Peter

[1] https://lore.kernel.org/lkml/20230630171052.985577-1-peter.hilber@opensynergy.com/T/#me7df2d4db4fe1119d821dc9c4054b9404c15b02d
[2] https://lists.oasis-open.org/archives/virtio-comment/202306/msg00592.html

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 4/7] clocksource: arm_arch_timer: Export counter type, clocksource

[Marc Zyngier]

On Thu, 27 Jul 2023 11:22:11 +0100,
Peter Hilber <peter.hilber@opensynergy.com> wrote:
> 
> On 03.07.23 10:13, Marc Zyngier wrote:
> > On Fri, 30 Jun 2023 18:10:47 +0100,
> > Peter Hilber <peter.hilber@opensynergy.com> wrote:
> >>
> >> Export helper functions to allow other code to
> >>
> >> - determine the counter type in use (virtual or physical, CP15 or memory),
> >>
> >> - get a pointer to the arm_arch_timer clocksource, which can be compared
> >>   with the current clocksource.
> >>
> >> The virtio_rtc driver will require the clocksource pointer when using
> >> get_device_system_crosststamp(), and should communicate the actual Arm
> >> counter type to the Virtio RTC device (cf. spec draft [1]).
> > 
> > I really don't see why you should poke at the clocksource backend:
> > 
> > - the MMIO clocksource is only used in PM situations, which a virtio
> >   driver has no business being involved with
> > 
> > - only the virtual counter is relevant -- it is always at a 0-offset
> >   from the physical one when userspace has an opportunity to run
> > 
> > So it really looks that out of the four combinations, only one is
> > relevant.
> 
> Thanks for the explanation. Dropping arch_timer_counter_get_type() and
> assuming that the CP15 virtual counter is in use should also work for
> now. With the physical/virtual counter distinction, I tried to be
> future-proof, as per the following considerations:
> 
> The intended consumer of arch_timer_counter_get_type() is the Virtio RTC
> device (draft spec [2], patch series [1]). If the Virtio device has
> optional cross-timestamp support, it must know the current Linux kernel
> view of the current clocksource counter. The Virtio driver tells the
> Virtio device the current counter type (in the Arm case, CNTVCT_EL0 or
> CNTPCT_EL0). It is intentionally left unspecified how the Virtio device
> would know the counter value. AFAIU, if the Linux kernel were a
> virtualization host itself, it would be better for the Virtio device to
> look at the physical counter, since the virtual counter could be set for
> a guest. And in other cases, the guest OSes use a virtual counter with
> an offset.

The physical counter can equally be offset (and KVM does offset it),
just like the virtual counter. With NV, the offsets themselves are
partially under control of the guest itself.

So either counters *as seen from the guest* are absolutely pointless
to an observer on the host. That observer sees a virtual counter that
is strictly equal to the physical counter.

> This was the rationale to come up with the physical/virtual counter
> distinction for the Virtio RTC device. Looking at extensions such as
> FEAT_ECV, where the CNTPCT_EL0 value can depend on the EL, or FEAT_NV*,
> it might be a bit simplistic.

Not just simplistic. It doesn't make sense. For this to work, you'd
need to know the global offset that KVM applies to the global counter,
plus the *virtualised* CNTPOFF/CNTVOFF values that the guest can
change at any time on a *per-CPU* basis. None of that is available
outside of KVM, nor would it make any sense anyway.

> Does this physical/virtual counter distinction sound like a good idea?
> Otherwise I would drop the arch_timer_counter_get_type() in the next
> iteration.

My take on this is that only the global counter value makes any sense.
That value is already available from the host as the virtual counter,
because we guarantee that CNTVOFF is 0 when outside of the guest
(well, technically, outside of the vcpu_load/vcpu_put section).

> 
> > 
> > I'm not Cc'd on the rest of the series, so I can't even see in which
> > context this is used. But as it is, the approach looks wrong.
> > 
> 
> Sorry, I will Cc you on all relevant patches in the next iteration,
> which I will send out soon.
> 
> The first patch series can be found at [1]. I think the second helper
> function in this patch, arch_timer_get_cs(), would still be needed, in
> order to supply the clocksource to get_device_system_crosststamp().

We already have to deal with the kvm_arch_ptp_get_crosststamp()
monstrosity (which I will forever regret having merged). Surely you
can reuse some of that?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 4/7] clocksource: arm_arch_timer: Export counter type, clocksource

[Peter Hilber]

On 28.07.23 10:11, Marc Zyngier wrote:
> On Thu, 27 Jul 2023 11:22:11 +0100,
> Peter Hilber <peter.hilber@opensynergy.com> wrote:
>>
>> On 03.07.23 10:13, Marc Zyngier wrote:
>>> On Fri, 30 Jun 2023 18:10:47 +0100,
>>> Peter Hilber <peter.hilber@opensynergy.com> wrote:
>>>>
>>>> Export helper functions to allow other code to
>>>>
>>>> - determine the counter type in use (virtual or physical, CP15 or memory),
>>>>
>>>> - get a pointer to the arm_arch_timer clocksource, which can be compared
>>>>   with the current clocksource.
>>>>
>>>> The virtio_rtc driver will require the clocksource pointer when using
>>>> get_device_system_crosststamp(), and should communicate the actual Arm
>>>> counter type to the Virtio RTC device (cf. spec draft [1]).
>>>
>>> I really don't see why you should poke at the clocksource backend:
>>>
>>> - the MMIO clocksource is only used in PM situations, which a virtio
>>>   driver has no business being involved with
>>>
>>> - only the virtual counter is relevant -- it is always at a 0-offset
>>>   from the physical one when userspace has an opportunity to run
>>>
>>> So it really looks that out of the four combinations, only one is
>>> relevant.
>>
>> Thanks for the explanation. Dropping arch_timer_counter_get_type() and
>> assuming that the CP15 virtual counter is in use should also work for
>> now. With the physical/virtual counter distinction, I tried to be
>> future-proof, as per the following considerations:
>>
>> The intended consumer of arch_timer_counter_get_type() is the Virtio RTC
>> device (draft spec [2], patch series [1]). If the Virtio device has
>> optional cross-timestamp support, it must know the current Linux kernel
>> view of the current clocksource counter. The Virtio driver tells the
>> Virtio device the current counter type (in the Arm case, CNTVCT_EL0 or
>> CNTPCT_EL0). It is intentionally left unspecified how the Virtio device
>> would know the counter value. AFAIU, if the Linux kernel were a
>> virtualization host itself, it would be better for the Virtio device to
>> look at the physical counter, since the virtual counter could be set for
>> a guest. And in other cases, the guest OSes use a virtual counter with
>> an offset.
> 
> The physical counter can equally be offset (and KVM does offset it),
> just like the virtual counter. With NV, the offsets themselves are
> partially under control of the guest itself.
> 
> So either counters *as seen from the guest* are absolutely pointless
> to an observer on the host. That observer sees a virtual counter that
> is strictly equal to the physical counter.
> 
>> This was the rationale to come up with the physical/virtual counter
>> distinction for the Virtio RTC device. Looking at extensions such as
>> FEAT_ECV, where the CNTPCT_EL0 value can depend on the EL, or FEAT_NV*,
>> it might be a bit simplistic.
> 
> Not just simplistic. It doesn't make sense. For this to work, you'd
> need to know the global offset that KVM applies to the global counter,
> plus the *virtualised* CNTPOFF/CNTVOFF values that the guest can
> change at any time on a *per-CPU* basis. None of that is available
> outside of KVM, nor would it make any sense anyway.
> 
>> Does this physical/virtual counter distinction sound like a good idea?
>> Otherwise I would drop the arch_timer_counter_get_type() in the next
>> iteration.
> 
> My take on this is that only the global counter value makes any sense.
> That value is already available from the host as the virtual counter,
> because we guarantee that CNTVOFF is 0 when outside of the guest
> (well, technically, outside of the vcpu_load/vcpu_put section).
> 

OK, I agree that a virtual/physical counter distinction doesn't make
sense on Linux (unless one wants to abuse it to distinguish very special
use cases with and without Linux).

Probably I'll also remove the distinction from the spec draft.

>>
>>>
>>> I'm not Cc'd on the rest of the series, so I can't even see in which
>>> context this is used. But as it is, the approach looks wrong.
>>>
>>
>> Sorry, I will Cc you on all relevant patches in the next iteration,
>> which I will send out soon.
>>
>> The first patch series can be found at [1]. I think the second helper
>> function in this patch, arch_timer_get_cs(), would still be needed, in
>> order to supply the clocksource to get_device_system_crosststamp().
> 
> We already have to deal with the kvm_arch_ptp_get_crosststamp()
> monstrosity (which I will forever regret having merged). Surely you
> can reuse some of that?
> 

I'm not sure what this is hinting at.

From the virtio_rtc perspective, the only behavior shared with
kvm_arch_ptp_get_crosststamp() would be exposing &clocksource_counter
(and distinguishing virtual/physical counter, but virtio_rtc should stop
doing this). Exposing &clocksource_counter is also the only thing
arch_timer_get_cs() does.

If &clocksource_counter should not be exposed, then I can see two
alternatives:

Alternative 1: Put a function of type

	int (*get_time_fn) (ktime_t *device_time, 
	                    struct system_counterval_t *sys_counterval,
			    void *ctx)

into arm_arch_timer.c, as required by get_device_system_crosststamp()
(and include a virtio_rtc header).

Alternative 2: Change get_device_system_crosststamp(), resp. struct
system_counterval_t, to use enum clocksource_ids to identify a
clocksource, instead of using struct clocksource *. Then there would be
no need to change arm_arch_timer.

Thanks for the feedback,

Peter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel