| Message ID | 20200421032304.26300-6-jianyong.wu@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable ptp_kvm for arm64 | expand |
On Tue, Apr 21, 2020 at 11:23:00AM +0800, Jianyong Wu wrote: > ptp_kvm modules will get this service through smccc call. > The service offers real time and counter cycle of host for guest. > Also let caller determine which cycle of virtual counter or physical counter > to return. > > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com> > --- > include/linux/arm-smccc.h | 21 +++++++++++++++++++ > virt/kvm/arm/hypercalls.c | 44 ++++++++++++++++++++++++++++++++++++++- > 2 files changed, 64 insertions(+), 1 deletion(-) > > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h > index 59494df0f55b..747b7595d0c6 100644 > --- a/include/linux/arm-smccc.h > +++ b/include/linux/arm-smccc.h > @@ -77,6 +77,27 @@ > ARM_SMCCC_SMC_32, \ > 0, 0x7fff) > > +/* PTP KVM call requests clock time from guest OS to host */ > +#define ARM_SMCCC_HYP_KVM_PTP_FUNC_ID \ > + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ > + ARM_SMCCC_SMC_32, \ > + ARM_SMCCC_OWNER_STANDARD_HYP, \ > + 0) > + > +/* request for virtual counter from ptp_kvm guest */ > +#define ARM_SMCCC_HYP_KVM_PTP_VIRT \ > + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ > + ARM_SMCCC_SMC_32, \ > + ARM_SMCCC_OWNER_STANDARD_HYP, \ > + 1) > + > +/* request for physical counter from ptp_kvm guest */ > +#define ARM_SMCCC_HYP_KVM_PTP_PHY \ > + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ > + ARM_SMCCC_SMC_32, \ > + ARM_SMCCC_OWNER_STANDARD_HYP, \ > + 2) ARM_SMCCC_OWNER_STANDARD_HYP is for standard calls as defined in SMCCC and companion documents, so we should refer to the specific documentation here. Where are these calls defined? If these calls are Linux-specific then ARM_SMCCC_OWNER_STANDARD_HYP isn't appropriate to use, as they are vendor-specific hypervisor service call. It looks like we don't currently have a ARM_SMCCC_OWNER_HYP for that (which IIUC would be 6), but we can add one as necessary. I think that Will might have added that as part of his SMCCC probing bits. > + > #ifndef __ASSEMBLY__ > > #include <linux/linkage.h> > diff --git a/virt/kvm/arm/hypercalls.c b/virt/kvm/arm/hypercalls.c > index 550dfa3e53cd..a5309c28d4dc 100644 > --- a/virt/kvm/arm/hypercalls.c > +++ b/virt/kvm/arm/hypercalls.c > @@ -3,6 +3,7 @@ > > #include <linux/arm-smccc.h> > #include <linux/kvm_host.h> > +#include <linux/clocksource_ids.h> > > #include <asm/kvm_emulate.h> > > @@ -11,8 +12,11 @@ > > int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) > { > - u32 func_id = smccc_get_function(vcpu); > + struct system_time_snapshot systime_snapshot; > + long arg[4]; > + u64 cycles; > long val = SMCCC_RET_NOT_SUPPORTED; > + u32 func_id = smccc_get_function(vcpu); > u32 feature; > gpa_t gpa; > > @@ -62,6 +66,44 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) > if (gpa != GPA_INVALID) > val = gpa; > break; > + /* > + * This serves virtual kvm_ptp. > + * Four values will be passed back. > + * reg0 stores high 32-bit host ktime; > + * reg1 stores low 32-bit host ktime; > + * reg2 stores high 32-bit difference of host cycles and cntvoff; > + * reg3 stores low 32-bit difference of host cycles and cntvoff. > + */ > + case ARM_SMCCC_HYP_KVM_PTP_FUNC_ID: Shouldn't the host opt-in to providing this to the guest, as with other features? > + /* > + * system time and counter value must captured in the same > + * time to keep consistency and precision. > + */ > + ktime_get_snapshot(&systime_snapshot); > + if (systime_snapshot.cs_id != CSID_ARM_ARCH_COUNTER) > + break; > + arg[0] = upper_32_bits(systime_snapshot.real); > + arg[1] = lower_32_bits(systime_snapshot.real); Why exactly does the guest need the host's real time? Neither the cover letter nor this commit message have explained that, and for those of us unfamliar with PTP it would be very helpful to know that to understand what's going on. > + /* > + * which of virtual counter or physical counter being > + * asked for is decided by the first argument. > + */ > + feature = smccc_get_arg1(vcpu); > + switch (feature) { > + case ARM_SMCCC_HYP_KVM_PTP_PHY: > + cycles = systime_snapshot.cycles; > + break; > + case ARM_SMCCC_HYP_KVM_PTP_VIRT: > + default: > + cycles = systime_snapshot.cycles - > + vcpu_vtimer(vcpu)->cntvoff; > + } > + arg[2] = upper_32_bits(cycles); > + arg[3] = lower_32_bits(cycles); > + > + smccc_set_retval(vcpu, arg[0], arg[1], arg[2], arg[3]); I think the 'arg' buffer is confusing here, and it'd be clearer to have: u64 snaphot; u64 cycles; ... and here do: smccc_set_retval(vcpu, upper_32_bits(snaphot), lower_32_bits(snapshot), upper_32_bits(cycles), lower_32_bits(cycles)); Thanks, Mark.
On 2020/4/21 5:57 PM, Mark Rutland wrote: Hi Mark, > On Tue, Apr 21, 2020 at 11:23:00AM +0800, Jianyong Wu wrote: >> ptp_kvm modules will get this service through smccc call. >> The service offers real time and counter cycle of host for guest. >> Also let caller determine which cycle of virtual counter or physical counter >> to return. >> >> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com> >> --- >> include/linux/arm-smccc.h | 21 +++++++++++++++++++ >> virt/kvm/arm/hypercalls.c | 44 ++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 64 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h >> index 59494df0f55b..747b7595d0c6 100644 >> --- a/include/linux/arm-smccc.h >> +++ b/include/linux/arm-smccc.h >> @@ -77,6 +77,27 @@ >> ARM_SMCCC_SMC_32, \ >> 0, 0x7fff) >> >> +/* PTP KVM call requests clock time from guest OS to host */ >> +#define ARM_SMCCC_HYP_KVM_PTP_FUNC_ID \ >> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ >> + ARM_SMCCC_SMC_32, \ >> + ARM_SMCCC_OWNER_STANDARD_HYP, \ >> + 0) >> + >> +/* request for virtual counter from ptp_kvm guest */ >> +#define ARM_SMCCC_HYP_KVM_PTP_VIRT \ >> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ >> + ARM_SMCCC_SMC_32, \ >> + ARM_SMCCC_OWNER_STANDARD_HYP, \ >> + 1) >> + >> +/* request for physical counter from ptp_kvm guest */ >> +#define ARM_SMCCC_HYP_KVM_PTP_PHY \ >> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ >> + ARM_SMCCC_SMC_32, \ >> + ARM_SMCCC_OWNER_STANDARD_HYP, \ >> + 2) > ARM_SMCCC_OWNER_STANDARD_HYP is for standard calls as defined in SMCCC > and companion documents, so we should refer to the specific > documentation here. Where are these calls defined? yeah, should add reference docs of "SMC CALLING CONVENTION" here. > If these calls are Linux-specific then ARM_SMCCC_OWNER_STANDARD_HYP > isn't appropriate to use, as they are vendor-specific hypervisor service > call. yeah, vendor-specific service is more suitable for ptp_kvm. > > It looks like we don't currently have a ARM_SMCCC_OWNER_HYP for that > (which IIUC would be 6), but we can add one as necessary. I think that > Will might have added that as part of his SMCCC probing bits. ok, I will add a new "ARM_SMCCC_OWNER_VENDOR_HYP" whose IIUC is 6 and create "ARM_SMCCC_HYP_KVM_PTP_ID" base on it. > >> + >> #ifndef __ASSEMBLY__ >> >> #include <linux/linkage.h> >> diff --git a/virt/kvm/arm/hypercalls.c b/virt/kvm/arm/hypercalls.c >> index 550dfa3e53cd..a5309c28d4dc 100644 >> --- a/virt/kvm/arm/hypercalls.c >> +++ b/virt/kvm/arm/hypercalls.c >> @@ -3,6 +3,7 @@ >> >> #include <linux/arm-smccc.h> >> #include <linux/kvm_host.h> >> +#include <linux/clocksource_ids.h> >> >> #include <asm/kvm_emulate.h> >> >> @@ -11,8 +12,11 @@ >> >> int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) >> { >> - u32 func_id = smccc_get_function(vcpu); >> + struct system_time_snapshot systime_snapshot; >> + long arg[4]; >> + u64 cycles; >> long val = SMCCC_RET_NOT_SUPPORTED; >> + u32 func_id = smccc_get_function(vcpu); >> u32 feature; >> gpa_t gpa; >> >> @@ -62,6 +66,44 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) >> if (gpa != GPA_INVALID) >> val = gpa; >> break; >> + /* >> + * This serves virtual kvm_ptp. >> + * Four values will be passed back. >> + * reg0 stores high 32-bit host ktime; >> + * reg1 stores low 32-bit host ktime; >> + * reg2 stores high 32-bit difference of host cycles and cntvoff; >> + * reg3 stores low 32-bit difference of host cycles and cntvoff. >> + */ >> + case ARM_SMCCC_HYP_KVM_PTP_FUNC_ID: > Shouldn't the host opt-in to providing this to the guest, as with other > features? er, do you mean that "ARM_SMCCC_HV_PV_TIME_XXX" as "opt-in"? if so, I think this kvm_ptp doesn't need a buddy. the driver in guest will call this service in a definite way. >> + /* >> + * system time and counter value must captured in the same >> + * time to keep consistency and precision. >> + */ >> + ktime_get_snapshot(&systime_snapshot); >> + if (systime_snapshot.cs_id != CSID_ARM_ARCH_COUNTER) >> + break; >> + arg[0] = upper_32_bits(systime_snapshot.real); >> + arg[1] = lower_32_bits(systime_snapshot.real); > Why exactly does the guest need the host's real time? Neither the cover > letter nor this commit message have explained that, and for those of us > unfamliar with PTP it would be very helpful to know that to understand > what's going on. oh, sorry, I should have added more background knowledge here. just give some hints here: the kvm_ptp targets to sync guest time with host. some services in user space like chrony can do time sync by inputing time(in kvm_ptp also clock counter sometimes) from remote clocksource(often network clocksource). This kvm_ptp driver can offer a interface for those user space service in guest to get the host time to do time sync in guest. >> + /* >> + * which of virtual counter or physical counter being >> + * asked for is decided by the first argument. >> + */ >> + feature = smccc_get_arg1(vcpu); >> + switch (feature) { >> + case ARM_SMCCC_HYP_KVM_PTP_PHY: >> + cycles = systime_snapshot.cycles; >> + break; >> + case ARM_SMCCC_HYP_KVM_PTP_VIRT: >> + default: >> + cycles = systime_snapshot.cycles - >> + vcpu_vtimer(vcpu)->cntvoff; >> + } >> + arg[2] = upper_32_bits(cycles); >> + arg[3] = lower_32_bits(cycles); >> + >> + smccc_set_retval(vcpu, arg[0], arg[1], arg[2], arg[3]); > I think the 'arg' buffer is confusing here, and it'd be clearer to have: > > u64 snaphot; > u64 cycles; > > ... and here do: > > smccc_set_retval(vcpu, > upper_32_bits(snaphot), > lower_32_bits(snapshot), > upper_32_bits(cycles), > lower_32_bits(cycles)); it's better to use a meaningful variant name. I will fix it. thanks Jianyong > > Thanks, > Mark.
On Fri, Apr 24, 2020 at 03:50:22AM +0100, Jianyong Wu wrote: > On 2020/4/21 5:57 PM, Mark Rutland wrote: > > On Tue, Apr 21, 2020 at 11:23:00AM +0800, Jianyong Wu wrote: > >> diff --git a/virt/kvm/arm/hypercalls.c b/virt/kvm/arm/hypercalls.c > >> index 550dfa3e53cd..a5309c28d4dc 100644 > >> --- a/virt/kvm/arm/hypercalls.c > >> +++ b/virt/kvm/arm/hypercalls.c > >> @@ -62,6 +66,44 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) > >> if (gpa != GPA_INVALID) > >> val = gpa; > >> break; > >> +/* > >> + * This serves virtual kvm_ptp. > >> + * Four values will be passed back. > >> + * reg0 stores high 32-bit host ktime; > >> + * reg1 stores low 32-bit host ktime; > >> + * reg2 stores high 32-bit difference of host cycles and cntvoff; > >> + * reg3 stores low 32-bit difference of host cycles and cntvoff. > >> + */ > >> +case ARM_SMCCC_HYP_KVM_PTP_FUNC_ID: > > Shouldn't the host opt-in to providing this to the guest, as with other > > features? > > er, do you mean that "ARM_SMCCC_HV_PV_TIME_XXX" as "opt-in"? if so, I > think this > > kvm_ptp doesn't need a buddy. the driver in guest will call this service > in a definite way. I mean that when creating the VM, userspace should be able to choose whether the PTP service is provided to the guest. The host shouldn't always provide it as there may be cases where doing so is undesireable. > >> +/* > >> + * system time and counter value must captured in the same > >> + * time to keep consistency and precision. > >> + */ > >> +ktime_get_snapshot(&systime_snapshot); > >> +if (systime_snapshot.cs_id != CSID_ARM_ARCH_COUNTER) > >> +break; > >> +arg[0] = upper_32_bits(systime_snapshot.real); > >> +arg[1] = lower_32_bits(systime_snapshot.real); > > Why exactly does the guest need the host's real time? Neither the cover > > letter nor this commit message have explained that, and for those of us > > unfamliar with PTP it would be very helpful to know that to understand > > what's going on. > > oh, sorry, I should have added more background knowledge here. > > just give some hints here: > > the kvm_ptp targets to sync guest time with host. some services in user > space > > like chrony can do time sync by inputing time(in kvm_ptp also clock > counter sometimes) from > > remote clocksource(often network clocksource). This kvm_ptp driver can > offer a interface for > > those user space service in guest to get the host time to do time sync > in guest. I think it would be very helpful for the commit message and/or cover letter to have a high-level desctiption of what PTP is meant to do, and an outline of the algorithmm (clearly splitting the host and guest bits). It's also not clear to me what notion of host time is being exposed to the guest (and consequently how this would interact with time changes on the host, time namespaces, etc). Having some description of that would be very helpful. Thanks, Mark.
Hi, On Sun, Apr 26, 2020 at 06:02:23AM +0100, Jianyong Wu wrote: > <html> > <head> > <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> > </head> > <body> Please fix your mail client to reply in plain text rather than HTML; it's much more painful than necessary to review and have a conversation when mails revert to HTML. It would be very helpful if you could resend this mail as plaintext as above. Thanks, Mark.
On 2020/4/24 6:39 PM, Mark Rutland wrote: it's a resend of the last email, please ignore if you have received this. Hi Mark, thank you for remainder, I hope this email is in the correct format. > On Fri, Apr 24, 2020 at 03:50:22AM +0100, Jianyong Wu wrote: >> On 2020/4/21 5:57 PM, Mark Rutland wrote: >>> On Tue, Apr 21, 2020 at 11:23:00AM +0800, Jianyong Wu wrote: >>>> diff --git a/virt/kvm/arm/hypercalls.c b/virt/kvm/arm/hypercalls.c >>>> index 550dfa3e53cd..a5309c28d4dc 100644 >>>> --- a/virt/kvm/arm/hypercalls.c >>>> +++ b/virt/kvm/arm/hypercalls.c >>>> @@ -62,6 +66,44 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) >>>> if (gpa != GPA_INVALID) >>>> val = gpa; >>>> break; >>>> +/* >>>> + * This serves virtual kvm_ptp. >>>> + * Four values will be passed back. >>>> + * reg0 stores high 32-bit host ktime; >>>> + * reg1 stores low 32-bit host ktime; >>>> + * reg2 stores high 32-bit difference of host cycles and cntvoff; >>>> + * reg3 stores low 32-bit difference of host cycles and cntvoff. >>>> + */ >>>> +case ARM_SMCCC_HYP_KVM_PTP_FUNC_ID: >>> Shouldn't the host opt-in to providing this to the guest, as with other >>> features? >> er, do you mean that "ARM_SMCCC_HV_PV_TIME_XXX" as "opt-in"? if so, I >> think this >> >> kvm_ptp doesn't need a buddy. the driver in guest will call this service >> in a definite way. > I mean that when creating the VM, userspace should be able to choose > whether the PTP service is provided to the guest. The host shouldn't > always provide it as there may be cases where doing so is undesireable. > I think I have implemented in patch 9/9 that userspace can get the info that if the host offers the kvm_ptp service. But for now, the host kernel will always offer the kvm_ptp capability in the current implementation. I think x86 follow the same behavior (see [1]). so I have not considered when and how to disable this kvm_ptp service in host. Do you think we should offer this opt-in? [1] kvm_pv_clock_pairing() in https://github.com/torvalds/linux/blob/master/arch/x86/kvm/x86.c >>>> +/* >>>> + * system time and counter value must captured in the same >>>> + * time to keep consistency and precision. >>>> + */ >>>> +ktime_get_snapshot(&systime_snapshot); >>>> +if (systime_snapshot.cs_id != CSID_ARM_ARCH_COUNTER) >>>> +break; >>>> +arg[0] = upper_32_bits(systime_snapshot.real); >>>> +arg[1] = lower_32_bits(systime_snapshot.real); >>> Why exactly does the guest need the host's real time? Neither the cover >>> letter nor this commit message have explained that, and for those of us >>> unfamliar with PTP it would be very helpful to know that to understand >>> what's going on. >> oh, sorry, I should have added more background knowledge here. >> >> just give some hints here: >> >> the kvm_ptp targets to sync guest time with host. some services in user >> space >> >> like chrony can do time sync by inputing time(in kvm_ptp also clock >> counter sometimes) from >> >> remote clocksource(often network clocksource). This kvm_ptp driver can >> offer a interface for >> >> those user space service in guest to get the host time to do time sync >> in guest. > I think it would be very helpful for the commit message and/or cover > letter to have a high-level desctiption of what PTP is meant to do, and > an outline of the algorithmm (clearly splitting the host and guest > bits). ok, I will add high-level principle of kvm_ptp in commit message. > It's also not clear to me what notion of host time is being exposed to > the guest (and consequently how this would interact with time changes on > the host, time namespaces, etc). Having some description of that would > be very helpful. sorry to have not made it clear. Time will not change in host and only time in guest will change to sync with host. host time is the target that time in guest want to adjust to. guest need to get the host time then compute the different of the time in guest and host, so the guest can adjust the time base on the difference. I will add the base principle of time sync service in guest using kvm_ptp in commit message. Thanks Jianyong > Thanks, > Mark. -->
On Tue, Apr 28, 2020 at 07:14:52AM +0100, Jianyong Wu wrote: > On 2020/4/24 6:39 PM, Mark Rutland wrote: > > On Fri, Apr 24, 2020 at 03:50:22AM +0100, Jianyong Wu wrote: > >> On 2020/4/21 5:57 PM, Mark Rutland wrote: > >>> On Tue, Apr 21, 2020 at 11:23:00AM +0800, Jianyong Wu wrote: > >>>> diff --git a/virt/kvm/arm/hypercalls.c b/virt/kvm/arm/hypercalls.c > >>>> index 550dfa3e53cd..a5309c28d4dc 100644 > >>>> --- a/virt/kvm/arm/hypercalls.c > >>>> +++ b/virt/kvm/arm/hypercalls.c > >>>> @@ -62,6 +66,44 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) > >>>> if (gpa != GPA_INVALID) > >>>> val = gpa; > >>>> break; > >>>> +/* > >>>> + * This serves virtual kvm_ptp. > >>>> + * Four values will be passed back. > >>>> + * reg0 stores high 32-bit host ktime; > >>>> + * reg1 stores low 32-bit host ktime; > >>>> + * reg2 stores high 32-bit difference of host cycles and cntvoff; > >>>> + * reg3 stores low 32-bit difference of host cycles and cntvoff. > >>>> + */ > >>>> +case ARM_SMCCC_HYP_KVM_PTP_FUNC_ID: > >>> Shouldn't the host opt-in to providing this to the guest, as with other > >>> features? > >> er, do you mean that "ARM_SMCCC_HV_PV_TIME_XXX" as "opt-in"? if so, I > >> think this > >> > >> kvm_ptp doesn't need a buddy. the driver in guest will call this service > >> in a definite way. > > I mean that when creating the VM, userspace should be able to choose > > whether the PTP service is provided to the guest. The host shouldn't > > always provide it as there may be cases where doing so is undesireable. > > > I think I have implemented in patch 9/9 that userspace can get the info > that if the host offers the kvm_ptp service. But for now, the host > kernel will always offer the kvm_ptp capability in the current > implementation. I think x86 follow the same behavior (see [1]). so I > have not considered when and how to disable this kvm_ptp service in > host. Do you think we should offer this opt-in? I think taht should be opt-in, yes. [...] > > It's also not clear to me what notion of host time is being exposed to > > the guest (and consequently how this would interact with time changes on > > the host, time namespaces, etc). Having some description of that would > > be very helpful. > > sorry to have not made it clear. > > Time will not change in host and only time in guest will change to sync > with host. host time is the target that time in guest want to adjust to. > guest need to get the host time then compute the different of the time > in guest and host, so the guest can adjust the time base on the difference. I understood that host time wouldn't change here, but what was not clear is which notion of host time is being exposed to the guest. e.g. is that a raw monotonic clock, or one subject to periodic adjument, or wall time in the host? What is the epoch of the host time? > I will add the base principle of time sync service in guest using > kvm_ptp in commit message. That would be great; thanks! Mark.
On 2020/4/30 6:36 PM, Mark Rutland wrote: > On Tue, Apr 28, 2020 at 07:14:52AM +0100, Jianyong Wu wrote: >> On 2020/4/24 6:39 PM, Mark Rutland wrote: >>> On Fri, Apr 24, 2020 at 03:50:22AM +0100, Jianyong Wu wrote: >>>> On 2020/4/21 5:57 PM, Mark Rutland wrote: >>>>> On Tue, Apr 21, 2020 at 11:23:00AM +0800, Jianyong Wu wrote: >>>>>> diff --git a/virt/kvm/arm/hypercalls.c b/virt/kvm/arm/hypercalls.c >>>>>> index 550dfa3e53cd..a5309c28d4dc 100644 >>>>>> --- a/virt/kvm/arm/hypercalls.c >>>>>> +++ b/virt/kvm/arm/hypercalls.c >>>>>> @@ -62,6 +66,44 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) >>>>>> if (gpa != GPA_INVALID) >>>>>> val = gpa; >>>>>> break; >>>>>> +/* >>>>>> + * This serves virtual kvm_ptp. >>>>>> + * Four values will be passed back. >>>>>> + * reg0 stores high 32-bit host ktime; >>>>>> + * reg1 stores low 32-bit host ktime; >>>>>> + * reg2 stores high 32-bit difference of host cycles and cntvoff; >>>>>> + * reg3 stores low 32-bit difference of host cycles and cntvoff. >>>>>> + */ >>>>>> +case ARM_SMCCC_HYP_KVM_PTP_FUNC_ID: >>>>> Shouldn't the host opt-in to providing this to the guest, as with other >>>>> features? >>>> er, do you mean that "ARM_SMCCC_HV_PV_TIME_XXX" as "opt-in"? if so, I >>>> think this >>>> >>>> kvm_ptp doesn't need a buddy. the driver in guest will call this service >>>> in a definite way. >>> I mean that when creating the VM, userspace should be able to choose >>> whether the PTP service is provided to the guest. The host shouldn't >>> always provide it as there may be cases where doing so is undesireable. >>> >> I think I have implemented in patch 9/9 that userspace can get the info >> that if the host offers the kvm_ptp service. But for now, the host >> kernel will always offer the kvm_ptp capability in the current >> implementation. I think x86 follow the same behavior (see [1]). so I >> have not considered when and how to disable this kvm_ptp service in >> host. Do you think we should offer this opt-in? > > I think taht should be opt-in, yes. ok, what about adding "CONFIG_ARM64_KVM_PTP_HOST" in kvm_ptp host service to implement this "opt-in"? > > [...] > >>> It's also not clear to me what notion of host time is being exposed to >>> the guest (and consequently how this would interact with time changes on >>> the host, time namespaces, etc). Having some description of that would >>> be very helpful. >> >> sorry to have not made it clear. >> >> Time will not change in host and only time in guest will change to sync >> with host. host time is the target that time in guest want to adjust to. >> guest need to get the host time then compute the different of the time >> in guest and host, so the guest can adjust the time base on the difference. > > I understood that host time wouldn't change here, but what was not clear > is which notion of host time is being exposed to the guest. > > e.g. is that a raw monotonic clock, or one subject to periodic adjument, > or wall time in the host? What is the epoch of the host time? sorry, I misunderstood your last comment. I think it is one of the key part of kvm_ptp. I have confused with these clock time and expect to hear the comments from experts of kernel time subsystem like you. IMO,kvm_ptp targets to sync time in VM with host and if all the VMs in the same host do so, they can get time sync from each other. with no kvm_ptp, both host and guest may affected by NTP, the time will diverge between them. kvm_ptp can avoid this issue. if the host time vary with something like NTP adjustment, guest will track this variation with the help of kvm_ptp. I acquire these knowledge originally from [2]. also ptp driver will compare the wall time(see [3]). so I think wall time clock which subject to NTP adjustment is a good choice for kvm_ptp. in the current implementation I get the wall time clock from ktime_get_snapshot. I'm not sure if I give the correct knowledge of kvm_ptp and make a right choice of host clock time. So WDYT? [2]https://opensource.com/article/17/6/timekeeping-linux-vms [3] see PTP_SYS_OFFSET2 in ptp_ioctl in https://github.com/torvalds/linux/blob/master/drivers/ptp/ptp_chardev.c, it uses ktime_get_real_ts64 as the host time to calculate the difference between ptp time and host time. Thanks Jianyong > >> I will add the base principle of time sync service in guest using >> kvm_ptp in commit message. > > That would be great; thanks! > > Mark. >
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h index 59494df0f55b..747b7595d0c6 100644 --- a/include/linux/arm-smccc.h +++ b/include/linux/arm-smccc.h @@ -77,6 +77,27 @@ ARM_SMCCC_SMC_32, \ 0, 0x7fff) +/* PTP KVM call requests clock time from guest OS to host */ +#define ARM_SMCCC_HYP_KVM_PTP_FUNC_ID \ + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ + ARM_SMCCC_SMC_32, \ + ARM_SMCCC_OWNER_STANDARD_HYP, \ + 0) + +/* request for virtual counter from ptp_kvm guest */ +#define ARM_SMCCC_HYP_KVM_PTP_VIRT \ + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ + ARM_SMCCC_SMC_32, \ + ARM_SMCCC_OWNER_STANDARD_HYP, \ + 1) + +/* request for physical counter from ptp_kvm guest */ +#define ARM_SMCCC_HYP_KVM_PTP_PHY \ + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ + ARM_SMCCC_SMC_32, \ + ARM_SMCCC_OWNER_STANDARD_HYP, \ + 2) + #ifndef __ASSEMBLY__ #include <linux/linkage.h> diff --git a/virt/kvm/arm/hypercalls.c b/virt/kvm/arm/hypercalls.c index 550dfa3e53cd..a5309c28d4dc 100644 --- a/virt/kvm/arm/hypercalls.c +++ b/virt/kvm/arm/hypercalls.c @@ -3,6 +3,7 @@ #include <linux/arm-smccc.h> #include <linux/kvm_host.h> +#include <linux/clocksource_ids.h> #include <asm/kvm_emulate.h> @@ -11,8 +12,11 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) { - u32 func_id = smccc_get_function(vcpu); + struct system_time_snapshot systime_snapshot; + long arg[4]; + u64 cycles; long val = SMCCC_RET_NOT_SUPPORTED; + u32 func_id = smccc_get_function(vcpu); u32 feature; gpa_t gpa; @@ -62,6 +66,44 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) if (gpa != GPA_INVALID) val = gpa; break; + /* + * This serves virtual kvm_ptp. + * Four values will be passed back. + * reg0 stores high 32-bit host ktime; + * reg1 stores low 32-bit host ktime; + * reg2 stores high 32-bit difference of host cycles and cntvoff; + * reg3 stores low 32-bit difference of host cycles and cntvoff. + */ + case ARM_SMCCC_HYP_KVM_PTP_FUNC_ID: + /* + * system time and counter value must captured in the same + * time to keep consistency and precision. + */ + ktime_get_snapshot(&systime_snapshot); + if (systime_snapshot.cs_id != CSID_ARM_ARCH_COUNTER) + break; + arg[0] = upper_32_bits(systime_snapshot.real); + arg[1] = lower_32_bits(systime_snapshot.real); + /* + * which of virtual counter or physical counter being + * asked for is decided by the first argument. + */ + feature = smccc_get_arg1(vcpu); + switch (feature) { + case ARM_SMCCC_HYP_KVM_PTP_PHY: + cycles = systime_snapshot.cycles; + break; + case ARM_SMCCC_HYP_KVM_PTP_VIRT: + default: + cycles = systime_snapshot.cycles - + vcpu_vtimer(vcpu)->cntvoff; + } + arg[2] = upper_32_bits(cycles); + arg[3] = lower_32_bits(cycles); + + smccc_set_retval(vcpu, arg[0], arg[1], arg[2], arg[3]); + return 1; + default: return kvm_psci_call(vcpu); }
ptp_kvm modules will get this service through smccc call. The service offers real time and counter cycle of host for guest. Also let caller determine which cycle of virtual counter or physical counter to return. Signed-off-by: Jianyong Wu <jianyong.wu@arm.com> --- include/linux/arm-smccc.h | 21 +++++++++++++++++++ virt/kvm/arm/hypercalls.c | 44 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 1 deletion(-)