Re: [PATCH] KVM: SEV: drop FOLL_LONGTERM for encrypted region registration

["Gupta, Pankaj" <pankaj.gupta@amd.com>]

Hi David, Sean,

Thank you for the discussion!

>> Yes. For file based mapping we don't allow long term pinning.
>>
>> If we take into account the fragmentation concerns for MIGRATE_CMA and
>> ZONE_MOVABLE allocations
>>
>> solvable with FOLL_LONGTERM, I can think of two options(tested) to allow file
>> based mappings as well:
>>
>> 1. Fallback on FOLL_WRITE when FOLL_LONGTERM fails as suggested by Sean.
> That is just not acceptable, as it breaks random other stuff (MIGRATE_CMA, as
> one example) besides the file-pinning problems that Lorenzo added.
>
> If we're going to hack something in, then that we bypass the file writeback check.
> Not that we don't use FOLL_LONGTERM.
>
> I'd hate to use a GUP flag to indicate "this is a legacy hack", but it clearly isolates the
> issue (needs a better name obviously):
>
>
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index ae9bca4eda5ca..e2c531f914d44 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -1912,6 +1912,9 @@ enum {
>           */
>          FOLL_HONOR_NUMA_FAULT = 1 << 12,
>   
> +       /* TODO */
> +       FOLL_LONGTERM = 1 << 13,
> +
>          /* See also internal only FOLL flags in mm/internal.h */
>   };
>   
> diff --git a/mm/gup.c b/mm/gup.c
> index 0692119b79043..1fa0aa0cdc99d 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1186,8 +1186,8 @@ static bool writable_file_mapping_allowed(struct vm_area_struct *vma,
>           * If we aren't pinning then no problematic write can occur. A long term
>           * pin is the most egregious case so this is the case we disallow.
>           */
> -       if ((gup_flags & (FOLL_PIN | FOLL_LONGTERM)) !=
> -           (FOLL_PIN | FOLL_LONGTERM))
> +       if ((gup_flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_LONGTERM_HACK)) !=
> +           (FOLL_PIN | FOLL_LONGTERM | FOLL_LONGTERM_HACK))
>                  return true;
>   
>          /*
> @@ -2746,7 +2746,7 @@ static bool gup_fast_folio_allowed(struct folio *folio, unsigned int flags)
>           * If we aren't pinning then no problematic write can occur. A long term
>           * pin is the most egregious case so this is the one we disallow.
>           */
> -       if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) ==
> +       if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE | FOLL_LONGTERM_HACK)) ==
>              (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE))
>                  reject_file_backed = true;
>   
> @@ -3180,7 +3180,7 @@ static int gup_fast_fallback(unsigned long start, unsigned long nr_pages,
>          int locked = 0;
>          int ret;
>   
> -       if (WARN_ON_ONCE(gup_flags & ~(FOLL_WRITE | FOLL_LONGTERM |
> +       if (WARN_ON_ONCE(gup_flags & ~(FOLL_WRITE | FOLL_LONGTERM | FOLL_LONGTERM_HACK |
>                                         FOLL_FORCE | FOLL_PIN | FOLL_GET |
>                                         FOLL_FAST_ONLY | FOLL_NOFAULT |
>                                         FOLL_PCI_P2PDMA | FOLL_HONOR_NUMA_FAULT)))
>
David,

Yes, the above approach works with few changes [2]. If it looks okay 
will send a v2 .

Best regards,

Pankaj

[2]

> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index b18c2b2e7d2c..f9af801788b0 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -1911,6 +1911,14 @@ enum {
>          */
>         FOLL_HONOR_NUMA_FAULT = 1 << 12,
>
> +       /*
> +        * Long-term pin without kernel GUP writes.  For callers that pin
> +        * writable file-backed mappings only to prevent migration.  
> Must be
> +        * used with FOLL_PIN and FOLL_LONGTERM.  Bypasses the writable
> +        * file-backed long-term pin restriction in gup.c.
> +        */
> +       FOLL_PIN_NO_GUP_WRITE = 1 << 13,
> +
>         /* See also internal only FOLL flags in mm/internal.h */
>  };
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 0692119b7904..a83d100f7950 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1186,6 +1186,10 @@ static bool 
> writable_file_mapping_allowed(struct vm_area_struct *vma,
>          * If we aren't pinning then no problematic write can occur. A 
> long term
>          * pin is the most egregious case so this is the case we disallow.
>          */
> +       if ((gup_flags & (FOLL_PIN | FOLL_LONGTERM | 
> FOLL_PIN_NO_GUP_WRITE)) ==
> +           (FOLL_PIN | FOLL_LONGTERM | FOLL_PIN_NO_GUP_WRITE))
> +               return true;
> +
>         if ((gup_flags & (FOLL_PIN | FOLL_LONGTERM)) !=
>             (FOLL_PIN | FOLL_LONGTERM))
>                 return true;
> @@ -2530,6 +2534,11 @@ static bool is_valid_gup_args(struct page 
> **pages, int *locked,
>         if (WARN_ON_ONCE(!(gup_flags & FOLL_PIN) && (gup_flags & 
> FOLL_LONGTERM)))
>                 return false;
>
> +       if (WARN_ON_ONCE((gup_flags & FOLL_PIN_NO_GUP_WRITE) &&
> +                        (gup_flags & (FOLL_PIN | FOLL_LONGTERM)) !=
> +                        (FOLL_PIN | FOLL_LONGTERM)))
> +               return false;
> +
>         /* Pages input must be given if using GET/PIN */
>         if (WARN_ON_ONCE((gup_flags & (FOLL_GET | FOLL_PIN)) && !pages))
>                 return false;
> @@ -2747,7 +2756,8 @@ static bool gup_fast_folio_allowed(struct folio 
> *folio, unsigned int flags)
>          * pin is the most egregious case so this is the one we disallow.
>          */
>
> +       if ((gup_flags & (FOLL_PIN | FOLL_LONGTERM | 
> FOLL_PIN_NO_GUP_WRITE)) ==
> +           (FOLL_PIN | FOLL_LONGTERM | FOLL_PIN_NO_GUP_WRITE))
> +               return true;
> +
>         if ((gup_flags & (FOLL_PIN | FOLL_LONGTERM)) !=
>             (FOLL_PIN | FOLL_LONGTERM))
>                 return true;
> @@ -2530,6 +2534,11 @@ static bool is_valid_gup_args(struct page 
> **pages, int *locked,
>         if (WARN_ON_ONCE(!(gup_flags & FOLL_PIN) && (gup_flags & 
> FOLL_LONGTERM)))
>                 return false;
>
> +       if (WARN_ON_ONCE((gup_flags & FOLL_PIN_NO_GUP_WRITE) &&
> +                        (gup_flags & (FOLL_PIN | FOLL_LONGTERM)) !=
> +                        (FOLL_PIN | FOLL_LONGTERM)))
> +               return false;
> +
>         /* Pages input must be given if using GET/PIN */
>         if (WARN_ON_ONCE((gup_flags & (FOLL_GET | FOLL_PIN)) && !pages))
>                 return false;
> @@ -2747,7 +2756,8 @@ static bool gup_fast_folio_allowed(struct folio 
> *folio, unsigned int flags)
>          * pin is the most egregious case so this is the one we disallow.
>          */
>         if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) ==
> -           (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE))
> +           (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE) &&
> +           !(flags & FOLL_PIN_NO_GUP_WRITE))
>                 reject_file_backed = true;
>
>         /* We hold a folio reference, so we can safely access folio 
> fields. */
> @@ -3180,7 +3190,7 @@ static int gup_fast_fallback(unsigned long 
> start, unsigned long nr_pages,
>         int locked = 0;
>         int ret;
>
> -       if (WARN_ON_ONCE(gup_flags & ~(FOLL_WRITE | FOLL_LONGTERM |
> +       if (WARN_ON_ONCE(gup_flags & ~(FOLL_WRITE | FOLL_LONGTERM | 
> FOLL_PIN_NO_GUP_WRITE |
>                                        FOLL_FORCE | FOLL_PIN | FOLL_GET |
>                                        FOLL_FAST_ONLY | FOLL_NOFAULT |
>                                        FOLL_PCI_P2PDMA | 
> FOLL_HONOR_NUMA_FAULT)))
>