Re: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm.

[Paolo Bonzini]

On 19/09/19 11:46, Jianyong Wu (Arm Technology China) wrote:
>> On 18/09/19 11:57, Jianyong Wu (Arm Technology China) wrote:
>>> Paolo Bonzini wrote:
>>>> This is not Y2038-safe.  Please use ktime_get_real_ts64 instead, and
>>>> split the 64-bit seconds value between val[0] and val[1].
>
> Val[] should be long not u32 I think, so in arm64 I can avoid that Y2038_safe, but
> also need rewrite for arm32.

I don't think there's anything inherently wrong with u32 val[], and as
you notice it lets you reuse code between arm and arm64.  It's up to you
and Marc to decide.

>>>> However, it seems to me that the new function is not needed and you
>>>> can just use ktime_get_snapshot.  You'll get the time in
>>>> systime_snapshot->real and the cycles value in systime_snapshot->cycles.
>>>
>>> See patch 5/6, I need both counter cycle and clocksource,
>> ktime_get_snapshot seems only offer cycles.
>>
>> No, patch 5/6 only needs the current clock (ptp_sc.cycles is never accessed).
>> So you could just use READ_ONCE(tk->tkr_mono.clock).
>>
> Yeah, patch 5/6 just need clocksource, but I think tk->tkr_mono.clock can't read in external like module,
> So I need an API to expose clocksource.
>  
>> However, even then I don't think it is correct to use ptp_sc.cs blindly in patch
>> 5.  I think there is a misunderstanding on the meaning of
>> system_counterval.cs as passed to get_device_system_crosststamp.
>> system_counterval.cs is not the active clocksource; it's the clocksource on
>> which system_counterval.cycles is based.
>>
> 
> I think we can use system_counterval_t as pass current clocksource to system_counterval_t.cs and its
> corresponding cycles to system_counterval_t.cycles. is it a big problem?

Yes, it is.  Because...

>> Hypothetically, the clocksource could be one for which ptp_sc.cycles is _not_
>> a cycle value.  If you set system_counterval.cs to the system clocksource,
>> get_device_system_crosststamp will return a bogus value.
> 
> Yeah, but in patch 3/6, we have a corresponding pair of clock source and cycle value. So I think there will be no
> that problem in this patch set.
> In the implementation of get_device_system_crosststamp:
> "
> ...
> if (tk->tkr_mono.clock != system_counterval.cs)
>                         return -ENODEV;
> ...
> "
> We need tk->tkr_mono.clock passed to get_device_system_crosststamp, just like patch 3/6 do, otherwise will return error.

... if the hypercall returns an architectural timer value, you must not
pass tk->tkr.mono.clock to get_device_system_crosststamp: you must pass
&clocksource_counter.  This way, PTP is disabled when using any other
clocksource.

>> So system_counterval.cs should be set to something like
>> &clocksource_counter (from drivers/clocksource/arm_arch_timer.c).
>> Perhaps the right place to define kvm_arch_ptp_get_clock_fn is in that file?
>>
> I have checked that ptp_sc.cs is arch_sys_counter.
> Also move the module API to arm_arch_timer.c will looks a little
> ugly and it's not easy to be accept by arm side I think.

I don't think it's ugly but more important, using tk->tkr_mono.clock is
incorrect.  See how the x86 code hardcodes &kvm_clock, it's the same for
ARM.

>> You also have to check here that the clocksource is based on the ARM
>> architectural timer.  Again, maybe you could place the implementation in
>> drivers/clocksource/arm_arch_timer.c, and make it return -ENODEV if the
>> active clocksource is not clocksource_counter.  Then KVM can look for errors
>> and return SMCCC_RET_NOT_SUPPORTED in that case.
> 
> I have checked it. The clock source is arch_sys_counter which is arm arch timer.
> I can try to do that but I'm not sure arm side will be happy for that change.

Just try.  For my taste, it's nice to include both sides of the
hypercall in drivers/clocksource/arm_arch_timer.c, possibly
conditionalizing them on #ifdef CONFIG_KVM and #ifdef
CONFIG_PTP_1588_CLOCK_KVM.  But there is an alternative which is simply
to export the clocksource struct.  Both choices are easy to implement so
you can just ask the ARM people what they prefer and they can judge from
the code.

Paolo