From: Paolo Bonzini <pbonzini@redhat.com>
To: "Jianyong Wu (Arm Technology China)" <Jianyong.Wu@arm.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"yangbo.lu@nxp.com" <yangbo.lu@nxp.com>,
"john.stultz@linaro.org" <john.stultz@linaro.org>,
"tglx@linutronix.de" <tglx@linutronix.de>,
"sean.j.christopherson@intel.com"
<sean.j.christopherson@intel.com>,
"maz@kernel.org" <maz@kernel.org>,
"richardcochran@gmail.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" <linux-kernel@vger.kernel.org>,
"kvm@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"
<linux-arm-kernel@lists.infradead.org>
Subject: Re: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm.
Date: Wed, 18 Sep 2019 12:23:47 +0200 [thread overview]
Message-ID: <629538ea-13fb-e666-8df6-8ad23f114755@redhat.com> (raw)
In-Reply-To: <HE1PR0801MB1676F57B317AE85E3B934B32F48E0@HE1PR0801MB1676.eurprd08.prod.outlook.com>
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.
next prev parent reply other threads:[~2019-09-18 10:23 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-18 8:07 [RFC PATCH v3 0/6] Enable ptp_kvm for arm64 Jianyong Wu
2019-09-18 8:07 ` [RFC PATCH v3 1/6] psci: Export psci_ops.conduit symbol as modules will use it Jianyong Wu
2019-09-18 8:07 ` [RFC PATCH v3 2/6] ptp: Reorganize ptp_kvm modules to make it arch-independent Jianyong Wu
2019-09-18 8:07 ` [RFC PATCH v3 3/6] timekeeping: Expose API allowing retrival of current clocksource and counter value Jianyong Wu
2019-09-18 8:29 ` Paolo Bonzini
2019-09-18 8:07 ` [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm Jianyong Wu
2019-09-18 8:25 ` Paolo Bonzini
2019-09-18 9:57 ` Jianyong Wu (Arm Technology China)
2019-09-18 10:23 ` Paolo Bonzini [this message]
2019-09-19 9:46 ` Jianyong Wu (Arm Technology China)
2019-09-19 11:07 ` Paolo Bonzini
2019-09-19 11:39 ` Marc Zyngier
2019-09-19 12:13 ` Paolo Bonzini
2019-09-23 3:19 ` Jianyong Wu (Arm Technology China)
2019-10-09 5:21 ` Jianyong Wu (Arm Technology China)
2019-10-09 6:36 ` Paolo Bonzini
2019-10-09 8:18 ` Jianyong Wu (Arm Technology China)
2019-10-09 9:13 ` Paolo Bonzini
2019-10-09 16:05 ` John Stultz
2019-10-09 20:56 ` Paolo Bonzini
2019-10-14 5:50 ` Jianyong Wu (Arm Technology China)
2019-10-14 6:58 ` Paolo Bonzini
2019-09-23 4:57 ` Jianyong Wu (Arm Technology China)
2019-09-24 14:20 ` Paolo Bonzini
2019-09-25 10:27 ` Jianyong Wu (Arm Technology China)
2019-09-18 8:07 ` [RFC PATCH v3 5/6] ptp: arm64: Enable ptp_kvm for arm64 Jianyong Wu
2019-09-18 8:07 ` [RFC PATCH v3 6/6] kvm: arm64: Add capability check extension for ptp_kvm Jianyong Wu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=629538ea-13fb-e666-8df6-8ad23f114755@redhat.com \
--to=pbonzini@redhat.com \
--cc=Jianyong.Wu@arm.com \
--cc=Justin.He@arm.com \
--cc=Kaly.Xin@arm.com \
--cc=Mark.Rutland@arm.com \
--cc=Steve.Capper@arm.com \
--cc=Suzuki.Poulose@arm.com \
--cc=Will.Deacon@arm.com \
--cc=john.stultz@linaro.org \
--cc=kvm@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maz@kernel.org \
--cc=nd@arm.com \
--cc=netdev@vger.kernel.org \
--cc=richardcochran@gmail.com \
--cc=sean.j.christopherson@intel.com \
--cc=tglx@linutronix.de \
--cc=yangbo.lu@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).