Message-ID: <YzYQe2Lc+l2KpLBl@google.com>
Date:   Thu, 29 Sep 2022 21:39:07 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Emanuele Giuseppe Esposito <eesposit@...hat.com>,
        David Hildenbrand <david@...hat.com>,
        Maxim Levitsky <mlevitsk@...hat.com>, kvm@...r.kernel.org,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org,
        Like Xu <like.xu.linux@...il.com>
Subject: Re: [RFC PATCH 0/9] kvm: implement atomic memslot updates

On Thu, Sep 29, 2022, Paolo Bonzini wrote:
> 
> On 9/28/22 22:41, Sean Christopherson wrote:
> If you think we should overhaul it even more than just providing atomic
> batched updates, that's fine.

Or maybe solve the problem(s) with more precision?  What I don't like about the
batching is that it's not "atomic", and AFAICT doesn't have line of sight to truly
becoming atomic.  When I hear "atomic" updates, I think "all transactions are
commited at once".  That is not what this proposed API does, and due to the TLB
flushing requirements, I don't see how it can be implemented.

> But still, the impossibility to perform
> atomic updates in batches *is* a suboptimal part of the KVM API.

I don't disagree, but I honestly don't see the difference between "batching" and
providing a KVM_MEM_USER_EXIT flag.  The batch API will provide better performance,
but I would be surprised if it's significant enough to matter.  I'm not saying
that KVM_MEM_USER_EXIT provides novel functionality, e.g. I think we're in
agreement that handling memslot metadata will suck regardless.  My point is that
it's simpler to implement in KVM, much easier to document, and for all intents and
purposes provides the same desired functionality: vCPUs that access in-flux memslots
are stalled and so userspace doesn't have to pause and rendezvous all vCPUs to
provide "atomic" updates.

> The main cases are:
> 
> - for the boot case, splitting and merging existing memslots.  QEMU likes to
> merge adjacent memory regions into a single memslot, so if something goes
> from read-write to read-only it has to be split and vice versa.  I guess a
> "don't merge this memory region" flag would be the less hideous way to solve
> it in userspace.
> 
> - however, there is also the case of resizing an existing memslot which is
> what David would like to have for virtio-mem.  This is not really fixable
> because part of the appeal of virtio-mem is to have a single huge memslot
> instead of many smaller ones, in order to reduce the granularity of
> add/remove (David, correct me if I'm wrong).
> 
> (The latter _would_ be needed by other VMMs).

If we really want to provide a better experience for userspace, why not provide
more primitives to address those specific use cases?  E.g. fix KVM's historic wart
of disallowing toggling of KVM_MEM_READONLY, and then add one or more ioctls to:

  - Merge 2+ memory regions that are contiguous in GPA and HVA
  - Split a memory region into 2+ memory regions
  - Truncate a memory region
  - Grow a memory region

That would probably require more KVM code overall, but each operation would be more
tightly bounded and thus simpler to define.  And I think more precise APIs would
provide other benefits, e.g. growing a region wouldn't require first deleting the
current region, and so could avoid zapping SPTEs and destroying metadata.  Merge,
split, and truncate would have similar benefits, though they might be more
difficult to realize in practice.

What I really don't like about the "batch" API, aside from the "atomic" name, is
that it's too open ended and creates conflicts.  E.g. to allow "atomic" merging
after converting from RO=>RW, KVM needs to allow DELETE=>DELETE=>CREATE, and that
implies that each operation in the batch operates on the working state created by
the previous operations.

But if userspace provides a batch that does DELETE=>CREATE=>DELETE, naively hoisting
all "invalidations" to the front half of the "atomic" operations breaks the
ordering.  And there's a ridiculous amount of potential weirdness with MOVE=>MOVE
operations.  

KVM could solve those issues by disallowing MOVE and disallowing DELETE after
CREATE or FLAGS_ONLY, but then userspace is effectively restricted to operating
on a single contiguous chunk of memslots.  At that point, it seems that providing
primitives to do each macro operation is an even better experience for userspace.

In other words, make memslots a bit more CISC ;-)

Powered by blists - more mailing lists