Re: [PATCH v6 3/8] KVM: Add support for using dirty ring in conjunction with bitmap

[Marc Zyngier <maz@kernel.org>]

On Tue, 25 Oct 2022 01:24:19 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Mon, Oct 24, 2022 at 11:50:29PM +0000, Sean Christopherson wrote:
> > On Sat, Oct 22, 2022, Marc Zyngier wrote:
> > > On Fri, 21 Oct 2022 17:05:26 +0100, Sean Christopherson <seanjc@google.com> wrote:
> 
> [...]
> 
> > > > Would it be possible to require a dirty bitmap when an ITS is
> > > > created?  That would allow treating the above condition as a KVM
> > > > bug.
> > > 
> > > No. This should be optional. Everything about migration should be
> > > absolutely optional (I run plenty of concurrent VMs on sub-2GB
> > > systems). You want to migrate a VM that has an ITS or will collect
> > > dirty bits originating from a SMMU with HTTU, you enable the dirty
> > > bitmap. You want to have *vcpu* based dirty rings, you enable them.
> > > 
> > > In short, there shouldn't be any reason for the two are either
> > > mandatory or conflated. Both should be optional, independent, because
> > > they cover completely disjoined use cases. *userspace* should be in
> > > charge of deciding this.
> > 
> > I agree about userspace being in control, what I want to avoid is letting userspace
> > put KVM into a bad state without any indication from KVM that the setup is wrong
> > until something actually dirties a page.
> > 
> > Specifically, if mark_page_dirty_in_slot() is invoked without a running vCPU, on
> > a memslot with dirty tracking enabled but without a dirty bitmap, then the migration
> > is doomed.  Dropping the dirty page isn't a sane response as that'd all but
> > guaranatee memory corruption in the guest.  At best, KVM could kick all vCPUs out
> > to userspace with a new exit reason, but that's not a very good experience for
> > userspace as either the VM is unexpectedly unmigratable or the VM ends up being
> > killed (or I suppose userspace could treat the exit as a per-VM dirty ring of
> > size '1').
> 
> This only works on the assumption that the VM is in fact running. In the
> case of the GIC ITS, I would expect that the VM has already been paused
> in preparation for serialization. So, there would never be a vCPU thread
> that would ever detect the kick.

This is indeed the case. The ioctl will return -EBUSY if any vcpu is
running.

> 
> > That's why I asked if it's possible for KVM to require a dirty_bitmap when KVM
> > might end up collecting dirty information without a vCPU.  KVM is still
> > technically prescribing a solution to userspace, but only because there's only
> > one solution.
> 
> I was trying to allude to something like this by flat-out requiring
> ring + bitmap on arm64.

And I claim that this is wrong. It may suit a particular use case, but
that's definitely not a universal truth.

> 
> Otherwise, we'd either need to:
> 
>  (1) Document the features that explicitly depend on ring + bitmap (i.e.
>  GIC ITS, whatever else may come) such that userspace sets up the
>  correct configuration based on what its using. The combined likelihood
>  of both KVM and userspace getting this right seems low.

But what is there to get wrong? Absolutely nothing. Today, you can
save/restore a GICv3-ITS VM without a bitmap at all. Just dump all of
the memory. The bitmap only allows you to do it while the vcpus are
running. Do you want a dirty ring because it makes things faster?
Fine. But you need to understand what this does.

Yes, this may require some additional documentation. But more
importantly, it requires VMM authors to pay attention to what is
happening. At least the QEMU folks are doing that.

>  (2) Outright reject the use of features that require ring + bitmap.
>  This pulls in ordering around caps and other UAPI.

I don't think this makes any sense. Neither bitmap nor ring should be
a prerequisite for *anything*.

	M.

-- 
Without deviation from the norm, progress is not possible.