On Thu, Mar 07, 2019 at 01:47:09PM +0000, Marc Zyngier wrote:

Hi Dave,

On 01/03/2019 14:55, Dave Martin wrote:

quoted

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

[...]

quoted

quoted

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.

quoted

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.

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel