On 28.04.25 18:21, Peter Xu wrote:
On Mon, Apr 28, 2025 at 04:58:46PM +0200, David Hildenbrand wrote:
What it does on PAT (only implementation so far ...) is looking up the
memory type to select the caching mode that can be use.
"sanitize" was IMHO a good fit, because we must make sure that we don't use
the wrong caching mode.
update/setup/... don't make that quite clear. Any other suggestions?
I'm very poor on naming.. :( So far anything seems slightly better than
sanitize to me, as the word "sanitize" is actually also used in memtype.c
for other purpose.. see sanitize_phys().
Sure, one can sanitize a lot of things. Here it's the cachemode/pgrpot, in
the other functions it's an address.
Likely we should just call it pfnmap_X_cachemode()/
Set/update don't really fit for X in case pfnmap_X_cachemode() is a NOP.
pfnmap_setup_cachemode() ? Hm.
Sounds good here.
Okay, I'll use that one. If ever something else besides PAT would
require different semantics, they can bother with finding a better name :)
+ * @pfn: the start of the pfn range
+ * @size: the size of the pfn range
+ * @prot: the pgprot to sanitize
+ *
+ * Sanitize the given pgprot for a pfn range, for example, adjusting the
+ * cachemode.
+ *
+ * This function cannot fail for a single page, but can fail for multiple
+ * pages.
+ *
+ * Returns 0 on success and -EINVAL on error.
+ */
+int pfnmap_sanitize_pgprot(unsigned long pfn, unsigned long size,
+ pgprot_t *prot);
extern int track_pfn_copy(struct vm_area_struct *dst_vma,
struct vm_area_struct *src_vma, unsigned long *pfn);
extern void untrack_pfn_copy(struct vm_area_struct *dst_vma,
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index fdcf0a6049b9f..b8ae5e1493315 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1455,7 +1455,9 @@ vm_fault_t vmf_insert_pfn_pmd(struct vm_fault *vmf, pfn_t
pfn, bool write)
return VM_FAULT_OOM;
}
- track_pfn_insert(vma, &pgprot, pfn);
+ if (pfnmap_sanitize_pgprot(pfn_t_to_pfn(pfn), PAGE_SIZE, &pgprot))
+ return VM_FAULT_FALLBACK;
Would "pgtable" leak if it fails? If it's PAGE_SIZE, IIUC it won't ever
trigger, though.
Maybe we could have a "void pfnmap_sanitize_pgprot_pfn(&pgprot, pfn)" to
replace track_pfn_insert() and never fail? Dropping vma ref is definitely
a win already in all cases.
It could be a simple wrapper around pfnmap_sanitize_pgprot(), yes. That's
certainly helpful for the single-page case.
Regarding never failing here: we should check the whole range. We have to
make sure that none of the pages has a memory type / caching mode that is
incompatible with what we setup.
Would it happen in real world?
IIUC per-vma registration needs to happen first, which checks for
memtype
conflicts in the first place, or reserve_pfn_range() could already have
failed.
Here it's the fault path looking up the memtype, so I would expect it is
guaranteed all pfns under the same vma is following the verified (and same)
memtype?
The whole point of track_pfn_insert() is that it is used when we *don't* use
reserve_pfn_range()->track_pfn_remap(), no?
track_pfn_remap() would check the whole range that gets mapped, so
track_pfn_insert() user must similarly check the whole range that gets
mapped.
Note that even track_pfn_insert() is already pretty clear on the intended
usage: "called when a _new_ single pfn is established"
We need to define "new" then.. But I agree it's not crystal clear at
least. I think I just wasn't the first to assume it was reserved, see this
(especially, the "Expectation" part..):
commit 5180da410db6369d1f95c9014da1c9bc33fb043e
Author: Suresh Siddha <suresh.b.sid...@intel.com>
Date: Mon Oct 8 16:28:29 2012 -0700
x86, pat: separate the pfn attribute tracking for remap_pfn_range and
vm_insert_pfn
With PAT enabled, vm_insert_pfn() looks up the existing pfn memory
attribute and uses it. Expectation is that the driver reserves the
memory attributes for the pfn before calling vm_insert_pfn().
It's all confusing.
We do have the following functions relevant in pat code:
(1) memtype_reserve(): used by ioremap and set_memory_XX
(2) memtype_reserve_io(): used by iomap
(3) reserve_pfn_range(): only remap_pfn_range() calls it
(4) arch_io_reserve_memtype_wc()
Which one would perform the reservation for, say, vfio?
I agree that if there would be a guarantee/expectation that all PFNs
have the same memtype (from previous reservation), it would be
sufficient to check a single PFN, and we could document that. I just
don't easily see where that reservation is happening.
So a pointer to that would be appreciated!
--
Cheers,
David / dhildenb
