Re: Question about removing memslots

[Avi Kivity <avi@redhat.com>]

On 03/28/2012 09:24 AM, Benjamin Herrenschmidt wrote:
> So I was chasing a bug today when I realized that some "drivers" in qemu
> do interesting things with memory regions.

They're usually called devices, drivers live in the guest.

> More specifically, cirrus emulation constantly flips the linear
> framebuffer between being mapped into the guest and being emulated MMIO
> (the latter for the purpose of image blits).
>
> This made me ponder ... whenever a memslot is "removed" like that (in
> the case for example where cirrus turns the fb into emulation), we need
> to ensure that any cached translation that involve those GPAs are
> flushed out of whatever caching (HW or SW) is done by the KVM arch
> code...
>
> So I started looking and the only thing I can find (let me know if I
> missed something) is kvm_arch_flush_shadow(). Is that it ? Because it
> doesn't take the memslot going away as an argument, so it doesn't know
> -what- to flush...
>
> Now I see that x86 just seems to flush everything, which is quite heavy
> handed considering how often cirrus does it, but maybe it doesn't have a
> choice (lack of reverse mapping from GPA ?).

We do have a reverse mapping, so we could easily flush just a single
slot.  The reason it hasn't been done is that slot changes are very are
on x86.  They're usually only done by 16-bit software; 32-bit software
just maps the entire framebuffer BAR and accesses it directly.  It's
also usually done in a tight loop, so flushing everything doesn't have a
large impact (and with a 20-bit address space, you couldn't cause a
large impact if you wanted to - memory is all of 256 pages).

> I also noticed that powerpc ... doesn't do anything :-) Ooops....
>
> So all translations still present in the TLB will remain there, all
> translations present in the MMU hash table as well, etc...
>
> Now, in order to implement that properly and efficiently, we would need
> to at least get the GPA (if not the whole memslot).
>
> Do you have any objection (provided I didn't completely misunderstand
> something which is quite possible) to us adding that argument to
> kvm_arch_flush_shadow() ? We can easily put in a small patch adding that
> as an unused argument, and later get the proper implementation for
> powerpc.

Sure, it makes sense.

> Another thing I noticed while at it is that my version of
> __kvm_set_memory_region() appears to call kvm_iommu_map_pages() whenever
> adding a memslot ... but never does the opposite unmapping when that
> memory slot is removed.... isn't that potentially an issue ?

It is.  Alex?

-- 
error compiling committee.c: too many arguments to function