mbox series

[v15,0/9] Enable ptp_kvm for arm/arm64

Message ID 20201111062211.33144-1-jianyong.wu@arm.com
Headers show
Series Enable ptp_kvm for arm/arm64 | expand

Message

Jianyong Wu Nov. 11, 2020, 6:22 a.m. UTC
Currently, we offen use ntp (sync time with remote network clock)
to sync time in VM. But the precision of ntp is subject to network delay
so it's difficult to sync time in a high precision.

kvm virtual ptp clock (ptp_kvm) offers another way to sync time in VM,
as the remote clock locates in the host instead of remote network clock.
It targets to sync time between guest and host in virtualization
environment and in this way, we can keep the time of all the VMs running
in the same host in sync. In general, the delay of communication between
host and guest is quiet small, so ptp_kvm can offer time sync precision
up to in order of nanosecond. Please keep in mind that ptp_kvm just
limits itself to be a channel which transmit the remote clock from
host to guest and leaves the time sync jobs to an application, eg. chrony,
in usersapce in VM.

How ptp_kvm works:
After ptp_kvm initialized, there will be a new device node under
/dev called ptp%d. A guest userspace service, like chrony, can use this
device to get host walltime, sometimes also counter cycle, which depends
on the service it calls. Then this guest userspace service can use those
data to do the time sync for guest.
here is a rough sketch to show how kvm ptp clock works.

|----------------------------|              |--------------------------|
|       guest userspace      |              |          host            |
|ioctl -> /dev/ptp%d         |              |                          |
|       ^   |                |              |                          |
|----------------------------|              |                          |
|       |   | guest kernel   |              |                          |
|       |   V      (get host walltime/counter cycle)                   |
|      ptp_kvm -> hypercall - - - - - - - - - - ->hypercall service    |
|                         <- - - - - - - - - - - -                     |
|----------------------------|              |--------------------------|

1. time sync service in guest userspace call ptp device through /dev/ptp%d.
2. ptp_kvm module in guest recive this request then invoke hypercall to route
into host kernel to request host walltime/counter cycle.
3. ptp_kvm hypercall service in host response to the request and send data back.
4. ptp (not ptp_kvm) in guest copy the data to userspace.

This ptp_kvm implementation focuses itself to step 2 and 3 and step 2 works
in guest comparing step 3 works in host kernel.

change log:

from v14 to v15
        (1) enable ptp_kvm on arm32 guest, also ptp_kvm has been tested
on both arm64 and arm32 guest running on arm64 kvm host.
        (2) move arch-agnostic part of ptp_kvm.rst into timekeeping.rst.
        (3) rename KVM_CAP_ARM_PTP_KVM to KVM_CAP_PTP_KVM as it should be
arch agnostic.
        (4) add description for KVM_CAP_PTP_KVM in Documentation/virt/kvm/api.rst.
        (5) adjust dependency in Kconfig for ptp_kvm.
        (6) refine multi-arch process in driver/ptp/Makefile.
        (7) fix make pdfdocs htmldocs issue for ptp_kvm doc.
        (8) address other issues from comments in v14.
        (9) fold hypercall service of ptp_kvm as a function.
        (10) rebase to 5.10-rc3.

from v13 to v14
        (1) rebase code on 5.9-rc3.
        (2) add a document to introduce implementation of PTP_KVM on
arm64.
        (3) fix comments issue in hypercall.c.
        (4) export arm_smccc_1_1_get_conduit using EXPORT_SYMBOL_GPL.
        (5) fix make issue on x86 reported by kernel test robot.

from v12 to v13:
        (1) rebase code on 5.8-rc1.
        (2) this patch set base on 2 patches of 1/8 and 2/8 from Will Decon.
        (3) remove the change to ptp device code of extend getcrosststamp.
        (4) remove the mechanism of letting user choose the counter type in
ptp_kvm for arm64.
        (5) add virtual counter option in ptp_kvm service to let user choose
the specific counter explicitly.

from v11 to v12:
        (1) rebase code on 5.7-rc6 and rebase 2 patches from Will Decon
including 1/11 and 2/11. as these patches introduce discover mechanism of
vendor smccc service.
        (2) rebase ptp_kvm hypercall service from standard smccc to vendor
smccc and add ptp_kvm to vendor smccc service discover mechanism.
        (3) add detail of why we need ptp_kvm and how ptp_kvm works in cover
letter.

from v10 to v11:
        (1) rebase code on 5.7-rc2.
        (2) remove support for arm32, as kvm support for arm32 will be
removed [1]
        (3) add error report in ptp_kvm initialization.

from v9 to v10:
        (1) change code base to v5.5.
        (2) enable ptp_kvm both for arm32 and arm64.
        (3) let user choose which of virtual counter or physical counter
should return when using crosstimestamp mode of ptp_kvm for arm/arm64.
        (4) extend input argument for getcrosstimestamp API.

from v8 to v9:
        (1) move ptp_kvm.h to driver/ptp/
        (2) replace license declaration of ptp_kvm.h the same with other
header files in the same directory.

from v7 to v8:
        (1) separate adding clocksource id for arm_arch_counter as a
single patch.
        (2) update commit message for patch 4/8.
        (3) refine patch 7/8 and patch 8/8 to make them more independent.

from v5 to v6:
        (1) apply Mark's patch[4] to get SMCCC conduit.
        (2) add mechanism to recognize current clocksource by add
clocksouce_id value into struct clocksource instead of method in patch-v5.
        (3) rename kvm_arch_ptp_get_clock_fn into
kvm_arch_ptp_get_crosststamp.

from v4 to v5:
        (1) remove hvc delay compensasion as it should leave to userspace.
        (2) check current clocksource in hvc call service.
        (3) expose current clocksource by adding it to
system_time_snapshot.
        (4) add helper to check if clocksource is arm_arch_counter.
        (5) rename kvm_ptp.c to ptp_kvm_common.c

from v3 to v4:
        (1) fix clocksource of ptp_kvm to arch_sys_counter.
        (2) move kvm_arch_ptp_get_clock_fn into arm_arch_timer.c
        (3) subtract cntvoff before return cycles from host.
        (4) use ktime_get_snapshot instead of getnstimeofday and
get_current_counterval to return time and counter value.
        (5) split ktime and counter into two 32-bit block respectively
to avoid Y2038-safe issue.
        (6) set time compensation to device time as half of the delay of
hvc call.
        (7) add ARM_ARCH_TIMER as dependency of ptp_kvm for
arm64.

from v2 to v3:
        (1) fix some issues in commit log.
        (2) add some receivers in send list.

from v1 to v2:
        (1) move arch-specific code from arch/ to driver/ptp/
        (2) offer mechanism to inform userspace if ptp_kvm service is
available.
        (3) separate ptp_kvm code for arm64 into hypervisor part and
guest part.
        (4) add API to expose monotonic clock and counter value.
        (5) refine code: remove no necessary part and reconsitution.

[1] https://patchwork.kernel.org/cover/11373351/


