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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
