Marc Zyngier <marc.zyngier@xxxxxxx> writes:
> Hi Punit,
>
> On 13/08/18 10:40, Punit Agrawal wrote:
[...]
>> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
>> index 1d90d79706bd..2ab977edc63c 100644
>> --- a/virt/kvm/arm/mmu.c
>> +++ b/virt/kvm/arm/mmu.c
>> @@ -1015,19 +1015,36 @@ static int stage2_set_pmd_huge(struct kvm *kvm, struct kvm_mmu_memory_cache
>> pmd = stage2_get_pmd(kvm, cache, addr);
>> VM_BUG_ON(!pmd);
>>
>> - /*
>> - * Mapping in huge pages should only happen through a fault. If a
>> - * page is merged into a transparent huge page, the individual
>> - * subpages of that huge page should be unmapped through MMU
>> - * notifiers before we get here.
>> - *
>> - * Merging of CompoundPages is not supported; they should become
>> - * splitting first, unmapped, merged, and mapped back in on-demand.
>> - */
>> - VM_BUG_ON(pmd_present(*pmd) && pmd_pfn(*pmd) != pmd_pfn(*new_pmd));
>> -
>> old_pmd = *pmd;
>> +
>> if (pmd_present(old_pmd)) {
>> + /*
>> + * Mapping in huge pages should only happen through a
>> + * fault. If a page is merged into a transparent huge
>> + * page, the individual subpages of that huge page
>> + * should be unmapped through MMU notifiers before we
>> + * get here.
>> + *
>> + * Merging of CompoundPages is not supported; they
>> + * should become splitting first, unmapped, merged,
>> + * and mapped back in on-demand.
>> + */
>> + VM_BUG_ON(pmd_pfn(old_pmd) != pmd_pfn(*new_pmd));
>> +
>> + /*
>> + * Multiple vcpus faulting on the same PMD entry, can
>> + * lead to them sequentially updating the PMD with the
>> + * same value. Following the break-before-make
>> + * (pmd_clear() followed by tlb_flush()) process can
>> + * hinder forward progress due to refaults generated
>> + * on missing translations.
>> + *
>> + * Skip updating the page table if the entry is
>> + * unchanged.
>> + */
>> + if (pmd_val(old_pmd) == pmd_val(*new_pmd))
>> + goto out;
>
> I think the order of these two checks should be reversed: the first one
> is clearly a subset of the second one, so it'd make sense to have the
> global comparison before having the more specific one. Not that it
> matter much in practice, but I just find it easier to reason about.
Makes sense. I've reordered the checks for the next version.
Thanks,
Punit
>
>> +
>> pmd_clear(pmd);
>> kvm_tlb_flush_vmid_ipa(kvm, addr);
>> } else {
>> @@ -1035,6 +1052,7 @@ static int stage2_set_pmd_huge(struct kvm *kvm, struct kvm_mmu_memory_cache
>> }
>>
>> kvm_set_pmd(pmd, *new_pmd);
>> +out:
>> return 0;
>> }
>>
>>
>
> Thanks,
>
> M.
