Hi steven,
> -----Original Message-----
> From: Steven Price <steven.pr...@arm.com>
> Sent: Monday, June 22, 2020 5:51 PM
> To: Jianyong Wu <jianyong...@arm.com>; net...@vger.kernel.org;
> yangbo...@nxp.com; john.stu...@linaro.org; t...@linutronix.de;
> pbonz...@redhat.com; sean.j.christopher...@intel.com; m...@kernel.org;
> richardcoch...@gmail.com; Mark Rutland <mark.rutl...@arm.com>;
> w...@kernel.org; Suzuki Poulose <suzuki.poul...@arm.com>
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> kvm...@lists.cs.columbia.edu; k...@vger.kernel.org; Steve Capper
> <steve.cap...@arm.com>; Kaly Xin <kaly....@arm.com>; Justin He
> <justin...@arm.com>; Wei Chen <wei.c...@arm.com>; nd <n...@arm.com>
> Subject: Re: [RFC PATCH v13 7/9] arm64/kvm: Add hypercall service for kvm
> ptp.
>
> On 22/06/2020 03:25, Jianyong Wu wrote:
> > Hi Steven,
>
> Hi Jianyong
>
> [...]
> >>> diff --git a/arch/arm64/kvm/hypercalls.c
> >>> b/arch/arm64/kvm/hypercalls.c index db6dce3d0e23..366b0646c360
> >>> 100644
> >>> --- a/arch/arm64/kvm/hypercalls.c
> >>> +++ b/arch/arm64/kvm/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,6 +12,10 @@
> >>>
> >>> int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> >>> {
> >>> +#ifdef CONFIG_ARM64_KVM_PTP_HOST
> >>> + struct system_time_snapshot systime_snapshot;
> >>> + u64 cycles = 0;
> >>> +#endif
> >>> u32 func_id = smccc_get_function(vcpu);
> >>> u32 val[4] = {SMCCC_RET_NOT_SUPPORTED};
> >>> u32 feature;
> >>> @@ -70,7 +75,52 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> >>> break;
> >>> case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID:
> >>> val[0] = BIT(ARM_SMCCC_KVM_FUNC_FEATURES);
> >>> +
> >>> +#ifdef CONFIG_ARM64_KVM_PTP_HOST
> >>> + val[0] |= BIT(ARM_SMCCC_KVM_FUNC_KVM_PTP); #endif
> >>> + break;
> >>> +
> >>> +#ifdef CONFIG_ARM64_KVM_PTP_HOST
> >>> + /*
> >>> + * 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_VENDOR_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;
> >>> + val[0] = upper_32_bits(systime_snapshot.real);
> >>> + val[1] = lower_32_bits(systime_snapshot.real);
> >>> + /*
> >>> + * which of virtual counter or physical counter being
> >>> + * asked for is decided by the first argument of smccc
> >>> + * call. If no first argument or invalid argument, zero
> >>> + * counter value will return;
> >>> + */
> >>
> >> It's not actually possible to have "no first argument" - there's no
> >> argument count, so whatever is in the register during the call with
> >> be passed. I'd also caution that "first argument" is ambigious: r0
> >> could be the 'first' but is also the function number, here you mean r1.
> >>
> > Sorry, I really make mistake here, I really mean no r1 value.
>
> My point is that it's not possible to have "no r1 value" - r1 always has a
> value.
> So you can have an "invalid argument" (r1 has a value which isn't valid), but
> it's not possible to have "no first argument". It would only be possible to
> have no argument if the interface told us how many arguments were valid,
> but SMCCC doesn't do that.
>
Oh, sorry again, it should be "no valid r1 value". Thanks for clarifying this
issue.
> >> There's also a subtle cast to 32 bits here (feature is u32), which
> >> might be worth a comment before someone 'optimises' by removing the
> 'feature'
> >> variable.
> >>
> > Yeah, it's better to add a note, but I think it's better add it at the
> > first time
> calling smccc_get_arg1.
> > WDYT?
>
> I'm a bit confused about where exactly you were suggesting. The assignment
> (and implicit cast) are just below, so this comment block seemed a sensible
> place to add the note. But I don't really mind exactly where you put it (as
> long
> as it's close), it's just a subtle detail that might get lost if there isn't a
> comment.
>
Ok, I will add a note before smccc_get_arg1 called.
> >> Finally I'm not sure if zero counter value is best - would it not be
> >> possible for this to be a valid counter value?
> >
> > We have two different ways to call this service in ptp_kvm guest, one
> > needs counter cycle, the other not. So I think it's vain to return a valid
> counter cycle back if the ptp_kvm does not needs it.
>
> Sorry, I didn't write that very clearly. What I meant is that returning '0'
> in the
> case of an invalid argument might be difficult to recognise.
> '0' may be a valid reading of a counter (e.g. reading the counter just after
> the
> VM has been created if the counter increments very slowly). So it may be
> worth using a different value when an invalid argument has been specified.
> E.g. an "all ones" (-1) value may be more recognisable.
>
Ok, -1 is better than 0.
> In practice most counters increment fast enough that this may not actually be
> an issue, but this sort of thing is a pain to fix if it becomes a problem in
> the
> future.
>
Yeah.
> >>
> >>> + feature = smccc_get_arg1(vcpu);
> >>> + switch (feature) {
> >>> + case ARM_PTP_VIRT_COUNTER:
> >>> + cycles = systime_snapshot.cycles -
> >>> + vcpu_vtimer(vcpu)->cntvoff;
> >>
> >> Please indent the continuation line so that it's obvious.
> > Ok,
> >
> >>
> >>> + break;
> >>> + case ARM_PTP_PHY_COUNTER:
> >>> + cycles = systime_snapshot.cycles;
> >>> + break;
> >>> + }
> >>> + val[2] = upper_32_bits(cycles);
> >>> + val[3] = lower_32_bits(cycles);
> >>> break;
> >>> +#endif
> >>> +
> >>> default:
> >>> return kvm_psci_call(vcpu);
> >>> }
> >>> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> >>> index 86ff30131e7b..e593ec515f82 100644
> >>> --- a/include/linux/arm-smccc.h
> >>> +++ b/include/linux/arm-smccc.h
> >>> @@ -98,6 +98,9 @@
> >>>
> >>> /* KVM "vendor specific" services */
> >>> #define ARM_SMCCC_KVM_FUNC_FEATURES 0
> >>> +#define ARM_SMCCC_KVM_FUNC_KVM_PTP 1
> >>> +#define ARM_SMCCC_KVM_FUNC_KVM_PTP_PHY 2
> >>> +#define ARM_SMCCC_KVM_FUNC_KVM_PTP_VIRT 3
> >>> #define ARM_SMCCC_KVM_FUNC_FEATURES_2 127
> >>> #define ARM_SMCCC_KVM_NUM_FUNCS 128
> >>>
> >>> @@ -107,6 +110,33 @@
> >>> ARM_SMCCC_OWNER_VENDOR_HYP,
> >> \
> >>> ARM_SMCCC_KVM_FUNC_FEATURES)
> >>>
> >>> +/*
> >>> + * kvm_ptp is a feature used for time sync between vm and host.
> >>> + * kvm_ptp module in guest kernel will get service from host using
> >>> + * this hypercall ID.
> >>> + */
> >>> +#define ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID
> >> \
> >>> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,
> >> \
> >>> + ARM_SMCCC_SMC_32,
> >> \
> >>> + ARM_SMCCC_OWNER_VENDOR_HYP,
> >> \
> >>> + ARM_SMCCC_KVM_FUNC_KVM_PTP)
> >>> +
> >>> +/*
> >>> + * kvm_ptp may get counter cycle from host and should ask for which
> >>> +of
> >>> + * physical counter or virtual counter by using
> ARM_PTP_PHY_COUNTER
> >>> +and
> >>> + * ARM_PTP_VIRT_COUNTER explicitly.
> >>> + */
> >>> +#define ARM_PTP_PHY_COUNTER
> >> \
> >>> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,
> >> \
> >>> + ARM_SMCCC_SMC_32,
> >> \
> >>> + ARM_SMCCC_OWNER_VENDOR_HYP,
> >> \
> >>> + ARM_SMCCC_KVM_FUNC_KVM_PTP_PHY)
> >>> +
> >>> +#define ARM_PTP_VIRT_COUNTER
> >> \
> >>> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,
> >> \
> >>> + ARM_SMCCC_SMC_32,
> >> \
> >>> + ARM_SMCCC_OWNER_VENDOR_HYP,
> >> \
> >>> + ARM_SMCCC_KVM_FUNC_KVM_PTP_VIRT)
> >>
> >> These two are not SMCCC calls themselves (just parameters to an
> >> SMCCC), so they really shouldn't be defined using
> ARM_SMCCC_CALL_VAL
> >> (it's just confusing and unnecessary). Can we not just pick small
> >> integers (e.g. 0 and 1) for these?
> >>
> > Yeah, I think so, it's better to define these parameters ID as single
> > number and not related to SMCCC. What about keep these 2 macros and
> define it directly as a number in include/linux/arm-smccc.h.
>
> Yes that sounds good.
>
> >> We also need some documentation of these SMCCC calls somewhere
> which
> >> would make this sort of review easier. For instance for
> >> paravirtualised stolen time there is
> >> Documentation/virt/kvm/arm/pvtime.rst (which also links to the
> published document from Arm).
> >>
> > Good point, a documentation is needed to explain these new SMCCC funcs.
> > Do you think we should do that in this patch serial? Does it beyond the
> scope of this patch set?
>
> Adding it in this patch series seems like the right place to me.
>
Ok, I will add the doc. This new doc will be named "ptp_kvm.rst" and placed in
the same
directory with pvtime.rst. I will compose this doc by reference to your
pvtime.rst which is a good example.
Thanks
Jianyong
> Thanks,
>
> Steve