Jianyong Wu (6):
  ptp: Reorganize ptp_kvm module to make it arch-independent.
  clocksource: Add clocksource id for arm arch counter
  arm64/kvm: Add hypercall service for kvm ptp.
  ptp: arm/arm64: Enable ptp_kvm for arm/arm64
  doc: add ptp_kvm introduction for arm64 support
  arm64: Add kvm capability check extension for ptp_kvm

Thomas Gleixner (1):
  time: Add mechanism to recognize clocksource in time_get_snapshot

Will Deacon (2):
  arm64: Probe for the presence of KVM hypervisor
  arm/arm64: KVM: Advertise KVM UID to guests via SMCCC

 Documentation/virt/kvm/api.rst              |  9 ++
 Documentation/virt/kvm/arm/index.rst        |  1 +
 Documentation/virt/kvm/arm/ptp_kvm.rst      | 29 +++++++
 Documentation/virt/kvm/timekeeping.rst      | 35 ++++++++
 arch/arm/kernel/setup.c                     |  1 +
 arch/arm64/kernel/setup.c                   |  1 +
 arch/arm64/kvm/arm.c                        |  1 +
 arch/arm64/kvm/hypercalls.c                 | 88 +++++++++++++++++--
 drivers/clocksource/arm_arch_timer.c        | 30 +++++++
 drivers/firmware/smccc/smccc.c              | 37 ++++++++
 drivers/ptp/Kconfig                         |  2 +-
 drivers/ptp/Makefile                        |  2 +
 drivers/ptp/ptp_kvm.h                       | 11 +++
 drivers/ptp/ptp_kvm_arm.c                   | 44 ++++++++++
 drivers/ptp/{ptp_kvm.c => ptp_kvm_common.c} | 85 +++++-------------
 drivers/ptp/ptp_kvm_x86.c                   | 95 +++++++++++++++++++++
 include/linux/arm-smccc.h                   | 60 +++++++++++++
 include/linux/clocksource.h                 |  6 ++
 include/linux/clocksource_ids.h             | 12 +++
 include/linux/timekeeping.h                 | 12 +--
 include/uapi/linux/kvm.h                    |  1 +
 kernel/time/clocksource.c                   |  2 +
 kernel/time/timekeeping.c                   |  1 +
 23 files changed, 488 insertions(+), 77 deletions(-)
 create mode 100644 Documentation/virt/kvm/arm/ptp_kvm.rst
 create mode 100644 drivers/ptp/ptp_kvm.h
 create mode 100644 drivers/ptp/ptp_kvm_arm.c
 rename drivers/ptp/{ptp_kvm.c => ptp_kvm_common.c} (60%)
 create mode 100644 drivers/ptp/ptp_kvm_x86.c
 create mode 100644 include/linux/clocksource_ids.h

Comments

Jianyong Wu Nov. 23, 2020, 9:26 a.m. UTC | #1
Hi,
Ping ...
Any comments?

> -----Original Message-----

> From: Jianyong Wu <jianyong.wu@arm.com>

> Sent: Wednesday, November 11, 2020 2:22 PM

> To: 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; richardcochran@gmail.com; Mark Rutland

> <Mark.Rutland@arm.com>; will@kernel.org; Suzuki Poulose

> <Suzuki.Poulose@arm.com>; Andre Przywara <Andre.Przywara@arm.com>;

> Steven Price <Steven.Price@arm.com>

> Cc: 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>; Justin He <Justin.He@arm.com>; Jianyong Wu

> <Jianyong.Wu@arm.com>; nd <nd@arm.com>

> Subject: [PATCH v15 0/9] Enable ptp_kvm for arm/arm64

> 

> Currently, we offen use ntp (sync time with remote network clock) to sync

> time in VM. But the precision of ntp is subject to network delay so it's difficult

> to sync time in a high precision.

> 

> kvm virtual ptp clock (ptp_kvm) offers another way to sync time in VM, as

> the remote clock locates in the host instead of remote network clock.

> It targets to sync time between guest and host in virtualization environment

> and in this way, we can keep the time of all the VMs running in the same host

> in sync. In general, the delay of communication between host and guest is

> quiet small, so ptp_kvm can offer time sync precision up to in order of

> nanosecond. Please keep in mind that ptp_kvm just limits itself to be a

> channel which transmit the remote clock from host to guest and leaves the

> time sync jobs to an application, eg. chrony, in usersapce in VM.

> 

> How ptp_kvm works:

> After ptp_kvm initialized, there will be a new device node under /dev called

> ptp%d. A guest userspace service, like chrony, can use this device to get host

> walltime, sometimes also counter cycle, which depends on the service it calls.

> Then this guest userspace service can use those data to do the time sync for

> guest.

> here is a rough sketch to show how kvm ptp clock works.

> 

> |----------------------------|              |--------------------------|

> |       guest userspace      |              |          host            |

> |ioctl -> /dev/ptp%d         |              |                          |

> |       ^   |                |              |                          |

> |----------------------------|              |                          |

> |       |   | guest kernel   |              |                          |

> |       |   V      (get host walltime/counter cycle)                   |

> |      ptp_kvm -> hypercall - - - - - - - - - - ->hypercall service    |

> |                         <- - - - - - - - - - - -                     |

> |----------------------------|              |--------------------------|

> 

> 1. time sync service in guest userspace call ptp device through /dev/ptp%d.

> 2. ptp_kvm module in guest recive this request then invoke hypercall to

> route into host kernel to request host walltime/counter cycle.

> 3. ptp_kvm hypercall service in host response to the request and send data

> back.

> 4. ptp (not ptp_kvm) in guest copy the data to userspace.

> 

> This ptp_kvm implementation focuses itself to step 2 and 3 and step 2 works

> in guest comparing step 3 works in host kernel.

> 

> change log:

> 

> from v14 to v15

>         (1) enable ptp_kvm on arm32 guest, also ptp_kvm has been tested on

> both arm64 and arm32 guest running on arm64 kvm host.

>         (2) move arch-agnostic part of ptp_kvm.rst into timekeeping.rst.

>         (3) rename KVM_CAP_ARM_PTP_KVM to KVM_CAP_PTP_KVM as it

> should be arch agnostic.

>         (4) add description for KVM_CAP_PTP_KVM in

> Documentation/virt/kvm/api.rst.

>         (5) adjust dependency in Kconfig for ptp_kvm.

>         (6) refine multi-arch process in driver/ptp/Makefile.

>         (7) fix make pdfdocs htmldocs issue for ptp_kvm doc.

>         (8) address other issues from comments in v14.

>         (9) fold hypercall service of ptp_kvm as a function.

>         (10) rebase to 5.10-rc3.

> 

> from v13 to v14

>         (1) rebase code on 5.9-rc3.

>         (2) add a document to introduce implementation of PTP_KVM on arm64.

>         (3) fix comments issue in hypercall.c.

>         (4) export arm_smccc_1_1_get_conduit using EXPORT_SYMBOL_GPL.

