On 01/11/2015 06:00 AM, Christoffer Dall wrote:
> On Fri, Jan 09, 2015 at 08:17:20PM -0800, Mario Smarduch wrote:
>> This patch adds support for 2nd stage page fault handling while dirty page
>> logging. On huge page faults, huge pages are dissolved to normal pages, and
>> rebuilding of 2nd stage huge pages is blocked. In case migration is
>> canceled this restriction is removed and huge pages may be rebuilt again.
>>
>> This patch applies cleanly on top of patch series posted Dec. 15'th:
>> https://lists.cs.columbia.edu/pipermail/kvmarm/2014-December/012826.html
>
> In the future such information should also go under the ---
> separator.
>
>>
>> Patch #11 has been dropped, and should not be applied.
>
> this should go under the '---' separator too.
Ok will keep that in mind.
>
>>
>> Signed-off-by: Mario Smarduch <[email protected]>
>> ---
>>
>> Change Log since last RESEND v1 --> v2:
>> - Disallow dirty page logging of IO region - fail for initial write protect
>> and disable logging code in 2nd stage page fault handler.
>> - Fixed auto spell correction errors
>>
>> Change Log RESEND v0 --> v1:
>> - fixed bug exposed by new generic __get_user_pages_fast(), when region is
>> writable, prevent write protection of pte on read fault
>> - Removed marking entire huge page dirty on initial access
>> - don't dissolve huge pages of non-writable regions
>> - Made updates based on Christoffers comments
>> - renamed logging status function to memslot_is_logging()
>> - changed few values to bool from longs
>> - streamlined user_mem_abort() to eliminate extra conditional checks
>> ---
>> arch/arm/kvm/mmu.c | 113
>> ++++++++++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 105 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index 73d506f..b878236 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -47,6 +47,18 @@ static phys_addr_t hyp_idmap_vector;
>> #define kvm_pmd_huge(_x) (pmd_huge(_x) || pmd_trans_huge(_x))
>> #define kvm_pud_huge(_x) pud_huge(_x)
>>
>> +#define KVM_S2PTE_FLAG_IS_IOMAP (1UL << 0)
>> +#define KVM_S2PTE_FLAG_LOGGING_ACTIVE (1UL << 1)
>> +
>> +static bool memslot_is_logging(struct kvm_memory_slot *memslot)
>> +{
>> +#ifdef CONFIG_ARM
>> + return !!memslot->dirty_bitmap;
>> +#else
>> + return false;
>> +#endif
>> +}
>> +
>> static void kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa)
>> {
>> /*
>> @@ -59,6 +71,25 @@ static void kvm_tlb_flush_vmid_ipa(struct kvm *kvm,
>> phys_addr_t ipa)
>> kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, kvm, ipa);
>> }
>>
>> +/**
>> + * stage2_dissolve_pmd() - clear and flush huge PMD entry
>> + * @kvm: pointer to kvm structure.
>> + * @addr: IPA
>> + * @pmd: pmd pointer for IPA
>> + *
>> + * Function clears a PMD entry, flushes addr 1st and 2nd stage TLBs. Marks
>> all
>> + * pages in the range dirty.
>> + */
>> +static void stage2_dissolve_pmd(struct kvm *kvm, phys_addr_t addr, pmd_t
>> *pmd)
>> +{
>> + if (!kvm_pmd_huge(*pmd))
>> + return;
>> +
>> + pmd_clear(pmd);
>> + kvm_tlb_flush_vmid_ipa(kvm, addr);
>> + put_page(virt_to_page(pmd));
>> +}
>> +
>> static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
>> int min, int max)
>> {
>> @@ -703,10 +734,13 @@ static int stage2_set_pmd_huge(struct kvm *kvm, struct
>> kvm_mmu_memory_cache
>> }
>>
>> static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache
>> *cache,
>> - phys_addr_t addr, const pte_t *new_pte, bool iomap)
>> + phys_addr_t addr, const pte_t *new_pte,
>> + unsigned long flags)
>> {
>> pmd_t *pmd;
>> pte_t *pte, old_pte;
>> + bool iomap = flags & KVM_S2PTE_FLAG_IS_IOMAP;
>> + bool logging_active = flags & KVM_S2PTE_FLAG_LOGGING_ACTIVE;
>>
>> /* Create stage-2 page table mapping - Levels 0 and 1 */
>> pmd = stage2_get_pmd(kvm, cache, addr);
>> @@ -718,6 +752,13 @@ static int stage2_set_pte(struct kvm *kvm, struct
>> kvm_mmu_memory_cache *cache,
>> return 0;
>> }
>>
>> + /*
>> + * While dirty page logging - dissolve huge PMD, then continue on to
>> + * allocate page.
>> + */
>> + if (logging_active)
>> + stage2_dissolve_pmd(kvm, addr, pmd);
>> +
>> /* Create stage-2 page mappings - Level 2 */
>> if (pmd_none(*pmd)) {
>> if (!cache)
>> @@ -774,7 +815,8 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t
>> guest_ipa,
>> if (ret)
>> goto out;
>> spin_lock(&kvm->mmu_lock);
>> - ret = stage2_set_pte(kvm, &cache, addr, &pte, true);
>> + ret = stage2_set_pte(kvm, &cache, addr, &pte,
>> + KVM_S2PTE_FLAG_IS_IOMAP);
>> spin_unlock(&kvm->mmu_lock);
>> if (ret)
>> goto out;
>> @@ -1002,6 +1044,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
>> phys_addr_t fault_ipa,
>> pfn_t pfn;
>> pgprot_t mem_type = PAGE_S2;
>> bool fault_ipa_uncached;
>> + bool can_set_pte_rw = true;
>> + unsigned long set_pte_flags = 0;
>>
>> write_fault = kvm_is_write_fault(vcpu);
>> if (fault_status == FSC_PERM && !write_fault) {
>> @@ -1009,6 +1053,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
>> phys_addr_t fault_ipa,
>> return -EFAULT;
>> }
>>
>> +
>
> stray whitespace change?
Got it.
>
>> /* Let's check if we will get back a huge page backed by hugetlbfs */
>> down_read(¤t->mm->mmap_sem);
>> vma = find_vma_intersection(current->mm, hva, hva + 1);
>> @@ -1059,12 +1104,35 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
>> phys_addr_t fault_ipa,
>> if (is_error_pfn(pfn))
>> return -EFAULT;
>>
>> - if (kvm_is_device_pfn(pfn))
>> + if (kvm_is_device_pfn(pfn)) {
>> mem_type = PAGE_S2_DEVICE;
>> + set_pte_flags = KVM_S2PTE_FLAG_IS_IOMAP;
>> + }
>>
>> spin_lock(&kvm->mmu_lock);
>> if (mmu_notifier_retry(kvm, mmu_seq))
>> goto out_unlock;
>> +
>> + /*
>> + * When logging is enabled general page fault handling changes:
>> + * - Writable huge pages are dissolved on a read or write fault.
>
> why dissolve huge pages on a read fault?
What I noticed on write you would dissolve, on read you
rebuild THPs, flip back and forth like that, performance
& convergence was really bad.
>
>> + * - pte's are not allowed write permission on a read fault to
>> + * writable region so future writes can be marked dirty
>
> new line
ok.
>
>> + * Access to non-writable region is unchanged, and logging of IO
>> + * regions is not allowed.
>> + */
>> + if (memslot_is_logging(memslot) && writable) {
>> + set_pte_flags = KVM_S2PTE_FLAG_LOGGING_ACTIVE;
>> + if (hugetlb) {
>> + gfn += pte_index(fault_ipa);
>> + pfn += pte_index(fault_ipa);
>> + hugetlb = false;
>> + }
>> + force_pte = true;
>
> uh, not this is not what I meant, see my example (untested, partial)
> patch in the end of this mail.
I put some comments on your patch.
>
>> + if (!write_fault)
>> + can_set_pte_rw = false;
>> + }
>> +
>> if (!hugetlb && !force_pte)
>> hugetlb = transparent_hugepage_adjust(&pfn, &fault_ipa);
>>
>> @@ -1082,16 +1150,23 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
>> phys_addr_t fault_ipa,
>> ret = stage2_set_pmd_huge(kvm, memcache, fault_ipa, &new_pmd);
>> } else {
>> pte_t new_pte = pfn_pte(pfn, mem_type);
>> - if (writable) {
>> +
>> + /*
>> + * Don't set write permission, for non-writable region, and
>> + * for read fault to writable region while logging.
>> + */
>> + if (writable && can_set_pte_rw) {
>> kvm_set_s2pte_writable(&new_pte);
>> kvm_set_pfn_dirty(pfn);
>> }
>> coherent_cache_guest_page(vcpu, hva, PAGE_SIZE,
>> fault_ipa_uncached);
>> ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte,
>> - pgprot_val(mem_type) == pgprot_val(PAGE_S2_DEVICE));
>> + set_pte_flags);
>> }
>>
>> + if (write_fault)
>> + mark_page_dirty(kvm, gfn);
>>
>> out_unlock:
>> spin_unlock(&kvm->mmu_lock);
>> @@ -1242,7 +1317,14 @@ static void kvm_set_spte_handler(struct kvm *kvm,
>> gpa_t gpa, void *data)
>> {
>> pte_t *pte = (pte_t *)data;
>>
>> - stage2_set_pte(kvm, NULL, gpa, pte, false);
>> + /*
>> + * We can always call stage2_set_pte with KVM_S2PTE_FLAG_LOGGING_ACTIVE
>> + * flag clear because MMU notifiers will have unmapped a huge PMD before
>> + * calling ->change_pte() (which in turn calls kvm_set_spte_hva()) and
>> + * therefore stage2_set_pte() never needs to clear out a huge PMD
>> + * through this calling path.
>> + */
>> + stage2_set_pte(kvm, NULL, gpa, pte, 0);
>> }
>>
>>
>> @@ -1396,7 +1478,13 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>> bool writable = !(mem->flags & KVM_MEM_READONLY);
>> int ret = 0;
>>
>> - if (change != KVM_MR_CREATE && change != KVM_MR_MOVE)
>> + /*
>> + * Let - enable of dirty page logging through, later check if it's for
>> + * an IO region and fail.
>> + */
>
> I don't understand this comment or find it helpful.
Will remove.
>
>> + if (change != KVM_MR_CREATE && change != KVM_MR_MOVE &&
>> + change == KVM_MR_FLAGS_ONLY &&
>> + !(memslot->flags & KVM_MEM_LOG_DIRTY_PAGES))
>
> this looks wrong, because you can now remove all the other checks of
> change != and you are not returning early for KVM_MR_DELETE.
>
> I think you want to add a check simply for 'change != KVM_MR_FLAGS_ONLY'
> and then after the 'return 0' check the subconditions for change ==
> KVM_MR_FLAGS_ONLY.
Yeah, oh boy time to get a new batch of brown bags.
I was trying to limit conditional down to add, remap and
dirty page flag only in case some other flags get toggled
often and waste time walking through VMAs.
>
>> return 0;
>>
>> /*
>> @@ -1447,15 +1535,24 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>> phys_addr_t pa = (vma->vm_pgoff << PAGE_SHIFT) +
>> vm_start - vma->vm_start;
>>
>> - ret = kvm_phys_addr_ioremap(kvm, gpa, pa,
>> + if (change != KVM_MR_FLAGS_ONLY)
>> + ret = kvm_phys_addr_ioremap(kvm, gpa, pa,
>> vm_end - vm_start,
>> writable);
>> + else
>> + /* IO region dirty page logging not allowed */
>> + return -EINVAL;
>> +
>
> this whole thing also looks weird. I think you just need to add a check
> before kvm_phys_addr_ioremap() for flags & KVM_MEM_LOG_DIRTY_PAGES and
> return an error in that case (you've identified a user attempting to set
> dirty page logging on something that points to device memory, it doesn't
> matter at this point through which 'change' it is done).
Yes explicitly using KVM_MEM_LOG_DIRTY_PAGES is more clear.
>
>> if (ret)
>> break;
>> }
>> hva = vm_end;
>> } while (hva < reg_end);
>>
>> + /* Anything after here doesn't apply to memslot flag changes */
>> + if (change == KVM_MR_FLAGS_ONLY)
>> + return ret;
>> +
>> spin_lock(&kvm->mmu_lock);
>> if (ret)
>> unmap_stage2_range(kvm, mem->guest_phys_addr, mem->memory_size);
>> --
>
>
> What I meant last time around concerning user_mem_abort was more
> something like this:
>
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 1dc9778..38ea58e 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -935,7 +935,14 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
> phys_addr_t fault_ipa,
> return -EFAULT;
> }
>
> - if (is_vm_hugetlb_page(vma)) {
> + /*
> + * Writes to pages in a memslot with logging enabled are always logged
> + * on a singe page-by-page basis.
> + */
> + if (memslot_is_logging(memslot) && write_fault)
> + force_pte = true;
If it's a write you take the pte route and
dissolves huge page, if it's a read you reconstruct the
THP that seems to yield pretty bad results.
> +
> + if (is_vm_hugetlb_page(vma) && !force_pte) {
> hugetlb = true;
> gfn = (fault_ipa & PMD_MASK) >> PAGE_SHIFT;
> } else {
> @@ -976,6 +983,9 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
> phys_addr_t fault_ipa,
> if (is_error_pfn(pfn))
> return -EFAULT;
>
> + if (memslot_is_logging(memslot) && !write_fault)
> + writable = false;
Ok reusing writable is better.
> +
> if (kvm_is_device_pfn(pfn))
> mem_type = PAGE_S2_DEVICE;
>
> @@ -998,15 +1008,23 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
> phys_addr_t fault_ipa,
> fault_ipa_uncached);
> ret = stage2_set_pmd_huge(kvm, memcache, fault_ipa, &new_pmd);
> } else {
> + unsigned long flags = 0;
> pte_t new_pte = pfn_pte(pfn, mem_type);
> +
> if (writable) {
> kvm_set_s2pte_writable(&new_pte);
> kvm_set_pfn_dirty(pfn);
> }
> coherent_cache_guest_page(vcpu, hva, PAGE_SIZE,
> fault_ipa_uncached);
> - ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte,
> - pgprot_val(mem_type) == pgprot_val(PAGE_S2_DEVICE));
> +
> + if (pgprot_val(mem_type) == pgprot_val(PAGE_S2_DEVICE))
> + flags |= KVM_S2PTE_FLAG_IS_IOMAP;
> +
> + if (memslot_is_logging(memslot))
> + flags |= KVM_S2_FLAG_LOGGING_ACTIVE;
Now that it either IOMAP or LOGGING_ACTIVE do we need to acumulate flags?
Although we don't know if device mappings will be handled here.
Thanks.
> +
> + ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte, flags);
> }
>
>
>
> Thanks,
> -Christoffer
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
