On 05/24/2012 09:26 AM, Xiao Guangrong wrote:
> On 05/23/2012 07:34 PM, Avi Kivity wrote:
>
>
>>> static bool spte_has_volatile_bits(u64 spte)
>>> {
>>> + /*
>>> + * Always atomicly update spte if it can be updated
>>> + * out of mmu-lock.
>>> + */
>>> + if (spte_can_lockless_update(spte))
>>> + return true;
>>
>>
>> This is a really subtle point, but is it really needed?
>>
>> Lockless spte updates should always set the dirty and accessed bits, so
>> we won't be overwriting any volatile bits there.
>>
>
>
> Avi,
>
> Currently, The spte update/clear paths in mmu-lock think the "Dirty bit" is
> not volatile if the spte is readonly. Then the "Dirty bit" caused by
> lockless update can be lost.
>
Maybe it's better to change that. In fact, changing
if ((spte & shadow_accessed_mask) &&
(!is_writable_pte(spte) || (spte & shadow_dirty_mask)))
return false;
to
if (~spte & (shadow_accessed_mask | shadow_dirty_mask))
return false;
is almost the same thing - we miss the case where the page is COW or
shadowed though.
If we release the page as dirty, as below, perhaps the whole thing
doesn't matter; the mm must drop spte.w (or spte.d) before it needs to
access spte.d again.
> And, for tlb flush:
>
> | * If we overwrite a writable spte with a read-only one we
> | * should flush remote TLBs. Otherwise rmap_write_protect
> | * will find a read-only spte, even though the writable spte
> | * might be cached on a CPU's TLB.
> | */
> | if (is_writable_pte(entry) && !is_writable_pte(*sptep))
> | kvm_flush_remote_tlbs(vcpu->kvm);
>
> Atomically update spte can help us to get a stable is_writable_pte().
Why is it unstable? mmu_set_spte() before cleared SPTE_MMU_WRITEABLE, so
the lockless path will keep its hands off *spte.
>
>
>>> +
>>> if (!shadow_accessed_mask)
>>> return false;
>>>
>>> @@ -498,13 +517,7 @@ static bool mmu_spte_update(u64 *sptep, u64 new_spte)
>>> return ret;
>>> }
>>>
>>> - new_spte |= old_spte & shadow_dirty_mask;
>>> -
>>> - mask = shadow_accessed_mask;
>>> - if (is_writable_pte(old_spte))
>>> - mask |= shadow_dirty_mask;
>>> -
>>> - if (!spte_has_volatile_bits(old_spte) || (new_spte & mask) == mask)
>>> + if (!spte_has_volatile_bits(old_spte))
>>> __update_clear_spte_fast(sptep, new_spte);
>>> else
>>> old_spte = __update_clear_spte_slow(sptep, new_spte);
>>
>>
>> It looks like the old code is bad.. why can we ignore volatile bits in
>> the old spte? Suppose pfn is changing?
>>
>
>
> /* Rules for using mmu_spte_update:
> * Update the state bits, it means the mapped pfn is not changged.
>
> If pfn is changed, we should clear spte first, then set the spte to
> the new pfn, in kvm_set_pte_rmapp(), we have:
>
> | mmu_spte_clear_track_bits(sptep);
> | mmu_spte_set(sptep, new_spte);
Okay, thanks.
--
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
