On Wed, Aug 25, 2021 at 11:39:34AM +0100, Marc Zyngier wrote: > On Wed, 25 Aug 2021 11:02:28 +0100, > Oliver Upton <oupton@xxxxxxxxxx> wrote: > > > > On Wed, Aug 25, 2021 at 2:27 AM Marc Zyngier <maz@xxxxxxxxxx> wrote: > > > > Exposing new hypercalls to guests in this manner seems very unsafe to > > > > me. Suppose an operator is trying to upgrade from kernel N to kernel > > > > N+1, which brings in the new 'widget' hypercall. Guests are live > > > > migrated onto the N+1 kernel, but the operator finds a defect that > > > > warrants a kernel rollback. VMs are then migrated from kernel N+1 -> N. > > > > Any guests that discovered the 'widget' hypercall are likely going to > > > > get fussy _very_ quickly on the old kernel. > > > > > > This goes against what we decided to support for the *only* publicly > > > available VMM that cares about save/restore, which is that we only > > > move forward and don't rollback. > > > > Ah, I was definitely missing this context. Current behavior makes much > > more sense then. > > > > > Hypercalls are the least of your > > > worries, and there is a whole range of other architectural features > > > that will have also appeared/disappeared (your own CNTPOFF series is a > > > glaring example of this). > > > > Isn't that a tad bit different though? I'll admit, I'm just as guilty > > with my own series forgetting to add a KVM_CAP (oops), but it is in my > > queue to kick out with the fix for nVHE/ptimer. Nonetheless, if a user > > takes up a new KVM UAPI, it is up to the user to run on a new kernel. > > The two are linked. Exposing a new register to userspace and/or guest > result in the same thing: you can't rollback. That's specially true in > the QEMU case, which *learns* from the kernel what registers are > available, and doesn't maintain a fixed list. > > > My concerns are explicitly with the 'under the nose' changes, where > > KVM modifies the guest feature set without userspace opting in. Based > > on your comment, though, it would appear that other parts of KVM are > > affected too. > > Any new system register that is exposed by a new kernel feature breaks > rollback. And so far, we only consider it a bug if the set of exposed > registers reduces. Anything can be added safely (as checked by one of > the selftests added by Drew). > > < It doesn't have to be rollback safety, either. There may > > simply be a hypercall which an operator doesn't want to give its > > guests, and it needs a way to tell KVM to hide it. > > Fair enough. But this has to be done in a scalable way, which > individual capability cannot provide. > > > > > Have I missed something blatantly obvious, or do others see this as an > > > > issue as well? I'll reply with an example of adding opt-out for PTP. > > > > I'm sure other hypercalls could be handled similarly. > > > > > > Why do we need this? For future hypercalls, we could have some buy-in > > > capabilities. For existing ones, it is too late, and negative features > > > are just too horrible. > > > > Oh, agreed on the nastiness. Lazy hack to realize the intended > > functional change.. > > Well, you definitely achieved your goal of attracting my attention :). > > > > For KVM-specific hypercalls, we could get the VMM to save/restore the > > > bitmap of supported functions. That would be "less horrible". This > > > could be implemented using extra "firmware pseudo-registers" such as > > > the ones described in Documentation/virt/kvm/arm/psci.rst. > > > > This seems more reasonable, especially since we do this for migrating > > the guest's PSCI version. > > > > Alternatively, I had thought about using a VM attribute, given the > > fact that it is non-architectural information and we avoid ABI issues > > in KVM_GET_REG_LIST without buy-in through a KVM_CAP. > > The whole point is that these settings get exposed by > KVM_GET_REG_LIST, as this is QEMU's way to dump a VM state. Given that > we already have this for things like the spectre management state, we > can just as well expose the bitmaps that deal with the KVM-specific > hypercalls. After all, this falls into the realm of "KVM as VM > firmware". > > For ARM-architected hypercalls (TRNG, stolen time), we may need a > similar extension. > Thanks for including me Marc. I think you've mentioned all the examples of why we don't generally expect N+1 -> N migrations to work that I can think of. While some of the examples like get-reg-list could eventually be eliminated if we had CPU models to tighten our machine type state, I think N+1 -> N migrations will always be best effort at most. I agree with giving userspace control over the exposer of the hypercalls though. Using pseudo-registers for that purpose rather than a pile of CAPs also seems reasonable to me. And, while I don't think this patch is going to proceed, I thought I'd point out that the opt-out approach doesn't help much with expanding our migration support unless we require the VMM to be upgraded first. And, even then, the (N_kern, N+1_vmm) -> (N+1_kern, N_vmm) case won't work as expected, since the source enforce opt-out, but the destination won't. Also, since the VMM doesn't key off the kernel version, for the most part N+1 VMMs won't know when they're supposed to opt-out or not, leaving it to the user to ensure they consider everything. opt-in usually only needs the user to consider what machine type they want to launch. Thanks, drew
