On 08/06/2014 06:39 AM, David Matlack wrote: > On Mon, Aug 4, 2014 at 8:36 PM, Xiao Guangrong > <xiaoguangr...@linux.vnet.ibm.com> wrote: >> On 08/05/2014 05:10 AM, David Matlack wrote: >>> >>> This patch fixes the issue by doing the following: >>> - Tag the mmio cache with the memslot generation and use it to >>> validate mmio cache lookups. >>> - Extend vcpu_clear_mmio_info to clear mmio_gfn in addition to >>> mmio_gva, since both can be used to fast path mmio faults. >>> - In mmu_sync_roots, unconditionally clear the mmio cache since >>> even direct_map (e.g. tdp) hosts use it. >> >> It's not needed. >> >> direct map only uses gpa (and never cache gva) and >> vcpu_clear_mmio_info only clears gva. > > Ok thanks for the clarification. > >>> +static inline void vcpu_cache_mmio_info(struct kvm_vcpu *vcpu, >>> + gva_t gva, gfn_t gfn, unsigned access) >>> +{ >>> + vcpu->arch.mmio_gen = kvm_current_mmio_generation(vcpu->kvm); >>> + >>> + /* >>> + * Ensure that the mmio_gen is set before the rest of the cache entry. >>> + * Otherwise we might see a new generation number attached to an old >>> + * cache entry if creating/deleting a memslot races with mmio caching. >>> + * The inverse case is possible (old generation number with new cache >>> + * info), but that is safe. The next access will just miss the cache >>> + * when it should have hit. >>> + */ >>> + smp_wmb(); >> >> The memory barrier can't help us, consider this scenario: >> >> CPU 0 CPU 1 >> page-fault >> see gpa is not mapped in memslot >> >> create new memslot containing gpa from Qemu >> update the slots's generation number >> cache mmio info >> >> !!! later when vcpu accesses gpa again >> it will cause mmio-exit. > > Ah! Thanks for catching my mistake. > >> The easy way to fix this is that we update slots's generation-number >> after synchronize_srcu_expedited when memslot is being updated that >> ensures other sides can see the new generation-number only after >> finishing update. > > It would be possible for a vcpu to see an inconsistent kvm_memslots struct > (new set of slots with an old generation number). Is that not an issue?
In this case, checking generation-number will fail, we will goto the slow path to handle mmio access - that's very rare, so i think it's ok. > > We could just use the generation number that is stored in the > spte. The only downside (that I can see) is that handle_abnormal_pfn() > calls vcpu_cache_mmio_info() but does not have access to the spte. > So presumably we'd have to do a page table walk. The issue is not only in vcpu_cache_mmio_info but also in mark_mmio_spte() where we may cache new generation-number and old memslots info. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html