On 18/09/19 11:57, Jianyong Wu (Arm Technology China) wrote: > Hi Paolo, > >> On 18/09/19 10:07, Jianyong Wu wrote: >>> + case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID: >>> + getnstimeofday(ts); >> >> 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]. >> > As far as I know, y2038-safe will only affect signed 32-bit integer, > how does it affect 64-bit integer? > And why split 64-bit number into two blocks is necessary? val is an u32, not an u64. (And val[0], where you store the seconds, is best treated as signed, since val[0] == -1 is returned for SMCCC_RET_NOT_SUPPORTED). >> 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). 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. 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. 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? >>> + get_current_counterval(&sc); >>> + val[0] = ts->tv_sec; >>> + val[1] = ts->tv_nsec; >>> + val[2] = sc.cycles; >>> + val[3] = 0; >>> + break; >> >> This should return a guest-cycles value. If the cycles values always the same >> between the host and the guest on ARM, then okay. If not, you have to >> apply whatever offset exists. >> > In my opinion, when use ptp_kvm as clock sources to sync time > between host and guest, user should promise the guest and host has no > clock offset. What would be the adverse effect of having a fixed offset between guest and host? If there were one, you'd have to check that and fail the hypercall if there is an offset. But again, I think it's enough to subtract vcpu_vtimer(vcpu)->cntvoff or something like that. 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. Thanks, Paolo > So we can be sure that the cycle between guest and > host should be keep consistent. But I need check it. > I think host cycle should be returned to guest as we should promise > we get clock and counter in the same time.
