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

[Jianyong Wu (Arm Technology China)]

Hi Paolo,

> -----Original Message-----
> From: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Sent: Wednesday, September 18, 2019 6:24 PM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@xxxxxxx>;
> netdev@xxxxxxxxxxxxxxx; yangbo.lu@xxxxxxx; john.stultz@xxxxxxxxxx;
> tglx@xxxxxxxxxxxxx; sean.j.christopherson@xxxxxxxxx; maz@xxxxxxxxxx;
> richardcochran@xxxxxxxxx; Mark Rutland <Mark.Rutland@xxxxxxx>; Will
> Deacon <Will.Deacon@xxxxxxx>; Suzuki Poulose
> <Suzuki.Poulose@xxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; Steve Capper
> <Steve.Capper@xxxxxxx>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@xxxxxxx>; Justin He (Arm Technology China)
> <Justin.He@xxxxxxx>; nd <nd@xxxxxxx>; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm.
> 
> 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).
> 
Yeah, need consider twice.
Val[] should be long not u32 I think, so in arm64 I can avoid that Y2038_safe, but
also need rewrite for arm32.

> >> 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?

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


> 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.
 
> >>> +		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.
> 
Sure, counter offset should be considered.

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

Thanks 
Jianyong Wu

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