On Fri, Apr 05, 2019 at 12:39:37PM +0200, Andrew Jones wrote: > On Fri, Apr 05, 2019 at 10:36:17AM +0100, Dave Martin wrote: > > On Thu, Apr 04, 2019 at 11:09:21PM +0200, Andrew Jones wrote: > > > On Fri, Mar 29, 2019 at 01:00:52PM +0000, Dave Martin wrote: > > > > This patch adds sections to the KVM API documentation describing > > > > the extensions for supporting the Scalable Vector Extension (SVE) > > > > in guests. > > > > > > > > Signed-off-by: Dave Martin <dave.mar...@arm.com> > > > > > > > > --- > > > > > > > > Changes since v5: > > > > > > > > * Document KVM_ARM_VCPU_FINALIZE and its interactions with SVE. > > > > --- > > > > Documentation/virtual/kvm/api.txt | 132 > > > > +++++++++++++++++++++++++++++++++++++- > > > > 1 file changed, 129 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/Documentation/virtual/kvm/api.txt > > > > b/Documentation/virtual/kvm/api.txt > > > > index cd920dd..68509de 100644 > > > > --- a/Documentation/virtual/kvm/api.txt > > > > +++ b/Documentation/virtual/kvm/api.txt > > > > @@ -1873,6 +1873,7 @@ Parameters: struct kvm_one_reg (in) > > > > Returns: 0 on success, negative value on failure > > > > Errors: > > > > ENOENT: no such register > > > > + EPERM: register access forbidden for architecture-dependent > > > > reasons > > > > EINVAL: other errors, such as bad size encoding for a known > > > > register > > > > > > > > struct kvm_one_reg { > > > > @@ -2127,13 +2128,20 @@ Specifically: > > > > 0x6030 0000 0010 004c SPSR_UND 64 spsr[KVM_SPSR_UND] > > > > 0x6030 0000 0010 004e SPSR_IRQ 64 spsr[KVM_SPSR_IRQ] > > > > 0x6060 0000 0010 0050 SPSR_FIQ 64 spsr[KVM_SPSR_FIQ] > > > > - 0x6040 0000 0010 0054 V0 128 fp_regs.vregs[0] > > > > - 0x6040 0000 0010 0058 V1 128 fp_regs.vregs[1] > > > > + 0x6040 0000 0010 0054 V0 128 fp_regs.vregs[0] (*) > > > > + 0x6040 0000 0010 0058 V1 128 fp_regs.vregs[1] (*) > > > > ... > > > > - 0x6040 0000 0010 00d0 V31 128 fp_regs.vregs[31] > > > > + 0x6040 0000 0010 00d0 V31 128 fp_regs.vregs[31] (*) > > > > 0x6020 0000 0010 00d4 FPSR 32 fp_regs.fpsr > > > > 0x6020 0000 0010 00d5 FPCR 32 fp_regs.fpcr > > > > > > > > +(*) These encodings are not accepted for SVE-enabled vcpus. See > > > > + KVM_ARM_VCPU_INIT. > > > > + > > > > + The equivalent register content can be accessed via bits [127:0] of > > > > + the corresponding SVE Zn registers instead for vcpus that have SVE > > > > + enabled (see below). > > > > + > > > > arm64 CCSIDR registers are demultiplexed by CSSELR value: > > > > 0x6020 0000 0011 00 <csselr:8> > > > > > > > > @@ -2143,6 +2151,61 @@ arm64 system registers have the following id bit > > > > patterns: > > > > arm64 firmware pseudo-registers have the following bit pattern: > > > > 0x6030 0000 0014 <regno:16> > > > > > > > > +arm64 SVE registers have the following bit patterns: > > > > + 0x6080 0000 0015 00 <n:5> <slice:5> Zn bits[2048*slice + 2047 : > > > > 2048*slice] > > > > + 0x6050 0000 0015 04 <n:4> <slice:5> Pn bits[256*slice + 255 : > > > > 256*slice] > > > > + 0x6050 0000 0015 060 <slice:5> FFR bits[256*slice + 255 : > > > > 256*slice] > > > > + 0x6060 0000 0015 ffff KVM_REG_ARM64_SVE_VLS > > > > pseudo-register > > > > + > > > > +Access to slices beyond the maximum vector length configured for the > > > > +vcpu (i.e., where 16 * slice >= max_vq (**)) will fail with ENOENT. > > > > > > I've been doing pretty well keeping track of the 16's, 128's, VL's and > > > VQ's, but this example lost me. Also, should it be >= or just > ? > > > > It should be >=. It's analogous to not being allowed to derefence an > > array index that is >= the size of the array. > > > > Also, the 16 here is not the number of bytes per quadword (as it often > > does with all things SVE), but the number of quadwords per 2048-bit > > KVM register slice. > > > > To make matters worse (**) resembles some weird C syntax. > > > > Maybe this would be less confusing written as > > > > Access to register IDs where 2048 * slice >= 128 * max_vq will fail > > with ENOENT. max_vq is the vcpu's maximum supported vector length > > in 128-bit quadwords: see (**) below. > > > > Does that help at all? > > Yes. Thanks.
OK, I'll do that. > > > > > > > > > > + > > > > +These registers are only accessible on vcpus for which SVE is enabled. > > > > +See KVM_ARM_VCPU_INIT for details. > > > > + > > > > +In addition, except for KVM_REG_ARM64_SVE_VLS, these registers are not > > > > +accessible until the vcpu's SVE configuration has been finalized > > > > +using KVM_ARM_VCPU_FINALIZE(KVM_ARM_VCPU_SVE). See KVM_ARM_VCPU_INIT > > > > +and KVM_ARM_VCPU_FINALIZE for more information about this procedure. > > > > + > > > > +KVM_REG_ARM64_SVE_VLS is a pseudo-register that allows the set of > > > > vector > > > > +lengths supported by the vcpu to be discovered and configured by > > > > +userspace. When transferred to or from user memory via KVM_GET_ONE_REG > > > > +or KVM_SET_ONE_REG, the value of this register is of type __u64[8], and > > > > +encodes the set of vector lengths as follows: > > > > + > > > > +__u64 vector_lengths[8]; > > > > + > > > > +if (vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX && > > > > + ((vector_lengths[(vq - 1) / 64] >> ((vq - 1) % 64)) & 1)) > > > > + /* Vector length vq * 16 bytes supported */ > > > > +else > > > > + /* Vector length vq * 16 bytes not supported */ > > > > + > > > > +(**) The maximum value vq for which the above condition is true is > > > > +max_vq. This is the maximum vector length available to the guest on > > > > +this vcpu, and determines which register slices are visible through > > > > +this ioctl interface. > > > > + > > > > +(See Documentation/arm64/sve.txt for an explanation of the "vq" > > > > +nomenclature.) > > > > + > > > > +KVM_REG_ARM64_SVE_VLS is only accessible after KVM_ARM_VCPU_INIT. > > > > +KVM_ARM_VCPU_INIT initialises it to the best set of vector lengths that > > > > +the host supports. > > > > + > > > > +Userspace may subsequently modify it if desired until the vcpu's SVE > > > > +configuration is finalized using > > > > KVM_ARM_VCPU_FINALIZE(KVM_ARM_VCPU_SVE). > > > > + > > > > +Apart from simply removing all vector lengths from the host set that > > > > +exceed some value, support for arbitrarily chosen sets of vector > > > > lengths > > > > +is hardware-dependent and may not be available. Attempting to > > > > configure > > > > +an invalid set of vector lengths via KVM_SET_ONE_REG will fail with > > > > +EINVAL. > > > > + > > > > +After the vcpu's SVE configuration is finalized, further attempts to > > > > +write this register will fail with EPERM. > > > > + > > > > > > > > MIPS registers are mapped using the lower 32 bits. The upper 16 of > > > > that is > > > > the register group type: > > > > @@ -2197,6 +2260,7 @@ Parameters: struct kvm_one_reg (in and out) > > > > Returns: 0 on success, negative value on failure > > > > Errors: > > > > ENOENT: no such register > > > > + EPERM: register access forbidden for architecture-dependent > > > > reasons > > > > EINVAL: other errors, such as bad size encoding for a known > > > > register > > > > > > > > This ioctl allows to receive the value of a single register implemented > > > > @@ -2690,6 +2754,33 @@ Possible features: > > > > - KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU. > > > > Depends on KVM_CAP_ARM_PMU_V3. > > > > > > > > + - KVM_ARM_VCPU_SVE: Enables SVE for the CPU (arm64 only). > > > > + Depends on KVM_CAP_ARM_SVE. > > > > + Requires KVM_ARM_VCPU_FINALIZE(KVM_ARM_VCPU_SVE): > > > > + > > > > + * After KVM_ARM_VCPU_INIT: > > > > + > > > > + - KVM_REG_ARM64_SVE_VLS may be read using > > > > KVM_GET_ONE_REG: the > > > > + initial value of this pseudo-register indicates the > > > > best set of > > > > + vector lengths possible for a vcpu on this host. > > > > + > > > > + * Before KVM_ARM_VCPU_FINALIZE(KVM_ARM_VCPU_SVE): > > > > + > > > > + - KVM_RUN and KVM_GET_REG_LIST are not available; > > > > + > > > > + - KVM_GET_ONE_REG and KVM_SET_ONE_REG cannot be used to > > > > access > > > > + the scalable archietctural SVE registers > > > > + KVM_REG_ARM64_SVE_ZREG(), KVM_REG_ARM64_SVE_PREG() or > > > > + KVM_REG_ARM64_SVE_FFR; > > > > + > > > > + - KVM_REG_ARM64_SVE_VLS may optionally be written using > > > > + KVM_SET_ONE_REG, to modify the set of vector lengths > > > > available > > > > + for the vcpu. > > > > + > > > > + * After KVM_ARM_VCPU_FINALIZE(KVM_ARM_VCPU_SVE): > > > > + > > > > + - the KVM_REG_ARM64_SVE_VLS pseudo-register is immutable, > > > > and can > > > > + no longer be written using KVM_SET_ONE_REG. > > > > > > > > 4.83 KVM_ARM_PREFERRED_TARGET > > > > > > > > @@ -3904,6 +3995,41 @@ number of valid entries in the 'entries' array, > > > > which is then filled. > > > > 'index' and 'flags' fields in 'struct kvm_cpuid_entry2' are currently > > > > reserved, > > > > userspace should not expect to get any particular value there. > > > > > > > > +4.119 KVM_ARM_VCPU_FINALIZE > > > > + > > > > +Capability: KVM_CAP_ARM_SVE > > > > > > The KVM_ARM_VCPU_FINALIZE vcpu ioctl isn't SVE specific, so it shouldn't > > > depend on the SVE cap. It should have it's own cap, or maybe it should > > > just be an KVM_ARM_VCPU_SVE_FINALIZE ioctl instead, i.e. not general. > > > > This one's a bit annoying. This would ideally read > > > > Capability: KVM_CAP_ARM_SVE, or any other capability that advertises the > > availability of a feature that requires KVM_ARM_VCPU_FINALIZE to be used. > > > > (which sounds vague). > > > > We could add a specific cap for KVM_ARM_VCPU_FINALIZE, but that seemed > > overkill. Or document KVM_ARM_VCPU_FINALIZE as unconditionally > > supported, but make the individual feature values cap-dependent. This > > works because the symptom of missing support is the same (EINVAL) > > ragardless of whether it is the specific feature or > > KVM_ARM_VCPU_FINALIZE that is unsupported. > > > > Thoughts? > > > > I like that unconditionally supported idea. OK, I'll see how to write this up in the documentation. > > > > +Architectures: arm, arm64 > > > > +Type: vcpu ioctl > > > > +Parameters: int feature (in) > > > > > > This was called 'what' in the code. > > > > Indeed it is. I wanted to avoid the implication that this paramter > > exactly maps onto the KVM_ARM_VCPU_INIT features. But "what" seemed > > a bit too vague for the documentation. > > > > Mind you, if lseek() can have a "whence" parameter, perhaps "what" is > > also acceptable. > > > > OTOH I don't mind changing the name in the code to "feature" if you > > think that's preferable. > > > > Thoughts? > > I prefer them to be the same, and I think both are fine. OK. I'll go with "feature". > > > > +Returns: 0 on success, -1 on error > > > > +Errors: > > > > + EPERM: feature not enabled, needs configuration, or already > > > > finalized > > > > + EINVAL: unknown feature > > > > + > > > > +Recognised values for feature: > > > > + arm64 KVM_ARM_VCPU_SVE > > > > + > > > > +Finalizes the configuration of the specified vcpu feature. > > > > + > > > > +The vcpu must already have been initialised, enabling the affected > > > > feature, by > > > > +means of a successful KVM_ARM_VCPU_INIT call with the appropriate flag > > > > set in > > > > +features[]. > > > > + > > > > +For affected vcpu features, this is a mandatory step that must be > > > > performed > > > > +before the vcpu is fully usable. > > > > + > > > > +Between KVM_ARM_VCPU_INIT and KVM_ARM_VCPU_FINALIZE, the feature may be > > > > +configured by use of ioctls such as KVM_SET_ONE_REG. The exact > > > > configuration > > > > +that should be performaned and how to do it are feature-dependent. > > > > + > > > > +Other calls that depend on a particular feature being finalized, such > > > > as > > > > +KVM_RUN, KVM_GET_REG_LIST, KVM_GET_ONE_REG and KVM_SET_ONE_REG, will > > > > fail with > > > > +-EPERM unless the feature has already been finalized by means of a > > > > +KVM_ARM_VCPU_FINALIZE call. > > > > + > > > > +See KVM_ARM_VCPU_INIT for details of vcpu features that require > > > > finalization > > > > +using this ioctl. > > > > + > > > > 5. The kvm_run structure > > > > ------------------------ > > > > > > > > > > I'm still not sure about EPERM vs. ENOEXEC. > > > > There is no need to distinguish the two: this is just a generic "vcpu in > > wrong state for this to work" error. I can't think of another subsystem > > that uses ENOEXEC for this meaning, but it's established in KVM. > > > > If you can't see a reason why we would need these to be distinct > > errors (?) then I'm happy for this to be ENOEXEC for all cases. > > > > I do see some value in keeping them distinct. I think it's just the use > of EPERM specifically that bothers me, but I don't have that strong of > an opinion against its use either. So I'll just shut up :) In an earlier incarnation I had EBADFD, which does kind of mean the right thing. If there's not a clear way forward, I may leave this all as-is for now (but I'll remove the explicit documentation for KVM_{GET,SET}_ONE_REG error codes to give us more flexibility about this in the future, as discussed). Any objection? Cheers ---Dave _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm