Re: [PATCH v5 2/6] mm: Add helper to convert HMM pfn to migrate pfn

From: David Hildenbrand (Arm)

Date: Tue Mar 17 2026 - 05:05:55 EST

On 2/11/26 09:12, mpenttil@xxxxxxxxxx wrote:
> From: Mika Penttilä <mpenttil@xxxxxxxxxx>
>
> The unified HMM/migrate_device pagewalk does the "collecting"
> in HMM side, so need a helper to transfer pfns to migrate_vma

s/in/on the/ ?

"so we" ?

"to the" ?

> world.
>
> Cc: David Hildenbrand <david@xxxxxxxxxx>
> Cc: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Cc: Leon Romanovsky <leonro@xxxxxxxxxx>
> Cc: Alistair Popple <apopple@xxxxxxxxxx>
> Cc: Balbir Singh <balbirs@xxxxxxxxxx>
> Cc: Zi Yan <ziy@xxxxxxxxxx>
> Cc: Matthew Brost <matthew.brost@xxxxxxxxx>
> Suggested-by: Alistair Popple <apopple@xxxxxxxxxx>
> Signed-off-by: Mika Penttilä <mpenttil@xxxxxxxxxx>
> ---
> include/linux/hmm.h | 18 +++++++++--
> include/linux/migrate.h | 3 +-
> mm/hmm.c | 6 ----
> mm/migrate_device.c | 69 +++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 87 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index db75ffc949a7..b5418e318782 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -12,7 +12,7 @@
> #include <linux/mm.h>
>
> struct mmu_interval_notifier;
> -
> +struct migrate_vma;

Keep the empty line, please.

