KVM/arm64: Guest ABI changes do not appear rollback-safe

[Oliver Upton]

On Wed, Aug 25, 2021 at 8:07 AM Andrew Jones <drjones at redhat.com> wrote:
>
> 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 at google.com> wrote:
> > >
> > > On Wed, Aug 25, 2021 at 2:27 AM Marc Zyngier <maz at kernel.org> 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.

Right, there's going to need to be a fence in both kernel and VMM
versions. Before the fence, you can't rollback with either component.
Once on the other side of the fence, the user may freely migrate
between kernel + VMM combinations.

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

Going the register route will implicitly require opt-out for all old
hypercalls. We exposed them unconditionally to the guest before, and
we must uphold that behavior. The default value for the bitmap will
have those features set. Any hypercalls added after that register
interface will then require explicit opt-in from userspace.

With regards to the pseudoregister interface, how would a VMM discover
new bits? From my perspective, you need to have two bitmaps that the
VMM can get at: the set of supported feature bits and the active
bitmap of features for a running guest.

> Thanks,
> drew
>