* [PATCH V2] KVM: SEV: fix wrong pinning of pages @ 2025-01-10 11:42 yangge1116 2025-01-10 11:53 ` David Hildenbrand 0 siblings, 1 reply; 3+ messages in thread From: yangge1116 @ 2025-01-10 11:42 UTC (permalink / raw) To: pbonzini Cc: kvm, linux-kernel, seanjc, 21cnbao, david, baolin.wang, liuzixing, yangge From: yangge <yangge1116@126.com> When pin_user_pages_fast() pins SEV guest memory without the FOLL_LONGTERM flag, the pages will not get migrated out of MIGRATE_CMA/ ZONE_MOVABLE, violating these mechanisms to avoid fragmentation with unmovable pages, for example making CMA allocations fail. To address the aforementioned problem, we propose adding the FOLL_LONGTERM flag to the pin_user_pages_fast() function. Signed-off-by: yangge <yangge1116@126.com> --- V2: - update code and commit message suggested by David arch/x86/kvm/svm/sev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 943bd07..96f3b8e 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -630,6 +630,7 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr, unsigned long locked, lock_limit; struct page **pages; unsigned long first, last; + unsigned int flags = FOLL_LONGTERM; int ret; lockdep_assert_held(&kvm->lock); @@ -662,8 +663,10 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr, if (!pages) return ERR_PTR(-ENOMEM); + flags |= write ? FOLL_WRITE : 0; + /* Pin the user virtual address. */ - npinned = pin_user_pages_fast(uaddr, npages, write ? FOLL_WRITE : 0, pages); + npinned = pin_user_pages_fast(uaddr, npages, flags, pages); if (npinned != npages) { pr_err("SEV: Failure locking %lu pages.\n", npages); ret = -ENOMEM; -- 2.7.4 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH V2] KVM: SEV: fix wrong pinning of pages 2025-01-10 11:42 [PATCH V2] KVM: SEV: fix wrong pinning of pages yangge1116 @ 2025-01-10 11:53 ` David Hildenbrand 2025-01-11 6:14 ` Ge Yang 0 siblings, 1 reply; 3+ messages in thread From: David Hildenbrand @ 2025-01-10 11:53 UTC (permalink / raw) To: yangge1116, pbonzini Cc: kvm, linux-kernel, seanjc, 21cnbao, baolin.wang, liuzixing On 10.01.25 12:42, yangge1116@126.com wrote: > From: yangge <yangge1116@126.com> > > When pin_user_pages_fast() pins SEV guest memory without the > FOLL_LONGTERM flag, the pages will not get migrated out of MIGRATE_CMA/ > ZONE_MOVABLE, violating these mechanisms to avoid fragmentation with > unmovable pages, for example making CMA allocations fail. > > To address the aforementioned problem, we propose adding the > FOLL_LONGTERM flag to the pin_user_pages_fast() function. > > Signed-off-by: yangge <yangge1116@126.com> > --- > > V2: > - update code and commit message suggested by David > > arch/x86/kvm/svm/sev.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 943bd07..96f3b8e 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -630,6 +630,7 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr, > unsigned long locked, lock_limit; > struct page **pages; > unsigned long first, last; > + unsigned int flags = FOLL_LONGTERM; > int ret; > > lockdep_assert_held(&kvm->lock); > @@ -662,8 +663,10 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr, > if (!pages) > return ERR_PTR(-ENOMEM); > > + flags |= write ? FOLL_WRITE : 0; > + > /* Pin the user virtual address. */ > - npinned = pin_user_pages_fast(uaddr, npages, write ? FOLL_WRITE : 0, pages); > + npinned = pin_user_pages_fast(uaddr, npages, flags, pages); > if (npinned != npages) { > pr_err("SEV: Failure locking %lu pages.\n", npages); > ret = -ENOMEM; Sorry, looking into it in more detail, there are some paths that immediately unpin again, and don't seem to have longterm semantics. Could we do the following, so longterm pinning would be limited to the memory regions that might stay pinned possible forever? diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 943bd074a5d37..4b0f03f0ea741 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -622,7 +622,7 @@ static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp) static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr, unsigned long ulen, unsigned long *n, - int write) + int flags) { struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; unsigned long npages, size; @@ -663,7 +663,7 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr, return ERR_PTR(-ENOMEM); /* Pin the user virtual address. */ - npinned = pin_user_pages_fast(uaddr, npages, write ? FOLL_WRITE : 0, pages); + npinned = pin_user_pages_fast(uaddr, npages, flags, pages); if (npinned != npages) { pr_err("SEV: Failure locking %lu pages.\n", npages); ret = -ENOMEM; @@ -751,7 +751,7 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp) vaddr_end = vaddr + size; /* Lock the user memory. */ - inpages = sev_pin_memory(kvm, vaddr, size, &npages, 1); + inpages = sev_pin_memory(kvm, vaddr, size, &npages, FOLL_WRITE); if (IS_ERR(inpages)) return PTR_ERR(inpages); @@ -1250,7 +1250,8 @@ static int sev_dbg_crypt(struct kvm *kvm, struct kvm_sev_cmd *argp, bool dec) if (IS_ERR(src_p)) return PTR_ERR(src_p); - dst_p = sev_pin_memory(kvm, dst_vaddr & PAGE_MASK, PAGE_SIZE, &n, 1); + dst_p = sev_pin_memory(kvm, dst_vaddr & PAGE_MASK, PAGE_SIZE, &n, + FOLL_WRITE); if (IS_ERR(dst_p)) { sev_unpin_memory(kvm, src_p, n); return PTR_ERR(dst_p); @@ -1316,7 +1317,8 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp) if (copy_from_user(¶ms, u64_to_user_ptr(argp->data), sizeof(params))) return -EFAULT; - pages = sev_pin_memory(kvm, params.guest_uaddr, params.guest_len, &n, 1); + pages = sev_pin_memory(kvm, params.guest_uaddr, params.guest_len, &n, + FOLL_WRITE); if (IS_ERR(pages)) return PTR_ERR(pages); @@ -1798,7 +1800,7 @@ static int sev_receive_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp) /* Pin guest memory */ guest_page = sev_pin_memory(kvm, params.guest_uaddr & PAGE_MASK, - PAGE_SIZE, &n, 1); + PAGE_SIZE, &n, FOLL_WRITE); if (IS_ERR(guest_page)) { ret = PTR_ERR(guest_page); goto e_free_trans; @@ -2696,7 +2698,8 @@ int sev_mem_enc_register_region(struct kvm *kvm, return -ENOMEM; mutex_lock(&kvm->lock); - region->pages = sev_pin_memory(kvm, range->addr, range->size, ®ion->npages, 1); + region->pages = sev_pin_memory(kvm, range->addr, range->size, ®ion->npages, + FOLL_WRITE | FOLL_LONGTERM); if (IS_ERR(region->pages)) { ret = PTR_ERR(region->pages); mutex_unlock(&kvm->lock); Thoughts? -- Cheers, David / dhildenb ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH V2] KVM: SEV: fix wrong pinning of pages 2025-01-10 11:53 ` David Hildenbrand @ 2025-01-11 6:14 ` Ge Yang 0 siblings, 0 replies; 3+ messages in thread From: Ge Yang @ 2025-01-11 6:14 UTC (permalink / raw) To: David Hildenbrand, pbonzini Cc: kvm, linux-kernel, seanjc, 21cnbao, baolin.wang, liuzixing 在 2025/1/10 19:53, David Hildenbrand 写道: > On 10.01.25 12:42, yangge1116@126.com wrote: >> From: yangge <yangge1116@126.com> >> >> When pin_user_pages_fast() pins SEV guest memory without the >> FOLL_LONGTERM flag, the pages will not get migrated out of MIGRATE_CMA/ >> ZONE_MOVABLE, violating these mechanisms to avoid fragmentation with >> unmovable pages, for example making CMA allocations fail. >> >> To address the aforementioned problem, we propose adding the >> FOLL_LONGTERM flag to the pin_user_pages_fast() function. >> >> Signed-off-by: yangge <yangge1116@126.com> >> --- >> >> V2: >> - update code and commit message suggested by David >> >> arch/x86/kvm/svm/sev.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c >> index 943bd07..96f3b8e 100644 >> --- a/arch/x86/kvm/svm/sev.c >> +++ b/arch/x86/kvm/svm/sev.c >> @@ -630,6 +630,7 @@ static struct page **sev_pin_memory(struct kvm >> *kvm, unsigned long uaddr, >> unsigned long locked, lock_limit; >> struct page **pages; >> unsigned long first, last; >> + unsigned int flags = FOLL_LONGTERM; >> int ret; >> lockdep_assert_held(&kvm->lock); >> @@ -662,8 +663,10 @@ static struct page **sev_pin_memory(struct kvm >> *kvm, unsigned long uaddr, >> if (!pages) >> return ERR_PTR(-ENOMEM); >> + flags |= write ? FOLL_WRITE : 0; >> + >> /* Pin the user virtual address. */ >> - npinned = pin_user_pages_fast(uaddr, npages, write ? FOLL_WRITE : >> 0, pages); >> + npinned = pin_user_pages_fast(uaddr, npages, flags, pages); >> if (npinned != npages) { >> pr_err("SEV: Failure locking %lu pages.\n", npages); >> ret = -ENOMEM; > > Sorry, looking into it in more detail, there are some paths that > immediately unpin again, > and don't seem to have longterm semantics. > > > Could we do the following, so longterm pinning would be limited to the > memory regions > that might stay pinned possible forever? ok, thanks. > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 943bd074a5d37..4b0f03f0ea741 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -622,7 +622,7 @@ static int sev_launch_start(struct kvm *kvm, struct > kvm_sev_cmd *argp) > > static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr, > unsigned long ulen, unsigned long *n, > - int write) > + int flags) > { > struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > unsigned long npages, size; > @@ -663,7 +663,7 @@ static struct page **sev_pin_memory(struct kvm *kvm, > unsigned long uaddr, > return ERR_PTR(-ENOMEM); > > /* Pin the user virtual address. */ > - npinned = pin_user_pages_fast(uaddr, npages, write ? > FOLL_WRITE : 0, pages); > + npinned = pin_user_pages_fast(uaddr, npages, flags, pages); > if (npinned != npages) { > pr_err("SEV: Failure locking %lu pages.\n", npages); > ret = -ENOMEM; > @@ -751,7 +751,7 @@ static int sev_launch_update_data(struct kvm *kvm, > struct kvm_sev_cmd *argp) > vaddr_end = vaddr + size; > > /* Lock the user memory. */ > - inpages = sev_pin_memory(kvm, vaddr, size, &npages, 1); > + inpages = sev_pin_memory(kvm, vaddr, size, &npages, FOLL_WRITE); > if (IS_ERR(inpages)) > return PTR_ERR(inpages); > > @@ -1250,7 +1250,8 @@ static int sev_dbg_crypt(struct kvm *kvm, struct > kvm_sev_cmd *argp, bool dec) > if (IS_ERR(src_p)) > return PTR_ERR(src_p); > > - dst_p = sev_pin_memory(kvm, dst_vaddr & PAGE_MASK, > PAGE_SIZE, &n, 1); > + dst_p = sev_pin_memory(kvm, dst_vaddr & PAGE_MASK, > PAGE_SIZE, &n, > + FOLL_WRITE); > if (IS_ERR(dst_p)) { > sev_unpin_memory(kvm, src_p, n); > return PTR_ERR(dst_p); > @@ -1316,7 +1317,8 @@ static int sev_launch_secret(struct kvm *kvm, > struct kvm_sev_cmd *argp) > if (copy_from_user(¶ms, u64_to_user_ptr(argp->data), > sizeof(params))) > return -EFAULT; > > - pages = sev_pin_memory(kvm, params.guest_uaddr, > params.guest_len, &n, 1); > + pages = sev_pin_memory(kvm, params.guest_uaddr, > params.guest_len, &n, > + FOLL_WRITE); > if (IS_ERR(pages)) > return PTR_ERR(pages); > > @@ -1798,7 +1800,7 @@ static int sev_receive_update_data(struct kvm > *kvm, struct kvm_sev_cmd *argp) > > /* Pin guest memory */ > guest_page = sev_pin_memory(kvm, params.guest_uaddr & PAGE_MASK, > - PAGE_SIZE, &n, 1); > + PAGE_SIZE, &n, FOLL_WRITE); > if (IS_ERR(guest_page)) { > ret = PTR_ERR(guest_page); > goto e_free_trans; > @@ -2696,7 +2698,8 @@ int sev_mem_enc_register_region(struct kvm *kvm, > return -ENOMEM; > > mutex_lock(&kvm->lock); > - region->pages = sev_pin_memory(kvm, range->addr, range->size, > ®ion->npages, 1); > + region->pages = sev_pin_memory(kvm, range->addr, range->size, > ®ion->npages, > + FOLL_WRITE | FOLL_LONGTERM); > if (IS_ERR(region->pages)) { > ret = PTR_ERR(region->pages); > mutex_unlock(&kvm->lock); > > > Thoughts? > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-01-11 6:15 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-01-10 11:42 [PATCH V2] KVM: SEV: fix wrong pinning of pages yangge1116 2025-01-10 11:53 ` David Hildenbrand 2025-01-11 6:14 ` Ge Yang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®