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?
>I think you should call this function pte_list_remove, and rename the
>existing pte_list_remove to __pte_list_remove.
>
Sure, I would do this in v2.
>> @@ -1669,7 +1676,7 @@ static bool kvm_zap_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head)
>> while ((sptep = rmap_get_first(rmap_head, &iter))) {
>> rmap_printk("%s: spte %p %llx.\n", __func__, sptep, *sptep);
>>
>> - drop_spte(kvm, sptep);
>> + drop_spte_fast(rmap_head, sptep);
>> flush = true;
>> }
>>
>> @@ -1705,7 +1712,7 @@ static int kvm_set_pte_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
>> need_flush = 1;
>>
>> if (pte_write(*ptep)) {
>> - drop_spte(kvm, sptep);
>> + drop_spte_fast(rmap_head, sptep);
>> goto restart;
>> } else {
>> new_spte = *sptep & ~PT64_BASE_ADDR_MASK;
>
>... so this should probably move
>
> mmu_spte_clear_track_bits(sptep)
>
>from the "else" branch to before the "if", and call __pte_list_remove
>directly.
>
Looks reasonable to me, will add this change in v2.
>Thanks,
>
>Paolo
>
>> @@ -5598,7 +5605,7 @@ static bool kvm_mmu_zap_collapsible_spte(struct kvm *kvm,
>> if (sp->role.direct &&
>> !kvm_is_reserved_pfn(pfn) &&
>> PageTransCompoundMap(pfn_to_page(pfn))) {
>> - drop_spte(kvm, sptep);
>> + drop_spte_fast(rmap_head, sptep);
>> need_tlb_flush = 1;
>> goto restart;
>> }
--
Wei Yang
Help you, Help me
