On Mon, Jun 09, 2014 at 10:58:18AM -0700, Mario Smarduch wrote:
> On 06/08/2014 05:05 AM, Christoffer Dall wrote:
> > On Tue, Jun 03, 2014 at 04:19:25PM -0700, Mario Smarduch wrote:
> >> Patch adds memslot support for initial write protection and split up of huge
> >> pages. This patch series assumes that huge PUDs will not be used to map VM
> >> memory. This patch depends on the unmap_range() patch, it needs to be applied
> >> first.
> >>
> >> Signed-off-by: Mario Smarduch <m.smarduch@xxxxxxxxxxx>
> >> ---
> >> arch/arm/include/asm/kvm_host.h | 2 +
> >> arch/arm/include/asm/kvm_mmu.h | 20 ++++++
> >> arch/arm/include/asm/pgtable-3level.h | 1 +
> >> arch/arm/kvm/arm.c | 6 ++
> >> arch/arm/kvm/mmu.c | 114 +++++++++++++++++++++++++++++++++
> >> 5 files changed, 143 insertions(+)
> >>
> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> >> index 193ceaf..59565f5 100644
> >> --- a/arch/arm/include/asm/kvm_host.h
> >> +++ b/arch/arm/include/asm/kvm_host.h
> >> @@ -231,4 +231,6 @@ int kvm_perf_teardown(void);
> >> u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid);
> >> int kvm_arm_timer_set_reg(struct kvm_vcpu *, u64 regid, u64 value);
> >>
> >> +void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
> >> +
> >> #endif /* __ARM_KVM_HOST_H__ */
> >> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> >> index 5cc0b0f..08ab5e8 100644
> >> --- a/arch/arm/include/asm/kvm_mmu.h
> >> +++ b/arch/arm/include/asm/kvm_mmu.h
> >> @@ -114,6 +114,26 @@ static inline void kvm_set_s2pmd_writable(pmd_t *pmd)
> >> pmd_val(*pmd) |= L_PMD_S2_RDWR;
> >> }
> >>
> >> +static inline void kvm_set_s2pte_readonly(pte_t *pte)
> >> +{
> >> + pte_val(*pte) = (pte_val(*pte) & ~L_PTE_S2_RDWR) | L_PTE_S2_RDONLY;
> >> +}
> >> +
> >> +static inline bool kvm_s2pte_readonly(pte_t *pte)
> >> +{
> >> + return (pte_val(*pte) & L_PTE_S2_RDWR) == L_PTE_S2_RDONLY;
> >> +}
> >> +
> >> +static inline void kvm_set_s2pmd_readonly(pmd_t *pmd)
> >> +{
> >> + pmd_val(*pmd) = (pmd_val(*pmd) & ~L_PMD_S2_RDWR) | L_PMD_S2_RDONLY;
> >> +}
> >> +
> >> +static inline bool kvm_s2pmd_readonly(pmd_t *pmd)
> >> +{
> >> + return (pmd_val(*pmd) & L_PMD_S2_RDWR) == L_PMD_S2_RDONLY;
> >> +}
> >> +
> >
> > not crazy about the names, how about kvm_set_s2_pte_readonly etc.?
> >
> So kvm_set_s2pte_writable(pte_t *pte) was there already just following
> that convention.
>
ah, ok, no problem then.
> > the fact that these don't exist for arm64 makes me think it may break
> > the build for arm64 as well...
>
> Yes will address it.
> >
> >> /* Open coded p*d_addr_end that can deal with 64bit addresses */
> >> #define kvm_pgd_addr_end(addr, end) \
> >> ({ u64 __boundary = ((addr) + PGDIR_SIZE) & PGDIR_MASK; \
> >> diff --git a/arch/arm/include/asm/pgtable-3level.h b/arch/arm/include/asm/pgtable-3level.h
> >> index 85c60ad..d8bb40b 100644
> >> --- a/arch/arm/include/asm/pgtable-3level.h
> >> +++ b/arch/arm/include/asm/pgtable-3level.h
> >> @@ -129,6 +129,7 @@
> >> #define L_PTE_S2_RDONLY (_AT(pteval_t, 1) << 6) /* HAP[1] */
> >> #define L_PTE_S2_RDWR (_AT(pteval_t, 3) << 6) /* HAP[2:1] */
> >>
> >> +#define L_PMD_S2_RDONLY (_AT(pteval_t, 1) << 6) /* HAP[1] */
> >> #define L_PMD_S2_RDWR (_AT(pmdval_t, 3) << 6) /* HAP[2:1] */
> >>
> >> /*
> >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> >> index 3c82b37..dfd63ac 100644
> >> --- a/arch/arm/kvm/arm.c
> >> +++ b/arch/arm/kvm/arm.c
> >> @@ -242,6 +242,12 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
> >> const struct kvm_memory_slot *old,
> >> enum kvm_mr_change change)
> >> {
> >> + /*
> >> + * At this point memslot has been committed and the there is an
> >> + * allocated dirty_bitmap[] so marking of diryt pages works now on.
> >
> > s/diryt/dirty/
> >
> > "works now on" ?
> Ok
I don't understand what "works now on" means, so you need to clarify.
> >
> >> + */
> >> + if ((change != KVM_MR_DELETE) && (mem->flags & KVM_MEM_LOG_DIRTY_PAGES))
> >> + kvm_mmu_wp_memory_region(kvm, mem->slot);
> >> }
> >>
> >> void kvm_arch_flush_shadow_all(struct kvm *kvm)
> >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> >> index ef29540..e5dff85 100644
> >> --- a/arch/arm/kvm/mmu.c
> >> +++ b/arch/arm/kvm/mmu.c
> >> @@ -760,6 +760,120 @@ static bool transparent_hugepage_adjust(pfn_t *pfnp, phys_addr_t *ipap)
> >> return false;
> >> }
> >>
> >> +
> >> +/**
> >> + * stage2_wp_pte_range - write protect PTE range
> >> + * @pmd: pointer to pmd entry
> >> + * @addr: range start address
> >> + * @end: range end address
> >> + */
> >> +static void stage2_wp_pte_range(pmd_t *pmd, phys_addr_t addr, phys_addr_t end)
> >> +{
> >> + pte_t *pte;
> >> +
> >> + pte = pte_offset_kernel(pmd, addr);
> >> + do {
> >> + if (!pte_none(*pte)) {
> >> + if (!kvm_s2pte_readonly(pte))
> >> + kvm_set_s2pte_readonly(pte);
> >
> > do you need the test before setting readonly?
> Probably not.
>
> Some memory regions have hardly any pages present and sometimes
> not dirty. Was thinking of couple enhancements not to flush if
> there are no dirty pages or few dirty pages then just flush by IPA.
> But currently not doing anything with this info, leave it for
> future.
>
hmmh, yeah, maybe it's better to keep it the way you have it now for
cache purposes, not sure.
> >
> >> + }
> >> + } while (pte++, addr += PAGE_SIZE, addr != end);
> >> +}
> >> +
> >> +/**
> >> + * stage2_wp_pmd_range - write protect PMD range
> >> + * @pud: pointer to pud entry
> >> + * @addr: range start address
> >> + * @end: range end address
> >> + */
> >> +static void stage2_wp_pmd_range(pud_t *pud, phys_addr_t addr, phys_addr_t end)
> >> +{
> >> + pmd_t *pmd;
> >> + phys_addr_t next;
> >> +
> >> + pmd = pmd_offset(pud, addr);
> >> +
> >> + do {
> >> + next = kvm_pmd_addr_end(addr, end);
> >> + if (!pmd_none(*pmd)) {
> >> + if (kvm_pmd_huge(*pmd)) {
> >> + /*
> >> + * Write Protect the PMD, give user_mem_abort()
> >> + * a choice to clear and fault on demand or
> >> + * break up the huge page.
> >> + */
> >
> > I think this comment here is unnecessary. If this function is used for
> > other purposes, it will be misleading.
>
> Ok.
> >
> >> + if (!kvm_s2pmd_readonly(pmd))
> >> + kvm_set_s2pmd_readonly(pmd);
> >
> > same as above
> >
> >> + } else
> >> + stage2_wp_pte_range(pmd, addr, next);
> >> +
> >> + }
> >> + } while (pmd++, addr = next, addr != end);
> >> +}
> >> +
> >> +/**
> >> + * stage2_wp_pud_range - write protect PUD range
> >> + * @kvm: pointer to kvm structure
> >> + * @pud: pointer to pgd entry
> >> + * @addr: range start address
> >> + * @end: range end address
> >> + *
> >> + * While walking the PUD range huge PUD pages are ignored, in the future this
> >> + * may need to be revisited. Determine how to handle huge PUDs when logging
> >> + * of dirty pages is enabled.
> >> + */
> >> +static void stage2_wp_pud_range(struct kvm *kvm, pgd_t *pgd,
> >> + phys_addr_t addr, phys_addr_t end)
> >> +{
> >> + pud_t *pud;
> >> + phys_addr_t next;
> >> +
> >> + pud = pud_offset(pgd, addr);
> >> + do {
> >> + /* Check for contention every PUD range and release CPU */
> >> + if (need_resched() || spin_needbreak(&kvm->mmu_lock)
> >
> > Why do we need this here?
> >
> >> + cond_resched_lock(&kvm->mmu_lock);
> >
> > is this really safe? This will unlock the mmu_lock and all sorts of
> > stuff could happen, in between. For example, couldn't the compiler have
> > cached the pud value here? It feels extremely dicy.
>
> During testing either DETECT_HUNG_TASK, LOCK_DETECTOR, LOCK_DEP I don't
> recall paniced the system with lockup detected I think the
> thread was running longer then 5s this was for a 2GB memory region. Back
> then I was splitting pages in the initial write protect. In addition
> you also starve the other vCPUs. But if you have huge VM with a huge
> memory region it can cause problems.
hmm, ok, fair enough.
>
> But code does have a bug it relies on a stale value of the pud entry.
> need to move up where PGDs are walked and recheck the value
> of the pgd after you emerge cond_resched_lock(), will reassess.
>
yeah, you need to check it after you've come back and taken the lock.
With 4-level page tables doing this at the pud level will probably
break, I think.
> >
> >
> >> +
> >> + next = kvm_pud_addr_end(addr, end);
> >> + /* TODO: huge PUD not supported, revisit later */
> >
> > BUG_ON(kvm_pud_huge(*pud)) ?
> >
> >> + if (!pud_none(*pud))
> >> + stage2_wp_pmd_range(pud, addr, next);
> >> + } while (pud++, addr = next, addr != end);
> >> +}
> >> +
> >> +/**
> >> + * kvm_mmu_wp_memory_region() - initial write protected of memory region slot
> >
> > I think this should be:
> >
> > ... - write protect stage 2 entries for memory slot
>
> sure.
> >
> >> + * @kvm: The KVM pointer
> >> + * @slot: The memory slot to write protect
> >> + *
> >> + * Called to start logging dirty pages after memory region
> >> + * KVM_MEM_LOG_DIRTY_PAGES operation is called. After this function returns
> >> + * all present PMD and PTEs are write protected in the memory region.
> >> + * Afterwards read of dirty page log can be called. Pages not present are
> >> + * write protected on future access in user_mem_abort().
> >> + *
> >> + * Acquires kvm_mmu_lock. Called with kvm->slots_lock mutex acquired,
> >> + * serializing operations for VM memory regions.
> >> + */
> >> +void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot)
> >> +{
> >> + pgd_t *pgd;
> >> + struct kvm_memory_slot *memslot = id_to_memslot(kvm->memslots, slot);
> >> + phys_addr_t addr = memslot->base_gfn << PAGE_SHIFT;
> >> + phys_addr_t end = (memslot->base_gfn + memslot->npages) << PAGE_SHIFT;
> >> + phys_addr_t next;
> >> +
> >> + spin_lock(&kvm->mmu_lock);
> >> + pgd = kvm->arch.pgd + pgd_index(addr);
> >> + do {
> >> + next = kvm_pgd_addr_end(addr, end);
> >> + if (pgd_present(*pgd))
> >> + stage2_wp_pud_range(kvm, pgd, addr, next);
> >> + } while (pgd++, addr = next, addr != end);
> >
> > you probably want to factor out everything beginnign with pgd = (and the
> > variable declarations) into stage2_wp_range(kvm, start, end).
>
> So I understand define another function stage2_wp_range()?
> If that's it that's fine.
yes, take the spinlock etc. in this function, but when it comes to
actually walking the tables etc. do this in a separate function - this
can then be reused for other purposes.
> >
> >> + kvm_flush_remote_tlbs(kvm);
> >> + spin_unlock(&kvm->mmu_lock);
> >> +}
> >> +
> >> static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> >> struct kvm_memory_slot *memslot,
> >> unsigned long fault_status)
> >> --
> >> 1.7.9.5
> >>
> >
> > This is moving in the right direction.
> >
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
