| Message ID | 20200522083724.38182-11-jianyong.wu@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable ptp_kvm for arm64 | expand |
On Fri, May 22, 2020 at 04:37:23PM +0800, Jianyong Wu wrote: > To use this feature, you should call PTP_EXTTS_REQUEST(2) ioctl with flag > set bit PTP_KVM_ARM_PHY_COUNTER in its argument then call > PTP_SYS_OFFSET_PRECISE(2) ioctl to get the cross timestamp and phyical > counter will return. If the bit not set or no call for PTP_EXTTS_REQUEST2, > virtual counter will return by default. I'm sorry, but NAK on this completely bizarre twisting of the user space API. > diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c > index fef72f29f3c8..8b0a7b328bcd 100644 > --- a/drivers/ptp/ptp_chardev.c > +++ b/drivers/ptp/ptp_chardev.c > @@ -123,6 +123,9 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg) > struct timespec64 ts; > int enable, err = 0; > > +#ifdef CONFIG_ARM64 > + static long flag; > +#endif > switch (cmd) { > > case PTP_CLOCK_GETCAPS: > @@ -149,6 +152,24 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg) > err = -EFAULT; > break; > } > + > +#ifdef CONFIG_ARM64 > + /* > + * Just using this ioctl to tell kvm ptp driver to get PHC > + * with physical counter, so if bit PTP_KVM_ARM_PHY_COUNTER > + * is set then just exit directly. > + * In most cases, we just need virtual counter from host and > + * there is limited scenario using this to get physical counter > + * in guest. > + * Be careful to use this as there is no way to set it back > + * unless you reinstall the module. > + * This is only for arm64. > + */ > + if (req.extts.flags & PTP_KVM_ARM_PHY_COUNTER) { > + flag = 1; > + break; > + } > +#endif This file contains the generic PTP Hardware Clock character device implementation. It is no place for platform specific hacks. Sorry, Richard
On Fri, May 22, 2020 at 04:37:23PM +0800, Jianyong Wu wrote: > In general, vm inside will use virtual counter compered with host use > phyical counter. But in some special scenarios, like nested > virtualization, phyical counter maybe used by vm. A interface added in > ptp_kvm driver to offer a mechanism to let user choose which counter > should be return from host. Sounds like you have two time sources, one for normal guest, and one for nested. Why not simply offer the correct one to user space automatically? If that cannot be done, then just offer two PHC devices with descriptive names. > diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c > index fef72f29f3c8..8b0a7b328bcd 100644 > --- a/drivers/ptp/ptp_chardev.c > +++ b/drivers/ptp/ptp_chardev.c > @@ -123,6 +123,9 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg) > struct timespec64 ts; > int enable, err = 0; > > +#ifdef CONFIG_ARM64 > + static long flag; static? This is not going to fly. > + * In most cases, we just need virtual counter from host and > + * there is limited scenario using this to get physical counter > + * in guest. > + * Be careful to use this as there is no way to set it back > + * unless you reinstall the module. How on earth is the user supposed to know this? From your description, this "flag" really should be a module parameter. Thanks, Richard
Hi Richard, > -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: Sunday, May 24, 2020 10:11 AM > To: Jianyong Wu <Jianyong.Wu@arm.com> > Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org; > tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com; > maz@kernel.org; Mark Rutland <Mark.Rutland@arm.com>; will@kernel.org; > Suzuki Poulose <Suzuki.Poulose@arm.com>; Steven Price > <Steven.Price@arm.com>; linux-kernel@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu; > kvm@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin > <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; Wei Chen > <Wei.Chen@arm.com>; nd <nd@arm.com> > Subject: Re: [RFC PATCH v12 10/11] arm64: add mechanism to let user choose > which counter to return > > On Fri, May 22, 2020 at 04:37:23PM +0800, Jianyong Wu wrote: > > In general, vm inside will use virtual counter compered with host use > > phyical counter. But in some special scenarios, like nested > > virtualization, phyical counter maybe used by vm. A interface added in > > ptp_kvm driver to offer a mechanism to let user choose which counter > > should be return from host. > > Sounds like you have two time sources, one for normal guest, and one for > nested. Why not simply offer the correct one to user space automatically? If > that cannot be done, then just offer two PHC devices with descriptive names. > It's a good idea, but in most case physical counter will not be used, so it's better not keep 2 ptp devices all the time. How about adding an extra argument in struct ptp_clock_info to serve as a flag, then we can control this flag using IOCTL to determine the counter type. In this way, no extra arguments needed in .getcrosststamp. But we also need specific code in ptp_ioctl to implement it like in this patch. The second way, maybe we can use the flag as a module parameter, this is easier to implement. @maz@kernel.org WDYT? > > diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c > > index fef72f29f3c8..8b0a7b328bcd 100644 > > --- a/drivers/ptp/ptp_chardev.c > > +++ b/drivers/ptp/ptp_chardev.c > > @@ -123,6 +123,9 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int > cmd, unsigned long arg) > > struct timespec64 ts; > > int enable, err = 0; > > > > +#ifdef CONFIG_ARM64 > > + static long flag; > > static? This is not going to fly. Need remove here. > > > + * In most cases, we just need virtual counter from host and > > + * there is limited scenario using this to get physical counter > > + * in guest. > > + * Be careful to use this as there is no way to set it back > > + * unless you reinstall the module. > > How on earth is the user supposed to know this? > Yeah, It's odd , should be removed. > From your description, this "flag" really should be a module parameter. Maybe use flag as a module parameter is a better way. Thanks Jianyong > > Thanks, > Richard
On Mon, May 25, 2020 at 04:50:28AM +0000, Jianyong Wu wrote: > How about adding an extra argument in struct ptp_clock_info to serve as a flag, then we can control this flag using IOCTL to determine the counter type. no, No, NO! > > From your description, this "flag" really should be a module parameter. > Maybe use flag as a module parameter is a better way. Yes. Thanks, Richard
Hi Richard, > -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: Monday, May 25, 2020 2:16 PM > To: Jianyong Wu <Jianyong.Wu@arm.com> > Cc: maz@kernel.org; netdev@vger.kernel.org; yangbo.lu@nxp.com; > john.stultz@linaro.org; tglx@linutronix.de; pbonzini@redhat.com; > sean.j.christopherson@intel.com; Mark Rutland <Mark.Rutland@arm.com>; > will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Steven Price > <Steven.Price@arm.com>; linux-kernel@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu; > kvm@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin > <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; Wei Chen > <Wei.Chen@arm.com>; nd <nd@arm.com> > Subject: Re: [RFC PATCH v12 10/11] arm64: add mechanism to let user choose > which counter to return > > On Mon, May 25, 2020 at 04:50:28AM +0000, Jianyong Wu wrote: > > How about adding an extra argument in struct ptp_clock_info to serve as a > flag, then we can control this flag using IOCTL to determine the counter type. > > no, No, NO! > Ok, > > > From your description, this "flag" really should be a module parameter. > > Maybe use flag as a module parameter is a better way. > > Yes. > It's fine for me, if @maz@kernel.org is not against with it. Thanks Jianyong > Thanks, > Richard
On 2020-05-24 03:11, Richard Cochran wrote: > On Fri, May 22, 2020 at 04:37:23PM +0800, Jianyong Wu wrote: >> In general, vm inside will use virtual counter compered with host use >> phyical counter. But in some special scenarios, like nested >> virtualization, phyical counter maybe used by vm. A interface added in >> ptp_kvm driver to offer a mechanism to let user choose which counter >> should be return from host. > > Sounds like you have two time sources, one for normal guest, and one > for nested. Why not simply offer the correct one to user space > automatically? If that cannot be done, then just offer two PHC > devices with descriptive names. There is no such thing as a distinction between nested or non-nested. Both counters are available to the guest at all times, and said guest can choose whichever it wants to use. So the hypervisor (KVM) has to support both counters as a reference. For a Linux guest, we always know which reference we're using (the virtual counter). So it is pointless to expose the choice to userspace at all. > >> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c >> index fef72f29f3c8..8b0a7b328bcd 100644 >> --- a/drivers/ptp/ptp_chardev.c >> +++ b/drivers/ptp/ptp_chardev.c >> @@ -123,6 +123,9 @@ long ptp_ioctl(struct posix_clock *pc, unsigned >> int cmd, unsigned long arg) >> struct timespec64 ts; >> int enable, err = 0; >> >> +#ifdef CONFIG_ARM64 >> + static long flag; > > static? This is not going to fly. > >> + * In most cases, we just need virtual counter from host and >> + * there is limited scenario using this to get physical counter >> + * in guest. >> + * Be careful to use this as there is no way to set it back >> + * unless you reinstall the module. > > How on earth is the user supposed to know this? > > From your description, this "flag" really should be a module > parameter. Not even that. If anything, the driver can obtain full knowledge of which counter is in use without any help. And the hard truth is that it is *always* the virtual counter as far as Linux is concerned. M.
Hi Marc, > -----Original Message----- > From: Marc Zyngier <maz@kernel.org> > Sent: Monday, May 25, 2020 5:17 PM > To: Richard Cochran <richardcochran@gmail.com>; Jianyong Wu > <Jianyong.Wu@arm.com> > Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org; > tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com; > Mark Rutland <Mark.Rutland@arm.com>; will@kernel.org; Suzuki Poulose > <Suzuki.Poulose@arm.com>; Steven Price <Steven.Price@arm.com>; linux- > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; Steve Capper > <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Justin He > <Justin.He@arm.com>; Wei Chen <Wei.Chen@arm.com>; nd <nd@arm.com> > Subject: Re: [RFC PATCH v12 10/11] arm64: add mechanism to let user > choose which counter to return > > On 2020-05-24 03:11, Richard Cochran wrote: > > On Fri, May 22, 2020 at 04:37:23PM +0800, Jianyong Wu wrote: > >> In general, vm inside will use virtual counter compered with host use > >> phyical counter. But in some special scenarios, like nested > >> virtualization, phyical counter maybe used by vm. A interface added > >> in ptp_kvm driver to offer a mechanism to let user choose which > >> counter should be return from host. > > > > Sounds like you have two time sources, one for normal guest, and one > > for nested. Why not simply offer the correct one to user space > > automatically? If that cannot be done, then just offer two PHC > > devices with descriptive names. > > There is no such thing as a distinction between nested or non-nested. > Both counters are available to the guest at all times, and said guest can > choose whichever it wants to use. So the hypervisor (KVM) has to support > both counters as a reference. > It's great that we can decide which counter to return in guest kernel. So we can abandon these code, including patch 9/11 and 10/11, that expose the interface to userspace to do the decision. > For a Linux guest, we always know which reference we're using (the virtual > counter). So it is pointless to expose the choice to userspace at all. > So, we should throw these code of deciding counter type in linux driver away and just keep the hypercall service of providing both virtual counter and physical counter in linux to server non-linux guest. Am I right? > > > >> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c > >> index fef72f29f3c8..8b0a7b328bcd 100644 > >> --- a/drivers/ptp/ptp_chardev.c > >> +++ b/drivers/ptp/ptp_chardev.c > >> @@ -123,6 +123,9 @@ long ptp_ioctl(struct posix_clock *pc, unsigned > >> int cmd, unsigned long arg) > >> struct timespec64 ts; > >> int enable, err = 0; > >> > >> +#ifdef CONFIG_ARM64 > >> + static long flag; > > > > static? This is not going to fly. > > > >> + * In most cases, we just need virtual counter from host and > >> + * there is limited scenario using this to get physical counter > >> + * in guest. > >> + * Be careful to use this as there is no way to set it back > >> + * unless you reinstall the module. > > > > How on earth is the user supposed to know this? > > > > From your description, this "flag" really should be a module > > parameter. > > Not even that. If anything, the driver can obtain full knowledge of which > counter is in use without any help. And the hard truth is that it is > *always* the virtual counter as far as Linux is concerned. Good! Thanks Jianyong > > M. > -- > Jazz is not dead. It just smells funny...
On 2020-05-25 15:18, Jianyong Wu wrote: > Hi Marc, > >> -----Original Message----- >> From: Marc Zyngier <maz@kernel.org> >> Sent: Monday, May 25, 2020 5:17 PM >> To: Richard Cochran <richardcochran@gmail.com>; Jianyong Wu >> <Jianyong.Wu@arm.com> >> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org; >> tglx@linutronix.de; pbonzini@redhat.com; >> sean.j.christopherson@intel.com; >> Mark Rutland <Mark.Rutland@arm.com>; will@kernel.org; Suzuki Poulose >> <Suzuki.Poulose@arm.com>; Steven Price <Steven.Price@arm.com>; linux- >> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; Steve Capper >> <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Justin He >> <Justin.He@arm.com>; Wei Chen <Wei.Chen@arm.com>; nd <nd@arm.com> >> Subject: Re: [RFC PATCH v12 10/11] arm64: add mechanism to let user >> choose which counter to return >> >> On 2020-05-24 03:11, Richard Cochran wrote: >> > On Fri, May 22, 2020 at 04:37:23PM +0800, Jianyong Wu wrote: >> >> In general, vm inside will use virtual counter compered with host use >> >> phyical counter. But in some special scenarios, like nested >> >> virtualization, phyical counter maybe used by vm. A interface added >> >> in ptp_kvm driver to offer a mechanism to let user choose which >> >> counter should be return from host. >> > >> > Sounds like you have two time sources, one for normal guest, and one >> > for nested. Why not simply offer the correct one to user space >> > automatically? If that cannot be done, then just offer two PHC >> > devices with descriptive names. >> >> There is no such thing as a distinction between nested or non-nested. >> Both counters are available to the guest at all times, and said guest >> can >> choose whichever it wants to use. So the hypervisor (KVM) has to >> support >> both counters as a reference. >> > It's great that we can decide which counter to return in guest kernel. > So we can abandon these code, including patch 9/11 and 10/11, that > expose the interface to userspace to do the decision. > >> For a Linux guest, we always know which reference we're using (the >> virtual >> counter). So it is pointless to expose the choice to userspace at all. >> > So, we should throw these code of deciding counter type in linux > driver away and just keep the hypercall service of providing both > virtual counter and physical counter in linux to server non-linux > guest. > Am I right? Exactly. We control Linux, and so far nothing is using the physical counter directly. It is only using the virtual counter. On the other side, this is *only* Linux. Other operating systems will need to pick the reference clock that matches their own. If one day we change Linux to use the physical counter, we'll have to do the same thing. M.
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 06959b901b0d..75a3bb118201 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -1650,7 +1650,18 @@ int kvm_arch_ptp_get_crosststamp(unsigned long *cycle, struct timespec64 *ts, struct arm_smccc_res hvc_res; ktime_t ktime_overall; - arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID, &hvc_res); + /* + * an argument will be passed by a0 to determine weather virtual + * counter or phyical counter should be passed back. + */ + if (ctx && *ctx) + arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID, + ARM_SMCCC_VENDOR_HYP_KVM_PTP_PHY_FUNC_ID, + &hvc_res); + else + arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID, + &hvc_res); + if ((int)(hvc_res.a0) < 0) return -EOPNOTSUPP; diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c index fef72f29f3c8..8b0a7b328bcd 100644 --- a/drivers/ptp/ptp_chardev.c +++ b/drivers/ptp/ptp_chardev.c @@ -123,6 +123,9 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg) struct timespec64 ts; int enable, err = 0; +#ifdef CONFIG_ARM64 + static long flag; +#endif switch (cmd) { case PTP_CLOCK_GETCAPS: @@ -149,6 +152,24 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg) err = -EFAULT; break; } + +#ifdef CONFIG_ARM64 + /* + * Just using this ioctl to tell kvm ptp driver to get PHC + * with physical counter, so if bit PTP_KVM_ARM_PHY_COUNTER + * is set then just exit directly. + * In most cases, we just need virtual counter from host and + * there is limited scenario using this to get physical counter + * in guest. + * Be careful to use this as there is no way to set it back + * unless you reinstall the module. + * This is only for arm64. + */ + if (req.extts.flags & PTP_KVM_ARM_PHY_COUNTER) { + flag = 1; + break; + } +#endif if (cmd == PTP_EXTTS_REQUEST2) { /* Tell the drivers to check the flags carefully. */ req.extts.flags |= PTP_STRICT_FLAGS; @@ -235,7 +256,11 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg) err = -EOPNOTSUPP; break; } +#ifdef CONFIG_ARM64 + err = ptp->info->getcrosststamp(ptp->info, &xtstamp, &flag); +#else err = ptp->info->getcrosststamp(ptp->info, &xtstamp, NULL); +#endif if (err) break; diff --git a/drivers/ptp/ptp_kvm_common.c b/drivers/ptp/ptp_kvm_common.c index 4fdd8ab11a28..39367e230176 100644 --- a/drivers/ptp/ptp_kvm_common.c +++ b/drivers/ptp/ptp_kvm_common.c @@ -36,7 +36,7 @@ static int ptp_kvm_get_time_fn(ktime_t *device_time, spin_lock(&kvm_ptp_lock); preempt_disable_notrace(); - ret = kvm_arch_ptp_get_crosststamp(&cycle, &tspec, &cs); + ret = kvm_arch_ptp_get_crosststamp(&cycle, &tspec, &cs, ctx); if (ret != 0) { pr_err_ratelimited("clock pairing hypercall ret %lu\n", ret); spin_unlock(&kvm_ptp_lock); @@ -57,9 +57,10 @@ static int ptp_kvm_get_time_fn(ktime_t *device_time, } static int ptp_kvm_getcrosststamp(struct ptp_clock_info *ptp, - struct system_device_crosststamp *xtstamp) + struct system_device_crosststamp *xtstamp, + long *flag) { - return get_device_system_crosststamp(ptp_kvm_get_time_fn, NULL, + return get_device_system_crosststamp(ptp_kvm_get_time_fn, flag, NULL, xtstamp); } diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h index 9dc9d0079e98..71e388a82244 100644 --- a/include/uapi/linux/ptp_clock.h +++ b/include/uapi/linux/ptp_clock.h @@ -32,6 +32,7 @@ #define PTP_RISING_EDGE (1<<1) #define PTP_FALLING_EDGE (1<<2) #define PTP_STRICT_FLAGS (1<<3) +#define PTP_KVM_ARM_PHY_COUNTER (1<<4) #define PTP_EXTTS_EDGES (PTP_RISING_EDGE | PTP_FALLING_EDGE) /* @@ -40,7 +41,8 @@ #define PTP_EXTTS_VALID_FLAGS (PTP_ENABLE_FEATURE | \ PTP_RISING_EDGE | \ PTP_FALLING_EDGE | \ - PTP_STRICT_FLAGS) + PTP_STRICT_FLAGS | \ + PTP_KVM_ARM_PHY_COUNTER) /* * flag fields valid for the original PTP_EXTTS_REQUEST ioctl.
In general, vm inside will use virtual counter compered with host use phyical counter. But in some special scenarios, like nested virtualization, phyical counter maybe used by vm. A interface added in ptp_kvm driver to offer a mechanism to let user choose which counter should be return from host. To use this feature, you should call PTP_EXTTS_REQUEST(2) ioctl with flag set bit PTP_KVM_ARM_PHY_COUNTER in its argument then call PTP_SYS_OFFSET_PRECISE(2) ioctl to get the cross timestamp and phyical counter will return. If the bit not set or no call for PTP_EXTTS_REQUEST2, virtual counter will return by default. Signed-off-by: Jianyong Wu <jianyong.wu@arm.com> Suggested-by: Marc Zyngier <maz@kernel.org> --- drivers/clocksource/arm_arch_timer.c | 13 ++++++++++++- drivers/ptp/ptp_chardev.c | 25 +++++++++++++++++++++++++ drivers/ptp/ptp_kvm_common.c | 7 ++++--- include/uapi/linux/ptp_clock.h | 4 +++- 4 files changed, 44 insertions(+), 5 deletions(-)