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
