From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4571D21018A for ; Tue, 17 Mar 2026 13:01:15 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773752477; cv=none; b=H6hm4uE7zcB8Yl1Ld6yoytiJ5zNQa5IWqci1RzX6acCa6O2jdFGZIVkZGJvaTPnhIgXVT4AR9wvGRkLkbihw7pVtPJomCV+C8aggpceqPaDGbmskqaU0oL22iktbslInAWzTzHWAJBUUdvEdQM5izV2mzPi5cV4t5GUHIlDUTzU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773752477; c=relaxed/simple; bh=7A44tczo8ocOAQ/tPbtu/XAdng69GnqP+uKcPripEec=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=UUmfzds9/dD1V7wR2Y6h58KWntTf80V4x2y2TZvdqO9Ebkirw+bMWxkKaMdAlSjyFn/dg2qTR1BHq2plh/zVnLuLn2mJUU5pXfiwtvml9UC3pQjG6PkHirGhSNg2n6O8IypUbwK2S4gEYfshm9kCwMSQ4VXpkEqyQKThf0YUsB4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=LIspQ8vJ; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=NjPFhpnI; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="LIspQ8vJ"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="NjPFhpnI" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1773752474; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=n20AEbq+bhcepRzgl/xYIcvQpLC81/e/06fpntOlePw=; b=LIspQ8vJGKW5F1DorgWnbcJghmNrt8NcRLf75Aott4WoCEWCUZelp/D8lFQOus1gUCpUU0 NC6+mJNplMaKJsgIfWrUtSDbZEdDzFZj5O7jLAv7E2sjssmksvwWCmZWtdaMigQJihR0Ku GyUR/PNpgJg9Fb/MNRdqrUNtTvo0UTU= Received: from mail-lj1-f200.google.com (mail-lj1-f200.google.com [209.85.208.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-509-c84uLBpQNQqknI9k4DRe1A-1; Tue, 17 Mar 2026 09:01:12 -0400 X-MC-Unique: c84uLBpQNQqknI9k4DRe1A-1 X-Mimecast-MFC-AGG-ID: c84uLBpQNQqknI9k4DRe1A_1773752471 Received: by mail-lj1-f200.google.com with SMTP id 38308e7fff4ca-38a4536bca4so27281071fa.3 for ; Tue, 17 Mar 2026 06:01:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1773752471; x=1774357271; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=n20AEbq+bhcepRzgl/xYIcvQpLC81/e/06fpntOlePw=; b=NjPFhpnIf2WQfSPc2PtevEJPCJwWuSBSqteGmbGzU4cZ5eVrcpfceCd07ykRIYiCvz FPNWMjiGBGH41GVpBL2FOblYIgOquFkH1CHgR9cfxxRIPxl9e85GckjWkjlCiVaZMcBq NXpTo/s6NyJrHHFsAE6/kkrcvbZX8Sw4iRbLF1EMJoaU2E6eqwmqOX2Oc03xeL7413NZ VizXaZWU4XewF+TtOnAYs0n2kbPdRK8Xwj1Eaqgpv13XnYw9lBvhSfQPSE4btqkhlRVO GDfQLBDAj+bvcsTqXNl/DUi2wbBMtsNqP5P40Vqa2zrFE9kMGrQn11EYobXNw1iOYyzv gKjA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1773752471; x=1774357271; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=n20AEbq+bhcepRzgl/xYIcvQpLC81/e/06fpntOlePw=; b=XVb8iDBGLqbr228p5B7gz4jUtu66xlUrFyfU+x2QeHzwC/BltaKnQqIYHaEr+47o3x qH28Pwd3RKuAtQ+cRg4stVYpLJ+vLpaME7joa43Myndy5Mm7jvKWJQxP9X+gYyernoGM PRMt1wAuxYgEfHDBTQmtoGwgvd07Vv/0V0RC3L2WQ+oYOsowJipCvwyHb7aBJwUDNXFc io9SFtvW7A9vKx1kvAXHkPhjv/vNV6RB9YKgYOSGKnHDUlUgzh1F4L5YjYI44vkYcLxO lkILDU02ST7AOPh1L3xDoUThRMDLdG5CS6ob1dBz3FKH5vpIsJdRHiDIMoM7zPkjtH3e dDwQ== X-Gm-Message-State: AOJu0YxgDu3kgbO0YfgULnxpWWBqCD2cxjo/2dYje4gPmz7u8ZgKbZAQ 5aoLET3osCpNLx8tHWueW23gPPwy4zfJm4shRdsnVdRndxxGr0LQhc7isAuQKpa7V0u0u4yv1fD WaTqakhF1FP3c/qopHIOskdAGR+IeW8XXxbMgwSsWRQSxwdq6iKT2WZrpmur+IT5k X-Gm-Gg: ATEYQzxTI5xPwDGmbX6wa2l+iFJXX9Wzrv6XnpJtJZD51BUP+x8OUeX8lkg6+w3l6CR 6XnkYOPEML7B/WCi8yEMRf9RiNv/BQ/cbo51QGFE1AYZUgm9CNiqx6pscFcROQVUXWDmfXeZkvY zwMBgy1aq6elHZt5IdSyptRc28PoURJxnRMi0QD2fTgLj66b7EapUmxImwB7s2a5t0YdQ+nRwFt X04h6CC964jOTm/5sVSwsCaRq+ufeExKHYlgvzempvxwV+5TdSlGNWeAL1AOjI/qIAvtAXtpX22 jnXbwInYtZczCLC347MPmuR/7TMsi40ev02e1pe9PVlbmPiHlBzgYszjYN31gkQG9Au4sldyrJN 2JevI1zgzmgsm8EOFSbZG+lmAqIlruOF0EgqV+9oTEXIE6dk= X-Received: by 2002:a2e:9251:0:b0:38a:3668:283c with SMTP id 38308e7fff4ca-38a89811c32mr41227361fa.33.1773752470637; Tue, 17 Mar 2026 06:01:10 -0700 (PDT) X-Received: by 2002:a2e:9251:0:b0:38a:3668:283c with SMTP id 38308e7fff4ca-38a89811c32mr41227161fa.33.1773752470026; Tue, 17 Mar 2026 06:01:10 -0700 (PDT) Received: from [192.168.1.86] (85-23-51-1.bb.dnainternet.fi. [85.23.51.1]) by smtp.gmail.com with ESMTPSA id 38308e7fff4ca-38a67d61f25sm41532751fa.7.2026.03.17.06.01.09 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 17 Mar 2026 06:01:09 -0700 (PDT) Message-ID: <59d3c0e5-23fb-4a30-825a-d90fd07bd34e@redhat.com> Date: Tue, 17 Mar 2026 15:01:09 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 2/6] mm: Add helper to convert HMM pfn to migrate pfn To: "David Hildenbrand (Arm)" , linux-mm@kvack.org Cc: linux-kernel@vger.kernel.org, Jason Gunthorpe , Leon Romanovsky , Alistair Popple , Balbir Singh , Zi Yan , Matthew Brost References: <20260211081301.2940672-1-mpenttil@redhat.com> <20260211081301.2940672-3-mpenttil@redhat.com> <3c0578a9-ce1c-4d26-86d5-681cae4b8200@kernel.org> Content-Language: en-US From: =?UTF-8?Q?Mika_Penttil=C3=A4?= In-Reply-To: <3c0578a9-ce1c-4d26-86d5-681cae4b8200@kernel.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hi, On 3/17/26 11:05, David Hildenbrand (Arm) wrote: > On 2/11/26 09:12, mpenttil@redhat.com wrote: >> From: Mika Penttilä >> >> 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" ? Yes will correct. >> world. >> >> Cc: David Hildenbrand >> Cc: Jason Gunthorpe >> Cc: Leon Romanovsky >> Cc: Alistair Popple >> Cc: Balbir Singh >> Cc: Zi Yan >> Cc: Matthew Brost >> Suggested-by: Alistair Popple >> Signed-off-by: Mika Penttilä >> --- >> 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 >> >> struct mmu_interval_notifier; >> - >> +struct migrate_vma; > Keep the empty line, please. ack > >> /* >> * 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. > Agreed needs more documentation. Look below for explanation also. > Shouldn't we document what HMM_PFN_COMPOUND and HMM_PFN_MIGRATE mean as well somewhere? I think they deserve some words as well. > >> * >> * 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). Yes I moved for visibility to migrate_device. > Is there any reason we can't move that into hmm_pfn_flags? Can't think of any, I think that would be good to do. > >> + >> /* >> * 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. Yes, a range. Will fix. > >> */ >> 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 >> +#include >> #include >> #include >> #include >> @@ -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" ? Tried to say range->migrate's src and dst arrays, cpages and npages get populated. > > Why can't we just pass the migrate_vma? Or is there another use case for > adding the migrate_vma to hmm_range? We have to pass hmm_range anyway because of range->hmm_pfns. migrate_hmm_range_setup() is supposed to be called after hmm_range_fault(), which populates migrate_vma's .start .end and .vma. So passing a hmm_range also emphasizes the intended usage. >> + * >> + * 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 :) Yes will add documentation. The caller of hmm_range_fault() is supposed to fill migrate_vma's pgmap_owner, src and dst array pointers, and flags for migrate direction. (there is minor change in v6 https://lore.kernel.org/linux-mm/20260316062407.3354636-1-mpenttil@redhat.com/) for the flags requirement. > >> + */ >> +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. ack > >> + * 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 As is HMM_PFN_MIGRATE is not supposed to be set without HMM_PFN_VALID... But, I just realized that in order to allow hmm pfn 0 we have to make HMM_PFN_MIGRATE alone mean the "empty page" (hole, pte_none, zero page) > >> + 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? For current code the above would be ok. With HMM_PFN_MIGRATE alone for empty page have to adjust. Will fix. > Thanks for the review! --Mika