>         (5) fix make issue on x86 reported by kernel test robot.

> 

> from v12 to v13:

>         (1) rebase code on 5.8-rc1.

>         (2) this patch set base on 2 patches of 1/8 and 2/8 from Will Decon.

>         (3) remove the change to ptp device code of extend getcrosststamp.

>         (4) remove the mechanism of letting user choose the counter type in

> ptp_kvm for arm64.

>         (5) add virtual counter option in ptp_kvm service to let user choose the

> specific counter explicitly.

> 

> from v11 to v12:

>         (1) rebase code on 5.7-rc6 and rebase 2 patches from Will Decon

> including 1/11 and 2/11. as these patches introduce discover mechanism of

> vendor smccc service.

>         (2) rebase ptp_kvm hypercall service from standard smccc to vendor

> smccc and add ptp_kvm to vendor smccc service discover mechanism.

>         (3) add detail of why we need ptp_kvm and how ptp_kvm works in cover

> letter.

> 

> from v10 to v11:

>         (1) rebase code on 5.7-rc2.

>         (2) remove support for arm32, as kvm support for arm32 will be removed

> [1]

>         (3) add error report in ptp_kvm initialization.

> 

> from v9 to v10:

>         (1) change code base to v5.5.

>         (2) enable ptp_kvm both for arm32 and arm64.

>         (3) let user choose which of virtual counter or physical counter should

> return when using crosstimestamp mode of ptp_kvm for arm/arm64.

>         (4) extend input argument for getcrosstimestamp API.

> 

> from v8 to v9:

>         (1) move ptp_kvm.h to driver/ptp/

>         (2) replace license declaration of ptp_kvm.h the same with other header

> files in the same directory.

> 

> from v7 to v8:

>         (1) separate adding clocksource id for arm_arch_counter as a single patch.

>         (2) update commit message for patch 4/8.

>         (3) refine patch 7/8 and patch 8/8 to make them more independent.

> 

> from v5 to v6:

>         (1) apply Mark's patch[4] to get SMCCC conduit.

>         (2) add mechanism to recognize current clocksource by add

> clocksouce_id value into struct clocksource instead of method in patch-v5.

>         (3) rename kvm_arch_ptp_get_clock_fn into

> kvm_arch_ptp_get_crosststamp.

> 

> from v4 to v5:

>         (1) remove hvc delay compensasion as it should leave to userspace.

>         (2) check current clocksource in hvc call service.

>         (3) expose current clocksource by adding it to system_time_snapshot.

>         (4) add helper to check if clocksource is arm_arch_counter.

>         (5) rename kvm_ptp.c to ptp_kvm_common.c

> 

> from v3 to v4:

>         (1) fix clocksource of ptp_kvm to arch_sys_counter.

>         (2) move kvm_arch_ptp_get_clock_fn into arm_arch_timer.c

>         (3) subtract cntvoff before return cycles from host.

>         (4) use ktime_get_snapshot instead of getnstimeofday and

> get_current_counterval to return time and counter value.

>         (5) split ktime and counter into two 32-bit block respectively to avoid

> Y2038-safe issue.

>         (6) set time compensation to device time as half of the delay of hvc call.

>         (7) add ARM_ARCH_TIMER as dependency of ptp_kvm for arm64.

> 

> from v2 to v3:

>         (1) fix some issues in commit log.

>         (2) add some receivers in send list.

> 

> from v1 to v2:

>         (1) move arch-specific code from arch/ to driver/ptp/

>         (2) offer mechanism to inform userspace if ptp_kvm service is available.

>         (3) separate ptp_kvm code for arm64 into hypervisor part and guest part.

>         (4) add API to expose monotonic clock and counter value.

>         (5) refine code: remove no necessary part and reconsitution.

> 

> [1] https://patchwork.kernel.org/cover/11373351/

> 

> 

> Jianyong Wu (6):

>   ptp: Reorganize ptp_kvm module to make it arch-independent.

>   clocksource: Add clocksource id for arm arch counter

>   arm64/kvm: Add hypercall service for kvm ptp.

>   ptp: arm/arm64: Enable ptp_kvm for arm/arm64

>   doc: add ptp_kvm introduction for arm64 support

>   arm64: Add kvm capability check extension for ptp_kvm

> 

> Thomas Gleixner (1):

>   time: Add mechanism to recognize clocksource in time_get_snapshot

> 

> Will Deacon (2):

>   arm64: Probe for the presence of KVM hypervisor

>   arm/arm64: KVM: Advertise KVM UID to guests via SMCCC

> 

>  Documentation/virt/kvm/api.rst              |  9 ++

>  Documentation/virt/kvm/arm/index.rst        |  1 +

>  Documentation/virt/kvm/arm/ptp_kvm.rst      | 29 +++++++

>  Documentation/virt/kvm/timekeeping.rst      | 35 ++++++++

>  arch/arm/kernel/setup.c                     |  1 +

>  arch/arm64/kernel/setup.c                   |  1 +

>  arch/arm64/kvm/arm.c                        |  1 +

>  arch/arm64/kvm/hypercalls.c                 | 88 +++++++++++++++++--

>  drivers/clocksource/arm_arch_timer.c        | 30 +++++++

>  drivers/firmware/smccc/smccc.c              | 37 ++++++++

>  drivers/ptp/Kconfig                         |  2 +-

>  drivers/ptp/Makefile                        |  2 +

>  drivers/ptp/ptp_kvm.h                       | 11 +++

>  drivers/ptp/ptp_kvm_arm.c                   | 44 ++++++++++

>  drivers/ptp/{ptp_kvm.c => ptp_kvm_common.c} | 85 +++++-------------

>  drivers/ptp/ptp_kvm_x86.c                   | 95 +++++++++++++++++++++

>  include/linux/arm-smccc.h                   | 60 +++++++++++++

>  include/linux/clocksource.h                 |  6 ++

>  include/linux/clocksource_ids.h             | 12 +++

>  include/linux/timekeeping.h                 | 12 +--

>  include/uapi/linux/kvm.h                    |  1 +

>  kernel/time/clocksource.c                   |  2 +

>  kernel/time/timekeeping.c                   |  1 +

>  23 files changed, 488 insertions(+), 77 deletions(-)  create mode 100644

> Documentation/virt/kvm/arm/ptp_kvm.rst

>  create mode 100644 drivers/ptp/ptp_kvm.h  create mode 100644

> drivers/ptp/ptp_kvm_arm.c  rename drivers/ptp/{ptp_kvm.c =>

> ptp_kvm_common.c} (60%)  create mode 100644

> drivers/ptp/ptp_kvm_x86.c  create mode 100644

> include/linux/clocksource_ids.h

> 

> --

> 2.17.1
Marc Zyngier Nov. 23, 2020, 10:49 a.m. UTC | #2
On 2020-11-11 06:22, Jianyong Wu wrote:
> Currently, there is no mechanism to keep time sync between guest and 

> host

