On Sat, Jul 11, 2020 at 02:13:43AM -0700, Ram Pai wrote:
> Merging of pages associated with each memslot of a SVM is
> disabled the page is migrated in H_SVM_PAGE_IN handler.
>
> This operation should have been done much earlier; the moment the VM
> is initiated for secure-transition. Delaying this operation, increases
> the probability for those pages to acquire new references , making it
> impossible to migrate those pages in H_SVM_PAGE_IN handler.
>
> Disable page-migration in H_SVM_INIT_START handling.
While it is a good idea to disable KSM merging for all VMAs during
H_SVM_INIT_START, I am curious if you did observe an actual case of
ksm_madvise() failing which resulted in subsequent H_SVM_PAGE_IN
failing to migrate?
>
> Signed-off-by: Ram Pai <linux...@us.ibm.com>
> ---
> arch/powerpc/kvm/book3s_hv_uvmem.c | 96
> +++++++++++++++++++++++++++++---------
> 1 file changed, 74 insertions(+), 22 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c
> b/arch/powerpc/kvm/book3s_hv_uvmem.c
> index 3d987b1..bfc3841 100644
> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
> @@ -211,6 +211,65 @@ static bool kvmppc_gfn_is_uvmem_pfn(unsigned long gfn,
> struct kvm *kvm,
> return false;
> }
>
> +static int kvmppc_memslot_page_merge(struct kvm *kvm,
> + struct kvm_memory_slot *memslot, bool merge)
> +{
> + unsigned long gfn = memslot->base_gfn;
> + unsigned long end, start = gfn_to_hva(kvm, gfn);
> + int ret = 0;
> + struct vm_area_struct *vma;
> + int merge_flag = (merge) ? MADV_MERGEABLE : MADV_UNMERGEABLE;
> +
> + if (kvm_is_error_hva(start))
> + return H_STATE;
This and other cases below seem to be a new return value from
H_SVM_INIT_START. May be update the documentation too along with
this patch?
> +
> + end = start + (memslot->npages << PAGE_SHIFT);
> +
> + down_write(&kvm->mm->mmap_sem);
When you rebase the patches against latest upstream you may want to
replace the above and other instances by mmap_write/read_lock().
> + do {
> + vma = find_vma_intersection(kvm->mm, start, end);
> + if (!vma) {
> + ret = H_STATE;
> + break;
> + }
> + ret = ksm_madvise(vma, vma->vm_start, vma->vm_end,
> + merge_flag, &vma->vm_flags);
> + if (ret) {
> + ret = H_STATE;
> + break;
> + }
> + start = vma->vm_end + 1;
> + } while (end > vma->vm_end);
> +
> + up_write(&kvm->mm->mmap_sem);
> + return ret;
> +}
> +
> +static int __kvmppc_page_merge(struct kvm *kvm, bool merge)
> +{
> + struct kvm_memslots *slots;
> + struct kvm_memory_slot *memslot;
> + int ret = 0;
> +
> + slots = kvm_memslots(kvm);
> + kvm_for_each_memslot(memslot, slots) {
> + ret = kvmppc_memslot_page_merge(kvm, memslot, merge);
> + if (ret)
> + break;
> + }
> + return ret;
> +}
> +
> +static inline int kvmppc_disable_page_merge(struct kvm *kvm)
> +{
> + return __kvmppc_page_merge(kvm, false);
> +}
> +
> +static inline int kvmppc_enable_page_merge(struct kvm *kvm)
> +{
> + return __kvmppc_page_merge(kvm, true);
> +}
> +
> unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
> {
> struct kvm_memslots *slots;
> @@ -232,11 +291,18 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
> return H_AUTHORITY;
>
> srcu_idx = srcu_read_lock(&kvm->srcu);
> +
> + /* disable page-merging for all memslot */
> + ret = kvmppc_disable_page_merge(kvm);
> + if (ret)
> + goto out;
> +
> + /* register the memslot */
> slots = kvm_memslots(kvm);
> kvm_for_each_memslot(memslot, slots) {
> if (kvmppc_uvmem_slot_init(kvm, memslot)) {
> ret = H_PARAMETER;
> - goto out;
> + break;
> }
> ret = uv_register_mem_slot(kvm->arch.lpid,
> memslot->base_gfn << PAGE_SHIFT,
> @@ -245,9 +311,12 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
> if (ret < 0) {
> kvmppc_uvmem_slot_free(kvm, memslot);
> ret = H_PARAMETER;
> - goto out;
> + break;
> }
> }
> +
> + if (ret)
> + kvmppc_enable_page_merge(kvm);
Is there any use of enabling KSM merging in the failure path here?
Won't UV terminate the VM if H_SVM_INIT_START fails? If there is no need,
you can do away with some extra routines above.
> out:
> srcu_read_unlock(&kvm->srcu, srcu_idx);
> return ret;
> @@ -384,7 +453,7 @@ static struct page *kvmppc_uvmem_get_page(unsigned long
> gpa, struct kvm *kvm)
> */
> static int kvmppc_svm_page_in(struct vm_area_struct *vma, unsigned long
> start,
> unsigned long end, unsigned long gpa, struct kvm *kvm,
> - unsigned long page_shift, bool *downgrade)
> + unsigned long page_shift)
> {
> unsigned long src_pfn, dst_pfn = 0;
> struct migrate_vma mig;
> @@ -400,18 +469,6 @@ static int kvmppc_svm_page_in(struct vm_area_struct
> *vma, unsigned long start,
> mig.src = &src_pfn;
> mig.dst = &dst_pfn;
>
> - /*
> - * We come here with mmap_sem write lock held just for
> - * ksm_madvise(), otherwise we only need read mmap_sem.
> - * Hence downgrade to read lock once ksm_madvise() is done.
> - */
> - ret = ksm_madvise(vma, vma->vm_start, vma->vm_end,
> - MADV_UNMERGEABLE, &vma->vm_flags);
I haven't seen the subsequent patches yet, but guess you are
taking care of disabling KSM mering for hot-plugged memory too.
Regards,
Bharata.
