Re: [PATCH V2] KVM: SEV: fix wrong pinning of pages

[Ge Yang <yangge1116@126.com>]

在 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(&params, 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, 
> &region->npages, 1);
> +       region->pages = sev_pin_memory(kvm, range->addr, range->size, 
> &region->npages,
> +                                      FOLL_WRITE | FOLL_LONGTERM);
>          if (IS_ERR(region->pages)) {
>                  ret = PTR_ERR(region->pages);
>                  mutex_unlock(&kvm->lock);
> 
> 
> Thoughts?
>