On Tue, Oct 02, 2018 at 12:25:18PM +0000, Wei Yang wrote:
>On Mon, Oct 01, 2018 at 06:41:21PM +0200, Paolo Bonzini wrote:
>>On 26/09/2018 09:58, Wei Yang wrote:
>>>
>>> +/* In case caller knows the rmap_head, just remove it by pte_list_remove() */
>>> +static void drop_spte_fast(struct kvm_rmap_head *rmap_head, u64 *sptep)
>>> +{
>>> + if (mmu_spte_clear_track_bits(sptep))
>>> + pte_list_remove(sptep, rmap_head);
>>> +}
>>> +
>
>Thanks for your comment.
>
>>
>>Here mmu_spte_clear_track_bits cannot return false, because
>>mmu_spte_clear_track_bits only returns false if
>>!is_shadow_present_pte(old_spte) and for_each_rmap_spte checks for that.
>>
>
>I think you are right, missed this point.
>
>BTW, I got one point not clear in mmu_spte_clear_track_bits().
>
>In this function, we tries to get old_spte in two cases:
>
> * at the very beginning
> * or use __update_clear_spte_slow() in case has_volatile_bits()
>
>This means there is a chance someone would change this spte at the same
>time? (I got some difficulty to understand this part. I would appreciate
>it if you would give me some insight.)
>
>If this is the case, then the old_spte returned from
>mmu_spte_clear_track_bits() would be NULL even for_each_rmap_spte() has
>done the check?
>
Let me try to explain my understanding. In case there is some error,
your comment is welcome.
In function spte_has_volatile_bits(), it checks
* guest/host writable
* A/D bit is clear, when A/D enabled
If either one is true, a page fault will be triggered when spte changes.
This means someone else would modify this spte simultaneously.
My conclusion:
Based on this, it looks possible to get a non-present spte from
mmu_spte_clear_track_bits() even it was present.
While I still confuse in one logic for is_access_track_spte().
It checks (spte & shadow_acc_track_mask), while I searched the project
and only find two places clear it, mark_spte_for_access_track() and
restore_acc_track_spte(). Not sure how it is used.
--
Wei Yang
Help you, Help me