> in arm/arm64 virtualization environment. Time in guest will drift 

> compared

> with host after boot up as they may both use third party time sources

> to correct their time respectively. The time deviation will be in order

> of milliseconds. But in some scenarios,like in cloud envirenment, we 

> ask


environment

> for higher time precision.

> 

> kvm ptp clock, which chooses the host clock source as a reference

> clock to sync time between guest and host, has been adopted by x86

> which takes the time sync order from milliseconds to nanoseconds.

> 

> This patch enables kvm ptp clock for arm/arm64 and improves clock sync 

> precison


precision

> significantly.

> 

> Test result comparisons between with kvm ptp clock and without it in 

> arm/arm64

> are as follows. This test derived from the result of command 'chronyc

> sources'. we should take more care of the last sample column which 

> shows

> the offset between the local clock and the source at the last 

> measurement.

> 

> no kvm ptp in guest:

> MS Name/IP address   Stratum Poll Reach LastRx Last sample

> ========================================================================

> ^* dns1.synet.edu.cn      2   6   377    13  +1040us[+1581us] +/-   

> 21ms

> ^* dns1.synet.edu.cn      2   6   377    21  +1040us[+1581us] +/-   

> 21ms

> ^* dns1.synet.edu.cn      2   6   377    29  +1040us[+1581us] +/-   

> 21ms

> ^* dns1.synet.edu.cn      2   6   377    37  +1040us[+1581us] +/-   

> 21ms

> ^* dns1.synet.edu.cn      2   6   377    45  +1040us[+1581us] +/-   

> 21ms

> ^* dns1.synet.edu.cn      2   6   377    53  +1040us[+1581us] +/-   

> 21ms

> ^* dns1.synet.edu.cn      2   6   377    61  +1040us[+1581us] +/-   

> 21ms

> ^* dns1.synet.edu.cn      2   6   377     4   -130us[ +796us] +/-   

> 21ms

> ^* dns1.synet.edu.cn      2   6   377    12   -130us[ +796us] +/-   

> 21ms

> ^* dns1.synet.edu.cn      2   6   377    20   -130us[ +796us] +/-   

> 21ms

> 

> in host:

> MS Name/IP address   Stratum Poll Reach LastRx Last sample

> ========================================================================

> ^* 120.25.115.20          2   7   377    72   -470us[ -603us] +/-   

> 18ms

> ^* 120.25.115.20          2   7   377    92   -470us[ -603us] +/-   

> 18ms

> ^* 120.25.115.20          2   7   377   112   -470us[ -603us] +/-   

> 18ms

> ^* 120.25.115.20          2   7   377     2   +872ns[-6808ns] +/-   

> 17ms

> ^* 120.25.115.20          2   7   377    22   +872ns[-6808ns] +/-   

> 17ms

> ^* 120.25.115.20          2   7   377    43   +872ns[-6808ns] +/-   

> 17ms

> ^* 120.25.115.20          2   7   377    63   +872ns[-6808ns] +/-   

> 17ms

> ^* 120.25.115.20          2   7   377    83   +872ns[-6808ns] +/-   

> 17ms

> ^* 120.25.115.20          2   7   377   103   +872ns[-6808ns] +/-   

> 17ms

> ^* 120.25.115.20          2   7   377   123   +872ns[-6808ns] +/-   

> 17ms

> 

> The dns1.synet.edu.cn is the network reference clock for guest and

> 120.25.115.20 is the network reference clock for host. we can't get the

> clock error between guest and host directly, but a roughly estimated 

> value

> will be in order of hundreds of us to ms.

> 

> with kvm ptp in guest:

> chrony has been disabled in host to remove the disturb by network 

> clock.

> 

> MS Name/IP address         Stratum Poll Reach LastRx Last sample

> ========================================================================

> * PHC0                    0   3   377     8     -7ns[   +1ns] +/-    

> 3ns

> * PHC0                    0   3   377     8     +1ns[  +16ns] +/-    

> 3ns

> * PHC0                    0   3   377     6     -4ns[   -0ns] +/-    

> 6ns

> * PHC0                    0   3   377     6     -8ns[  -12ns] +/-    

> 5ns

> * PHC0                    0   3   377     5     +2ns[   +4ns] +/-    

> 4ns

> * PHC0                    0   3   377    13     +2ns[   +4ns] +/-    

> 4ns

> * PHC0                    0   3   377    12     -4ns[   -6ns] +/-    

> 4ns

> * PHC0                    0   3   377    11     -8ns[  -11ns] +/-    

> 6ns

> * PHC0                    0   3   377    10    -14ns[  -20ns] +/-    

> 4ns

> * PHC0                    0   3   377     8     +4ns[   +5ns] +/-    

> 4ns

> 

> The PHC0 is the ptp clock which choose the host clock as its source

> clock. So we can see that the clock difference between host and guest

> is in order of ns.

> 

> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>

> ---

>  drivers/clocksource/arm_arch_timer.c | 28 ++++++++++++++++++

>  drivers/ptp/Kconfig                  |  2 +-

>  drivers/ptp/Makefile                 |  1 +

>  drivers/ptp/ptp_kvm_arm.c            | 44 ++++++++++++++++++++++++++++

>  4 files changed, 74 insertions(+), 1 deletion(-)

>  create mode 100644 drivers/ptp/ptp_kvm_arm.c

> 

> diff --git a/drivers/clocksource/arm_arch_timer.c

> b/drivers/clocksource/arm_arch_timer.c

> index d55acffb0b90..b33c5a663d30 100644

> --- a/drivers/clocksource/arm_arch_timer.c

> +++ b/drivers/clocksource/arm_arch_timer.c

> @@ -25,6 +25,7 @@

>  #include <linux/sched/clock.h>

>  #include <linux/sched_clock.h>

>  #include <linux/acpi.h>

> +#include <linux/arm-smccc.h>

> 

>  #include <asm/arch_timer.h>

>  #include <asm/virt.h>

> @@ -1650,3 +1651,30 @@ static int __init arch_timer_acpi_init(struct

> acpi_table_header *table)

>  }

>  TIMER_ACPI_DECLARE(arch_timer, ACPI_SIG_GTDT, arch_timer_acpi_init);

>  #endif

> +

> +int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *ts,

> +			      struct clocksource **cs)

> +{

> +	struct arm_smccc_res hvc_res;

> +	ktime_t ktime;

> +	u32 ptp_counter;

> +

> +	if (arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI)

> +		ptp_counter = ARM_PTP_VIRT_COUNTER;

> +	else

> +		ptp_counter = ARM_PTP_PHY_COUNTER;

> +

> +	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,

> +			     ptp_counter, &hvc_res);

> +

> +	if ((int)(hvc_res.a0) < 0)

> +		return -EOPNOTSUPP;

> +

> +	ktime = (u64)hvc_res.a0 << 32 | hvc_res.a1;

> +	*ts = ktime_to_timespec64(ktime);

> +	*cycle = (u64)hvc_res.a2 << 32 | hvc_res.a3;


