Re: [PATCH v7 27/27] KVM: arm64/sve: Document KVM API extensions for SVE

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Apr 05, 2019 at 02:00:07PM +0100, Dave Martin wrote:
> 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.Martin@xxxxxxx>
> > > > > 
> > > > > ---
> > > > > 
> > > > > 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?

Nope. Let's do that. EBADFD doesn't sound good, because we're using the FD
without trouble before and even after we attempt these ioctls. I think
EBADFD would indicate that no future ioctl (or read,write) should be
expected to work.

Thanks,
drew
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm




[Index of Archives]     [Linux KVM]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux