linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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: Thu, 19 Sep 2019 13:07:24 +0200	[thread overview]
Message-ID: <ef6ab8bd-41ad-88f8-9cfd-dc749ca65310@redhat.com> (raw)
In-Reply-To: <HE1PR0801MB167639E2F025998058A77F86F4890@HE1PR0801MB1676.eurprd08.prod.outlook.com>

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


  reply	other threads:[~2019-09-19 11:07 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
2019-09-19  9:46         ` Jianyong Wu (Arm Technology China)
2019-09-19 11:07           ` Paolo Bonzini [this message]
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=ef6ab8bd-41ad-88f8-9cfd-dc749ca65310@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).