Endianness.

> +	*cs = &clocksource_counter;

> +

> +	return 0;

> +}

> +EXPORT_SYMBOL_GPL(kvm_arch_ptp_get_crosststamp);

> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig

> index 942f72d8151d..677c7f696b70 100644

> --- a/drivers/ptp/Kconfig

> +++ b/drivers/ptp/Kconfig

> @@ -106,7 +106,7 @@ config PTP_1588_CLOCK_PCH

>  config PTP_1588_CLOCK_KVM

>  	tristate "KVM virtual PTP clock"

>  	depends on PTP_1588_CLOCK

> -	depends on KVM_GUEST && X86

> +	depends on KVM_GUEST && X86 || (HAVE_ARM_SMCCC_DISCOVERY && 

> ARM_ARCH_TIMER)

>  	default y

>  	help

>  	  This driver adds support for using kvm infrastructure as a PTP

> diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile

> index 699a4e4d19c2..9fa5ede44b2b 100644

> --- a/drivers/ptp/Makefile

> +++ b/drivers/ptp/Makefile

> @@ -5,6 +5,7 @@

> 

>  ptp-y					:= ptp_clock.o ptp_chardev.o ptp_sysfs.o

>  ptp_kvm-$(CONFIG_X86)			:= ptp_kvm_x86.o ptp_kvm_common.o

> +ptp_kvm-$(CONFIG_HAVE_ARM_SMCCC)	:= ptp_kvm_arm.o ptp_kvm_common.o

>  obj-$(CONFIG_PTP_1588_CLOCK)		+= ptp.o

>  obj-$(CONFIG_PTP_1588_CLOCK_DTE)	+= ptp_dte.o

>  obj-$(CONFIG_PTP_1588_CLOCK_INES)	+= ptp_ines.o

> diff --git a/drivers/ptp/ptp_kvm_arm.c b/drivers/ptp/ptp_kvm_arm.c

> new file mode 100644

> index 000000000000..2212827c0384

> --- /dev/null

> +++ b/drivers/ptp/ptp_kvm_arm.c

> @@ -0,0 +1,44 @@

> +// SPDX-License-Identifier: GPL-2.0-only

> +/*

> + *  Virtual PTP 1588 clock for use with KVM guests

> + *  Copyright (C) 2019 ARM Ltd.

> + *  All Rights Reserved

> + */

> +

> +#include <linux/kernel.h>

> +#include <linux/err.h>

> +#include <asm/hypervisor.h>

> +#include <linux/module.h>

> +#include <linux/psci.h>

> +#include <linux/arm-smccc.h>

> +#include <linux/timecounter.h>

> +#include <linux/sched/clock.h>

> +#include <asm/arch_timer.h>

> +#include <asm/hypervisor.h>

> +

> +int kvm_arch_ptp_init(void)

> +{

> +	int ret;

> +

> +	ret = kvm_arm_hyp_service_available(ARM_SMCCC_KVM_FUNC_KVM_PTP);

> +	if (ret <= 0)

> +		return -EOPNOTSUPP;

> +

> +	return 0;

> +}

> +

> +int kvm_arch_ptp_get_clock(struct timespec64 *ts)

> +{

> +	ktime_t ktime;

> +	struct arm_smccc_res hvc_res;

> +

> +	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,

> +			     ARM_PTP_NONE_COUNTER, &hvc_res);


I really don't see the need to use a non-architectural counter ID.
Using the virtual counter ID should just be fine, and shouldn't
lead to any issue.

Am I missing something?

> +	if ((int)(hvc_res.a0) < 0)

> +		return -EOPNOTSUPP;

> +

> +	ktime = (u64)hvc_res.a0 << 32 | hvc_res.a1;


Endianness.

> +	*ts = ktime_to_timespec64(ktime);

> +

> +	return 0;

> +}


Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
Marc Zyngier Nov. 23, 2020, 10:58 a.m. UTC | #3
On 2020-11-11 06:22, Jianyong Wu wrote:
> PTP_KVM implementation depends on hypercall using SMCCC. So we

> introduce a new SMCCC service ID. This doc explains how does the

> ID define and how does PTP_KVM works on arm/arm64.

> 

> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>

> ---

>  Documentation/virt/kvm/api.rst         |  9 +++++++

>  Documentation/virt/kvm/arm/index.rst   |  1 +

>  Documentation/virt/kvm/arm/ptp_kvm.rst | 29 +++++++++++++++++++++

>  Documentation/virt/kvm/timekeeping.rst | 35 ++++++++++++++++++++++++++

>  4 files changed, 74 insertions(+)

>  create mode 100644 Documentation/virt/kvm/arm/ptp_kvm.rst

> 

> diff --git a/Documentation/virt/kvm/api.rst 

> b/Documentation/virt/kvm/api.rst

> index 36d5f1f3c6dd..9843dbcbf770 100644

> --- a/Documentation/virt/kvm/api.rst

> +++ b/Documentation/virt/kvm/api.rst

> @@ -6391,3 +6391,12 @@ When enabled, KVM will disable paravirtual

> features provided to the

>  guest according to the bits in the KVM_CPUID_FEATURES CPUID leaf

>  (0x40000001). Otherwise, a guest may use the paravirtual features

>  regardless of what has actually been exposed through the CPUID leaf.

> +

> +8.27 KVM_CAP_PTP_KVM

> +--------------------

> +

> +:Architectures: arm64

> +

> +This capability indicates that KVM virtual PTP service is supported in 

> host.

> +It must company with the implementation of KVM virtual PTP service in 

> host

> +so VMM can probe if there is the service in host by checking this 

> capability.

> diff --git a/Documentation/virt/kvm/arm/index.rst

> b/Documentation/virt/kvm/arm/index.rst

> index 3e2b2aba90fc..78a9b670aafe 100644

> --- a/Documentation/virt/kvm/arm/index.rst

> +++ b/Documentation/virt/kvm/arm/index.rst

> @@ -10,3 +10,4 @@ ARM

>     hyp-abi

>     psci

>     pvtime

> +   ptp_kvm

> diff --git a/Documentation/virt/kvm/arm/ptp_kvm.rst

> b/Documentation/virt/kvm/arm/ptp_kvm.rst

> new file mode 100644

> index 000000000000..bb1e6cfefe44

> --- /dev/null

> +++ b/Documentation/virt/kvm/arm/ptp_kvm.rst

> @@ -0,0 +1,29 @@

> +.. SPDX-License-Identifier: GPL-2.0

> +

> +PTP_KVM support for arm/arm64

> +=============================

> +

> +PTP_KVM is used for time sync between guest and host in a high 

> precision.

> +It needs to get the wall time and counter value from the host and

> transfer these

> +to guest via hypercall service. So one more hypercall service has been 

> added.

> +

> +This new SMCCC hypercall is defined as:

> +

> +* ARM_SMCCC_HYP_KVM_PTP_FUNC_ID: 0x86000001

> +

