From: Jianyong Wu <Jianyong.Wu@arm.com>
To: Steven Price <Steven.Price@arm.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"yangbo.lu@nxp.com" <yangbo.lu@nxp.com>,
"john.stultz@linaro.org" <john.stultz@linaro.org>,
"tglx@linutronix.de" <tglx@linutronix.de>,
"pbonzini@redhat.com" <pbonzini@redhat.com>,
"sean.j.christopherson@intel.com"
<sean.j.christopherson@intel.com>,
"maz@kernel.org" <maz@kernel.org>,
"richardcochran@gmail.com" <richardcochran@gmail.com>,
Mark Rutland <Mark.Rutland@arm.com>,
"will@kernel.org" <will@kernel.org>,
Suzuki Poulose <Suzuki.Poulose@arm.com>
Cc: Justin He <Justin.He@arm.com>, Wei Chen <Wei.Chen@arm.com>,
"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
nd <nd@arm.com>,
"kvmarm@lists.cs.columbia.edu" <kvmarm@lists.cs.columbia.edu>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>
Subject: RE: [RFC PATCH v13 7/9] arm64/kvm: Add hypercall service for kvm ptp.
Date: Tue, 23 Jun 2020 10:23:19 +0000 [thread overview]
Message-ID: <HE1PR0802MB2555318C0E7BCC653BC1D4F2F4940@HE1PR0802MB2555.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <f331dc59-5642-33b0-9a37-553b7f536afe@arm.com>
Hi steven,
> -----Original Message-----
> From: Steven Price <steven.price@arm.com>
> Sent: Monday, June 22, 2020 5:51 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>; 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>
> 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>; 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 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
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
next prev parent reply other threads:[~2020-06-23 10:23 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-19 9:30 [RFC PATCH v13 0/9] Enable ptp_kvm for arm64 Jianyong Wu
2020-06-19 9:30 ` [RFC PATCH v13 1/9] arm64: Probe for the presence of KVM hypervisor services during boot Jianyong Wu
2020-06-19 9:30 ` [RFC PATCH v13 2/9] arm/arm64: KVM: Advertise KVM UID to guests via SMCCC Jianyong Wu
2020-06-19 9:30 ` [PATCH v13 3/9] smccc: Export smccc conduit get helper Jianyong Wu
2020-06-19 9:30 ` [RFC PATCH v13 4/9] ptp: Reorganize ptp_kvm module to make it arch-independent Jianyong Wu
2020-06-19 9:30 ` [RFC PATCH v13 5/9] time: Add mechanism to recognize clocksource in time_get_snapshot Jianyong Wu
2020-06-19 9:30 ` [RFC PATCH v13 6/9] clocksource: Add clocksource id for arm arch counter Jianyong Wu
2020-06-19 9:30 ` [RFC PATCH v13 7/9] arm64/kvm: Add hypercall service for kvm ptp Jianyong Wu
2020-06-19 10:44 ` Steven Price
2020-06-22 2:25 ` Jianyong Wu
2020-06-22 9:51 ` Steven Price
2020-06-23 10:23 ` Jianyong Wu [this message]
2020-06-19 9:30 ` [RFC PATCH v13 8/9] ptp: arm64: Enable ptp_kvm for arm64 Jianyong Wu
2020-06-19 9:30 ` [RFC PATCH v13 9/9] arm64: Add kvm capability check extension for ptp_kvm Jianyong Wu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=HE1PR0802MB2555318C0E7BCC653BC1D4F2F4940@HE1PR0802MB2555.eurprd08.prod.outlook.com \
--to=jianyong.wu@arm.com \
--cc=Justin.He@arm.com \
--cc=Mark.Rutland@arm.com \
--cc=Steven.Price@arm.com \
--cc=Suzuki.Poulose@arm.com \
--cc=Wei.Chen@arm.com \
--cc=john.stultz@linaro.org \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maz@kernel.org \
--cc=nd@arm.com \
--cc=netdev@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=richardcochran@gmail.com \
--cc=sean.j.christopherson@intel.com \
--cc=tglx@linutronix.de \
--cc=will@kernel.org \
--cc=yangbo.lu@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).