lkml.org 
[lkml]   [2020]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 17/22] kvm: mmu: Support dirty logging for the TDP MMU
From
Date
On 25/09/20 23:22, Ben Gardon wrote:
> + for_each_tdp_pte_root(iter, root, start, end) {
> +iteration_start:
> + if (!is_shadow_present_pte(iter.old_spte))
> + continue;
> +
> + /*
> + * If this entry points to a page of 4K entries, and 4k entries
> + * should be skipped, skip the whole page. If the non-leaf
> + * entry is at a higher level, move on to the next,
> + * (lower level) entry.
> + */
> + if (!is_last_spte(iter.old_spte, iter.level)) {
> + if (skip_4k && iter.level == PG_LEVEL_2M) {
> + tdp_iter_next_no_step_down(&iter);
> + if (iter.valid && iter.gfn >= end)
> + goto iteration_start;
> + else
> + break;

The iteration_start label confuses me mightily. :) That would be a case
where iter.gfn >= end (so for_each_tdp_pte_root would exit) but you want
to proceed anyway with the gfn that was found by
tdp_iter_next_no_step_down. Are you sure you didn't mean

if (iter.valid && iter.gfn < end)
goto iteration_start;
else
break;

because that would make much more sense: basically a "continue" that
skips the tdp_iter_next. With the min_level change I suggested no
Friday, it would become something like this:

for_each_tdp_pte_root_level(iter, root, start, end, min_level) {
if (!is_shadow_present_pte(iter.old_spte) ||
!is_last_spte(iter.old_spte, iter.level))
continue;

new_spte = iter.old_spte & ~PT_WRITABLE_MASK;

*iter.sptep = new_spte;
handle_change_spte(kvm, as_id, iter.gfn, iter.old_spte,
new_spte, iter.level);

spte_set = true;
tdp_mmu_iter_cond_resched(kvm, &iter);
}

which is all nice and understandable.

Also, related to this function, why ignore the return value of
tdp_mmu_iter_cond_resched? It does makes sense to assign spte_set =
true since, just like in kvm_mmu_slot_largepage_remove_write_access's
instance of slot_handle_large_level, you don't even need to flush on
cond_resched. However, in order to do that you would have to add some
kind of "bool flush_on_resched" argument to tdp_mmu_iter_cond_resched,
or have two separate functions tdp_mmu_iter_cond_{flush_and_,}resched.

The same is true of clear_dirty_gfn_range and set_dirty_gfn_range.

Paolo

\
 
 \ /
  Last update: 2020-09-29 17:09    [W:0.504 / U:0.596 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site