> +As both 32 and 64-bits ptp_kvm client should be supported, we choose

> SMC32/HVC32

> +calling convention.

> +

> +ARM_SMCCC_HYP_KVM_PTP_FUNC_ID:

> +

> +    =============    ==========    ==========

> +    Function ID:     (uint32)      0x86000001

> +    Arguments:	     (uint32)      ARM_PTP_PHY_COUNTER(1) or

> ARM_PTP_VIRT_COUNTER(0)

> +                                   which indicate acquiring physical 

> counter or

> +                                   virtual counter respectively.

> +    return value:    (uint32)      NOT_SUPPORTED(-1) or val0 and val1 

> represent

> +                                   wall clock time and val2 and val3 

> represent

> +                                   counter cycle.


This needs a lot more description:

- Which word contains what part of the data (upper/lower part of the 
64bit data)
- The endianness of the data returned

         M.
-- 
Jazz is not dead. It just smells funny...
Jianyong Wu Nov. 24, 2020, 5:21 a.m. UTC | #4
Hi Marc,

> -----Original Message-----

> From: Marc Zyngier <maz@kernel.org>

> Sent: Monday, November 23, 2020 6:58 PM

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

> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;

> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Andre

> Przywara <Andre.Przywara@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>; Justin He

> <Justin.He@arm.com>; nd <nd@arm.com>

> Subject: Re: [PATCH v15 8/9] doc: add ptp_kvm introduction for arm64

> support

> 

> On 2020-11-11 06:22, Jianyong Wu wrote:

> > PTP_KVM implementation depends on hypercall using SMCCC. So we

> > introduce a new SMCCC service ID. This doc explains how does the ID

> > define and how does PTP_KVM works on arm/arm64.

> >

> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>

> > ---

> >  Documentation/virt/kvm/api.rst         |  9 +++++++

> >  Documentation/virt/kvm/arm/index.rst   |  1 +

> >  Documentation/virt/kvm/arm/ptp_kvm.rst | 29

> +++++++++++++++++++++

> > Documentation/virt/kvm/timekeeping.rst | 35

> ++++++++++++++++++++++++++

> >  4 files changed, 74 insertions(+)

> >  create mode 100644 Documentation/virt/kvm/arm/ptp_kvm.rst

> >

> > diff --git a/Documentation/virt/kvm/api.rst

> > b/Documentation/virt/kvm/api.rst index 36d5f1f3c6dd..9843dbcbf770

> > 100644

> > --- a/Documentation/virt/kvm/api.rst

> > +++ b/Documentation/virt/kvm/api.rst

> > @@ -6391,3 +6391,12 @@ When enabled, KVM will disable paravirtual

> > features provided to the  guest according to the bits in the

> > KVM_CPUID_FEATURES CPUID leaf  (0x40000001). Otherwise, a guest may

> > use the paravirtual features  regardless of what has actually been

> > exposed through the CPUID leaf.

> > +

> > +8.27 KVM_CAP_PTP_KVM

> > +--------------------

> > +

> > +:Architectures: arm64

> > +

> > +This capability indicates that KVM virtual PTP service is supported

> > +in

> > host.

> > +It must company with the implementation of KVM virtual PTP service in

> > host

> > +so VMM can probe if there is the service in host by checking this

> > capability.

> > diff --git a/Documentation/virt/kvm/arm/index.rst

> > b/Documentation/virt/kvm/arm/index.rst

> > index 3e2b2aba90fc..78a9b670aafe 100644

> > --- a/Documentation/virt/kvm/arm/index.rst

> > +++ b/Documentation/virt/kvm/arm/index.rst

> > @@ -10,3 +10,4 @@ ARM

> >     hyp-abi

> >     psci

> >     pvtime

> > +   ptp_kvm

> > diff --git a/Documentation/virt/kvm/arm/ptp_kvm.rst

> > b/Documentation/virt/kvm/arm/ptp_kvm.rst

> > new file mode 100644

> > index 000000000000..bb1e6cfefe44

> > --- /dev/null

> > +++ b/Documentation/virt/kvm/arm/ptp_kvm.rst

> > @@ -0,0 +1,29 @@

> > +.. SPDX-License-Identifier: GPL-2.0

> > +

> > +PTP_KVM support for arm/arm64

> > +=============================

> > +

> > +PTP_KVM is used for time sync between guest and host in a high

> > precision.

> > +It needs to get the wall time and counter value from the host and

> > transfer these

> > +to guest via hypercall service. So one more hypercall service has

> > +been

> > added.

> > +

> > +This new SMCCC hypercall is defined as:

> > +

> > +* ARM_SMCCC_HYP_KVM_PTP_FUNC_ID: 0x86000001

> > +

> > +As both 32 and 64-bits ptp_kvm client should be supported, we choose

> > SMC32/HVC32

> > +calling convention.

> > +

> > +ARM_SMCCC_HYP_KVM_PTP_FUNC_ID:

> > +

> > +    =============    ==========    ==========

> > +    Function ID:     (uint32)      0x86000001

> > +    Arguments:	     (uint32)      ARM_PTP_PHY_COUNTER(1) or

> > ARM_PTP_VIRT_COUNTER(0)

> > +                                   which indicate acquiring physical

> > counter or

> > +                                   virtual counter respectively.

> > +    return value:    (uint32)      NOT_SUPPORTED(-1) or val0 and val1

> > represent

> > +                                   wall clock time and val2 and val3

> > represent

> > +                                   counter cycle.

> 

> This needs a lot more description:

> 

> - Which word contains what part of the data (upper/lower part of the 64bit

> data)

> - The endianness of the data returned


OK.

Thanks
Jianyong 
> 

>          M.

> --

> Jazz is not dead. It just smells funny...
Jianyong Wu Nov. 24, 2020, 5:37 a.m. UTC | #5
Hi Marc,

> -----Original Message-----

> From: Marc Zyngier <maz@kernel.org>

> Sent: Monday, November 23, 2020 6:49 PM

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

> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;

> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Andre

> Przywara <Andre.Przywara@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>; Justin He

> <Justin.He@arm.com>; nd <nd@arm.com>

> Subject: Re: [PATCH v15 7/9] ptp: arm/arm64: Enable ptp_kvm for

> arm/arm64

> 

> On 2020-11-11 06:22, Jianyong Wu wrote:

> > Currently, there is no mechanism to keep time sync between guest and

> > host in arm/arm64 virtualization environment. Time in guest will drift

> > compared with host after boot up as they may both use third party time

> > sources to correct their time respectively. The time deviation will be

> > in order of milliseconds. But in some scenarios,like in cloud

> > envirenment, we ask

> 

> environment

> 

OK

> > for higher time precision.

> >

> > kvm ptp clock, which chooses the host clock source as a reference

> > clock to sync time between guest and host, has been adopted by x86

> > which takes the time sync order from milliseconds to nanoseconds.

> >

> > This patch enables kvm ptp clock for arm/arm64 and improves clock sync

> > precison

> 

> precision

>

OK
 
