Re: [PATCH v7 00/27] KVM: arm64: SVE guest support
On Fri, Mar 29, 2019 at 01:00:25PM +, Dave Martin wrote: > This series implements support for allowing KVM guests to use the Arm > Scalable Vector Extension (SVE), superseding the previous v6 series [1]. [...] FYI, I'm working on a series of minor fixups based on Andrew's comments which I should get out early next week, probably Tuesday. Apart from some discussion about which error codes should be returned in certain situations, all the changes discussed so far will be non-functional. Except for adding a couple of #defines to the UAPI headers, there's no other ABI impact so far. Thanks to Andrew for his review efforts... Cheers ---Dave ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 23/27] KVM: arm64/sve: Add pseudo-register for the guest's vector lengths
On Fri, Apr 05, 2019 at 03:06:40PM +0100, Dave Martin wrote:
> On Fri, Apr 05, 2019 at 12:22:04PM +0200, Andrew Jones wrote:
> > On Fri, Apr 05, 2019 at 10:36:10AM +0100, Dave Martin wrote:
> > > On Thu, Apr 04, 2019 at 10:31:09PM +0200, Andrew Jones wrote:
> > > > On Fri, Mar 29, 2019 at 01:00:48PM +, Dave Martin wrote:
> > > > > This patch adds a new pseudo-register KVM_REG_ARM64_SVE_VLS to
> > > > > allow userspace to set and query the set of vector lengths visible
> > > > > to the guest.
> > > > >
> > > > > In the future, multiple register slices per SVE register may be
> > > > > visible through the ioctl interface. Once the set of slices has
> > > > > been determined we would not be able to allow the vector length set
> > > > > to be changed any more, in order to avoid userspace seeing
> > > > > inconsistent sets of registers. For this reason, this patch adds
> > > > > support for explicit finalization of the SVE configuration via the
> > > > > KVM_ARM_VCPU_FINALIZE ioctl.
> > > > >
> > > > > Finalization is the proper place to allocate the SVE register state
> > > > > storage in vcpu->arch.sve_state, so this patch adds that as
> > > > > appropriate. The data is freed via kvm_arch_vcpu_uninit(), which
> > > > > was previously a no-op on arm64.
> > > > >
> > > > > To simplify the logic for determining what vector lengths can be
> > > > > supported, some code is added to KVM init to work this out, in the
> > > > > kvm_arm_init_arch_resources() hook.
> > > > >
> > > > > The KVM_REG_ARM64_SVE_VLS pseudo-register is not exposed yet.
> > > > > Subsequent patches will allow SVE to be turned on for guest vcpus,
> > > > > making it visible.
> > > > >
> > > > > Signed-off-by: Dave Martin
> > > > > Reviewed-by: Julien Thierry
> > > > > Tested-by: zhang.lei
> > >
> > > [...]
> > >
> > > > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > >
> > > [...]
> > >
> > > > > +/*
> > > > > + * Finalize vcpu's maximum SVE vector length, allocating
> > > > > + * vcpu->arch.sve_state as necessary.
> > > > > + */
> > > > > +static int kvm_vcpu_finalize_sve(struct kvm_vcpu *vcpu)
> > > > > +{
> > > > > + void *buf;
> > > > > + unsigned int vl;
> > > > > +
> > > > > + vl = vcpu->arch.sve_max_vl;
> > > > > +
> > > > > + /*
> > > > > + * Resposibility for these properties is shared between
> > > > > + * kvm_arm_init_arch_resources(), kvm_vcpu_enable_sve() and
> > > > > + * set_sve_vls(). Double-check here just to be sure:
> > > > > + */
> > > > > + if (WARN_ON(!sve_vl_valid(vl) || vl > sve_max_virtualisable_vl
> > > > > ||
> > > > > + vl > SVE_VL_ARCH_MAX))
> > > > > + return -EIO;
> > > > > +
> > > > > + buf = kzalloc(SVE_SIG_REGS_SIZE(sve_vq_from_vl(vl)),
> > > > > GFP_KERNEL);
> > > > > + if (!buf)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + vcpu->arch.sve_state = buf;
> > > > > + vcpu->arch.flags |= KVM_ARM64_VCPU_SVE_FINALIZED;
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int what)
> > > > > +{
> > > > > + switch (what) {
> > > > > + case KVM_ARM_VCPU_SVE:
> > > > > + if (!vcpu_has_sve(vcpu))
> > > > > + return -EINVAL;
> > > > > +
> > > > > + if (kvm_arm_vcpu_sve_finalized(vcpu))
> > > > > + return -EPERM;
> > > > > +
> > > > > + return kvm_vcpu_finalize_sve(vcpu);
> > > >
> > > > In the next patch I see that we already have KVM_ARM64_GUEST_HAS_SVE
> > > > set in vcpu->arch.flags at this point. So if this
> > > > kvm_vcpu_finalize_sve()
> > > > call fails, shouldn't we unset KVM_ARM64_GUEST_HAS_SVE and anything
> > > > else used to indicate the vcpu has sve?
> > >
> > > If this fails, either userspace did the wrong thing, or there was some
> > > fatal error (like the ENOMEM case). Either way, the vcpu is not runnable
> > > and userspace is expected to bail out.
> > >
> > > Further, KVM_ARM_VCPU_INIT fixes the set of features requested by
> > > userspace: we shouldn't change the set of features under userspace's
> > > feet and try to limp on somehow. We have no means for userspace to find
> > > out that some features went away in the meantime...
> > >
> > > So, what would you be trying to achieve with this change?
> >
> > Being able to do additional vcpu ioctls with only partially initialized
> > sve (sve_state is still null, but we otherwise believe the vcpu has sve).
> > That sounds risky. Although I see we only set KVM_ARM64_VCPU_SVE_FINALIZED
> > when everything, like the memory allocation, succeeds, so we're probably
> > fine. Indeed having KVM_ARM64_GUEST_HAS_SVE and not
> > KVM_ARM64_VCPU_SVE_FINALIZED
> > is likely the safest vcpu state for a vcpu that failed to finalize sve.
>
> This was my rationale: every non-trivial operation that is affected by
> SVE operation checks for kvm_arm_vcpu_sve_finalized(): accessing
> KV
Re: [PATCH v7 27/27] KVM: arm64/sve: Document KVM API extensions for SVE
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 +, 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
> > > > >
> > > > > ---
> > > > >
> > > > > 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 0010 004c SPSR_UND64 spsr[KVM_SPSR_UND]
> > > > >0x6030 0010 004e SPSR_IRQ64 spsr[KVM_SPSR_IRQ]
> > > > >0x6060 0010 0050 SPSR_FIQ64 spsr[KVM_SPSR_FIQ]
> > > > > - 0x6040 0010 0054 V0 128 fp_regs.vregs[0]
> > > > > - 0x6040 0010 0058 V1 128 fp_regs.vregs[1]
> > > > > + 0x6040 0010 0054 V0 128 fp_regs.vregs[0](*)
> > > > > + 0x6040 0010 0058 V1 128 fp_regs.vregs[1](*)
> > > > > ...
> > > > > - 0x6040 0010 00d0 V31128 fp_regs.vregs[31]
> > > > > + 0x6040 0010 00d0 V31128 fp_regs.vregs[31] (*)
> > > > >0x6020 0010 00d4 FPSR32 fp_regs.fpsr
> > > > >0x6020 0010 00d5 FPCR32 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 0011 00
> > > > >
> > > > > @@ -2143,6 +2151,61 @@ arm64 system registers have the following id
> > > > > bit patterns:
> > > > > arm64 firmware pseudo-registers have the following bit pattern:
> > > > >0x6030 0014
> > > > >
> > > > > +arm64 SVE registers have the following bit patterns:
> > > > > + 0x6080 0015 00 Zn bits[2048*slice + 2047 :
> > > > > 2048*slice]
> > > > > + 0x6050 0015 04 Pn bits[256*slice + 255 :
> > > > > 256*slice]
> > > > > + 0x6050 0015 060 FFR bits[256*slice + 255 :
> > > > > 256*slice]
> > > > > + 0x6060 0015 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_
Re: [PATCH v7 23/27] KVM: arm64/sve: Add pseudo-register for the guest's vector lengths
On Fri, Apr 05, 2019 at 01:54:13PM +0100, Dave Martin wrote:
> On Fri, Apr 05, 2019 at 12:14:51PM +0200, Andrew Jones wrote:
> > On Fri, Apr 05, 2019 at 10:36:03AM +0100, Dave Martin wrote:
> > > On Thu, Apr 04, 2019 at 10:18:54PM +0200, Andrew Jones wrote:
> > > > On Fri, Mar 29, 2019 at 01:00:48PM +, Dave Martin wrote:
> > > > > This patch adds a new pseudo-register KVM_REG_ARM64_SVE_VLS to
> > > > > allow userspace to set and query the set of vector lengths visible
> > > > > to the guest.
> > > > >
> > > > > In the future, multiple register slices per SVE register may be
> > > > > visible through the ioctl interface. Once the set of slices has
> > > > > been determined we would not be able to allow the vector length set
> > > > > to be changed any more, in order to avoid userspace seeing
> > > > > inconsistent sets of registers. For this reason, this patch adds
> > > > > support for explicit finalization of the SVE configuration via the
> > > > > KVM_ARM_VCPU_FINALIZE ioctl.
> > > > >
> > > > > Finalization is the proper place to allocate the SVE register state
> > > > > storage in vcpu->arch.sve_state, so this patch adds that as
> > > > > appropriate. The data is freed via kvm_arch_vcpu_uninit(), which
> > > > > was previously a no-op on arm64.
> > > > >
> > > > > To simplify the logic for determining what vector lengths can be
> > > > > supported, some code is added to KVM init to work this out, in the
> > > > > kvm_arm_init_arch_resources() hook.
> > > > >
> > > > > The KVM_REG_ARM64_SVE_VLS pseudo-register is not exposed yet.
> > > > > Subsequent patches will allow SVE to be turned on for guest vcpus,
> > > > > making it visible.
> > > > >
> > > > > Signed-off-by: Dave Martin
> > > > > Reviewed-by: Julien Thierry
> > > > > Tested-by: zhang.lei
> > > > >
> > > > > ---
> > >
> > > [...]
> > >
> > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > > > > index 2aa80a5..086ab05 100644
> > > > > --- a/arch/arm64/kvm/guest.c
> > > > > +++ b/arch/arm64/kvm/guest.c
> > > > > @@ -206,6 +206,73 @@ static int set_core_reg(struct kvm_vcpu *vcpu,
> > > > > const struct kvm_one_reg *reg)
> > > > > return err;
> > > > > }
> > > > >
> > > > > +#define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
> > > > > +#define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
> > > > > +
> > > > > +static bool vq_present(
> > > > > + const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN +
> > > > > 1, 64)],
> > > > > + unsigned int vq)
> > > > > +{
> > > > > + return (*vqs)[vq_word(vq)] & vq_mask(vq);
> > > > > +}
> > > > > +
> > > > > +static int get_sve_vls(struct kvm_vcpu *vcpu, const struct
> > > > > kvm_one_reg *reg)
> > > > > +{
> > > > > + unsigned int max_vq, vq;
> > > > > + u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > > > +
> > > > > + if (WARN_ON(!sve_vl_valid(vcpu->arch.sve_max_vl)))
> > > > > + return -EINVAL;
> > > > > +
> > > > > + memset(vqs, 0, sizeof(vqs));
> > > > > +
> > > > > + max_vq = sve_vq_from_vl(vcpu->arch.sve_max_vl);
> > > > > + for (vq = SVE_VQ_MIN; vq <= max_vq; ++vq)
> > > > > + if (sve_vq_available(vq))
> > > > > + vqs[vq_word(vq)] |= vq_mask(vq);
> > > > > +
> > > > > + if (copy_to_user((void __user *)reg->addr, vqs, sizeof(vqs)))
> > > > > + return -EFAULT;
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static int set_sve_vls(struct kvm_vcpu *vcpu, const struct
> > > > > kvm_one_reg *reg)
> > > > > +{
> > > > > + unsigned int max_vq, vq;
> > > > > + u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > >
> > > > There are three of these long 'DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN +
> > > > 1, 64)'.
> > > > A macro is probably warranted.
> > >
> > > Fair point. These are a bit cumbersome. How about:
> > >
> > > #define SVE_VLS_WORDS DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)
> > >
> > > Annoyingly, this is virtually identical to a Linux bitmap: the base type
> > > is u64 instead of unsigned long; otherwise there's no intentional
> > > difference.
> >
> > Yeah, I noticed it was a reinvented bitmap, but I liked that it was,
> > because, for the userspace api, removing the bitmap abstractions
> > ensures we know what is what and where exactly it is.
>
> Agreed. I thought of using the bitmap API inside the kernel, but even
> if this is arm64-specific code, it made me uneasy: the compiler doesn't
> necessarily think that u64 and unsigned long are the same type even if
> they're both 64-bit, so there's the potential for weird optimisations to
> happen.
>
> > > (Aside: do you know why the KVM API insists on everything being u64?
> > > This makes sense for non-native types (like guest registers), but not
> > > for native things like host userspace addresses etc.)
> >
> > Would that work when the kernel is 64-bit and the userspace is 32? I
> > personally like the interfaces being explicit
Re: [PATCH kvmtool 08/16] arm/pci: Do not use first PCI IO space bytes for devices
On Thu, 7 Mar 2019 08:36:09 + Julien Thierry wrote: Hi, > Linux has this convention that the lower 0x1000 bytes of the IO space > should not be used. (cf PCIBIOS_MIN_IO). > > Just allocate those bytes to prevent future allocation assigning it to > devices. > > Signed-off-by: Julien Thierry > --- > arm/pci.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arm/pci.c b/arm/pci.c > index 83238ca..559e0cf 100644 > --- a/arm/pci.c > +++ b/arm/pci.c > @@ -37,6 +37,9 @@ void pci__arm_init(struct kvm *kvm) > > /* Make PCI port allocation start at a properly aligned address */ > pci_get_io_space_block(align_pad); > + > + /* Convention, don't allocate first 0x1000 bytes of PCI IO */ > + pci_get_io_space_block(0x1000); Is this the same problem with mixing up I/O and MMIO space as in the other patch? io_space means MMIO, right? Cheers, Andre. > } > > void pci__generate_fdt_nodes(void *fdt) ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 23/27] KVM: arm64/sve: Add pseudo-register for the guest's vector lengths
On Fri, Apr 05, 2019 at 12:22:04PM +0200, Andrew Jones wrote:
> On Fri, Apr 05, 2019 at 10:36:10AM +0100, Dave Martin wrote:
> > On Thu, Apr 04, 2019 at 10:31:09PM +0200, Andrew Jones wrote:
> > > On Fri, Mar 29, 2019 at 01:00:48PM +, Dave Martin wrote:
> > > > This patch adds a new pseudo-register KVM_REG_ARM64_SVE_VLS to
> > > > allow userspace to set and query the set of vector lengths visible
> > > > to the guest.
> > > >
> > > > In the future, multiple register slices per SVE register may be
> > > > visible through the ioctl interface. Once the set of slices has
> > > > been determined we would not be able to allow the vector length set
> > > > to be changed any more, in order to avoid userspace seeing
> > > > inconsistent sets of registers. For this reason, this patch adds
> > > > support for explicit finalization of the SVE configuration via the
> > > > KVM_ARM_VCPU_FINALIZE ioctl.
> > > >
> > > > Finalization is the proper place to allocate the SVE register state
> > > > storage in vcpu->arch.sve_state, so this patch adds that as
> > > > appropriate. The data is freed via kvm_arch_vcpu_uninit(), which
> > > > was previously a no-op on arm64.
> > > >
> > > > To simplify the logic for determining what vector lengths can be
> > > > supported, some code is added to KVM init to work this out, in the
> > > > kvm_arm_init_arch_resources() hook.
> > > >
> > > > The KVM_REG_ARM64_SVE_VLS pseudo-register is not exposed yet.
> > > > Subsequent patches will allow SVE to be turned on for guest vcpus,
> > > > making it visible.
> > > >
> > > > Signed-off-by: Dave Martin
> > > > Reviewed-by: Julien Thierry
> > > > Tested-by: zhang.lei
> >
> > [...]
> >
> > > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> >
> > [...]
> >
> > > > +/*
> > > > + * Finalize vcpu's maximum SVE vector length, allocating
> > > > + * vcpu->arch.sve_state as necessary.
> > > > + */
> > > > +static int kvm_vcpu_finalize_sve(struct kvm_vcpu *vcpu)
> > > > +{
> > > > + void *buf;
> > > > + unsigned int vl;
> > > > +
> > > > + vl = vcpu->arch.sve_max_vl;
> > > > +
> > > > + /*
> > > > +* Resposibility for these properties is shared between
> > > > +* kvm_arm_init_arch_resources(), kvm_vcpu_enable_sve() and
> > > > +* set_sve_vls(). Double-check here just to be sure:
> > > > +*/
> > > > + if (WARN_ON(!sve_vl_valid(vl) || vl > sve_max_virtualisable_vl
> > > > ||
> > > > + vl > SVE_VL_ARCH_MAX))
> > > > + return -EIO;
> > > > +
> > > > + buf = kzalloc(SVE_SIG_REGS_SIZE(sve_vq_from_vl(vl)),
> > > > GFP_KERNEL);
> > > > + if (!buf)
> > > > + return -ENOMEM;
> > > > +
> > > > + vcpu->arch.sve_state = buf;
> > > > + vcpu->arch.flags |= KVM_ARM64_VCPU_SVE_FINALIZED;
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int what)
> > > > +{
> > > > + switch (what) {
> > > > + case KVM_ARM_VCPU_SVE:
> > > > + if (!vcpu_has_sve(vcpu))
> > > > + return -EINVAL;
> > > > +
> > > > + if (kvm_arm_vcpu_sve_finalized(vcpu))
> > > > + return -EPERM;
> > > > +
> > > > + return kvm_vcpu_finalize_sve(vcpu);
> > >
> > > In the next patch I see that we already have KVM_ARM64_GUEST_HAS_SVE
> > > set in vcpu->arch.flags at this point. So if this kvm_vcpu_finalize_sve()
> > > call fails, shouldn't we unset KVM_ARM64_GUEST_HAS_SVE and anything
> > > else used to indicate the vcpu has sve?
> >
> > If this fails, either userspace did the wrong thing, or there was some
> > fatal error (like the ENOMEM case). Either way, the vcpu is not runnable
> > and userspace is expected to bail out.
> >
> > Further, KVM_ARM_VCPU_INIT fixes the set of features requested by
> > userspace: we shouldn't change the set of features under userspace's
> > feet and try to limp on somehow. We have no means for userspace to find
> > out that some features went away in the meantime...
> >
> > So, what would you be trying to achieve with this change?
>
> Being able to do additional vcpu ioctls with only partially initialized
> sve (sve_state is still null, but we otherwise believe the vcpu has sve).
> That sounds risky. Although I see we only set KVM_ARM64_VCPU_SVE_FINALIZED
> when everything, like the memory allocation, succeeds, so we're probably
> fine. Indeed having KVM_ARM64_GUEST_HAS_SVE and not
> KVM_ARM64_VCPU_SVE_FINALIZED
> is likely the safest vcpu state for a vcpu that failed to finalize sve.
This was my rationale: every non-trivial operation that is affected by
SVE operation checks for kvm_arm_vcpu_sve_finalized(): accessing
KVM_REG_ARM64_SVE_VLS should be the only exception.
Of course, people might forget that in new code, but I think all the
major ABI code paths that are likely to exist for SVE are already there
now.
Does this sound OK, or do you still have c
Re: [PATCH v7 27/27] KVM: arm64/sve: Document KVM API extensions for SVE
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 +, 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
> > > >
> > > > ---
> > > >
> > > > 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 0010 004c SPSR_UND64 spsr[KVM_SPSR_UND]
> > > >0x6030 0010 004e SPSR_IRQ64 spsr[KVM_SPSR_IRQ]
> > > >0x6060 0010 0050 SPSR_FIQ64 spsr[KVM_SPSR_FIQ]
> > > > - 0x6040 0010 0054 V0 128 fp_regs.vregs[0]
> > > > - 0x6040 0010 0058 V1 128 fp_regs.vregs[1]
> > > > + 0x6040 0010 0054 V0 128 fp_regs.vregs[0](*)
> > > > + 0x6040 0010 0058 V1 128 fp_regs.vregs[1](*)
> > > > ...
> > > > - 0x6040 0010 00d0 V31128 fp_regs.vregs[31]
> > > > + 0x6040 0010 00d0 V31128 fp_regs.vregs[31] (*)
> > > >0x6020 0010 00d4 FPSR32 fp_regs.fpsr
> > > >0x6020 0010 00d5 FPCR32 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 0011 00
> > > >
> > > > @@ -2143,6 +2151,61 @@ arm64 system registers have the following id bit
> > > > patterns:
> > > > arm64 firmware pseudo-registers have the following bit pattern:
> > > >0x6030 0014
> > > >
> > > > +arm64 SVE registers have the following bit patterns:
> > > > + 0x6080 0015 00 Zn bits[2048*slice + 2047 :
> > > > 2048*slice]
> > > > + 0x6050 0015 04 Pn bits[256*slice + 255 :
> > > > 256*slice]
> > > > + 0x6050 0015 060 FFR bits[256*slice + 255 :
> > > > 256*slice]
> > > > + 0x6060 0015 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 f
Re: [PATCH v7 23/27] KVM: arm64/sve: Add pseudo-register for the guest's vector lengths
On Fri, Apr 05, 2019 at 12:14:51PM +0200, Andrew Jones wrote:
> On Fri, Apr 05, 2019 at 10:36:03AM +0100, Dave Martin wrote:
> > On Thu, Apr 04, 2019 at 10:18:54PM +0200, Andrew Jones wrote:
> > > On Fri, Mar 29, 2019 at 01:00:48PM +, Dave Martin wrote:
> > > > This patch adds a new pseudo-register KVM_REG_ARM64_SVE_VLS to
> > > > allow userspace to set and query the set of vector lengths visible
> > > > to the guest.
> > > >
> > > > In the future, multiple register slices per SVE register may be
> > > > visible through the ioctl interface. Once the set of slices has
> > > > been determined we would not be able to allow the vector length set
> > > > to be changed any more, in order to avoid userspace seeing
> > > > inconsistent sets of registers. For this reason, this patch adds
> > > > support for explicit finalization of the SVE configuration via the
> > > > KVM_ARM_VCPU_FINALIZE ioctl.
> > > >
> > > > Finalization is the proper place to allocate the SVE register state
> > > > storage in vcpu->arch.sve_state, so this patch adds that as
> > > > appropriate. The data is freed via kvm_arch_vcpu_uninit(), which
> > > > was previously a no-op on arm64.
> > > >
> > > > To simplify the logic for determining what vector lengths can be
> > > > supported, some code is added to KVM init to work this out, in the
> > > > kvm_arm_init_arch_resources() hook.
> > > >
> > > > The KVM_REG_ARM64_SVE_VLS pseudo-register is not exposed yet.
> > > > Subsequent patches will allow SVE to be turned on for guest vcpus,
> > > > making it visible.
> > > >
> > > > Signed-off-by: Dave Martin
> > > > Reviewed-by: Julien Thierry
> > > > Tested-by: zhang.lei
> > > >
> > > > ---
> >
> > [...]
> >
> > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > > > index 2aa80a5..086ab05 100644
> > > > --- a/arch/arm64/kvm/guest.c
> > > > +++ b/arch/arm64/kvm/guest.c
> > > > @@ -206,6 +206,73 @@ static int set_core_reg(struct kvm_vcpu *vcpu,
> > > > const struct kvm_one_reg *reg)
> > > > return err;
> > > > }
> > > >
> > > > +#define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
> > > > +#define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
> > > > +
> > > > +static bool vq_present(
> > > > + const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN +
> > > > 1, 64)],
> > > > + unsigned int vq)
> > > > +{
> > > > + return (*vqs)[vq_word(vq)] & vq_mask(vq);
> > > > +}
> > > > +
> > > > +static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg
> > > > *reg)
> > > > +{
> > > > + unsigned int max_vq, vq;
> > > > + u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > > +
> > > > + if (WARN_ON(!sve_vl_valid(vcpu->arch.sve_max_vl)))
> > > > + return -EINVAL;
> > > > +
> > > > + memset(vqs, 0, sizeof(vqs));
> > > > +
> > > > + max_vq = sve_vq_from_vl(vcpu->arch.sve_max_vl);
> > > > + for (vq = SVE_VQ_MIN; vq <= max_vq; ++vq)
> > > > + if (sve_vq_available(vq))
> > > > + vqs[vq_word(vq)] |= vq_mask(vq);
> > > > +
> > > > + if (copy_to_user((void __user *)reg->addr, vqs, sizeof(vqs)))
> > > > + return -EFAULT;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg
> > > > *reg)
> > > > +{
> > > > + unsigned int max_vq, vq;
> > > > + u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > >
> > > There are three of these long 'DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1,
> > > 64)'.
> > > A macro is probably warranted.
> >
> > Fair point. These are a bit cumbersome. How about:
> >
> > #define SVE_VLS_WORDS DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)
> >
> > Annoyingly, this is virtually identical to a Linux bitmap: the base type
> > is u64 instead of unsigned long; otherwise there's no intentional
> > difference.
>
> Yeah, I noticed it was a reinvented bitmap, but I liked that it was,
> because, for the userspace api, removing the bitmap abstractions
> ensures we know what is what and where exactly it is.
Agreed. I thought of using the bitmap API inside the kernel, but even
if this is arm64-specific code, it made me uneasy: the compiler doesn't
necessarily think that u64 and unsigned long are the same type even if
they're both 64-bit, so there's the potential for weird optimisations to
happen.
> > (Aside: do you know why the KVM API insists on everything being u64?
> > This makes sense for non-native types (like guest registers), but not
> > for native things like host userspace addresses etc.)
>
> Would that work when the kernel is 64-bit and the userspace is 32? I
> personally like the interfaces being explicit with type size, as it
> removes the brain burden of translating long and int when moving arch
> to arch and kernel to userspace.
Ah yes, the joys of compat. And ioctl. Ignore me.
> > > > +
> > > > + if (kvm_arm_vcpu_sve_finalized(vcpu))
> >
Re: [PATCH v12 8/8] arm64: docs: document perf event attributes
On Thu, Apr 04, 2019 at 08:33:51PM +0100, Andrew Murray wrote: > On Thu, Apr 04, 2019 at 05:21:28PM +0100, Will Deacon wrote: > > On Thu, Mar 28, 2019 at 10:37:31AM +, Andrew Murray wrote: > > > +exclude_kernel > > > +-- > > > + > > > +This attribute excludes the kernel. > > > + > > > +The kernel runs at EL2 with VHE and EL1 without. Guest kernels always run > > > +at EL1. > > > + > > > +This attribute will exclude EL1 and additionally EL2 on a VHE system. > > > > I find this last sentence a bit confusing, because it can be read to imply > > that if you don't set exclude_kernel and you're in a guest on a VHE system, > > then you can profile EL2. > > Yes this could be misleading. > > However from the perspective of the guest, when exclude_kernel is not set we > do indeed allow the guest to program it's PMU with ARMV8_PMU_INCLUDE_EL2 - and > thus the statement above is correct in terms of what the kernel believes it is > doing. > > I think these statements are less confusing if we treat the exception levels > as those 'detected' by the running context (e.g. consider the impact of nested > virt here) - and we if ignore what the hypervisor (KVM) does outside (e.g. > stops counting upon switching between guest/host, translating PMU filters in > kvm_pmu_set_counter_event_type etc, etc). This then makes this document useful > for those wishing to change this logic (which is the intent) rather than those > trying to understand how we filter for EL levels as seen bare-metal. > > With regards to the example you gave (exclude_kernel, EL2) - yes we want the > kernel to believe it can count EL2 - because one day we may want to update > KVM to allow the guest to count it's hypervisor overhead (e.g. host kernel > time associated with the guest). If we were to support this in the future, then exclude_hv will suddenly start meaning something in a guest, so this could be considered to be an ABI break. > I could write some preface that describes this outlook. Alternatively I could > just spell out what happens on a guest, e.g. > > "For the host this attribute will exclude EL1 and additionally EL2 on a VHE > system. > > For the guest this attribute will exclude EL1." > > Though I'm less comfortable with this, as the last statement "For the guest > this > attribute will exclude EL1." describes the product of both > kvm_pmu_set_counter_event_type and armv8pmu_set_event_filter which is > confusing > to work out and also makes an assumption that we don't have nested virt (true > for now at least) and also reasons about bare-metal EL levels which probably > aren't that useful for someone changing this logic or understanding what the > flags do for there performance analysis. > > Do you have a preference for how this is improved? I think you should be explicit about what is counted. If we don't count EL2 when profiling in a guest (regardless of the exclude_*) flags, then we should say that. By not documenting this we don't actually buy ourselves room to change things in future, we should have an emergent behaviour which isn't covered by our docs. Will ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 21/27] KVM: arm/arm64: Add hook for arch-specific KVM initialisation
On Fri, Apr 05, 2019 at 12:40:57PM +0200, Andrew Jones wrote:
> On Fri, Apr 05, 2019 at 10:36:24AM +0100, Dave Martin wrote:
> > On Thu, Apr 04, 2019 at 06:33:08PM +0200, Andrew Jones wrote:
> > > On Thu, Apr 04, 2019 at 03:53:55PM +0100, Dave Martin wrote:
> > > > On Thu, Apr 04, 2019 at 04:25:28PM +0200, Andrew Jones wrote:
> > > > > On Fri, Mar 29, 2019 at 01:00:46PM +, Dave Martin wrote:
> >
> > [...]
> >
> > > > > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > > > > index 99c3738..c69e137 100644
> > > > > > --- a/virt/kvm/arm/arm.c
> > > > > > +++ b/virt/kvm/arm/arm.c
> > > > > > @@ -1664,6 +1664,10 @@ int kvm_arch_init(void *opaque)
> > > > > > if (err)
> > > > > > return err;
> > > > > >
> > > > > > + err = kvm_arm_init_arch_resources();
> > > > > > + if (err)
> > > > > > + return err;
> > > > > > +
> > > > > > if (!in_hyp_mode) {
> > > > > > err = init_hyp_mode();
> > > > > > if (err)
> > > > > > --
> > > > > > 2.1.4
> > > > > >
> > > > >
> > > > > It's not clear to me from the commit message why
> > > > > init_common_resources()
> > > > > won't work for this. Maybe it'll be more clear as I continue the
> > > > > review.
> > > >
> > > > init_common_resources() is for stuff common to arm and arm64.
> > >
> > > Well currently init_common_resources() only calls kvm_set_ipa_limit(),
> > > which isn't implemented for arm. So if there was a plan to only use
> > > that function for init that actually does something on both, it doesn't.
> >
> > Hmmm, perhaps I was wishfully interpreting the word "common" to mean
> > what I would like it to mean...
> >
> > > > kvm_arm_init_arch_resources() is intended for stuff specific to the
> > > > particular arch backend.
> > >
> > > I'm not sure we need that yet. We just need an arm setup sve stub like
> > > arm's kvm_set_ipa_limit() stub. OTOH, if we have to keep adding stubs
> > > to arm we should probably have something like
> > > kvm_arm_init_arch_resources() instead, and kvm_set_ipa_limit() should
> > > be moved inside the arm64 one and the arm ipa limit stub can go. Then
> > > since init_common_resources() would no longer be used, it could just
> > > be deleted.
> >
> > OK, for simplicity I may call the sve setup directly as you suggest, and
> > make an arm stub for it: that feels a bit weird, but there is precedent.
> >
> > If we end up accumulating a lot of these, we can revisit it and maybe
> > invent something like kvm_arm_init_arch_resources() at that point.
> >
> > Does that sound reasonable?
>
> Yup. Sounds good to me.
OK, can do
Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 20/27] arm64/sve: In-kernel vector length availability query interface
On Fri, Apr 05, 2019 at 11:54:07AM +0200, Andrew Jones wrote:
> On Fri, Apr 05, 2019 at 10:35:55AM +0100, Dave Martin wrote:
> > On Thu, Apr 04, 2019 at 04:20:34PM +0200, Andrew Jones wrote:
> > > On Fri, Mar 29, 2019 at 01:00:45PM +, Dave Martin wrote:
> > > > KVM will need to interrogate the set of SVE vector lengths
> > > > available on the system.
> > > >
> > > > This patch exposes the relevant bits to the kernel, along with a
> > > > sve_vq_available() helper to check whether a particular vector
> > > > length is supported.
> > > >
> > > > __vq_to_bit() and __bit_to_vq() are not intended for use outside
> > > > these functions: now that these are exposed outside fpsimd.c, they
> > > > are prefixed with __ in order to provide an extra hint that they
> > > > are not intended for general-purpose use.
> > > >
> > > > Signed-off-by: Dave Martin
> > > > Reviewed-by: Alex Bennée
> > > > Tested-by: zhang.lei
> > > > ---
> > > > arch/arm64/include/asm/fpsimd.h | 29 +
> > > > arch/arm64/kernel/fpsimd.c | 35
> > > > ---
> > > > 2 files changed, 37 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/fpsimd.h
> > > > b/arch/arm64/include/asm/fpsimd.h
[...]
> > > > +/* Set of available vector lengths, as vq_to_bit(vq): */
> > >
> > > s/as/for use with/ ?
> >
> > Not exactly. Does the following work for you:
> >
> > /*
> > * Set of available vector lengths
> > * Vector length vq is encoded as bit __vq_to_bit(vq):
> > */
>
> Yes. That reads much better.
OK
> > > s/vq_to_bit/__vq_to_bit/
> >
> > Ack: that got renamed when I moved it to fpsimd.h, bit I clearly didn't
> > update the comment as I pasted it across.
> >
> > >
> > > > +extern __ro_after_init DECLARE_BITMAP(sve_vq_map, SVE_VQ_MAX);
> > > > +
> > > > +/*
> > > > + * Helpers to translate bit indices in sve_vq_map to VQ values (and
> > > > + * vice versa). This allows find_next_bit() to be used to find the
> > > > + * _maximum_ VQ not exceeding a certain value.
> > > > + */
> > > > +static inline unsigned int __vq_to_bit(unsigned int vq)
> > > > +{
> > >
> > > Why not have the same WARN_ON and clamping here as we do
> > > in __bit_to_vq. Here a vq > SVE_VQ_MAX will wrap around
> > > to a super high bit.
> > >
> > > > + return SVE_VQ_MAX - vq;
> > > > +}
> > > > +
> > > > +static inline unsigned int __bit_to_vq(unsigned int bit)
> > > > +{
> > > > + if (WARN_ON(bit >= SVE_VQ_MAX))
> > > > + bit = SVE_VQ_MAX - 1;
> > > > +
> > > > + return SVE_VQ_MAX - bit;
> > > > +}
> > > > +
> > > > +/* Ensure vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX before calling this
> > > > function */
> > >
> > > Are we avoiding putting these tests and WARN_ONs in this function to
> > > keep it fast?
> >
> > These are intended as backend for use only by fpsimd.c and this header,
> > so peppering them with WARN_ON() felt excessive. I don't expect a lot
> > of new calls to these (or any, probably).
> >
> > I don't recall why I kept the WARN_ON() just in __bit_to_vq(), except
> > that the way that gets called is a bit more complex in some places.
> >
> > Are you happy to replace these with comments? e.g.:
> >
> > /* Only valid when vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX */
> > __vq_to_bit()
> >
> > /* Only valid when bit < SVE_VQ_MAX */
> > __bit_to_vq()
> >
> >
> > OTOH, these are not used on fast paths, so maybe having both as
> > WARN_ON() would be better. Part of the problem is knowing what to clamp
> > to: these are generally used in conjunction with looping or bitmap find
> > operations, so the caller may be making assumptions about the return
> > value that may wrong when the value is clamped.
> >
> > Alternatively, these could be BUG() -- but that seems heavy.
> >
> > What do you think?
>
> I like the idea of having WARN_ON's to enforce the constraints. I
> wouldn't be completely opposed to not having anything other than
> the comments, though, as there is a limit to how defensive we should
> be. I'll abstain from this vote.
I'll have a think about whether there's anything non-toxic that we can
return in the error cases. If not, I may demote these to comments:
returning an actual error code for this sort of things feels like a
step too far.
Otherwise we can have WARNs.
Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 19/27] KVM: arm64: Enumerate SVE register indices for KVM_GET_REG_LIST
On Fri, Apr 05, 2019 at 11:45:56AM +0200, Andrew Jones wrote:
> On Fri, Apr 05, 2019 at 10:35:45AM +0100, Dave Martin wrote:
> > On Thu, Apr 04, 2019 at 04:08:32PM +0200, Andrew Jones wrote:
> > > On Fri, Mar 29, 2019 at 01:00:44PM +, Dave Martin wrote:
> > > > This patch includes the SVE register IDs in the list returned by
> > > > KVM_GET_REG_LIST, as appropriate.
> > > >
> > > > On a non-SVE-enabled vcpu, no new IDs are added.
> > > >
> > > > On an SVE-enabled vcpu, IDs for the FPSIMD V-registers are removed
> > > > from the list, since userspace is required to access the Z-
> > > > registers instead in order to access the V-register content. For
> > > > the variably-sized SVE registers, the appropriate set of slice IDs
> > > > are enumerated, depending on the maximum vector length for the
> > > > vcpu.
> > > >
> > > > As it currently stands, the SVE architecture never requires more
> > > > than one slice to exist per register, so this patch adds no
> > > > explicit support for enumerating multiple slices. The code can be
> > > > extended straightforwardly to support this in the future, if
> > > > needed.
> > > >
> > > > Signed-off-by: Dave Martin
> > > > Reviewed-by: Julien Thierry
> > > > Tested-by: zhang.lei
> > > >
> > > > ---
[...]
> > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
[...]
> > > > +static int copy_sve_reg_indices(const struct kvm_vcpu *vcpu,
> > > > + u64 __user *uindices)
> > > > +{
> > > > + /* Only the first slice ever exists, for now */
[...]
> > > > + const unsigned int slices = vcpu_sve_slices(vcpu);
> > > > + u64 reg;
> > > > + unsigned int i, n;
> > > > + int num_regs = 0;
> > > > +
> > > > + if (!vcpu_has_sve(vcpu))
> > > > + return 0;
> > > > +
> > > > + for (i = 0; i < slices; i++) {
> > > > + for (n = 0; n < SVE_NUM_ZREGS; n++) {
> > > > + reg = KVM_REG_ARM64_SVE_ZREG(n, i);
> > > > + if (put_user(reg, uindices++))
> > > > + return -EFAULT;
> > > > +
> > > > + num_regs++;
> > > > + }
> > > > +
> > > > + for (n = 0; n < SVE_NUM_PREGS; n++) {
> > > > + reg = KVM_REG_ARM64_SVE_PREG(n, i);
> > > > + if (put_user(reg, uindices++))
> > > > + return -EFAULT;
> > > > +
> > > > + num_regs++;
> > > > + }
> > > > +
> > > > + reg = KVM_REG_ARM64_SVE_FFR(i);
> > > > + if (put_user(reg, uindices++))
> > > > + return -EFAULT;
> > > > +
> > > > + num_regs++;
> > > > + }
> > >
> > > nit: the extra blank lines above the num_regs++'s give the code an odd
> > > look (to me)
> >
> > There's no guaranteed fall-through onto the increments: the blank line
> > was there to highlight the fact that we may jump out using a return
> > instead.
> >
> > But I'm happy enough to change it if you have a strong preference or
> > you feel the code is equally clear without.
>
> It's just a nit, so I don't have a strong preference :)
OK, well given that you mentioned it and there are other tweaks to make
on top of this patch anyway, I'll go ahead and make the change.
This saves a few lines, if nothing else.
Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvmtool PATCH v8 9/9] KVM: arm/arm64: Add a vcpu feature for pointer authentication
On Tue, Apr 02, 2019 at 07:57:17AM +0530, Amit Daniel Kachhap wrote:
> This is a runtime capabality for KVM tool to enable Arm64 8.3 Pointer
> Authentication in guest kernel. Two vcpu features
> KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] are supplied together to enable
> Pointer Authentication in KVM guest after checking the capability.
>
> A command line option --ptrauth is also required to select this feature.
>
> Signed-off-by: Amit Daniel Kachhap
> ---
>
> Changes since v7:
> * Added check for capability KVM_CAP_ARM_PTRAUTH_GENERIC
>
> arm/aarch32/include/kvm/kvm-cpu-arch.h| 1 +
> arm/aarch64/include/asm/kvm.h | 2 ++
> arm/aarch64/include/kvm/kvm-config-arch.h | 4 +++-
> arm/aarch64/include/kvm/kvm-cpu-arch.h| 2 ++
> arm/include/arm-common/kvm-config-arch.h | 1 +
> arm/kvm-cpu.c | 7 +++
> include/linux/kvm.h | 2 ++
> 7 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/arm/aarch32/include/kvm/kvm-cpu-arch.h
> b/arm/aarch32/include/kvm/kvm-cpu-arch.h
> index d28ea67..520ea76 100644
> --- a/arm/aarch32/include/kvm/kvm-cpu-arch.h
> +++ b/arm/aarch32/include/kvm/kvm-cpu-arch.h
> @@ -13,4 +13,5 @@
> #define ARM_CPU_ID 0, 0, 0
> #define ARM_CPU_ID_MPIDR 5
>
> +#define ARM_VCPU_PTRAUTH_FEATURE 0
> #endif /* KVM__KVM_CPU_ARCH_H */
> diff --git a/arm/aarch64/include/asm/kvm.h b/arm/aarch64/include/asm/kvm.h
> index 97c3478..d4d0d8c 100644
> --- a/arm/aarch64/include/asm/kvm.h
> +++ b/arm/aarch64/include/asm/kvm.h
> @@ -102,6 +102,8 @@ struct kvm_regs {
> #define KVM_ARM_VCPU_EL1_32BIT 1 /* CPU running a 32bit VM */
> #define KVM_ARM_VCPU_PSCI_0_22 /* CPU uses PSCI v0.2 */
> #define KVM_ARM_VCPU_PMU_V3 3 /* Support guest PMUv3 */
> +#define KVM_ARM_VCPU_PTRAUTH_ADDRESS 4 /* CPU uses address pointer
> authentication */
> +#define KVM_ARM_VCPU_PTRAUTH_GENERIC 5 /* CPU uses generic pointer
> authentication */
>
> struct kvm_vcpu_init {
> __u32 target;
> diff --git a/arm/aarch64/include/kvm/kvm-config-arch.h
> b/arm/aarch64/include/kvm/kvm-config-arch.h
> index 04be43d..2074684 100644
> --- a/arm/aarch64/include/kvm/kvm-config-arch.h
> +++ b/arm/aarch64/include/kvm/kvm-config-arch.h
> @@ -8,7 +8,9 @@
> "Create PMUv3 device"), \
> OPT_U64('\0', "kaslr-seed", &(cfg)->kaslr_seed, \
> "Specify random seed for Kernel Address Space " \
> - "Layout Randomization (KASLR)"),
> + "Layout Randomization (KASLR)"),\
> + OPT_BOOLEAN('\0', "ptrauth", &(cfg)->has_ptrauth, \
> + "Enable address authentication"),
This should probably say "pointer", not "address" now, since we enable
both kinds of ptrauth together. (Sorry!)
When discussing how to control SVE, I was eventually convinced that it
is more user-friendly to make SVE default to on if present, and maybe
provide two options --disable-sve, --enable-sve, in case the user wants
to force it off or on instead of just getting what the host supports.
Passing --enable-sve on a host that doesn't support SVE would then lead
to kvmtool bailing out with an error, which is probably better then
silently turning it off.
I don't have this change in my kvmtool patches yet.
What's your view? It makes sense to do things the same way for all
features if we can.
> #include "arm-common/kvm-config-arch.h"
>
> diff --git a/arm/aarch64/include/kvm/kvm-cpu-arch.h
> b/arm/aarch64/include/kvm/kvm-cpu-arch.h
> index a9d8563..fcc2107 100644
> --- a/arm/aarch64/include/kvm/kvm-cpu-arch.h
> +++ b/arm/aarch64/include/kvm/kvm-cpu-arch.h
> @@ -17,4 +17,6 @@
> #define ARM_CPU_CTRL 3, 0, 1, 0
> #define ARM_CPU_CTRL_SCTLR_EL1 0
>
> +#define ARM_VCPU_PTRAUTH_FEATURE ((1UL << KVM_ARM_VCPU_PTRAUTH_ADDRESS) \
> + | (1UL << KVM_ARM_VCPU_PTRAUTH_GENERIC))
> #endif /* KVM__KVM_CPU_ARCH_H */
> diff --git a/arm/include/arm-common/kvm-config-arch.h
> b/arm/include/arm-common/kvm-config-arch.h
> index 5734c46..5badcbd 100644
> --- a/arm/include/arm-common/kvm-config-arch.h
> +++ b/arm/include/arm-common/kvm-config-arch.h
> @@ -10,6 +10,7 @@ struct kvm_config_arch {
> boolaarch32_guest;
> boolhas_pmuv3;
> u64 kaslr_seed;
> + boolhas_ptrauth;
> enum irqchip_type irqchip;
> u64 fw_addr;
> };
> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
> index 7780251..398c9d6 100644
> --- a/arm/kvm-cpu.c
> +++ b/arm/kvm-cpu.c
> @@ -68,6 +68,13 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm,
> unsigned long cpu_id)
> vcpu_init.features[0] |= (1UL << KVM_ARM_VCPU_PSCI_0_2);
> }
>
> + /* Set KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] if available */
> + if (kvm__supports_extension(kvm, KVM_CAP_AR
Re: [PATCH v8 8/9] KVM: arm64: Add capability to advertise ptrauth for guest
On Tue, Apr 02, 2019 at 07:57:16AM +0530, Amit Daniel Kachhap wrote: > This patch advertises the capability of two cpu feature called address > pointer authentication and generic pointer authentication. These > capabilities depend upon system support for pointer authentication and > VHE mode. > > The current arm64 KVM partially implements pointer authentication and > support of address/generic authentication are tied together. However, > separate ABI requirements for both of them is added so that the future > isolated implementation will not require any ABI changes. > > Signed-off-by: Amit Daniel Kachhap > Cc: Mark Rutland > Cc: Marc Zyngier > Cc: Christoffer Dall > Cc: kvmarm@lists.cs.columbia.edu > --- > > Changes since v7: > * Created 2 capabilities KVM_CAP_ARM_PTRAUTH_ADDRESS and > KVM_CAP_ARM_PTRAUTH_GENERIC > instead of one KVM_CAP_ARM_PTRAUTH [Kristina Martsenko]. > * Added documentation here itself instead of in a new patch. > > Documentation/virtual/kvm/api.txt | 3 +++ > arch/arm64/kvm/reset.c| 6 ++ > include/uapi/linux/kvm.h | 2 ++ > 3 files changed, 11 insertions(+) > > diff --git a/Documentation/virtual/kvm/api.txt > b/Documentation/virtual/kvm/api.txt > index aaa048d..9b56892 100644 > --- a/Documentation/virtual/kvm/api.txt > +++ b/Documentation/virtual/kvm/api.txt > @@ -2661,8 +2661,11 @@ Possible features: > Depends on KVM_CAP_ARM_PMU_V3. > - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication > for the CPU and supported only on arm64 architecture. > + Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS. > - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication > for the CPU and supported only on arm64 architecture. > + Depends on KVM_CAP_ARM_PTRAUTH_GENERIC. > > > 4.83 KVM_ARM_PREFERRED_TARGET > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c > index 717afed..8aa8982 100644 > --- a/arch/arm64/kvm/reset.c > +++ b/arch/arm64/kvm/reset.c > @@ -92,6 +92,12 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, > long ext) > case KVM_CAP_ARM_VM_IPA_SIZE: > r = kvm_ipa_limit; > break; > + case KVM_CAP_ARM_PTRAUTH_ADDRESS: > + r = has_vhe() && system_supports_address_auth(); > + break; > + case KVM_CAP_ARM_PTRAUTH_GENERIC: > + r = has_vhe() && system_supports_generic_auth(); > + break; If some hardware supports just one auth type, we would report just one of these caps. Although we have the rule that userspace is not allowed to request these independently in KVM_ARM_VCPU_INIT anyway, I think it would be easier for userspace if we suppress both caps if either auth type isn't available on the host. e.g.: case KVM_ARM_ARM_PTRAUTH_ADDRESS: case KVM_ARM_ARM_PTRAUTH_GENERIC: r = has_vhe() && system_supports_address_auth() && system_supports_generic_auth(); We could revert back to the above code later on, and apply the ABI relaxations described in my response to the vcpu features patch, if someday we add support to KVM for coping with host hardware that supports just one auth type. I'd like Mark to comment on this, since he's more aware of the architectural situation than I am. Cheers ---Dave ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v8 5/9] KVM: arm/arm64: preserve host MDCR_EL2 value
On Tue, Apr 02, 2019 at 07:57:13AM +0530, Amit Daniel Kachhap wrote:
> Save host MDCR_EL2 value during kvm HYP initialisation and restore
> after every switch from host to guest. There should not be any
> change in functionality due to this.
>
> The value of mdcr_el2 is now stored in struct kvm_cpu_context as
> both host and guest can now use this field in a common way.
>
> Signed-off-by: Amit Daniel Kachhap
> Acked-by: Mark Rutland
> Cc: Marc Zyngier
> Cc: Mark Rutland
> Cc: Christoffer Dall
> Cc: kvmarm@lists.cs.columbia.edu
> ---
>
> Changes since v7:
> * Removed unused function __kvm_get_mdcr_el2 [Kristina].
>
> arch/arm/include/asm/kvm_host.h | 1 -
> arch/arm64/include/asm/kvm_asm.h | 2 --
> arch/arm64/include/asm/kvm_host.h | 6 ++
> arch/arm64/include/asm/kvm_hyp.h | 2 +-
> arch/arm64/kvm/debug.c| 28 ++--
> arch/arm64/kvm/hyp/debug-sr.c | 5 -
> arch/arm64/kvm/hyp/switch.c | 18 +-
> arch/arm64/kvm/hyp/sysreg-sr.c| 8 +++-
> virt/kvm/arm/arm.c| 1 -
> 9 files changed, 21 insertions(+), 50 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 6d0aac4..a928565 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -343,7 +343,6 @@ static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu
> *vcpu) {}
> static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
> static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {}
>
> -static inline void kvm_arm_init_debug(void) {}
> static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {}
> static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {}
> static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {}
> diff --git a/arch/arm64/include/asm/kvm_asm.h
> b/arch/arm64/include/asm/kvm_asm.h
> index a68205c..a15ba55 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -78,8 +78,6 @@ extern u64 __vgic_v3_read_vmcr(void);
> extern void __vgic_v3_write_vmcr(u32 vmcr);
> extern void __vgic_v3_init_lrs(void);
>
> -extern u32 __kvm_get_mdcr_el2(void);
> -
> extern void __kvm_populate_host_regs(void);
>
> /*
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index 3b09fd0..e3ccd7b 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -211,6 +211,8 @@ struct kvm_cpu_context {
>
> /* HYP host/guest configuration */
> u64 hcr_el2;
> + u32 mdcr_el2;
> +
> struct kvm_vcpu *__hyp_running_vcpu;
> };
>
> @@ -226,9 +228,6 @@ struct vcpu_reset_state {
> struct kvm_vcpu_arch {
> struct kvm_cpu_context ctxt;
>
> - /* HYP configuration */
> - u32 mdcr_el2;
> -
> /* Exception Information */
> struct kvm_vcpu_fault_info fault;
>
> @@ -498,7 +497,6 @@ static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu
> *vcpu) {}
> static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
> static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {}
>
> -void kvm_arm_init_debug(void);
> void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
> void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
> void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/include/asm/kvm_hyp.h
> b/arch/arm64/include/asm/kvm_hyp.h
> index 4da765f..7fcde8a 100644
> --- a/arch/arm64/include/asm/kvm_hyp.h
> +++ b/arch/arm64/include/asm/kvm_hyp.h
> @@ -152,7 +152,7 @@ void __fpsimd_restore_state(struct user_fpsimd_state
> *fp_regs);
> bool __fpsimd_enabled(void);
>
> void activate_traps_vhe_load(struct kvm_vcpu *vcpu);
> -void deactivate_traps_vhe_put(void);
> +void deactivate_traps_vhe_put(struct kvm_vcpu *vcpu);
>
> u64 __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context *host_ctxt);
> void __noreturn __hyp_do_panic(unsigned long, ...);
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index fd917d6..99dc0a4 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -32,8 +32,6 @@
> DBG_MDSCR_KDE | \
> DBG_MDSCR_MDE)
>
> -static DEFINE_PER_CPU(u32, mdcr_el2);
> -
> /**
> * save/restore_guest_debug_regs
> *
> @@ -65,21 +63,6 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
> }
>
> /**
> - * kvm_arm_init_debug - grab what we need for debug
> - *
> - * Currently the sole task of this function is to retrieve the initial
> - * value of mdcr_el2 so we can preserve MDCR_EL2.HPMN which has
> - * presumably been set-up by some knowledgeable bootcode.
> - *
> - * It is called once per-cpu during CPU hyp initialisation.
> - */
> -
> -void kvm_arm_init_debug(void)
> -{
> - __this_cpu_write(mdcr_el2, kvm_call_hyp_ret(__kvm_get_mdcr_el2));
> -}
> -
> -/**
> * kvm_arm_reset_debug_ptr - reset the debug ptr to
Re: [PATCH v8 3/9] KVM: arm64: Move hyp_symbol_addr to fix dependency
On Tue, Apr 02, 2019 at 07:57:11AM +0530, Amit Daniel Kachhap wrote:
> Currently hyp_symbol_addr is placed in kvm_mmu.h which is mostly
> used by __hyp_this_cpu_ptr in kvm_asm.h but it cannot include
> kvm_mmu.h directly as kvm_mmu.h uses kvm_ksym_ref which is
> defined inside kvm_asm.h. Hence, hyp_symbol_addr is moved inside
> kvm_asm.h to fix this dependency on each other.
>
> Also, hyp_symbol_addr corresponding counterpart, kvm_ksym_ref,
> is already in kvm_asm.h, making it more sensible to move
> kvm_symbol_addr to the same file.
>
> Suggested by: James Morse
> Signed-off-by: Amit Daniel Kachhap
> Reviewed-by: Julien Thierry
> Cc: Marc Zyngier
> Cc: Christoffer Dall
> Cc: kvmarm@lists.cs.columbia.edu
> ---
>
> Changes since v7:
> * Slight change in commit message [Julien Thierry].
>
> arch/arm64/include/asm/kvm_asm.h | 20
> arch/arm64/include/asm/kvm_mmu.h | 20
> 2 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_asm.h
> b/arch/arm64/include/asm/kvm_asm.h
> index f5b79e9..57a07e8 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -80,6 +80,26 @@ extern void __vgic_v3_init_lrs(void);
>
> extern u32 __kvm_get_mdcr_el2(void);
>
> +/*
> + * Obtain the PC-relative address of a kernel symbol
> + * s: symbol
> + *
> + * The goal of this macro is to return a symbol's address based on a
> + * PC-relative computation, as opposed to a loading the VA from a
> + * constant pool or something similar. This works well for HYP, as an
> + * absolute VA is guaranteed to be wrong. Only use this if trying to
> + * obtain the address of a symbol (i.e. not something you obtained by
> + * following a pointer).
> + */
> +#define hyp_symbol_addr(s) \
> + ({ \
> + typeof(s) *addr;\
> + asm("adrp %0, %1\n" \
> + "add%0, %0, :lo12:%1\n" \
> + : "=r" (addr) : "S" (&s)); \
> + addr; \
> + })
> +
Do we need to include in vgic-v2-cpuif-proxy.c now?
Otherwise,
Reviewed-by: Dave Martin
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v8 4/9] KVM: arm/arm64: preserve host HCR_EL2 value
On Tue, Apr 02, 2019 at 07:57:12AM +0530, Amit Daniel Kachhap wrote:
> From: Mark Rutland
>
> When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which
> is a constant value. This works today, as the host HCR_EL2 value is
> always the same, but this will get in the way of supporting extensions
> that require HCR_EL2 bits to be set conditionally for the host.
>
> To allow such features to work without KVM having to explicitly handle
> every possible host feature combination, this patch has KVM save/restore
> for the host HCR when switching to/from a guest HCR. The saving of the
> register is done once during cpu hypervisor initialization state and is
> just restored after switch from guest.
>
> For fetching HCR_EL2 during kvm initialisation, a hyp call is made using
> kvm_call_hyp and is helpful in non-VHE case.
>
> For the hyp TLB maintenance code, __tlb_switch_to_host_vhe() is updated
> to toggle the TGE bit with a RMW sequence, as we already do in
> __tlb_switch_to_guest_vhe().
>
> The value of hcr_el2 is now stored in struct kvm_cpu_context as both host
> and guest can now use this field in a common way.
>
> Signed-off-by: Mark Rutland
> [Added cpu_init_host_ctxt, hcr_el2 field in struct kvm_cpu_context,
> save hcr_el2 in hyp init stage]
> Signed-off-by: Amit Daniel Kachhap
> Reviewed-by: James Morse
> Cc: Marc Zyngier
> Cc: Christoffer Dall
> Cc: kvmarm@lists.cs.columbia.edu
[...]
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index a01fe087..3b09fd0 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -209,6 +209,8 @@ struct kvm_cpu_context {
> u32 copro[NR_COPRO_REGS];
> };
>
> + /* HYP host/guest configuration */
> + u64 hcr_el2;
Minor nit: You could delete "host/guest" from the comment here. This is
implied by the fact that the member is in struct kvm_cpu_context in the
first place.
> struct kvm_vcpu *__hyp_running_vcpu;
> };
[...]
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 3563fe6..f5cefa1 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
[...]
> @@ -159,9 +159,10 @@ static void deactivate_traps_vhe(void)
> }
> NOKPROBE_SYMBOL(deactivate_traps_vhe);
>
> -static void __hyp_text __deactivate_traps_nvhe(void)
> +static void __hyp_text __deactivate_traps_nvhe(struct kvm_cpu_context
> *host_ctxt)
Where __hyp_text functions accept pointer arguments, they are usually
hyp pointers already... (see below)
> {
> u64 mdcr_el2 = read_sysreg(mdcr_el2);
> + struct kvm_cpu_context *hyp_host_ctxt = kern_hyp_va(host_ctxt);
>
> __deactivate_traps_common();
>
> @@ -169,25 +170,28 @@ static void __hyp_text __deactivate_traps_nvhe(void)
> mdcr_el2 |= MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT;
>
> write_sysreg(mdcr_el2, mdcr_el2);
> - write_sysreg(HCR_HOST_NVHE_FLAGS, hcr_el2);
> + write_sysreg(hyp_host_ctxt->hcr_el2, hcr_el2);
> write_sysreg(CPTR_EL2_DEFAULT, cptr_el2);
> }
>
> static void __hyp_text __deactivate_traps(struct kvm_vcpu *vcpu)
> {
> + struct kvm_cpu_context *host_ctxt;
> +
> + host_ctxt = vcpu->arch.host_cpu_context;
host_ctxt is not otherwise used here, so can we convert it up-front so
that the argument to __deactivate_traps_nvhe() and
deactivate_traps_vhe() is a hyp pointer already?
So:
struct kvm_cpu_context *hyp_host_ctxt;
hyp_host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
> /*
>* If we pended a virtual abort, preserve it until it gets
>* cleared. See D1.14.3 (Virtual Interrupts) for details, but
>* the crucial bit is "On taking a vSError interrupt,
>* HCR_EL2.VSE is cleared to 0."
>*/
> - if (vcpu->arch.hcr_el2 & HCR_VSE)
> - vcpu->arch.hcr_el2 = read_sysreg(hcr_el2);
> + if (vcpu->arch.ctxt.hcr_el2 & HCR_VSE)
> + vcpu->arch.ctxt.hcr_el2 = read_sysreg(hcr_el2);
>
> if (has_vhe())
> - deactivate_traps_vhe();
> + deactivate_traps_vhe(host_ctxt);
> else
> - __deactivate_traps_nvhe();
> + __deactivate_traps_nvhe(host_ctxt);
Then just pass hyp_host_ctxt to both of these, and drop the
kern_hyp_va() conversion from __deactivate_traps_nvhe().
This may be a bit less confusing.
Alternatively, just pass in the vcpu pointer (since this pattern is
already well established all over the place).
Another option could be to pull the hcr_el2 write out of the backends
entirely and put it in this common code instead. This doesn't look
straightforward though (or at least, I don't remember enough about how
all these traps handling functions fit together...)
[...]
Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v8 6/9] KVM: arm64: Add vcpu feature flags to control ptrauth accessibility
On Tue, Apr 02, 2019 at 07:57:14AM +0530, Amit Daniel Kachhap wrote: > Since Pointer authentication will be enabled or disabled on a > per-vcpu basis, vcpu feature flags are added in order to know which > vcpus have it enabled from userspace. > > This features will allow the KVM guest to allow the handling of > pointer authentication instructions or to treat them as undefined > if not set. > > The helper macro added checks the feature flag along with other > conditions such as VHE mode present and system support for > pointer address/generic authentication. Can this patch be put after the context switch patch instead? Here, we accept a request from userspace to enable ptrauth, but it will mysteriously fail to work. I worked around a similar issue by defining KVM_ARM64_GUEST_HAS_SVE early in the SVE series, but putting the logic to set this flag in vcpu->arch.flags later on (see also comments about this below). > Necessary documentations are added to reflect the changes done. > > Signed-off-by: Amit Daniel Kachhap > Cc: Mark Rutland > Cc: Marc Zyngier > Cc: Christoffer Dall > Cc: kvmarm@lists.cs.columbia.edu > --- > > Changes since v7: > * Moved the check for userspace features in this patch [James Morse]. > * Moved the vcpu feature flags Documentation in this patch [James Morse]. > > Documentation/arm64/pointer-authentication.txt | 13 + > Documentation/virtual/kvm/api.txt | 4 > arch/arm64/include/asm/kvm_host.h | 8 +++- > arch/arm64/include/uapi/asm/kvm.h | 2 ++ > arch/arm64/kvm/reset.c | 7 +++ > 5 files changed, 29 insertions(+), 5 deletions(-) > > diff --git a/Documentation/arm64/pointer-authentication.txt > b/Documentation/arm64/pointer-authentication.txt > index 5baca42..b164886 100644 > --- a/Documentation/arm64/pointer-authentication.txt > +++ b/Documentation/arm64/pointer-authentication.txt > @@ -87,7 +87,12 @@ used to get and set the keys for a thread. > Virtualization > -- > > -Pointer authentication is not currently supported in KVM guests. KVM > -will mask the feature bits from ID_AA64ISAR1_EL1, and attempted use of > -the feature will result in an UNDEFINED exception being injected into > -the guest. > +Pointer authentication is enabled in KVM guest when each virtual cpu is > +initialised by passing flags KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] and > +requesting these two separate cpu features to be enabled. The current KVM > +guest implementation works by enabling both features together, so both these > +userspace flags are checked together before enabling pointer authentication. > +The separate userspace flag will allow to have no userspace ABI changes when > +both features are implemented in an isolated way in future. Nit: we might make this change, but we don't promise that it will happsen. So, maybe write: "[...] have no userspace ABI changes if support is added in the future to allow these two features to be enabled independently of one another." > + > +Pointer Authentication is supported in KVM guest only in VHE mode. > diff --git a/Documentation/virtual/kvm/api.txt > b/Documentation/virtual/kvm/api.txt > index 7de9eee..aaa048d 100644 > --- a/Documentation/virtual/kvm/api.txt > +++ b/Documentation/virtual/kvm/api.txt > @@ -2659,6 +2659,10 @@ Possible features: > Depends on KVM_CAP_ARM_PSCI_0_2. > - KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU. > Depends on KVM_CAP_ARM_PMU_V3. > + - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication > + for the CPU and supported only on arm64 architecture. We should probably add: Must be requested if KVM_ARM_VCPU_PTRAUTH_GENERIC is also requested. > + - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication > + for the CPU and supported only on arm64 architecture. Similarly: Must be requested if KVM_ARM_VCPU_PTRAUTH_ADDRESS is also requested. (Or otherwise explain that both features must enabled together or not at all.) > diff --git a/arch/arm64/include/asm/kvm_host.h > b/arch/arm64/include/asm/kvm_host.h > index e3ccd7b..9dd2918 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -45,7 +45,7 @@ > > #define KVM_MAX_VCPUS VGIC_V3_MAX_CPUS > > -#define KVM_VCPU_MAX_FEATURES 4 > +#define KVM_VCPU_MAX_FEATURES 6 > > #define KVM_REQ_SLEEP \ > KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP) > @@ -491,6 +491,12 @@ static inline bool kvm_arch_requires_vhe(void) > return false; > } > > +#define vcpu_has_ptrauth(vcpu) (has_vhe() && \ > + system_supports_address_auth() && \ > + system_supports_generic_auth() && \ > + test_bit(KVM_ARM_VCPU_PTRAUTH_ADDRESS, vcpu->arch.features) && \ > + test_bit(KVM_ARM_VCPU_PTRAUTH_GENERIC, vcpu->arch.features)) We're checking 5 things here, which we don't necessarily w
Re: [PATCH v8 7/9] KVM: arm/arm64: context-switch ptrauth registers
Hi Kristina,
On 4/5/19 12:59 AM, Kristina Martsenko wrote:
On 02/04/2019 03:27, Amit Daniel Kachhap wrote:
From: Mark Rutland
When pointer authentication is supported, a guest may wish to use it.
This patch adds the necessary KVM infrastructure for this to work, with
a semi-lazy context switch of the pointer auth state.
[...]
diff --git a/arch/arm64/include/asm/kvm_ptrauth_asm.h
b/arch/arm64/include/asm/kvm_ptrauth_asm.h
new file mode 100644
index 000..65f99e9
--- /dev/null
+++ b/arch/arm64/include/asm/kvm_ptrauth_asm.h
@@ -0,0 +1,106 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* arch/arm64/include/asm/kvm_ptrauth_asm.h: Guest/host ptrauth save/restore
+ * Copyright 2019 Arm Limited
+ * Author: Mark Rutland
+ * Amit Daniel Kachhap
+ */
+
+#ifndef __ASM_KVM_PTRAUTH_ASM_H
+#define __ASM_KVM_PTRAUTH_ASM_H
+
+#ifndef __ASSEMBLY__
+
+#define __ptrauth_save_key(regs, key)
\
+({
\
+ regs[key ## KEYLO_EL1] = read_sysreg_s(SYS_ ## key ## KEYLO_EL1);
\
+ regs[key ## KEYHI_EL1] = read_sysreg_s(SYS_ ## key ## KEYHI_EL1);
\
+})
+
+#define __ptrauth_save_state(ctxt)
\
+({
\
+ __ptrauth_save_key(ctxt->sys_regs, APIA);\
+ __ptrauth_save_key(ctxt->sys_regs, APIB);\
+ __ptrauth_save_key(ctxt->sys_regs, APDA);\
+ __ptrauth_save_key(ctxt->sys_regs, APDB);\
+ __ptrauth_save_key(ctxt->sys_regs, APGA);\
+})
+
+#else /* __ASSEMBLY__ */
+
+#include
+
+#ifdef CONFIG_ARM64_PTR_AUTH
+
+#define PTRAUTH_REG_OFFSET(x) (x - CPU_APIAKEYLO_EL1)
+
+/*
+ * CPU_AP*_EL1 values exceed immediate offset range (512) for stp instruction
+ * so below macros takes CPU_APIAKEYLO_EL1 as base and calculates the offset of
+ * the keys from this base to avoid an extra add instruction. These macros
+ * assumes the keys offsets are aligned in a specific increasing order.
+ */
+.macro ptrauth_save_state base, reg1, reg2
+ mrs_s \reg1, SYS_APIAKEYLO_EL1
+ mrs_s \reg2, SYS_APIAKEYHI_EL1
+ stp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIAKEYLO_EL1)]
+ mrs_s \reg1, SYS_APIBKEYLO_EL1
+ mrs_s \reg2, SYS_APIBKEYHI_EL1
+ stp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIBKEYLO_EL1)]
+ mrs_s \reg1, SYS_APDAKEYLO_EL1
+ mrs_s \reg2, SYS_APDAKEYHI_EL1
+ stp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDAKEYLO_EL1)]
+ mrs_s \reg1, SYS_APDBKEYLO_EL1
+ mrs_s \reg2, SYS_APDBKEYHI_EL1
+ stp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDBKEYLO_EL1)]
+ mrs_s \reg1, SYS_APGAKEYLO_EL1
+ mrs_s \reg2, SYS_APGAKEYHI_EL1
+ stp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APGAKEYLO_EL1)]
+.endm
+
+.macro ptrauth_restore_state base, reg1, reg2
+ ldp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIAKEYLO_EL1)]
+ msr_s SYS_APIAKEYLO_EL1, \reg1
+ msr_s SYS_APIAKEYHI_EL1, \reg2
+ ldp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIBKEYLO_EL1)]
+ msr_s SYS_APIBKEYLO_EL1, \reg1
+ msr_s SYS_APIBKEYHI_EL1, \reg2
+ ldp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDAKEYLO_EL1)]
+ msr_s SYS_APDAKEYLO_EL1, \reg1
+ msr_s SYS_APDAKEYHI_EL1, \reg2
+ ldp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDBKEYLO_EL1)]
+ msr_s SYS_APDBKEYLO_EL1, \reg1
+ msr_s SYS_APDBKEYHI_EL1, \reg2
+ ldp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APGAKEYLO_EL1)]
+ msr_s SYS_APGAKEYLO_EL1, \reg1
+ msr_s SYS_APGAKEYHI_EL1, \reg2
+.endm
+
+.macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3
+ ldr \reg1, [\g_ctxt, #CPU_HCR_EL2]
+ and \reg1, \reg1, #(HCR_API | HCR_APK)
+ cbz \reg1, 1f
+ add \reg1, \g_ctxt, #CPU_APIAKEYLO_EL1
+ ptrauth_restore_state \reg1, \reg2, \reg3
+1:
Nit: the label in assembly macros is usually a larger number (see
assembler.h or alternative.h for example). I think this is to avoid
future accidents like
cbz x0, 1f
ptrauth_switch_to_guest x1, x2, x3, x4
add x5, x5, x6
1:
...
where the code would incorrectly branch to the label inside
ptrauth_switch_to_guest, instead of the one after it.
Yes agree that these kind of labels are problematic. I will update in
the next iteration.
Thanks,
Amit Daniel
Thanks,
Kristina
+.endm
+
+.macro ptrauth_switch_to_host g_ctxt, h_ctxt, reg1, reg2, reg3
+ ldr \reg1, [\g_ctxt, #CPU_HCR_EL2]
+ and \reg1, \reg1, #(HCR_API | HCR_APK)
+ cbz \reg1, 2f
+ add \reg1, \g_ctxt, #CPU_APIAKEYLO_EL1
+ ptraut
Re: [PATCH v7 21/27] KVM: arm/arm64: Add hook for arch-specific KVM initialisation
On Fri, Apr 05, 2019 at 10:36:24AM +0100, Dave Martin wrote:
> On Thu, Apr 04, 2019 at 06:33:08PM +0200, Andrew Jones wrote:
> > On Thu, Apr 04, 2019 at 03:53:55PM +0100, Dave Martin wrote:
> > > On Thu, Apr 04, 2019 at 04:25:28PM +0200, Andrew Jones wrote:
> > > > On Fri, Mar 29, 2019 at 01:00:46PM +, Dave Martin wrote:
>
> [...]
>
> > > > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > > > index 99c3738..c69e137 100644
> > > > > --- a/virt/kvm/arm/arm.c
> > > > > +++ b/virt/kvm/arm/arm.c
> > > > > @@ -1664,6 +1664,10 @@ int kvm_arch_init(void *opaque)
> > > > > if (err)
> > > > > return err;
> > > > >
> > > > > + err = kvm_arm_init_arch_resources();
> > > > > + if (err)
> > > > > + return err;
> > > > > +
> > > > > if (!in_hyp_mode) {
> > > > > err = init_hyp_mode();
> > > > > if (err)
> > > > > --
> > > > > 2.1.4
> > > > >
> > > >
> > > > It's not clear to me from the commit message why init_common_resources()
> > > > won't work for this. Maybe it'll be more clear as I continue the review.
> > >
> > > init_common_resources() is for stuff common to arm and arm64.
> >
> > Well currently init_common_resources() only calls kvm_set_ipa_limit(),
> > which isn't implemented for arm. So if there was a plan to only use
> > that function for init that actually does something on both, it doesn't.
>
> Hmmm, perhaps I was wishfully interpreting the word "common" to mean
> what I would like it to mean...
>
> > > kvm_arm_init_arch_resources() is intended for stuff specific to the
> > > particular arch backend.
> >
> > I'm not sure we need that yet. We just need an arm setup sve stub like
> > arm's kvm_set_ipa_limit() stub. OTOH, if we have to keep adding stubs
> > to arm we should probably have something like
> > kvm_arm_init_arch_resources() instead, and kvm_set_ipa_limit() should
> > be moved inside the arm64 one and the arm ipa limit stub can go. Then
> > since init_common_resources() would no longer be used, it could just
> > be deleted.
>
> OK, for simplicity I may call the sve setup directly as you suggest, and
> make an arm stub for it: that feels a bit weird, but there is precedent.
>
> If we end up accumulating a lot of these, we can revisit it and maybe
> invent something like kvm_arm_init_arch_resources() at that point.
>
> Does that sound reasonable?
Yup. Sounds good to me.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 27/27] KVM: arm64/sve: Document KVM API extensions for SVE
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 +, 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
> > >
> > > ---
> > >
> > > 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 0010 004c SPSR_UND64 spsr[KVM_SPSR_UND]
> > >0x6030 0010 004e SPSR_IRQ64 spsr[KVM_SPSR_IRQ]
> > >0x6060 0010 0050 SPSR_FIQ64 spsr[KVM_SPSR_FIQ]
> > > - 0x6040 0010 0054 V0 128 fp_regs.vregs[0]
> > > - 0x6040 0010 0058 V1 128 fp_regs.vregs[1]
> > > + 0x6040 0010 0054 V0 128 fp_regs.vregs[0](*)
> > > + 0x6040 0010 0058 V1 128 fp_regs.vregs[1](*)
> > > ...
> > > - 0x6040 0010 00d0 V31128 fp_regs.vregs[31]
> > > + 0x6040 0010 00d0 V31128 fp_regs.vregs[31] (*)
> > >0x6020 0010 00d4 FPSR32 fp_regs.fpsr
> > >0x6020 0010 00d5 FPCR32 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 0011 00
> > >
> > > @@ -2143,6 +2151,61 @@ arm64 system registers have the following id bit
> > > patterns:
> > > arm64 firmware pseudo-registers have the following bit pattern:
> > >0x6030 0014
> > >
> > > +arm64 SVE registers have the following bit patterns:
> > > + 0x6080 0015 00 Zn bits[2048*slice + 2047 :
> > > 2048*slice]
> > > + 0x6050 0015 04 Pn bits[256*slice + 255 :
> > > 256*slice]
> > > + 0x6050 0015 060 FFR bits[256*slice + 255 :
> > > 256*slice]
> > > + 0x6060 0015 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.
>
> >
> > > +
> > > +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))
Re: [PATCH v7 23/27] KVM: arm64/sve: Add pseudo-register for the guest's vector lengths
On Fri, Apr 05, 2019 at 10:36:10AM +0100, Dave Martin wrote:
> On Thu, Apr 04, 2019 at 10:31:09PM +0200, Andrew Jones wrote:
> > On Fri, Mar 29, 2019 at 01:00:48PM +, Dave Martin wrote:
> > > This patch adds a new pseudo-register KVM_REG_ARM64_SVE_VLS to
> > > allow userspace to set and query the set of vector lengths visible
> > > to the guest.
> > >
> > > In the future, multiple register slices per SVE register may be
> > > visible through the ioctl interface. Once the set of slices has
> > > been determined we would not be able to allow the vector length set
> > > to be changed any more, in order to avoid userspace seeing
> > > inconsistent sets of registers. For this reason, this patch adds
> > > support for explicit finalization of the SVE configuration via the
> > > KVM_ARM_VCPU_FINALIZE ioctl.
> > >
> > > Finalization is the proper place to allocate the SVE register state
> > > storage in vcpu->arch.sve_state, so this patch adds that as
> > > appropriate. The data is freed via kvm_arch_vcpu_uninit(), which
> > > was previously a no-op on arm64.
> > >
> > > To simplify the logic for determining what vector lengths can be
> > > supported, some code is added to KVM init to work this out, in the
> > > kvm_arm_init_arch_resources() hook.
> > >
> > > The KVM_REG_ARM64_SVE_VLS pseudo-register is not exposed yet.
> > > Subsequent patches will allow SVE to be turned on for guest vcpus,
> > > making it visible.
> > >
> > > Signed-off-by: Dave Martin
> > > Reviewed-by: Julien Thierry
> > > Tested-by: zhang.lei
>
> [...]
>
> > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
>
> [...]
>
> > > +/*
> > > + * Finalize vcpu's maximum SVE vector length, allocating
> > > + * vcpu->arch.sve_state as necessary.
> > > + */
> > > +static int kvm_vcpu_finalize_sve(struct kvm_vcpu *vcpu)
> > > +{
> > > + void *buf;
> > > + unsigned int vl;
> > > +
> > > + vl = vcpu->arch.sve_max_vl;
> > > +
> > > + /*
> > > + * Resposibility for these properties is shared between
> > > + * kvm_arm_init_arch_resources(), kvm_vcpu_enable_sve() and
> > > + * set_sve_vls(). Double-check here just to be sure:
> > > + */
> > > + if (WARN_ON(!sve_vl_valid(vl) || vl > sve_max_virtualisable_vl ||
> > > + vl > SVE_VL_ARCH_MAX))
> > > + return -EIO;
> > > +
> > > + buf = kzalloc(SVE_SIG_REGS_SIZE(sve_vq_from_vl(vl)), GFP_KERNEL);
> > > + if (!buf)
> > > + return -ENOMEM;
> > > +
> > > + vcpu->arch.sve_state = buf;
> > > + vcpu->arch.flags |= KVM_ARM64_VCPU_SVE_FINALIZED;
> > > + return 0;
> > > +}
> > > +
> > > +int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int what)
> > > +{
> > > + switch (what) {
> > > + case KVM_ARM_VCPU_SVE:
> > > + if (!vcpu_has_sve(vcpu))
> > > + return -EINVAL;
> > > +
> > > + if (kvm_arm_vcpu_sve_finalized(vcpu))
> > > + return -EPERM;
> > > +
> > > + return kvm_vcpu_finalize_sve(vcpu);
> >
> > In the next patch I see that we already have KVM_ARM64_GUEST_HAS_SVE
> > set in vcpu->arch.flags at this point. So if this kvm_vcpu_finalize_sve()
> > call fails, shouldn't we unset KVM_ARM64_GUEST_HAS_SVE and anything
> > else used to indicate the vcpu has sve?
>
> If this fails, either userspace did the wrong thing, or there was some
> fatal error (like the ENOMEM case). Either way, the vcpu is not runnable
> and userspace is expected to bail out.
>
> Further, KVM_ARM_VCPU_INIT fixes the set of features requested by
> userspace: we shouldn't change the set of features under userspace's
> feet and try to limp on somehow. We have no means for userspace to find
> out that some features went away in the meantime...
>
> So, what would you be trying to achieve with this change?
Being able to do additional vcpu ioctls with only partially initialized
sve (sve_state is still null, but we otherwise believe the vcpu has sve).
That sounds risky. Although I see we only set KVM_ARM64_VCPU_SVE_FINALIZED
when everything, like the memory allocation, succeeds, so we're probably
fine. Indeed having KVM_ARM64_GUEST_HAS_SVE and not KVM_ARM64_VCPU_SVE_FINALIZED
is likely the safest vcpu state for a vcpu that failed to finalize sve.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 23/27] KVM: arm64/sve: Add pseudo-register for the guest's vector lengths
On Fri, Apr 05, 2019 at 10:36:03AM +0100, Dave Martin wrote:
> On Thu, Apr 04, 2019 at 10:18:54PM +0200, Andrew Jones wrote:
> > On Fri, Mar 29, 2019 at 01:00:48PM +, Dave Martin wrote:
> > > This patch adds a new pseudo-register KVM_REG_ARM64_SVE_VLS to
> > > allow userspace to set and query the set of vector lengths visible
> > > to the guest.
> > >
> > > In the future, multiple register slices per SVE register may be
> > > visible through the ioctl interface. Once the set of slices has
> > > been determined we would not be able to allow the vector length set
> > > to be changed any more, in order to avoid userspace seeing
> > > inconsistent sets of registers. For this reason, this patch adds
> > > support for explicit finalization of the SVE configuration via the
> > > KVM_ARM_VCPU_FINALIZE ioctl.
> > >
> > > Finalization is the proper place to allocate the SVE register state
> > > storage in vcpu->arch.sve_state, so this patch adds that as
> > > appropriate. The data is freed via kvm_arch_vcpu_uninit(), which
> > > was previously a no-op on arm64.
> > >
> > > To simplify the logic for determining what vector lengths can be
> > > supported, some code is added to KVM init to work this out, in the
> > > kvm_arm_init_arch_resources() hook.
> > >
> > > The KVM_REG_ARM64_SVE_VLS pseudo-register is not exposed yet.
> > > Subsequent patches will allow SVE to be turned on for guest vcpus,
> > > making it visible.
> > >
> > > Signed-off-by: Dave Martin
> > > Reviewed-by: Julien Thierry
> > > Tested-by: zhang.lei
> > >
> > > ---
>
> [...]
>
> > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > > index 2aa80a5..086ab05 100644
> > > --- a/arch/arm64/kvm/guest.c
> > > +++ b/arch/arm64/kvm/guest.c
> > > @@ -206,6 +206,73 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const
> > > struct kvm_one_reg *reg)
> > > return err;
> > > }
> > >
> > > +#define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
> > > +#define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
> > > +
> > > +static bool vq_present(
> > > + const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)],
> > > + unsigned int vq)
> > > +{
> > > + return (*vqs)[vq_word(vq)] & vq_mask(vq);
> > > +}
> > > +
> > > +static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg
> > > *reg)
> > > +{
> > > + unsigned int max_vq, vq;
> > > + u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > +
> > > + if (WARN_ON(!sve_vl_valid(vcpu->arch.sve_max_vl)))
> > > + return -EINVAL;
> > > +
> > > + memset(vqs, 0, sizeof(vqs));
> > > +
> > > + max_vq = sve_vq_from_vl(vcpu->arch.sve_max_vl);
> > > + for (vq = SVE_VQ_MIN; vq <= max_vq; ++vq)
> > > + if (sve_vq_available(vq))
> > > + vqs[vq_word(vq)] |= vq_mask(vq);
> > > +
> > > + if (copy_to_user((void __user *)reg->addr, vqs, sizeof(vqs)))
> > > + return -EFAULT;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg
> > > *reg)
> > > +{
> > > + unsigned int max_vq, vq;
> > > + u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> >
> > There are three of these long 'DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1,
> > 64)'.
> > A macro is probably warranted.
>
> Fair point. These are a bit cumbersome. How about:
>
> #define SVE_VLS_WORDS DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)
>
> Annoyingly, this is virtually identical to a Linux bitmap: the base type
> is u64 instead of unsigned long; otherwise there's no intentional
> difference.
Yeah, I noticed it was a reinvented bitmap, but I liked that it was,
because, for the userspace api, removing the bitmap abstractions
ensures we know what is what and where exactly it is.
>
> (Aside: do you know why the KVM API insists on everything being u64?
> This makes sense for non-native types (like guest registers), but not
> for native things like host userspace addresses etc.)
Would that work when the kernel is 64-bit and the userspace is 32? I
personally like the interfaces being explicit with type size, as it
removes the brain burden of translating long and int when moving arch
to arch and kernel to userspace.
>
> > > +
> > > + if (kvm_arm_vcpu_sve_finalized(vcpu))
> > > + return -EPERM; /* too late! */
> > > +
> > > + if (WARN_ON(vcpu->arch.sve_state))
> > > + return -EINVAL;
> > > +
> > > + if (copy_from_user(vqs, (const void __user *)reg->addr, sizeof(vqs)))
> > > + return -EFAULT;
> > > +
> > > + max_vq = 0;
> > > + for (vq = SVE_VQ_MIN; vq <= SVE_VQ_MAX; ++vq)
> > > + if (vq_present(&vqs, vq))
> > > + max_vq = vq;
> > > +
> > > + if (max_vq > sve_vq_from_vl(kvm_sve_max_vl))
> > > + return -EINVAL;
> > > +
> > > + for (vq = SVE_VQ_MIN; vq <= max_vq; ++vq)
> > > + if (vq_present(&vqs, vq) != sve_vq_available(vq))
> > > + return -EINVAL;
> >
> > What about supporting the optional non-power-of-2 vector lengths? For
> >
Re: [PATCH v7 20/27] arm64/sve: In-kernel vector length availability query interface
On Fri, Apr 05, 2019 at 10:35:55AM +0100, Dave Martin wrote:
> On Thu, Apr 04, 2019 at 04:20:34PM +0200, Andrew Jones wrote:
> > On Fri, Mar 29, 2019 at 01:00:45PM +, Dave Martin wrote:
> > > KVM will need to interrogate the set of SVE vector lengths
> > > available on the system.
> > >
> > > This patch exposes the relevant bits to the kernel, along with a
> > > sve_vq_available() helper to check whether a particular vector
> > > length is supported.
> > >
> > > __vq_to_bit() and __bit_to_vq() are not intended for use outside
> > > these functions: now that these are exposed outside fpsimd.c, they
> > > are prefixed with __ in order to provide an extra hint that they
> > > are not intended for general-purpose use.
> > >
> > > Signed-off-by: Dave Martin
> > > Reviewed-by: Alex Bennée
> > > Tested-by: zhang.lei
> > > ---
> > > arch/arm64/include/asm/fpsimd.h | 29 +
> > > arch/arm64/kernel/fpsimd.c | 35 ---
> > > 2 files changed, 37 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/fpsimd.h
> > > b/arch/arm64/include/asm/fpsimd.h
> > > index df7a143..ad6d2e4 100644
> > > --- a/arch/arm64/include/asm/fpsimd.h
> > > +++ b/arch/arm64/include/asm/fpsimd.h
> > > @@ -24,10 +24,13 @@
> > >
> > > #ifndef __ASSEMBLY__
> > >
> > > +#include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > +#include
> > >
> > > #if defined(__KERNEL__) && defined(CONFIG_COMPAT)
> > > /* Masks for extracting the FPSR and FPCR from the FPSCR */
> > > @@ -89,6 +92,32 @@ extern u64 read_zcr_features(void);
> > >
> > > extern int __ro_after_init sve_max_vl;
> > > extern int __ro_after_init sve_max_virtualisable_vl;
> > > +/* Set of available vector lengths, as vq_to_bit(vq): */
> >
> > s/as/for use with/ ?
>
> Not exactly. Does the following work for you:
>
> /*
> * Set of available vector lengths
> * Vector length vq is encoded as bit __vq_to_bit(vq):
> */
Yes. That reads much better.
>
> > s/vq_to_bit/__vq_to_bit/
>
> Ack: that got renamed when I moved it to fpsimd.h, bit I clearly didn't
> update the comment as I pasted it across.
>
> >
> > > +extern __ro_after_init DECLARE_BITMAP(sve_vq_map, SVE_VQ_MAX);
> > > +
> > > +/*
> > > + * Helpers to translate bit indices in sve_vq_map to VQ values (and
> > > + * vice versa). This allows find_next_bit() to be used to find the
> > > + * _maximum_ VQ not exceeding a certain value.
> > > + */
> > > +static inline unsigned int __vq_to_bit(unsigned int vq)
> > > +{
> >
> > Why not have the same WARN_ON and clamping here as we do
> > in __bit_to_vq. Here a vq > SVE_VQ_MAX will wrap around
> > to a super high bit.
> >
> > > + return SVE_VQ_MAX - vq;
> > > +}
> > > +
> > > +static inline unsigned int __bit_to_vq(unsigned int bit)
> > > +{
> > > + if (WARN_ON(bit >= SVE_VQ_MAX))
> > > + bit = SVE_VQ_MAX - 1;
> > > +
> > > + return SVE_VQ_MAX - bit;
> > > +}
> > > +
> > > +/* Ensure vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX before calling this
> > > function */
> >
> > Are we avoiding putting these tests and WARN_ONs in this function to
> > keep it fast?
>
> These are intended as backend for use only by fpsimd.c and this header,
> so peppering them with WARN_ON() felt excessive. I don't expect a lot
> of new calls to these (or any, probably).
>
> I don't recall why I kept the WARN_ON() just in __bit_to_vq(), except
> that the way that gets called is a bit more complex in some places.
>
> Are you happy to replace these with comments? e.g.:
>
> /* Only valid when vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX */
> __vq_to_bit()
>
> /* Only valid when bit < SVE_VQ_MAX */
> __bit_to_vq()
>
>
> OTOH, these are not used on fast paths, so maybe having both as
> WARN_ON() would be better. Part of the problem is knowing what to clamp
> to: these are generally used in conjunction with looping or bitmap find
> operations, so the caller may be making assumptions about the return
> value that may wrong when the value is clamped.
>
> Alternatively, these could be BUG() -- but that seems heavy.
>
> What do you think?
I like the idea of having WARN_ON's to enforce the constraints. I
wouldn't be completely opposed to not having anything other than
the comments, though, as there is a limit to how defensive we should
be. I'll abstain from this vote.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 19/27] KVM: arm64: Enumerate SVE register indices for KVM_GET_REG_LIST
On Fri, Apr 05, 2019 at 10:35:45AM +0100, Dave Martin wrote:
> On Thu, Apr 04, 2019 at 04:08:32PM +0200, Andrew Jones wrote:
> > On Fri, Mar 29, 2019 at 01:00:44PM +, Dave Martin wrote:
> > > This patch includes the SVE register IDs in the list returned by
> > > KVM_GET_REG_LIST, as appropriate.
> > >
> > > On a non-SVE-enabled vcpu, no new IDs are added.
> > >
> > > On an SVE-enabled vcpu, IDs for the FPSIMD V-registers are removed
> > > from the list, since userspace is required to access the Z-
> > > registers instead in order to access the V-register content. For
> > > the variably-sized SVE registers, the appropriate set of slice IDs
> > > are enumerated, depending on the maximum vector length for the
> > > vcpu.
> > >
> > > As it currently stands, the SVE architecture never requires more
> > > than one slice to exist per register, so this patch adds no
> > > explicit support for enumerating multiple slices. The code can be
> > > extended straightforwardly to support this in the future, if
> > > needed.
> > >
> > > Signed-off-by: Dave Martin
> > > Reviewed-by: Julien Thierry
> > > Tested-by: zhang.lei
> > >
> > > ---
> > >
> > > Changes since v6:
> > >
> > > * [Julien Thierry] Add a #define to replace the magic "slices = 1",
> > >and add a comment explaining to maintainers what needs to happen if
> > >this is updated in the future.
> > >
> > > Changes since v5:
> > >
> > > (Dropped Julien Thierry's Reviewed-by due to non-trivial rebasing)
> > >
> > > * Move mis-split reword to prevent put_user()s being accidentally the
> > >correct size from KVM: arm64/sve: Add pseudo-register for the guest's
> > >vector lengths.
> > > ---
> > > arch/arm64/kvm/guest.c | 63
> > > ++
> > > 1 file changed, 63 insertions(+)
> > >
> > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > > index 736d8cb..2aa80a5 100644
> > > --- a/arch/arm64/kvm/guest.c
> > > +++ b/arch/arm64/kvm/guest.c
> > > @@ -222,6 +222,13 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const
> > > struct kvm_one_reg *reg)
> > > #define KVM_SVE_ZREG_SIZE KVM_REG_SIZE(KVM_REG_ARM64_SVE_ZREG(0, 0))
> > > #define KVM_SVE_PREG_SIZE KVM_REG_SIZE(KVM_REG_ARM64_SVE_PREG(0, 0))
> > >
> > > +/*
> > > + * number of register slices required to cover each whole SVE register
> > > on vcpu
> >
> > s/number/Number/
>
> Not a sentence -> no capital letter.
>
> Due to the adjacent note it does look a little odd though. I'm happy to
> change it.
>
> > s/on vcpu//
>
> Agreed, I can drop that.
>
> > > + * NOTE: If you are tempted to modify this, you must also to rework
> >
> > s/to rework/rework/
>
> Ack
>
> > > + * sve_reg_to_region() to match:
> > > + */
> > > +#define vcpu_sve_slices(vcpu) 1
> > > +
> > > /* Bounds of a single SVE register slice within vcpu->arch.sve_state */
> > > struct sve_state_reg_region {
> > > unsigned int koffset; /* offset into sve_state in kernel memory */
> > > @@ -411,6 +418,56 @@ static int get_timer_reg(struct kvm_vcpu *vcpu,
> > > const struct kvm_one_reg *reg)
> > > return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)) ? -EFAULT : 0;
> > > }
> > >
> > > +static unsigned long num_sve_regs(const struct kvm_vcpu *vcpu)
> > > +{
> > > + /* Only the first slice ever exists, for now */
> >
> > I'd move this comment up into the one above vcpu_sve_slices(),
> > and then nothing needs to change here when more slices come.
> >
> > > + const unsigned int slices = vcpu_sve_slices(vcpu);
> > > +
> > > + if (!vcpu_has_sve(vcpu))
> > > + return 0;
> > > +
> > > + return slices * (SVE_NUM_PREGS + SVE_NUM_ZREGS + 1 /* FFR */);
> > > +}
> > > +
> > > +static int copy_sve_reg_indices(const struct kvm_vcpu *vcpu,
> > > + u64 __user *uindices)
> > > +{
> > > + /* Only the first slice ever exists, for now */
> >
> > Same comment as above.
>
> Fair point: this was to explain the magic "1" that was previously here,
> but the comments are a bit redundant here now: better to move the
> comments where the 1 itself went.
>
> > > + const unsigned int slices = vcpu_sve_slices(vcpu);
> > > + u64 reg;
> > > + unsigned int i, n;
> > > + int num_regs = 0;
> > > +
> > > + if (!vcpu_has_sve(vcpu))
> > > + return 0;
> > > +
> > > + for (i = 0; i < slices; i++) {
> > > + for (n = 0; n < SVE_NUM_ZREGS; n++) {
> > > + reg = KVM_REG_ARM64_SVE_ZREG(n, i);
> > > + if (put_user(reg, uindices++))
> > > + return -EFAULT;
> > > +
> > > + num_regs++;
> > > + }
> > > +
> > > + for (n = 0; n < SVE_NUM_PREGS; n++) {
> > > + reg = KVM_REG_ARM64_SVE_PREG(n, i);
> > > + if (put_user(reg, uindices++))
> > > + return -EFAULT;
> > > +
> > > + num_regs++;
> > > + }
> > > +
> > > + reg = KVM_REG_ARM64_SVE_FFR(i);
> > > + if (put_
Re: [PATCH v7 27/27] KVM: arm64/sve: Document KVM API extensions for SVE
On Thu, Apr 04, 2019 at 11:09:21PM +0200, Andrew Jones wrote:
> On Fri, Mar 29, 2019 at 01:00:52PM +, 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
> >
> > ---
> >
> > 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 0010 004c SPSR_UND64 spsr[KVM_SPSR_UND]
> >0x6030 0010 004e SPSR_IRQ64 spsr[KVM_SPSR_IRQ]
> >0x6060 0010 0050 SPSR_FIQ64 spsr[KVM_SPSR_FIQ]
> > - 0x6040 0010 0054 V0 128 fp_regs.vregs[0]
> > - 0x6040 0010 0058 V1 128 fp_regs.vregs[1]
> > + 0x6040 0010 0054 V0 128 fp_regs.vregs[0](*)
> > + 0x6040 0010 0058 V1 128 fp_regs.vregs[1](*)
> > ...
> > - 0x6040 0010 00d0 V31128 fp_regs.vregs[31]
> > + 0x6040 0010 00d0 V31128 fp_regs.vregs[31] (*)
> >0x6020 0010 00d4 FPSR32 fp_regs.fpsr
> >0x6020 0010 00d5 FPCR32 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 0011 00
> >
> > @@ -2143,6 +2151,61 @@ arm64 system registers have the following id bit
> > patterns:
> > arm64 firmware pseudo-registers have the following bit pattern:
> >0x6030 0014
> >
> > +arm64 SVE registers have the following bit patterns:
> > + 0x6080 0015 00 Zn bits[2048*slice + 2047 :
> > 2048*slice]
> > + 0x6050 0015 04 Pn bits[256*slice + 255 :
> > 256*slice]
> > + 0x6050 0015 060 FFR bits[256*slice + 255 :
> > 256*slice]
> > + 0x6060 0015 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?
>
> > +
> > +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 deter
Re: [PATCH v7 21/27] KVM: arm/arm64: Add hook for arch-specific KVM initialisation
On Thu, Apr 04, 2019 at 06:33:08PM +0200, Andrew Jones wrote:
> On Thu, Apr 04, 2019 at 03:53:55PM +0100, Dave Martin wrote:
> > On Thu, Apr 04, 2019 at 04:25:28PM +0200, Andrew Jones wrote:
> > > On Fri, Mar 29, 2019 at 01:00:46PM +, Dave Martin wrote:
[...]
> > > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > > index 99c3738..c69e137 100644
> > > > --- a/virt/kvm/arm/arm.c
> > > > +++ b/virt/kvm/arm/arm.c
> > > > @@ -1664,6 +1664,10 @@ int kvm_arch_init(void *opaque)
> > > > if (err)
> > > > return err;
> > > >
> > > > + err = kvm_arm_init_arch_resources();
> > > > + if (err)
> > > > + return err;
> > > > +
> > > > if (!in_hyp_mode) {
> > > > err = init_hyp_mode();
> > > > if (err)
> > > > --
> > > > 2.1.4
> > > >
> > >
> > > It's not clear to me from the commit message why init_common_resources()
> > > won't work for this. Maybe it'll be more clear as I continue the review.
> >
> > init_common_resources() is for stuff common to arm and arm64.
>
> Well currently init_common_resources() only calls kvm_set_ipa_limit(),
> which isn't implemented for arm. So if there was a plan to only use
> that function for init that actually does something on both, it doesn't.
Hmmm, perhaps I was wishfully interpreting the word "common" to mean
what I would like it to mean...
> > kvm_arm_init_arch_resources() is intended for stuff specific to the
> > particular arch backend.
>
> I'm not sure we need that yet. We just need an arm setup sve stub like
> arm's kvm_set_ipa_limit() stub. OTOH, if we have to keep adding stubs
> to arm we should probably have something like
> kvm_arm_init_arch_resources() instead, and kvm_set_ipa_limit() should
> be moved inside the arm64 one and the arm ipa limit stub can go. Then
> since init_common_resources() would no longer be used, it could just
> be deleted.
OK, for simplicity I may call the sve setup directly as you suggest, and
make an arm stub for it: that feels a bit weird, but there is precedent.
If we end up accumulating a lot of these, we can revisit it and maybe
invent something like kvm_arm_init_arch_resources() at that point.
Does that sound reasonable?
Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 23/27] KVM: arm64/sve: Add pseudo-register for the guest's vector lengths
On Thu, Apr 04, 2019 at 10:31:09PM +0200, Andrew Jones wrote:
> On Fri, Mar 29, 2019 at 01:00:48PM +, Dave Martin wrote:
> > This patch adds a new pseudo-register KVM_REG_ARM64_SVE_VLS to
> > allow userspace to set and query the set of vector lengths visible
> > to the guest.
> >
> > In the future, multiple register slices per SVE register may be
> > visible through the ioctl interface. Once the set of slices has
> > been determined we would not be able to allow the vector length set
> > to be changed any more, in order to avoid userspace seeing
> > inconsistent sets of registers. For this reason, this patch adds
> > support for explicit finalization of the SVE configuration via the
> > KVM_ARM_VCPU_FINALIZE ioctl.
> >
> > Finalization is the proper place to allocate the SVE register state
> > storage in vcpu->arch.sve_state, so this patch adds that as
> > appropriate. The data is freed via kvm_arch_vcpu_uninit(), which
> > was previously a no-op on arm64.
> >
> > To simplify the logic for determining what vector lengths can be
> > supported, some code is added to KVM init to work this out, in the
> > kvm_arm_init_arch_resources() hook.
> >
> > The KVM_REG_ARM64_SVE_VLS pseudo-register is not exposed yet.
> > Subsequent patches will allow SVE to be turned on for guest vcpus,
> > making it visible.
> >
> > Signed-off-by: Dave Martin
> > Reviewed-by: Julien Thierry
> > Tested-by: zhang.lei
[...]
> > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
[...]
> > +/*
> > + * Finalize vcpu's maximum SVE vector length, allocating
> > + * vcpu->arch.sve_state as necessary.
> > + */
> > +static int kvm_vcpu_finalize_sve(struct kvm_vcpu *vcpu)
> > +{
> > + void *buf;
> > + unsigned int vl;
> > +
> > + vl = vcpu->arch.sve_max_vl;
> > +
> > + /*
> > +* Resposibility for these properties is shared between
> > +* kvm_arm_init_arch_resources(), kvm_vcpu_enable_sve() and
> > +* set_sve_vls(). Double-check here just to be sure:
> > +*/
> > + if (WARN_ON(!sve_vl_valid(vl) || vl > sve_max_virtualisable_vl ||
> > + vl > SVE_VL_ARCH_MAX))
> > + return -EIO;
> > +
> > + buf = kzalloc(SVE_SIG_REGS_SIZE(sve_vq_from_vl(vl)), GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + vcpu->arch.sve_state = buf;
> > + vcpu->arch.flags |= KVM_ARM64_VCPU_SVE_FINALIZED;
> > + return 0;
> > +}
> > +
> > +int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int what)
> > +{
> > + switch (what) {
> > + case KVM_ARM_VCPU_SVE:
> > + if (!vcpu_has_sve(vcpu))
> > + return -EINVAL;
> > +
> > + if (kvm_arm_vcpu_sve_finalized(vcpu))
> > + return -EPERM;
> > +
> > + return kvm_vcpu_finalize_sve(vcpu);
>
> In the next patch I see that we already have KVM_ARM64_GUEST_HAS_SVE
> set in vcpu->arch.flags at this point. So if this kvm_vcpu_finalize_sve()
> call fails, shouldn't we unset KVM_ARM64_GUEST_HAS_SVE and anything
> else used to indicate the vcpu has sve?
If this fails, either userspace did the wrong thing, or there was some
fatal error (like the ENOMEM case). Either way, the vcpu is not runnable
and userspace is expected to bail out.
Further, KVM_ARM_VCPU_INIT fixes the set of features requested by
userspace: we shouldn't change the set of features under userspace's
feet and try to limp on somehow. We have no means for userspace to find
out that some features went away in the meantime...
So, what would you be trying to achieve with this change?
[...]
Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 23/27] KVM: arm64/sve: Add pseudo-register for the guest's vector lengths
On Thu, Apr 04, 2019 at 10:18:54PM +0200, Andrew Jones wrote:
> On Fri, Mar 29, 2019 at 01:00:48PM +, Dave Martin wrote:
> > This patch adds a new pseudo-register KVM_REG_ARM64_SVE_VLS to
> > allow userspace to set and query the set of vector lengths visible
> > to the guest.
> >
> > In the future, multiple register slices per SVE register may be
> > visible through the ioctl interface. Once the set of slices has
> > been determined we would not be able to allow the vector length set
> > to be changed any more, in order to avoid userspace seeing
> > inconsistent sets of registers. For this reason, this patch adds
> > support for explicit finalization of the SVE configuration via the
> > KVM_ARM_VCPU_FINALIZE ioctl.
> >
> > Finalization is the proper place to allocate the SVE register state
> > storage in vcpu->arch.sve_state, so this patch adds that as
> > appropriate. The data is freed via kvm_arch_vcpu_uninit(), which
> > was previously a no-op on arm64.
> >
> > To simplify the logic for determining what vector lengths can be
> > supported, some code is added to KVM init to work this out, in the
> > kvm_arm_init_arch_resources() hook.
> >
> > The KVM_REG_ARM64_SVE_VLS pseudo-register is not exposed yet.
> > Subsequent patches will allow SVE to be turned on for guest vcpus,
> > making it visible.
> >
> > Signed-off-by: Dave Martin
> > Reviewed-by: Julien Thierry
> > Tested-by: zhang.lei
> >
> > ---
[...]
> > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > index 2aa80a5..086ab05 100644
> > --- a/arch/arm64/kvm/guest.c
> > +++ b/arch/arm64/kvm/guest.c
> > @@ -206,6 +206,73 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const
> > struct kvm_one_reg *reg)
> > return err;
> > }
> >
> > +#define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
> > +#define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
> > +
> > +static bool vq_present(
> > + const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)],
> > + unsigned int vq)
> > +{
> > + return (*vqs)[vq_word(vq)] & vq_mask(vq);
> > +}
> > +
> > +static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg
> > *reg)
> > +{
> > + unsigned int max_vq, vq;
> > + u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > +
> > + if (WARN_ON(!sve_vl_valid(vcpu->arch.sve_max_vl)))
> > + return -EINVAL;
> > +
> > + memset(vqs, 0, sizeof(vqs));
> > +
> > + max_vq = sve_vq_from_vl(vcpu->arch.sve_max_vl);
> > + for (vq = SVE_VQ_MIN; vq <= max_vq; ++vq)
> > + if (sve_vq_available(vq))
> > + vqs[vq_word(vq)] |= vq_mask(vq);
> > +
> > + if (copy_to_user((void __user *)reg->addr, vqs, sizeof(vqs)))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +
> > +static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg
> > *reg)
> > +{
> > + unsigned int max_vq, vq;
> > + u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
>
> There are three of these long 'DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)'.
> A macro is probably warranted.
Fair point. These are a bit cumbersome. How about:
#define SVE_VLS_WORDS DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)
Annoyingly, this is virtually identical to a Linux bitmap: the base type
is u64 instead of unsigned long; otherwise there's no intentional
difference.
(Aside: do you know why the KVM API insists on everything being u64?
This makes sense for non-native types (like guest registers), but not
for native things like host userspace addresses etc.)
> > +
> > + if (kvm_arm_vcpu_sve_finalized(vcpu))
> > + return -EPERM; /* too late! */
> > +
> > + if (WARN_ON(vcpu->arch.sve_state))
> > + return -EINVAL;
> > +
> > + if (copy_from_user(vqs, (const void __user *)reg->addr, sizeof(vqs)))
> > + return -EFAULT;
> > +
> > + max_vq = 0;
> > + for (vq = SVE_VQ_MIN; vq <= SVE_VQ_MAX; ++vq)
> > + if (vq_present(&vqs, vq))
> > + max_vq = vq;
> > +
> > + if (max_vq > sve_vq_from_vl(kvm_sve_max_vl))
> > + return -EINVAL;
> > +
> > + for (vq = SVE_VQ_MIN; vq <= max_vq; ++vq)
> > + if (vq_present(&vqs, vq) != sve_vq_available(vq))
> > + return -EINVAL;
>
> What about supporting the optional non-power-of-2 vector lengths? For
> example, if the maximum VL is 512, then in addition to 512, 128, and
> 256 there could be a 384. If we plan to start a guest on a machine
> that has all four and then migrate it to a machine that only has
> 128,256,512 we would want to set the VL set to 128,256,512, even on
> the machine that supports 384. If I understand the above test correctly,
> then that wouldn't work.
This is exactly what the code is trying to forbid, though it may not be
obvious here why.
The trouble is, we can't correctly emulate a vcpu supporting
{128,256,512} on hardware that also supports 384-bit vectors: the
architecture says that the vector length you get is determined by
rounding ZC
Re: [PATCH v7 19/27] KVM: arm64: Enumerate SVE register indices for KVM_GET_REG_LIST
On Thu, Apr 04, 2019 at 04:08:32PM +0200, Andrew Jones wrote:
> On Fri, Mar 29, 2019 at 01:00:44PM +, Dave Martin wrote:
> > This patch includes the SVE register IDs in the list returned by
> > KVM_GET_REG_LIST, as appropriate.
> >
> > On a non-SVE-enabled vcpu, no new IDs are added.
> >
> > On an SVE-enabled vcpu, IDs for the FPSIMD V-registers are removed
> > from the list, since userspace is required to access the Z-
> > registers instead in order to access the V-register content. For
> > the variably-sized SVE registers, the appropriate set of slice IDs
> > are enumerated, depending on the maximum vector length for the
> > vcpu.
> >
> > As it currently stands, the SVE architecture never requires more
> > than one slice to exist per register, so this patch adds no
> > explicit support for enumerating multiple slices. The code can be
> > extended straightforwardly to support this in the future, if
> > needed.
> >
> > Signed-off-by: Dave Martin
> > Reviewed-by: Julien Thierry
> > Tested-by: zhang.lei
> >
> > ---
> >
> > Changes since v6:
> >
> > * [Julien Thierry] Add a #define to replace the magic "slices = 1",
> >and add a comment explaining to maintainers what needs to happen if
> >this is updated in the future.
> >
> > Changes since v5:
> >
> > (Dropped Julien Thierry's Reviewed-by due to non-trivial rebasing)
> >
> > * Move mis-split reword to prevent put_user()s being accidentally the
> >correct size from KVM: arm64/sve: Add pseudo-register for the guest's
> >vector lengths.
> > ---
> > arch/arm64/kvm/guest.c | 63
> > ++
> > 1 file changed, 63 insertions(+)
> >
> > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > index 736d8cb..2aa80a5 100644
> > --- a/arch/arm64/kvm/guest.c
> > +++ b/arch/arm64/kvm/guest.c
> > @@ -222,6 +222,13 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const
> > struct kvm_one_reg *reg)
> > #define KVM_SVE_ZREG_SIZE KVM_REG_SIZE(KVM_REG_ARM64_SVE_ZREG(0, 0))
> > #define KVM_SVE_PREG_SIZE KVM_REG_SIZE(KVM_REG_ARM64_SVE_PREG(0, 0))
> >
> > +/*
> > + * number of register slices required to cover each whole SVE register on
> > vcpu
>
> s/number/Number/
Not a sentence -> no capital letter.
Due to the adjacent note it does look a little odd though. I'm happy to
change it.
> s/on vcpu//
Agreed, I can drop that.
> > + * NOTE: If you are tempted to modify this, you must also to rework
>
> s/to rework/rework/
Ack
> > + * sve_reg_to_region() to match:
> > + */
> > +#define vcpu_sve_slices(vcpu) 1
> > +
> > /* Bounds of a single SVE register slice within vcpu->arch.sve_state */
> > struct sve_state_reg_region {
> > unsigned int koffset; /* offset into sve_state in kernel memory */
> > @@ -411,6 +418,56 @@ static int get_timer_reg(struct kvm_vcpu *vcpu, const
> > struct kvm_one_reg *reg)
> > return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)) ? -EFAULT : 0;
> > }
> >
> > +static unsigned long num_sve_regs(const struct kvm_vcpu *vcpu)
> > +{
> > + /* Only the first slice ever exists, for now */
>
> I'd move this comment up into the one above vcpu_sve_slices(),
> and then nothing needs to change here when more slices come.
>
> > + const unsigned int slices = vcpu_sve_slices(vcpu);
> > +
> > + if (!vcpu_has_sve(vcpu))
> > + return 0;
> > +
> > + return slices * (SVE_NUM_PREGS + SVE_NUM_ZREGS + 1 /* FFR */);
> > +}
> > +
> > +static int copy_sve_reg_indices(const struct kvm_vcpu *vcpu,
> > + u64 __user *uindices)
> > +{
> > + /* Only the first slice ever exists, for now */
>
> Same comment as above.
Fair point: this was to explain the magic "1" that was previously here,
but the comments are a bit redundant here now: better to move the
comments where the 1 itself went.
> > + const unsigned int slices = vcpu_sve_slices(vcpu);
> > + u64 reg;
> > + unsigned int i, n;
> > + int num_regs = 0;
> > +
> > + if (!vcpu_has_sve(vcpu))
> > + return 0;
> > +
> > + for (i = 0; i < slices; i++) {
> > + for (n = 0; n < SVE_NUM_ZREGS; n++) {
> > + reg = KVM_REG_ARM64_SVE_ZREG(n, i);
> > + if (put_user(reg, uindices++))
> > + return -EFAULT;
> > +
> > + num_regs++;
> > + }
> > +
> > + for (n = 0; n < SVE_NUM_PREGS; n++) {
> > + reg = KVM_REG_ARM64_SVE_PREG(n, i);
> > + if (put_user(reg, uindices++))
> > + return -EFAULT;
> > +
> > + num_regs++;
> > + }
> > +
> > + reg = KVM_REG_ARM64_SVE_FFR(i);
> > + if (put_user(reg, uindices++))
> > + return -EFAULT;
> > +
> > + num_regs++;
> > + }
>
> nit: the extra blank lines above the num_regs++'s give the code an odd
> look (to me)
There's no guaranteed fall-through onto the increments: the blank line
was
Re: [PATCH v7 20/27] arm64/sve: In-kernel vector length availability query interface
On Thu, Apr 04, 2019 at 04:20:34PM +0200, Andrew Jones wrote:
> On Fri, Mar 29, 2019 at 01:00:45PM +, Dave Martin wrote:
> > KVM will need to interrogate the set of SVE vector lengths
> > available on the system.
> >
> > This patch exposes the relevant bits to the kernel, along with a
> > sve_vq_available() helper to check whether a particular vector
> > length is supported.
> >
> > __vq_to_bit() and __bit_to_vq() are not intended for use outside
> > these functions: now that these are exposed outside fpsimd.c, they
> > are prefixed with __ in order to provide an extra hint that they
> > are not intended for general-purpose use.
> >
> > Signed-off-by: Dave Martin
> > Reviewed-by: Alex Bennée
> > Tested-by: zhang.lei
> > ---
> > arch/arm64/include/asm/fpsimd.h | 29 +
> > arch/arm64/kernel/fpsimd.c | 35 ---
> > 2 files changed, 37 insertions(+), 27 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/fpsimd.h
> > b/arch/arm64/include/asm/fpsimd.h
> > index df7a143..ad6d2e4 100644
> > --- a/arch/arm64/include/asm/fpsimd.h
> > +++ b/arch/arm64/include/asm/fpsimd.h
> > @@ -24,10 +24,13 @@
> >
> > #ifndef __ASSEMBLY__
> >
> > +#include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > +#include
> >
> > #if defined(__KERNEL__) && defined(CONFIG_COMPAT)
> > /* Masks for extracting the FPSR and FPCR from the FPSCR */
> > @@ -89,6 +92,32 @@ extern u64 read_zcr_features(void);
> >
> > extern int __ro_after_init sve_max_vl;
> > extern int __ro_after_init sve_max_virtualisable_vl;
> > +/* Set of available vector lengths, as vq_to_bit(vq): */
>
> s/as/for use with/ ?
Not exactly. Does the following work for you:
/*
* Set of available vector lengths
* Vector length vq is encoded as bit __vq_to_bit(vq):
*/
> s/vq_to_bit/__vq_to_bit/
Ack: that got renamed when I moved it to fpsimd.h, bit I clearly didn't
update the comment as I pasted it across.
>
> > +extern __ro_after_init DECLARE_BITMAP(sve_vq_map, SVE_VQ_MAX);
> > +
> > +/*
> > + * Helpers to translate bit indices in sve_vq_map to VQ values (and
> > + * vice versa). This allows find_next_bit() to be used to find the
> > + * _maximum_ VQ not exceeding a certain value.
> > + */
> > +static inline unsigned int __vq_to_bit(unsigned int vq)
> > +{
>
> Why not have the same WARN_ON and clamping here as we do
> in __bit_to_vq. Here a vq > SVE_VQ_MAX will wrap around
> to a super high bit.
>
> > + return SVE_VQ_MAX - vq;
> > +}
> > +
> > +static inline unsigned int __bit_to_vq(unsigned int bit)
> > +{
> > + if (WARN_ON(bit >= SVE_VQ_MAX))
> > + bit = SVE_VQ_MAX - 1;
> > +
> > + return SVE_VQ_MAX - bit;
> > +}
> > +
> > +/* Ensure vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX before calling this
> > function */
>
> Are we avoiding putting these tests and WARN_ONs in this function to
> keep it fast?
These are intended as backend for use only by fpsimd.c and this header,
so peppering them with WARN_ON() felt excessive. I don't expect a lot
of new calls to these (or any, probably).
I don't recall why I kept the WARN_ON() just in __bit_to_vq(), except
that the way that gets called is a bit more complex in some places.
Are you happy to replace these with comments? e.g.:
/* Only valid when vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX */
__vq_to_bit()
/* Only valid when bit < SVE_VQ_MAX */
__bit_to_vq()
OTOH, these are not used on fast paths, so maybe having both as
WARN_ON() would be better. Part of the problem is knowing what to clamp
to: these are generally used in conjunction with looping or bitmap find
operations, so the caller may be making assumptions about the return
value that may wrong when the value is clamped.
Alternatively, these could be BUG() -- but that seems heavy.
What do you think?
[...]
Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 07/27] arm64/sve: Check SVE virtualisability
On Thu, Apr 04, 2019 at 11:21:04PM +0200, Andrew Jones wrote: > On Fri, Mar 29, 2019 at 01:00:32PM +, Dave Martin wrote: > > Due to the way the effective SVE vector length is controlled and > > trapped at different exception levels, certain mismatches in the > > sets of vector lengths supported by different physical CPUs in the > > system may prevent straightforward virtualisation of SVE at parity > > with the host. > > > > This patch analyses the extent to which SVE can be virtualised > > safely without interfering with migration of vcpus between physical > > CPUs, and rejects late secondary CPUs that would erode the > > situation further. > > > > It is left up to KVM to decide what to do with this information. > > > > Signed-off-by: Dave Martin > > Reviewed-by: Julien Thierry > > Tested-by: zhang.lei > > > > --- > > > > QUESTION: The final structure of this code makes it quite natural to > > clamp the vector length for KVM guests to the maximum value supportable > > across all CPUs; such a value is guaranteed to exist, but may be > > surprisingly small on a given hardware platform. > > > > Should we be stricter and actually refuse to support KVM at all on such > > hardware? This may help to force people to fix Linux (or the > > architecture) if/when this issue comes up. > > Blocking KVM would be too harsh, since the users of the host may not > care about guests with SVE, but still care about guests. > > > > > For now, I stick with pr_warn() and make do with a limited SVE vector > > length for guests. > > I think that's probably the best we can do. Agreed. Since it fell out quite nicely this way in the code, this was my preferred option. [...] > Reviewed-by: Andrew Jones Thanks ---Dave ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm/arm64: ensure vcpu target is unset on reset failure
On Fri, Apr 05, 2019 at 07:38:59AM +0100, Marc Zyngier wrote:
> On Thu, 04 Apr 2019 18:42:30 +0100,
> Andrew Jones wrote:
> >
> > A failed KVM_ARM_VCPU_INIT, should not set the vcpu target,
> > as the vcpu target is used by kvm_vcpu_initialized() to
> > determine if other vcpu ioctls may proceed. We need to set
> > the target before calling kvm_reset_vcpu(), but if that call
> > fails, we should then unset it.
> >
> > Signed-off-by: Andrew Jones
> > ---
> > virt/kvm/arm/arm.c | 12 +---
> > 1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > index 99c37384ba7b..7e5724ae1efd 100644
> > --- a/virt/kvm/arm/arm.c
> > +++ b/virt/kvm/arm/arm.c
> > @@ -934,7 +934,7 @@ int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct
> > kvm_irq_level *irq_level,
> > static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
> >const struct kvm_vcpu_init *init)
> > {
> > - unsigned int i;
> > + unsigned int i, ret;
> > int phys_target = kvm_target_cpu();
> >
> > if (init->target != phys_target)
> > @@ -969,9 +969,15 @@ static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
> > vcpu->arch.target = phys_target;
> >
> > /* Now we know what it is, we can reset it. */
> > - return kvm_reset_vcpu(vcpu);
> > -}
> > + ret = kvm_reset_vcpu(vcpu);
> > + if (ret) {
> > + vcpu->arch.target = -1;
> > + bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
> > + return ret;
>
> This return could trivially be removed...
>
> > + }
> >
> > + return 0;
> > +}
>
> ... and this turned into 'return ret'.
>
> I've tentatively applied this to the 5.1-fixes branch. Let me know if
> you're OK with it.
>
Either way is fine by me. I actually did it this way on purpose though
because I preferred the way the explicit 'return 0' at the bottom
documented that we were sure at that point of success, so no longer
needed to be concerned that target should be reset to -1.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
