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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx/T/#me7df2d4db4fe1119d821dc9c4054b9404c15b02d
[2] https://lists.oasis-open.org/archives/virtio-comment/202306/msg00592.html