Re: [PATCH v7 23/27] KVM: arm64/sve: Add pseudo-register for the guest's vector lengths

[Dave Martin <Dave.Martin@arm.com>]

On Thu, Apr 04, 2019 at 10:31:09PM +0200, Andrew Jones wrote:
> On Fri, Mar 29, 2019 at 01:00:48PM +0000, 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 <Dave.Martin@arm.com>
> > Reviewed-by: Julien Thierry <julien.thierry@arm.com>
> > Tested-by: zhang.lei <zhang.lei@jp.fujitsu.com>

[...]

> > 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