> /*
> * On output:
> * 0 - The page is faultable and a future call with
> @@ -27,6 +27,7 @@ struct mmu_interval_notifier;
> * HMM_PFN_P2PDMA_BUS - Bus mapped P2P transfer
> * HMM_PFN_DMA_MAPPED - Flag preserved on input-to-output transformation
> * to mark that page is already DMA mapped
> + * HMM_PFN_MIGRATE - Migrate PTE installed

I have no idea what exactly that means. Can we a bit clearer?
Especially, what does it mean when this is set alongside HMM_PFN_VALID? What if it
is not set alongside HMM_PFN_VALID.

Shouldn't we document what HMM_PFN_COMPOUND and HMM_PFN_MIGRATE mean as well somewhere?

> *
> * On input:
> * 0 - Return the current state of the page, do not fault it.
> @@ -34,6 +35,7 @@ struct mmu_interval_notifier;
> * will fail
> * HMM_PFN_REQ_WRITE - The output must have HMM_PFN_WRITE or hmm_range_fault()
> * will fail. Must be combined with HMM_PFN_REQ_FAULT.
> + * HMM_PFN_REQ_MIGRATE - For default_flags, request to migrate to device
> */
> enum hmm_pfn_flags {
> /* Output fields and flags */
> @@ -48,15 +50,25 @@ enum hmm_pfn_flags {
> HMM_PFN_P2PDMA = 1UL << (BITS_PER_LONG - 5),
> HMM_PFN_P2PDMA_BUS = 1UL << (BITS_PER_LONG - 6),
>
> - HMM_PFN_ORDER_SHIFT = (BITS_PER_LONG - 11),
> + /* Migrate request */
> + HMM_PFN_MIGRATE = 1UL << (BITS_PER_LONG - 7),
> + HMM_PFN_COMPOUND = 1UL << (BITS_PER_LONG - 8),
> + HMM_PFN_ORDER_SHIFT = (BITS_PER_LONG - 13),
>
> /* Input flags */
> HMM_PFN_REQ_FAULT = HMM_PFN_VALID,
> HMM_PFN_REQ_WRITE = HMM_PFN_WRITE,
> + HMM_PFN_REQ_MIGRATE = HMM_PFN_MIGRATE,
>
> HMM_PFN_FLAGS = ~((1UL << HMM_PFN_ORDER_SHIFT) - 1),
> };
>
> +enum {
> + /* These flags are carried from input-to-output */
> + HMM_PFN_INOUT_FLAGS = HMM_PFN_DMA_MAPPED | HMM_PFN_P2PDMA |
> + HMM_PFN_P2PDMA_BUS,
> +};

Why is that an anonymous enum with a single value? Ah, you are moving
that. This should be spelled out in the patch description (and why you
are doing it).

Is there any reason we can't move that into hmm_pfn_flags?

> +
> /*
> * hmm_pfn_to_page() - return struct page pointed to by a device entry
> *
> @@ -107,6 +119,7 @@ static inline unsigned int hmm_pfn_to_map_order(unsigned long hmm_pfn)
> * @default_flags: default flags for the range (write, read, ... see hmm doc)
> * @pfn_flags_mask: allows to mask pfn flags so that only default_flags matter
> * @dev_private_owner: owner of device private pages
> + * @migrate: structure for migrating the associated vma

Is it really for "migrating a vma"? I assume it's for migrating a range
of a VMA.

> */
> struct hmm_range {
> struct mmu_interval_notifier *notifier;
> @@ -117,6 +130,7 @@ struct hmm_range {
> unsigned long default_flags;
> unsigned long pfn_flags_mask;
> void *dev_private_owner;
> + struct migrate_vma *migrate;
> };
>
> /*
> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index 26ca00c325d9..8e6c28efd4f8 100644
> --- a/include/linux/migrate.h
> +++ b/include/linux/migrate.h
> @@ -3,6 +3,7 @@
> #define _LINUX_MIGRATE_H
>
> #include <linux/mm.h>
> +#include <linux/hmm.h>
> #include <linux/mempolicy.h>
> #include <linux/migrate_mode.h>
> #include <linux/hugetlb.h>
> @@ -192,7 +193,7 @@ void migrate_device_pages(unsigned long *src_pfns, unsigned long *dst_pfns,
> unsigned long npages);
> void migrate_device_finalize(unsigned long *src_pfns,
> unsigned long *dst_pfns, unsigned long npages);
> -
> +void migrate_hmm_range_setup(struct hmm_range *range);
> #endif /* CONFIG_MIGRATION */
>
> #endif /* _LINUX_MIGRATE_H */
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 4ec74c18bef6..21ff99379836 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -41,12 +41,6 @@ enum {
> HMM_NEED_ALL_BITS = HMM_NEED_FAULT | HMM_NEED_WRITE_FAULT,
> };
>
> -enum {
> - /* These flags are carried from input-to-output */
> - HMM_PFN_INOUT_FLAGS = HMM_PFN_DMA_MAPPED | HMM_PFN_P2PDMA |
> - HMM_PFN_P2PDMA_BUS,
> -};
> -
> static int hmm_pfns_fill(unsigned long addr, unsigned long end,
> struct hmm_range *range, unsigned long cpu_flags)
> {
> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> index 23379663b1e1..c773a82ea1ed 100644
> --- a/mm/migrate_device.c
> +++ b/mm/migrate_device.c
> @@ -1489,3 +1489,72 @@ int migrate_device_coherent_folio(struct folio *folio)
> return 0;
> return -EBUSY;
> }
> +
> +/**
> + * migrate_hmm_range_setup() - prepare to migrate a range of memory
> + * @range: contains pointer to migrate_vma to be populated

"pointer to hmm_range to be migrated" ?

Why can't we just pass the migrate_vma? Or is there another use case for
adding the migrate_vma to hmm_range?

> + *
> + * When collecting happens by hmm_range_fault(), this populates
> + * the migrate->src[] and migrate->dst[] using range->hmm_pfns[].
> + * Also, migrate->cpages and migrate->npages get initialized.

Can you please document what exactly this function does with
the magical HMM_PFN_VALID | HMM_PFN_MIGRATE, and why it skips
everything else?

What is the caller supposed to setup?

I am getting the feeling that this is severely under-documented :)

> + */
> +void migrate_hmm_range_setup(struct hmm_range *range)
> +{
> +
> + struct migrate_vma *migrate = range->migrate;
> +
> + if (!migrate)
> + return;
> +
> + migrate->npages = (migrate->end - migrate->start) >> PAGE_SHIFT;
> + migrate->cpages = 0;
> +
> + for (unsigned long i = 0; i < migrate->npages; i++) {
> +
> + unsigned long pfn = range->hmm_pfns[i];
> +
> + pfn &= ~HMM_PFN_INOUT_FLAGS;
> +
> + /*
> + *

Please don't add unnecessary empty comment lines above/below the text.

> + * Don't do migration if valid and migrate flags are not both set.
> + *
> + */

/*
* There is nothing to migrate if the entry is not valid
* migration entry.
*/

Which raises the question:

When would HMM_PFN_MIGRATE be set without HMM_PFN_VALID

> + if ((pfn & (HMM_PFN_VALID | HMM_PFN_MIGRATE)) !=
> + (HMM_PFN_VALID | HMM_PFN_MIGRATE)) {
> + migrate->src[i] = 0;
> + migrate->dst[i] = 0;
> + continue;
> + }
> +
> + migrate->cpages++;
> +
> + /*
> + *
> + * The zero page is encoded in a special way, valid and migrate is
> + * set, and pfn part is zero. Encode specially for migrate also.
> + *
> + */
> + if (pfn == (HMM_PFN_VALID|HMM_PFN_MIGRATE)) {

I think you can simplify all that by doing a

if (pfn & ~(HMM_PFN_VALID|HMM_PFN_COMPOUND|HMM_PFN_MIGRATE))
migrate->src[i] = migrate_pfn(page_to_pfn(hmm_pfn_to_page(pfn)));
else
migrate->src[i] = 0;

migrate->src[i] |= (pfn & HMM_PFN_WRITE) ? MIGRATE_PFN_WRITE : 0;
migrate->src[i] |= (pfn & HMM_PFN_COMPOUND) ? MIGRATE_PFN_COMPOUND : 0;
migrate->dst[i] = 0;

And I wonder whether you could replace the (HMM_PFN_VALID|HMM_PFN_COMPOUND|HMM_PFN_MIGRATE)
by HMM_PFN_FLAGS, to only check for non-zero PFNs?

--
Cheers,

David