| Message ID | 20190918080716.64242-5-jianyong.wu@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable ptp_kvm for arm64 | expand |
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]. 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. > + 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. Thanks, Paolo
Hi Paolo, > -----Original Message----- > From: Paolo Bonzini <pbonzini@redhat.com> > Sent: Wednesday, September 18, 2019 4:26 PM > To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>; > netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org; > tglx@linutronix.de; sean.j.christopherson@intel.com; maz@kernel.org; > richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>; Will > Deacon <Will.Deacon@arm.com>; Suzuki Poulose > <Suzuki.Poulose@arm.com> > Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org; Steve Capper > <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China) > <Kaly.Xin@arm.com>; Justin He (Arm Technology China) > <Justin.He@arm.com>; nd <nd@arm.com>; linux-arm- > kernel@lists.infradead.org > Subject: Re: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm. > > 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? Also, split number will lead to shortage of return value. > 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. > > > + 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. 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. Thanks Jianyong Wu > Thanks, > > Paolo
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.
Hi Paolo, > -----Original Message----- > From: Paolo Bonzini <pbonzini@redhat.com> > Sent: Wednesday, September 18, 2019 6:24 PM > To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>; > netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org; > tglx@linutronix.de; sean.j.christopherson@intel.com; maz@kernel.org; > richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>; Will > Deacon <Will.Deacon@arm.com>; Suzuki Poulose > <Suzuki.Poulose@arm.com> > Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org; Steve Capper > <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China) > <Kaly.Xin@arm.com>; Justin He (Arm Technology China) > <Justin.He@arm.com>; nd <nd@arm.com>; linux-arm- > kernel@lists.infradead.org > 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.
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
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.
On 19/09/19 13:39, Marc Zyngier wrote: >> 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. Understood, in fact it's the same on x86. However, for PTP to work, the cycles value returned by the clocksource must match the one returned by the hypercall. So for ARM get_device_system_crosststamp must receive the arch timer clocksource, so that it will return -ENODEV if the active clocksource is anything else. Paolo > 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.
Hi Marc, Paolo, > -----Original Message----- > From: Paolo Bonzini <pbonzini@redhat.com> > Sent: Thursday, September 19, 2019 8:13 PM > To: Marc Zyngier <maz@kernel.org>; Jianyong Wu (Arm Technology China) > <Jianyong.Wu@arm.com>; netdev@vger.kernel.org; yangbo.lu@nxp.com; > john.stultz@linaro.org; tglx@linutronix.de; sean.j.christopherson@intel.com; > richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>; Will > Deacon <Will.Deacon@arm.com>; Suzuki Poulose > <Suzuki.Poulose@arm.com> > Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org; Steve Capper > <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China) > <Kaly.Xin@arm.com>; Justin He (Arm Technology China) > <Justin.He@arm.com>; nd <nd@arm.com>; linux-arm- > kernel@lists.infradead.org > Subject: Re: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm. > > On 19/09/19 13:39, Marc Zyngier wrote: > >> 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. > > Understood, in fact it's the same on x86. > > However, for PTP to work, the cycles value returned by the clocksource must > match the one returned by the hypercall. So for ARM > get_device_system_crosststamp must receive the arch timer clocksource, so > that it will return -ENODEV if the active clocksource is anything else. > After day's reflection on this, I'm a little clear about this issue, let me clarify it. I think there is three principles for this issue: (1): guest and host can use different clocksouces as their current clocksouce at the same time and we can't or it's not easy to probe that if they use the same one. (2): the cycle value and the clocksouce which passed to get_device_system_crosststamp must be match. (3): ptp_kvm target to increase the time sync precision so we should choose a high rate clocksource as ptp_kvm clocksource. Base on (1) and (2) we can deduce that the ptp_kvm clocksource should be better a fixed one. So we can test if the cycle and clocksource is match. Base on (3) the arch_sys_timer should be chosen, as it's the best clocksource by far for arm. Thanks Jianyong Wu > Paolo > > > 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.
Hi Paolo, Marc > -----Original Message----- > From: Paolo Bonzini <pbonzini@redhat.com> > Sent: Thursday, September 19, 2019 7:07 PM > To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>; > netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org; > tglx@linutronix.de; sean.j.christopherson@intel.com; maz@kernel.org; > richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>; Will > Deacon <Will.Deacon@arm.com>; Suzuki Poulose > <Suzuki.Poulose@arm.com> > Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org; Steve Capper > <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China) > <Kaly.Xin@arm.com>; Justin He (Arm Technology China) > <Justin.He@arm.com>; nd <nd@arm.com>; linux-arm- > kernel@lists.infradead.org > Subject: Re: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm. > > 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. > To compatible 32-bit, Integrates second value and nanosecond value as a nanosecond value then split it into val[0] and val[1] and split cycle value into val[2] and val[3], In this way, time will overflow at Y2262. WDYT? Thanks Jianyong Wu
On 23/09/19 06:57, Jianyong Wu (Arm Technology China) 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. >> > To compatible 32-bit, Integrates second value and nanosecond value as a nanosecond value then split it into val[0] and val[1] and split cycle value into val[2] and val[3], > In this way, time will overflow at Y2262. > WDYT? So if I understand correctly you'd multiply by 10^9 (or better shift by 30) the nanoseconds. That works, but why not provide 5 output registers? Alternatively, take an address as input and write there. Finally, on x86 we added an argument for the CLOCK_* that is being read (currently only CLOCK_REALTIME, but having room for extensibility in the API is always nice). Paolo
Hi Paolo, > -----Original Message----- > From: Paolo Bonzini <pbonzini@redhat.com> > Sent: Tuesday, September 24, 2019 10:20 PM > To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>; > netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org; > tglx@linutronix.de; sean.j.christopherson@intel.com; maz@kernel.org; > richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>; Will > Deacon <Will.Deacon@arm.com>; Suzuki Poulose > <Suzuki.Poulose@arm.com> > Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org; Steve Capper > <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China) > <Kaly.Xin@arm.com>; Justin He (Arm Technology China) > <Justin.He@arm.com>; nd <nd@arm.com>; linux-arm- > kernel@lists.infradead.org > Subject: Re: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm. > > On 23/09/19 06:57, Jianyong Wu (Arm Technology China) 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. > >> > > To compatible 32-bit, Integrates second value and nanosecond value as > > a nanosecond value then split it into val[0] and val[1] and split cycle value > into val[2] and val[3], In this way, time will overflow at Y2262. > > WDYT? > > So if I understand correctly you'd multiply by 10^9 (or better shift by > 30) the nanoseconds. > Yeah, > That works, but why not provide 5 output registers? Alternatively, take an > address as input and write there. It will be easy, if I could have expanded the store room. But these code is the infrastructure for hypercall, I can't change them at my will. I think only value but pointer can delivered by smccc_set_retval call. > > Finally, on x86 we added an argument for the CLOCK_* that is being read > (currently only CLOCK_REALTIME, but having room for extensibility in the API > is always nice). > IMO, I will be limited by the design of hypercall on arm64, I can only design my code under it. maybe it will be better sometime but for now I could just obey it. Thanks Jianyong Wu > Paolo
Hi Paolo, > -----Original Message----- > From: Paolo Bonzini <pbonzini@redhat.com> > Sent: Thursday, September 19, 2019 8:13 PM > To: Marc Zyngier <maz@kernel.org>; Jianyong Wu (Arm Technology China) > <Jianyong.Wu@arm.com>; netdev@vger.kernel.org; yangbo.lu@nxp.com; > john.stultz@linaro.org; tglx@linutronix.de; sean.j.christopherson@intel.com; > richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>; Will > Deacon <Will.Deacon@arm.com>; Suzuki Poulose > <Suzuki.Poulose@arm.com> > Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org; Steve Capper > <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China) > <Kaly.Xin@arm.com>; Justin He (Arm Technology China) > <Justin.He@arm.com>; nd <nd@arm.com>; linux-arm- > kernel@lists.infradead.org > Subject: Re: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm. > > On 19/09/19 13:39, Marc Zyngier wrote: > >> 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. > > Understood, in fact it's the same on x86. > > However, for PTP to work, the cycles value returned by the clocksource must > match the one returned by the hypercall. So for ARM > get_device_system_crosststamp must receive the arch timer clocksource, so > that it will return -ENODEV if the active clocksource is anything else. > As ptp_kvm clock has fixed to arm arch system counter in patch set v4, we need check if the current clocksource is system counter when return clock cycle in host, so a helper needed to return the current clocksource. Could I add this helper in next patch set? Thanks Jianyong wu > Paolo > > > 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.
On 09/10/19 07:21, Jianyong Wu (Arm Technology China) wrote: > As ptp_kvm clock has fixed to arm arch system counter in patch set > v4, we need check if the current clocksource is system counter when > return clock cycle in host, so a helper needed to return the current > clocksource. Could I add this helper in next patch set? You don't need a helper. You need to return the ARM arch counter clocksource in the struct system_counterval_t that you return. get_device_system_crosststamp will then check that the clocksource matches the active one. Paolo
Hi Paolo, > -----Original Message----- > From: Paolo Bonzini <pbonzini@redhat.com> > Sent: Wednesday, October 9, 2019 2:36 PM > To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>; Marc > Zyngier <maz@kernel.org>; netdev@vger.kernel.org; yangbo.lu@nxp.com; > john.stultz@linaro.org; tglx@linutronix.de; sean.j.christopherson@intel.com; > richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>; Will > Deacon <Will.Deacon@arm.com>; Suzuki Poulose > <Suzuki.Poulose@arm.com> > Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org; Steve Capper > <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China) > <Kaly.Xin@arm.com>; Justin He (Arm Technology China) > <Justin.He@arm.com>; nd <nd@arm.com>; linux-arm- > kernel@lists.infradead.org > Subject: Re: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm. > > On 09/10/19 07:21, Jianyong Wu (Arm Technology China) wrote: > > As ptp_kvm clock has fixed to arm arch system counter in patch set v4, > > we need check if the current clocksource is system counter when return > > clock cycle in host, so a helper needed to return the current > > clocksource. Could I add this helper in next patch set? > > You don't need a helper. You need to return the ARM arch counter > clocksource in the struct system_counterval_t that you return. > get_device_system_crosststamp will then check that the clocksource > matches the active one. > We must ensure both of the host and guest using the same clocksource. get_device_system_crosststamp will check the clocksource of guest and we also need check the clocksource in host, and struct type can't be transferred from host to guest using arm hypercall. now we lack of a mechanism to check the current clocksource. I think this will be useful if we add one. Thanks Jianyong Wu > Paolo
On 09/10/19 10:18, Jianyong Wu (Arm Technology China) wrote: > Hi Paolo, > >> -----Original Message----- >> From: Paolo Bonzini <pbonzini@redhat.com> >> Sent: Wednesday, October 9, 2019 2:36 PM >> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>; Marc >> Zyngier <maz@kernel.org>; netdev@vger.kernel.org; yangbo.lu@nxp.com; >> john.stultz@linaro.org; tglx@linutronix.de; sean.j.christopherson@intel.com; >> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>; Will >> Deacon <Will.Deacon@arm.com>; Suzuki Poulose >> <Suzuki.Poulose@arm.com> >> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org; Steve Capper >> <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China) >> <Kaly.Xin@arm.com>; Justin He (Arm Technology China) >> <Justin.He@arm.com>; nd <nd@arm.com>; linux-arm- >> kernel@lists.infradead.org >> Subject: Re: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm. >> >> On 09/10/19 07:21, Jianyong Wu (Arm Technology China) wrote: >>> As ptp_kvm clock has fixed to arm arch system counter in patch set v4, >>> we need check if the current clocksource is system counter when return >>> clock cycle in host, so a helper needed to return the current >>> clocksource. Could I add this helper in next patch set? >> >> You don't need a helper. You need to return the ARM arch counter >> clocksource in the struct system_counterval_t that you return. >> get_device_system_crosststamp will then check that the clocksource >> matches the active one. > > We must ensure both of the host and guest using the same clocksource. > get_device_system_crosststamp will check the clocksource of guest and we also need check > the clocksource in host, and struct type can't be transferred from host to guest using arm hypercall. > now we lack of a mechanism to check the current clocksource. I think this will be useful if we add one. Got it---yes, I think adding a struct clocksource to struct system_time_snapshot would make sense. Then the hypercall can just use ktime_get_snapshot and fail if the clocksource is not the ARM arch counter. John (Stultz), does that sound good to you? The context is that Jianyong would like to add a hypercall that returns a (cycles, nanoseconds) pair to the guest. On x86 we're relying on the vclock_mode field that is already there for the vDSO, but being able to just use ktime_get_snapshot would be much nicer. Paolo
On Wed, Oct 9, 2019 at 2:13 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > On 09/10/19 10:18, Jianyong Wu (Arm Technology China) wrote: > > > > We must ensure both of the host and guest using the same clocksource. > > get_device_system_crosststamp will check the clocksource of guest and we also need check > > the clocksource in host, and struct type can't be transferred from host to guest using arm hypercall. > > now we lack of a mechanism to check the current clocksource. I think this will be useful if we add one. > > Got it---yes, I think adding a struct clocksource to struct > system_time_snapshot would make sense. Then the hypercall can just use > ktime_get_snapshot and fail if the clocksource is not the ARM arch counter. > > John (Stultz), does that sound good to you? The context is that > Jianyong would like to add a hypercall that returns a (cycles, > nanoseconds) pair to the guest. On x86 we're relying on the vclock_mode > field that is already there for the vDSO, but being able to just use > ktime_get_snapshot would be much nicer. I've not really looked at the code closely in awhile, so I'm not sure my suggestions will be too useful. My only instinct is maybe to not include the clocksource pointer in the system_time_snapshot, as I worry that structure will then be abused by the interface users. If you're just wanting to make sure the clocksource is what you're expecting, would instead putting only the clocksource name in the structure suffice? thanks -john
On 09/10/19 18:05, John Stultz wrote: > On Wed, Oct 9, 2019 at 2:13 AM Paolo Bonzini <pbonzini@redhat.com> wrote: >> John (Stultz), does that sound good to you? The context is that >> Jianyong would like to add a hypercall that returns a (cycles, >> nanoseconds) pair to the guest. On x86 we're relying on the vclock_mode >> field that is already there for the vDSO, but being able to just use >> ktime_get_snapshot would be much nicer. > > I've not really looked at the code closely in awhile, so I'm not sure > my suggestions will be too useful. > > My only instinct is maybe to not include the clocksource pointer in > the system_time_snapshot, as I worry that structure will then be > abused by the interface users. If you're just wanting to make sure > the clocksource is what you're expecting, would instead putting only > the clocksource name in the structure suffice? Well, it would suffice but it would be quite ugly to do a string comparison later. What kind of abuse are you thinking of? We already have struct system_counterval_t for a clocksource+cycles tuple, so it seemed obvious to me to make system_time_snapshot a superset of it... In fact, system_time_snapshot's cycles member is even unused currently, so it could even be easily replaced by a struct system_counterval_t, instead of adding an extra field. Paolo
Hi Paolo, > -----Original Message----- > From: Paolo Bonzini <pbonzini@redhat.com> > Sent: Wednesday, October 9, 2019 5:13 PM > To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>; Marc > Zyngier <maz@kernel.org>; netdev@vger.kernel.org; yangbo.lu@nxp.com; > john.stultz@linaro.org; tglx@linutronix.de; sean.j.christopherson@intel.com; > richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>; Will > Deacon <Will.Deacon@arm.com>; Suzuki Poulose > <Suzuki.Poulose@arm.com> > Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org; Steve Capper > <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China) > <Kaly.Xin@arm.com>; Justin He (Arm Technology China) > <Justin.He@arm.com>; nd <nd@arm.com>; linux-arm- > kernel@lists.infradead.org > Subject: Re: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm. > > On 09/10/19 10:18, Jianyong Wu (Arm Technology China) wrote: > > Hi Paolo, > > > >> -----Original Message----- > >> From: Paolo Bonzini <pbonzini@redhat.com> > >> Sent: Wednesday, October 9, 2019 2:36 PM > >> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>; Marc > >> Zyngier <maz@kernel.org>; netdev@vger.kernel.org; > yangbo.lu@nxp.com; > >> john.stultz@linaro.org; tglx@linutronix.de; > >> sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark > >> Rutland <Mark.Rutland@arm.com>; Will Deacon > <Will.Deacon@arm.com>; > >> Suzuki Poulose <Suzuki.Poulose@arm.com> > >> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org; Steve Capper > >> <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China) > >> <Kaly.Xin@arm.com>; Justin He (Arm Technology China) > >> <Justin.He@arm.com>; nd <nd@arm.com>; linux-arm- > >> kernel@lists.infradead.org > >> Subject: Re: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm. > >> > >> On 09/10/19 07:21, Jianyong Wu (Arm Technology China) wrote: > >>> As ptp_kvm clock has fixed to arm arch system counter in patch set > >>> v4, we need check if the current clocksource is system counter when > >>> return clock cycle in host, so a helper needed to return the current > >>> clocksource. Could I add this helper in next patch set? > >> > >> You don't need a helper. You need to return the ARM arch counter > >> clocksource in the struct system_counterval_t that you return. > >> get_device_system_crosststamp will then check that the clocksource > >> matches the active one. > > > > We must ensure both of the host and guest using the same clocksource. > > get_device_system_crosststamp will check the clocksource of guest and > > we also need check the clocksource in host, and struct type can't be > transferred from host to guest using arm hypercall. > > now we lack of a mechanism to check the current clocksource. I think this > will be useful if we add one. > > Got it---yes, I think adding a struct clocksource to struct > system_time_snapshot would make sense. Then the hypercall can just use > ktime_get_snapshot and fail if the clocksource is not the ARM arch counter. > > John (Stultz), does that sound good to you? The context is that Jianyong > would like to add a hypercall that returns a (cycles, > nanoseconds) pair to the guest. On x86 we're relying on the vclock_mode > field that is already there for the vDSO, but being able to just use > ktime_get_snapshot would be much nicer. > Could I add struct clocksource to system_time_snapshot struct in next version of my patch set? Jianyong Wu Thanks > Paolo
On 14/10/19 07:50, Jianyong Wu (Arm Technology China) wrote: >> >> John (Stultz), does that sound good to you? The context is that Jianyong >> would like to add a hypercall that returns a (cycles, >> nanoseconds) pair to the guest. On x86 we're relying on the vclock_mode >> field that is already there for the vDSO, but being able to just use >> ktime_get_snapshot would be much nicer. >> > Could I add struct clocksource to system_time_snapshot struct in next version of my patch set? Yes, I say go ahead. At least it will get closer to a final design. Paolo
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h index a6e4d3e3d10a..bc0cdad10f35 100644 --- a/include/linux/arm-smccc.h +++ b/include/linux/arm-smccc.h @@ -94,6 +94,7 @@ /* KVM "vendor specific" services */ #define ARM_SMCCC_KVM_FUNC_FEATURES 0 +#define ARM_SMCCC_KVM_PTP 1 #define ARM_SMCCC_KVM_FUNC_FEATURES_2 127 #define ARM_SMCCC_KVM_NUM_FUNCS 128 @@ -103,6 +104,17 @@ ARM_SMCCC_OWNER_VENDOR_HYP, \ ARM_SMCCC_KVM_FUNC_FEATURES) +/* + * This ID used for virtual ptp kvm clock and it will pass second value + * and nanosecond value of host real time and system counter by vcpu + * register to guest. + */ +#define ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID \ + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ + ARM_SMCCC_SMC_32, \ + ARM_SMCCC_OWNER_VENDOR_HYP, \ + ARM_SMCCC_KVM_PTP) + #ifndef __ASSEMBLY__ #include <linux/linkage.h> diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c index 0debf49bf259..2c5d53817a28 100644 --- a/virt/kvm/arm/psci.c +++ b/virt/kvm/arm/psci.c @@ -392,6 +392,8 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) u32 func_id = smccc_get_function(vcpu); u32 val[4] = {}; u32 option; + struct timespec *ts; + struct system_counterval_t sc; val[0] = SMCCC_RET_NOT_SUPPORTED; @@ -431,6 +433,21 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID: val[0] = BIT(ARM_SMCCC_KVM_FUNC_FEATURES); break; + /* + * This will used for virtual ptp kvm clock. three + * values will be passed back. + * reg0 stores seconds of host real time; + * reg1 stores nanoseconds of host real time; + * reg2 stores system counter cycle value. + */ + case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID: + getnstimeofday(ts); + get_current_counterval(&sc); + val[0] = ts->tv_sec; + val[1] = ts->tv_nsec; + val[2] = sc.cycles; + val[3] = 0; + break; default: return kvm_psci_call(vcpu); }
This patch is the base of ptp_kvm for arm64. ptp_kvm modules will call hvc to get this service. The service offers real time and counter cycle of host for guest. Signed-off-by: Jianyong Wu <jianyong.wu@arm.com> --- include/linux/arm-smccc.h | 12 ++++++++++++ virt/kvm/arm/psci.c | 17 +++++++++++++++++ 2 files changed, 29 insertions(+)