> > significantly.

> >

> > Test result comparisons between with kvm ptp clock and without it in

> > arm/arm64

> > are as follows. This test derived from the result of command 'chronyc

> > sources'. we should take more care of the last sample column which

> > shows the offset between the local clock and the source at the last

> > measurement.

> >

> > no kvm ptp in guest:

> > MS Name/IP address   Stratum Poll Reach LastRx Last sample

> >

> ==========================================================

> ==============

> > ^* dns1.synet.edu.cn      2   6   377    13  +1040us[+1581us] +/-

> > 21ms

> > ^* dns1.synet.edu.cn      2   6   377    21  +1040us[+1581us] +/-

> > 21ms

> > ^* dns1.synet.edu.cn      2   6   377    29  +1040us[+1581us] +/-

> > 21ms

> > ^* dns1.synet.edu.cn      2   6   377    37  +1040us[+1581us] +/-

> > 21ms

> > ^* dns1.synet.edu.cn      2   6   377    45  +1040us[+1581us] +/-

> > 21ms

> > ^* dns1.synet.edu.cn      2   6   377    53  +1040us[+1581us] +/-

> > 21ms

> > ^* dns1.synet.edu.cn      2   6   377    61  +1040us[+1581us] +/-

> > 21ms

> > ^* dns1.synet.edu.cn      2   6   377     4   -130us[ +796us] +/-

> > 21ms

> > ^* dns1.synet.edu.cn      2   6   377    12   -130us[ +796us] +/-

> > 21ms

> > ^* dns1.synet.edu.cn      2   6   377    20   -130us[ +796us] +/-

> > 21ms

> >

> > in host:

> > MS Name/IP address   Stratum Poll Reach LastRx Last sample

> >

> ==========================================================

> ==============

> > ^* 120.25.115.20          2   7   377    72   -470us[ -603us] +/-

> > 18ms

> > ^* 120.25.115.20          2   7   377    92   -470us[ -603us] +/-

> > 18ms

> > ^* 120.25.115.20          2   7   377   112   -470us[ -603us] +/-

> > 18ms

> > ^* 120.25.115.20          2   7   377     2   +872ns[-6808ns] +/-

> > 17ms

> > ^* 120.25.115.20          2   7   377    22   +872ns[-6808ns] +/-

> > 17ms

> > ^* 120.25.115.20          2   7   377    43   +872ns[-6808ns] +/-

> > 17ms

> > ^* 120.25.115.20          2   7   377    63   +872ns[-6808ns] +/-

> > 17ms

> > ^* 120.25.115.20          2   7   377    83   +872ns[-6808ns] +/-

> > 17ms

> > ^* 120.25.115.20          2   7   377   103   +872ns[-6808ns] +/-

> > 17ms

> > ^* 120.25.115.20          2   7   377   123   +872ns[-6808ns] +/-

> > 17ms

> >

> > The dns1.synet.edu.cn is the network reference clock for guest and

> > 120.25.115.20 is the network reference clock for host. we can't get

> > the clock error between guest and host directly, but a roughly

> > estimated value will be in order of hundreds of us to ms.

> >

> > with kvm ptp in guest:

> > chrony has been disabled in host to remove the disturb by network

> > clock.

> >

> > MS Name/IP address         Stratum Poll Reach LastRx Last sample

> >

> ==========================================================

> ==============

> > * PHC0                    0   3   377     8     -7ns[   +1ns] +/-

> > 3ns

> > * PHC0                    0   3   377     8     +1ns[  +16ns] +/-

> > 3ns

> > * PHC0                    0   3   377     6     -4ns[   -0ns] +/-

> > 6ns

> > * PHC0                    0   3   377     6     -8ns[  -12ns] +/-

> > 5ns

> > * PHC0                    0   3   377     5     +2ns[   +4ns] +/-

> > 4ns

> > * PHC0                    0   3   377    13     +2ns[   +4ns] +/-

> > 4ns

> > * PHC0                    0   3   377    12     -4ns[   -6ns] +/-

> > 4ns

> > * PHC0                    0   3   377    11     -8ns[  -11ns] +/-

> > 6ns

> > * PHC0                    0   3   377    10    -14ns[  -20ns] +/-

> > 4ns

> > * PHC0                    0   3   377     8     +4ns[   +5ns] +/-

> > 4ns

> >

> > The PHC0 is the ptp clock which choose the host clock as its source

> > clock. So we can see that the clock difference between host and guest

> > is in order of ns.

> >

> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>

> > ---

> >  drivers/clocksource/arm_arch_timer.c | 28 ++++++++++++++++++

> >  drivers/ptp/Kconfig                  |  2 +-

> >  drivers/ptp/Makefile                 |  1 +

> >  drivers/ptp/ptp_kvm_arm.c            | 44

> ++++++++++++++++++++++++++++

> >  4 files changed, 74 insertions(+), 1 deletion(-)  create mode 100644

> > drivers/ptp/ptp_kvm_arm.c

> >

> > diff --git a/drivers/clocksource/arm_arch_timer.c

> > b/drivers/clocksource/arm_arch_timer.c

> > index d55acffb0b90..b33c5a663d30 100644

> > --- a/drivers/clocksource/arm_arch_timer.c

> > +++ b/drivers/clocksource/arm_arch_timer.c

> > @@ -25,6 +25,7 @@

> >  #include <linux/sched/clock.h>

> >  #include <linux/sched_clock.h>

> >  #include <linux/acpi.h>

> > +#include <linux/arm-smccc.h>

> >

> >  #include <asm/arch_timer.h>

> >  #include <asm/virt.h>

> > @@ -1650,3 +1651,30 @@ static int __init arch_timer_acpi_init(struct

> > acpi_table_header *table)  }  TIMER_ACPI_DECLARE(arch_timer,

> > ACPI_SIG_GTDT, arch_timer_acpi_init);  #endif

> > +

> > +int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *ts,

> > +			      struct clocksource **cs)

> > +{

> > +	struct arm_smccc_res hvc_res;

> > +	ktime_t ktime;

> > +	u32 ptp_counter;

> > +

> > +	if (arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI)

> > +		ptp_counter = ARM_PTP_VIRT_COUNTER;

> > +	else

> > +		ptp_counter = ARM_PTP_PHY_COUNTER;

> > +

> > +

> 	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU

> NC_ID,

> > +			     ptp_counter, &hvc_res);

> > +

> > +	if ((int)(hvc_res.a0) < 0)

> > +		return -EOPNOTSUPP;

> > +

> > +	ktime = (u64)hvc_res.a0 << 32 | hvc_res.a1;

> > +	*ts = ktime_to_timespec64(ktime);

> > +	*cycle = (u64)hvc_res.a2 << 32 | hvc_res.a3;

> 

> Endianness.

> 

> > +	*cs = &clocksource_counter;

> > +

> > +	return 0;

> > +}

> > +EXPORT_SYMBOL_GPL(kvm_arch_ptp_get_crosststamp);

> > diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig index

> > 942f72d8151d..677c7f696b70 100644

> > --- a/drivers/ptp/Kconfig

> > +++ b/drivers/ptp/Kconfig

> > @@ -106,7 +106,7 @@ config PTP_1588_CLOCK_PCH  config

> > PTP_1588_CLOCK_KVM

> >  	tristate "KVM virtual PTP clock"

> >  	depends on PTP_1588_CLOCK

> > -	depends on KVM_GUEST && X86

> > +	depends on KVM_GUEST && X86 ||

> (HAVE_ARM_SMCCC_DISCOVERY &&

> > ARM_ARCH_TIMER)

> >  	default y

> >  	help

> >  	  This driver adds support for using kvm infrastructure as a PTP

> > diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile index

> > 699a4e4d19c2..9fa5ede44b2b 100644

> > --- a/drivers/ptp/Makefile

> > +++ b/drivers/ptp/Makefile

> > @@ -5,6 +5,7 @@

> >

> >  ptp-y					:= ptp_clock.o ptp_chardev.o

> ptp_sysfs.o

> >  ptp_kvm-$(CONFIG_X86)			:= ptp_kvm_x86.o

> ptp_kvm_common.o

> > +ptp_kvm-$(CONFIG_HAVE_ARM_SMCCC)	:= ptp_kvm_arm.o

> ptp_kvm_common.o

> >  obj-$(CONFIG_PTP_1588_CLOCK)		+= ptp.o

> >  obj-$(CONFIG_PTP_1588_CLOCK_DTE)	+= ptp_dte.o

> >  obj-$(CONFIG_PTP_1588_CLOCK_INES)	+= ptp_ines.o

> > diff --git a/drivers/ptp/ptp_kvm_arm.c b/drivers/ptp/ptp_kvm_arm.c new

> > file mode 100644 index 000000000000..2212827c0384

> > --- /dev/null

> > +++ b/drivers/ptp/ptp_kvm_arm.c

> > @@ -0,0 +1,44 @@

> > +// SPDX-License-Identifier: GPL-2.0-only

> > +/*

> > + *  Virtual PTP 1588 clock for use with KVM guests

> > + *  Copyright (C) 2019 ARM Ltd.

> > + *  All Rights Reserved

> > + */

> > +

> > +#include <linux/kernel.h>

> > +#include <linux/err.h>

> > +#include <asm/hypervisor.h>

> > +#include <linux/module.h>

> > +#include <linux/psci.h>

> > +#include <linux/arm-smccc.h>

> > +#include <linux/timecounter.h>

> > +#include <linux/sched/clock.h>

> > +#include <asm/arch_timer.h>

> > +#include <asm/hypervisor.h>

> > +

> > +int kvm_arch_ptp_init(void)

> > +{

> > +	int ret;

> > +

> > +	ret =

> kvm_arm_hyp_service_available(ARM_SMCCC_KVM_FUNC_KVM_PTP);

> > +	if (ret <= 0)

> > +		return -EOPNOTSUPP;

> > +

> > +	return 0;

> > +}

> > +

> > +int kvm_arch_ptp_get_clock(struct timespec64 *ts) {

> > +	ktime_t ktime;

> > +	struct arm_smccc_res hvc_res;

> > +

> > +

> 	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU

> NC_ID,

> > +			     ARM_PTP_NONE_COUNTER, &hvc_res);

> 

> I really don't see the need to use a non-architectural counter ID.

> Using the virtual counter ID should just be fine, and shouldn't lead to any

> issue.

> 


> Am I missing something?


In this function, no counter data is needed. If virtual counter ID is used here, user may be confused with why we get virtual counter
data and do nothing with it. So I explicitly add a new "NONE" counter ID to make it clear.

WDYT?

Thanks
Jianyong
> 

> > +	if ((int)(hvc_res.a0) < 0)

> > +		return -EOPNOTSUPP;

> > +

> > +	ktime = (u64)hvc_res.a0 << 32 | hvc_res.a1;

> 

> Endianness.

> 

> > +	*ts = ktime_to_timespec64(ktime);

> > +

> > +	return 0;

> > +}

> 

> Thanks,

> 

>          M.

> --

> Jazz is not dead. It just smells funny...
Marc Zyngier Nov. 24, 2020, 9:04 a.m. UTC | #6
Jianyong,

On 2020-11-24 05:37, Jianyong Wu wrote:
> Hi Marc,


[...]

>> > +

>> 	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU

>> NC_ID,

>> > +			     ARM_PTP_NONE_COUNTER, &hvc_res);

>> 

>> I really don't see the need to use a non-architectural counter ID.

>> Using the virtual counter ID should just be fine, and shouldn't lead 

>> to any

>> issue.

>> 

> 

>> Am I missing something?

> 

> In this function, no counter data is needed. If virtual counter ID is

> used here, user may be confused with why we get virtual counter

> data and do nothing with it. So I explicitly add a new "NONE" counter

> ID to make it clear.

> 

> WDYT?


ITIABI (I Think It's A Bad Idea). There are two counters, and the API
allows to retrieve the data for any of these two. If the "user" doesn't
want to do anything with the data, that's their problem.

Here, you can just sue the virtual counter, and that will give you the
exact same semantic, without inventing non-architectural state.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
Jianyong Wu Nov. 24, 2020, 10:18 a.m. UTC | #7
Hi Marc,

> -----Original Message-----

> From: Marc Zyngier <maz@kernel.org>

> Sent: Tuesday, November 24, 2020 5:05 PM

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

> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;

> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Andre

> Przywara <Andre.Przywara@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>; Justin He

> <Justin.He@arm.com>; nd <nd@arm.com>

> Subject: Re: [PATCH v15 7/9] ptp: arm/arm64: Enable ptp_kvm for

> arm/arm64

> 

> Jianyong,

> 

> On 2020-11-24 05:37, Jianyong Wu wrote:

> > Hi Marc,

> 

> [...]

> 

> >> > +

> >> 	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU

> >> NC_ID,

> >> > +			     ARM_PTP_NONE_COUNTER, &hvc_res);

> >>

> >> I really don't see the need to use a non-architectural counter ID.

> >> Using the virtual counter ID should just be fine, and shouldn't lead

> >> to any issue.

> >>

> >

> >> Am I missing something?

> >

> > In this function, no counter data is needed. If virtual counter ID is

> > used here, user may be confused with why we get virtual counter

> > data and do nothing with it. So I explicitly add a new "NONE" counter

> > ID to make it clear.

> >

> > WDYT?

> 

> ITIABI (I Think It's A Bad Idea). There are two counters, and the API

> allows to retrieve the data for any of these two. If the "user" doesn't

> want to do anything with the data, that's their problem.

> 

> Here, you can just sue the virtual counter, and that will give you the

> exact same semantic, without inventing non-architectural state.

> 

OK, that's it.

Thanks
Jianyong Wu

> Thanks,

> 

>          M.

> --

> Jazz is not dead. It just smells funny...