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. > > 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 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. > I may not have time to rework this for -rc1, and being a late ABI > change I'd like to get others' views on it before heading too far into > the tunnel. 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. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
