On 08/06/2014 06:39 AM, David Matlack wrote:
> On Mon, Aug 4, 2014 at 8:36 PM, Xiao Guangrong
> <xiaoguangrong@xxxxxxxxxxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
