On 19/09/2019 12:07, Paolo Bonzini wrote: > 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. Not really. The guest kernel is free to use any clocksource it wishes. In some cases, it is actually desirable (like these broken systems that cannot use an in-kernel irqchip...). Maybe it is that on x86 the guest only uses the kvm_clock, but that's a much harder sell on ARM. The fact that ptp_kvm assumes that the clocksource is fixed doesn't seem correct in that case. M. -- Jazz is not dead, it just smells funny...
