Re: [PATCH 3/5] KVM: PPC: Book3S HV: Handle memory slot deletion and modification correctly

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
To: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Avi Kivity <avi@redhat.com>, Paul Mackerras <paulus@samba.org>,
	Gleb Natapov <gleb@redhat.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>,
	Alexander Graf <agraf@suse.de>,
	kvm-ppc@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH 3/5] KVM: PPC: Book3S HV: Handle memory slot deletion and modification correctly
Date: Sat, 18 Aug 2012 06:32:25 +1000	[thread overview]
Message-ID: <1345235545.11751.89.camel@pasglop> (raw)
In-Reply-To: <20120817183939.GA26687@amt.cnet>

On Fri, 2012-08-17 at 15:39 -0300, Marcelo Tosatti wrote:
> On Fri, Aug 17, 2012 at 05:06:18PM +1000, Benjamin Herrenschmidt wrote:
> > On Wed, 2012-08-15 at 14:59 -0300, Marcelo Tosatti wrote:
> > > 
> > > The guest should not expect memory accesses to an address
> > > to behave sanely while changing a BAR anyway.
> > > 
> > > Yes, compatibility for change of GPA base can be done in the
> > > kernel. I can look into it next week if nobody has done so at
> > > that point. 
> > 
> > There's one thing to be extra careful about here is if we start
> > doing that for normal memory (in case we start breaking it up
> > in slots, such as NUMA setups etc...).
> > 
> > The problem is that we must not allow normal memory accesses to be
> > handled via the "emulation" code (ie MMIO emulation or load/store
> > emulation, whatever we call it).
> > 
> > Part of the issues is that on architectures that don't use IPIs for
> > TLB invalidations but instead use some form of HW broadcast such as
> > PowerPC or ARM, there is an inherent race in that the emulation code can
> > keep a guest physical address (and perform the relevant access to the
> > corresponding memory region) way beyond the point where the guest
> > virtual->physical translation leading to that address has been
> > invalidated.
> > 
> > This doesn't happen on x86 because essentially the completion of the
> > invalidation IPI has to wait for all VCPUs to "respond" and thus to
> > finish whatever emulation they are doing. This is not the case on archs
> > with a HW invalidate broadcast.
> > 
> > This is a nasty race, and while we more/less decided that it was
> > survivable as long as we only go through emulation for devices (as we
> > don't play swapping games with them in the guest kernel), the minute we
> > allow normal guest memory access to "slip through", we have broken the
> > guest virtual memory model.
> 
> This emulation is in hardware, yes? It is the lack of a TLB entry (or
> the lack of a valid pagetable to fill the TLB) that triggers it?

What do you mean ? I'm talking about KVM emulating load and store
instructions (either in the kernel or passing them down to qemu).

This happens whenever an access triggers a host page fault which we
can't resolve because there is no memory slot. In that case, the access
is treated as "emulation".

Thus removing a memory slot and later on adding it back is broken for
that reason on those architectures if that memory slot is used to cover
actual guest memory or anything for which the guest kernel can
potentially play mapping game (yes, potentially this can be an issue
with emulated graphics BARs if/when we start doing fancy stuff with them
such as using the DRM with the TTM which can "Swap" objects in/out of
the emulated vram and play with mappings).

The memory slot update must either be atomic or as you mention below,
whoever does the update must stop all vcpu's before doing the update
which sucks as well.

> > So if we are manipulated memory slots used for guest RAM we must -not-
> > break atomicity, since during the time the slot is gone, it will
> > fallback to emulation, introducing the above race (at least on PowerPC
> > and ARM).
> 
> You could say get the vcpus to a known state (which has a side effect of
> making sure that emulation is stopped), no? (just as a mental exercise).

Yes, you could do that.

> > Cheers,
> > Ben.
> 
> Yes. Well, Avi mentioned earlier that there are users for change of GPA
> base. But, if my understanding is correct, the code that emulates
> change of BAR in QEMU is:
> 
>         /* now do the real mapping */
>         if (r->addr != PCI_BAR_UNMAPPED) {
>             memory_region_del_subregion(r->address_space, r->memory);
>         }
>         r->addr = new_addr;
>         if (r->addr != PCI_BAR_UNMAPPED) {
>             memory_region_add_subregion_overlap(r->address_space,
>                                                 r->addr, r->memory, 1);
> 
> These translate to two kvm_set_user_memory ioctls. 
> 
> "> Without taking into consideration backwards compatibility, userspace 
>  > can first delete the slot and later create a new one.
> 
>  Current qemu will in fact do that.  Not sure about older ones.
> "
> 
> Avi, where it does that?
> 
Cheers,
Ben.