Re: [PATCH v5 22/26] KVM: arm64/sve: Add pseudo-register for the guest's vector lengths

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

On Thu, Mar 07, 2019 at 01:47:09PM +0000, Marc Zyngier wrote:
> Hi Dave,
> 
> On 01/03/2019 14:55, Dave Martin wrote:
> > [FYI Peter, your views on the proposal outlined torward the end of the
> > mail would be appreciated.]
> > 
> > On Fri, Mar 01, 2019 at 01:28:19PM +0000, Julien Thierry wrote:
> 
> [...]
> 
> >> I might be over-thinking it, but if there is a way to move that
> >> finalization call I'd prefer that.
> > 
> > I know what you mean, but there's not really another clear place to put
> > it either, right now.  Making finalization a side-effect of only KVM_RUN
> > and KVM_GET_REG_LIST would mean that if userspace is squirting in the
> > state of a migrated vcpu, a dummy KVM_GET_REG_LIST call would need to be
> > inserted between setting KVM_REG_ARM64_SVE_VLS and setting the other SVE
> > regs.
> > 
> > One thing we could to is get rid of the implicit finalization behaviour
> > completely, and require an explicit ioctl KVM_ARM_VCPU_FINALIZE to do
> 
> +1. In the past, implicit finalization has been a major pain, and the
> "implicit GICv2 configuration" has been an absolute disaster.
> 
> > this.  This makes a clear barrier between reg writes that manipulate the
> > "physical" configuration of the vcpu, and reg reads/writes that merely
> > manipulate the vcpu's runtime state.  Say:
> > 
> > 	KVM_ARM_VCPU_INIT(features[] = KVM_ARM_VCPU_SVE) -> ok
> > 	KVM_RUN -> -EPERM (too early)
> > 	KVM_GET_REG_LIST -> -EPERM (too early)
> > 	...
> > 	KVM_SET_ONE_REG(KVM_REG_ARM64_SVE_VLS) -> ok
> > 	KVM_SET_ONE_REG(KVM_REG_ARM64_SVE_ZREG(0, 0)) -> -EPERM (too early)
> > 	...
> > 	KVM_RUN -> -EPERM (too early)
> > 	...
> > 	KVM_ARM_VCPU_FINALIZE -> ok
> > 	KVM_ARM_VCPU_FINALIZE -> -EPERM (too late)
> > 	...
> > 	KVM_REG_REG_LIST -> ok
> > 	KVM_SET_ONE_REG(KVM_REG_ARM64_SVE_VLS) -> -EPERM (too late)
> > 	KVM_SET_ONE_REG(KVM_REG_ARM64_SVE_ZREG(0, 0)) -> ok
> > 	KVM_RUN -> ok
> > 	KVM_ARM_VCPU_FINALIZE -> -EPERM (too late)
> > 
> > This would change all existing kvm_arm_vcpu_finalize() calls to
> > 
> > 	if (!kvm_arm_vcpu_finalized())
> > 		return -EPERM;
> > 
> > (or similar).
> 
> I find this rather compelling, assuming we can easily map features that
> are associated to finalization.

OK ... thanks for taking a look.

> > Without an affected vcpu feature enabled, for backwards compatibility
> > we would not require the KVM_ARM_VCPU_FINALIZE call (or perhaps even
> > forbid it, since userspace that wants to backwards compatible cannot
> > use the new ioctl anyway.  I'm in two minds about this.  Either way,
> > calling _FINALIZE on an old kvm is harmless, so long as userspace knows
> > to ignore this failure case.)
> > 
> > The backwards-compatible flow would be:
> > 
> > 	KVM_ARM_VCPU_INIT(features[] = 0) -> ok
> > 	...
> > 	KVM_ARM_VCPU_FINALIZE -> ok (or -EPERM)
> > 	...
> > 	KVM_RUN -> ok
> > 	KVM_ARM_VCPU_FINALIZE -> -EPERM (too late)
> > 
> > 
> > Thoughts?
> 
> This goes back to the above question: how do we ensure that userspace
> knows which features are subject to being finalized. As it is currently
> described, KVM_ARM_VCPU_FINALIZE isn't qualified by a feature set, nor
> can the kernel report what feature flags requires being finalized. It is
> also unclear to me whether all "finalizable" features would have the
> same init cycle.

So, it's not clear whether any other features will ever need
finalization, and even if they do there's a fair chance they won't be
interdependent with SVE in such a way as to require multiple
finalization steps.

For now, it's seems reasonable to make the finalization call a no-op
when no finalization is required.

Where finalization is required, KVM_ARM_VCPU_FINALIZE becomes mandatory,
but the requirement is a) strictly opt-in, and b) userspace will quickly
discover if it gets this wrong.

For this reason I'd rather not have any explicit reporting of whether
finalization is needed or not (or why): we just document and enforce at
run-time.

> So either we take the simplest approach and make KVM_ARM_VCPU_FINALIZE
> strictly SVE specific (renaming it in the process), or it takes some
> argument that allow specifying the feature set it applies to. Maybe we
> can get away with the kernel not reporting which features require to be
> finalized as long that it is clearly documented.

OK, what about:

 * Userspace treats KVM_ARM_VCPU_FINALIZE() -> EINVAL as no error
   (so that userspace can simply insert this into its init sequence,
   even on old kernels).

 * We add an argument, so that KVM_ARM_VCPU_FINALIZE(0) means
   "finalize default stuff, including SVE".  We can add explicit flags
   later if needed to finalize individual features separately.

   I don't know whether any features ever will have interdependent
   finalization requirements, but this should help get us off the
   hook if so.


Question:

 * KVM_REG_ARM64_SVE_VLS is a weird register because of its special
   sequencing requirements.

   The main reason for this is to make it easier to detect migration
   to a mismatched destination node.  But userspace needs some explicit
   code to make all this work anyway, so should we just have a couple
   of ioctls to get/set it instead?

   If there's no strong view either way, I'll leave it as-is, to
   minimise churn.

[...]

> Please do your best to have something as close as possible to the final
> version for -rc1. From that point on, I'd expect changes to be mostly
> cosmetic.

This ought to be feasible.  The changes being discussed so far are non-
invasive.

Cheers
---Dave