Message-ID: <8f669a0b-6d49-4a77-cf2e-692afc2a38e2@opensynergy.com>
Date: Wed, 13 Sep 2023 11:10:21 +0200
From: Peter Hilber <peter.hilber@...nsynergy.com>
To: John Stultz <jstultz@...gle.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
 "kvmarm@...ts.linux.dev" <kvmarm@...ts.linux.dev>,
 Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
 "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 Paolo Bonzini <pbonzini@...hat.com>,
 Richard Cochran <richardcochran@...il.com>, Stephen Boyd <sboyd@...nel.org>,
 Thomas Gleixner <tglx@...utronix.de>, Vitaly Kuznetsov
 <vkuznets@...hat.com>, Wanpeng Li <wanpengli@...cent.com>,
 "x86@...nel.org" <x86@...nel.org>, Marc Zyngier <maz@...nel.org>,
 Oliver Upton <oliver.upton@...ux.dev>, James Morse <james.morse@....com>,
 Suzuki K Poulose <suzuki.poulose@....com>, Zenghui Yu
 <yuzenghui@...wei.com>, Sean Christopherson <seanjc@...gle.com>
Subject: Re: [RFC PATCH 0/4] treewide: Use clocksource id for
 get_device_system_crosststamp()

On Fri, Aug 25, 2023 6:18 John Stultz <jstultz@...gle.com> wrote:
> On Thu, Aug 17, 2023 at 6:13 PM Peter Hilber
> <peter.hilber@...nsynergy.com> wrote:
>>
>> This patch series changes struct system_counterval_t to identify the
>> clocksource through enum clocksource_ids, rather than through struct
>> clocksource *. The net effect of the patch series is that
>> get_device_system_crosststamp() callers can supply clocksource ids instead
>> of clocksource pointers, which can be problematic to get hold of.
>
> Hey Peter,
>   Thanks for sending this out. I'm a little curious though, can you
> expand a bit on how clocksource pointers can be problematic to get a
> hold of? What exactly is the problem that is motivating this change?
> 

Hi John,

I'm very sorry for the late reply; there was some unexpected delay.

Thank you for the remark; I'll expand on the motivation in the next patch
series iteration, similar to the explanation below.

The immediate motivation for this patch series is to enable the virtio_rtc
RFC v2 driver [4] to refer to the Arm Generic Timer without requiring new
helper functions in the arm_arch_timer driver. Other future
get_device_system_crosststamp() users may profit from this change as well.

Clocksource structs are normally private to clocksource drivers. Therefore,
get_device_system_crosststamp() callers require that clocksource drivers
expose the clocksource of interest in some way.

Drivers such as virtio_rtc [4] could obtain all information for calling
get_device_system_crosststamp() from their bound device, except for
clocksource identification. Such drivers' only direct relation with the
clocksource driver is clocksource identification. So using the clocksource
enum, rather than obtaining pointers in a clocksource driver specific way,
would reduce the coupling between the get_device_system_crosststamp()
callers and clocksource drivers.

Next, I provide some details to support the low coupling argument. There
are two sorts of get_device_system_crosststamp() callers in the current
kernel:

1) On Intel platforms, some PTP hardware clocks obtain the clocksource
pointer for get_device_system_crosststamp() using convert_art_to_tsc()
or convert_art_ns_to_tsc() from arch/x86.

2) The ptp_kvm driver uses kvm_arch_ptp_get_crosststamp(), which is
implemented for platforms with kvm_clock or arm_arch_timer.
Amongst other things, kvm_arch_ptp_get_crosststamp() returns a clocksource
pointer. The Arm implementation is in the arm_arch_timer driver.

When I proposed in the virtio_rtc RFC v1 patch series [3] to obtain the
clocksource pointer of the arm_arch_timer driver through a generic
helper function, one of the maintainers wasn't very enthusiastic about
it and suggested reusing kvm_arch_ptp_get_crosststamp() somehow [1]. But
to me there seems not to be much in common [2].

Quoting myself from [2]:

> 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).

This looks inelegant, since it would require virtio_rtc to put part of its
code into arm_arch_timer.c, and would require including a virtio_rtc header
in arm_arch_timer.c.

The second alternative is using this patch series to expand the use of the
clocksource enum to get_device_system_crosststamp(). This should also make
it easy to use get_device_system_crosststamp() with other clocksources in
the future, by just extending the clocksource enum.

> I just worry that switching to an enumeration solution might be
> eventually exposing more than we would like to userland.

ATM the enum is not in a UAPI header. So IMHO exposing this to userland in
the future would require a pretty explicit change.

Thanks for the review,

Peter

[1] https://lore.kernel.org/all/87ila4qwuw.wl-maz@kernel.org/
[2] https://lore.kernel.org/all/151befb2-8fbc-b796-47bb-39960a979065@opensynergy.com/
[3] https://lore.kernel.org/all/20230630171052.985577-1-peter.hilber@opensynergy.com/
[4] https://lore.kernel.org/all/20230818012014.212155-1-peter.hilber@opensynergy.com/

Powered by blists - more mailing lists