linuxkpi: honor the prot argument in vmap

[ashafer]

Summary

This adds missing support to the vmap function to respect what the user
requested via the prot argument. This matches what we do for
linuxkpi_vmap_pfn. We create a memattr from the passed in prot and apply
it to all mapped pages. This also updates vunmap to clear any
non-default memattrs that are set before we return the page to the
vm_page allocator.

We hit this in the wild with drm-kmod, specifically when the i915 driver
tries to initialize firmware. The firmware is supposed to be mapped as
write combined but doesn't due to us not honoring prot, which goes on to
fail due to the memory not being visible to the GPU properly.

[ashafer]

I found that this was needed for the GT1 hardware unit to come up with GuC firmware enabled on meteorlake

[bz]

In D59436#1363212, @ashafer wrote:

I found that this was needed for the GT1 hardware unit to come up with GuC firmware enabled on meteorlake

Can you tell us in which version and possibly where the code (argument) comes from?
I am trying to understand how this is supposed to work but cannot find many PAGE_* definitions (well PAGE_KERNEL[_IO]) in LinuxKPI.

sys/compat/linuxkpi/common/src/linux_page.c

426

We should probably fix the prot argument type as well into something resembling "unsigned long" or uint64_t, like pgprot_t.
I am not sure if it matters for LinuxKPI given vm_memattr_t is a char, but for "correctness".
Probably a separate commit?

[ashafer]

It gets called from drm-kmod, in the case of i915 this seems to usually originate from i915_gem_object_map_page()->vmap() when the caller uses I915_MAP_WC. There's quite a few paths that make it to using this function, some of them will set I915_MAP_(WB|WC) such as from intel_guc_init->intel_uc_fw_init()

[bz]

In D59436#1363989, @ashafer wrote:

It gets called from drm-kmod, in the case of i915 this seems to usually originate from i915_gem_object_map_page()->vmap() when the caller uses I915_MAP_WC. There's quite a few paths that make it to using this function, some of them will set I915_MAP_(WB|WC) such as from intel_guc_init->intel_uc_fw_init()

Let's stay with this example: I915_MAP_WC is an internal value from an enum; how does that then get mapped to PAGE_KERNEL* values (which we do not define). I wonder does it get mapped straight to FreeBSD internal VM_ values?

[ashafer]

306     case I915_MAP_WC:
307         pgprot = pgprot_writecombine(PAGE_KERNEL_IO);
308         break;

Here's the bit from i915_gem_object_map_page. We calculate page protections from PAGE_KERNEL_IO and linuxkpi's pgprot_writecombine really just turns that into VM_*. Then i915 passes pgprot into vmap. We do also define PAGE_KERNEL in vmalloc.h and PAGE_KERNEL_IO in page.h

[bz]

In D59436#1364598, @ashafer wrote:

306     case I915_MAP_WC:
307         pgprot = pgprot_writecombine(PAGE_KERNEL_IO);
308         break;

Here's the bit from i915_gem_object_map_page. We calculate page protections from PAGE_KERNEL_IO and linuxkpi's pgprot_writecombine really just turns that into VM_*. Then i915 passes pgprot into vmap. We do also define PAGE_KERNEL in vmalloc.h and PAGE_KERNEL_IO in page.h

Yes, that's what I meant in the first comment; We have PAGE_KERNEL[_IO] but none of all the others.

Neither is used beyond the define.

% grep -r PAGE_KERNEL sys/compat/linuxkpi/
sys/compat/linuxkpi/common/include/linux/vmalloc.h:#define      PAGE_KERNEL     0x0000
sys/compat/linuxkpi/common/include/linux/page.h:#define PAGE_KERNEL_IO  0x0000
%

And pgprot2cachemode() then maps these both to VM_MEMATTR_DEFAULT.

But in your sample I see that pgprot_writecombine(); and that and others map things to native VM_MEMATTR_ values in LinuxKPI directly.

Thanks a lot for the explanations and code samples! Really helped to understand how this is supposed to come together.

[bz]

sys/compat/linuxkpi/common/src/linux_page.c

531

Are we always doing full size pages here?
Or may we be possible missing a last partial page due to round-down?
Hmm given the previous pmap_qremove() this is hopefully fine.

[oleglelchuk_gmail.com]

I take it, this code fixes the problem reported here: https://github.com/freebsd/drm-kmod/pull/496 ? If it does, thank you very much for writing it! I'll test this code later.

[ashafer]

Not 100% confident, but that drm-kmod issue seems similar enough I think there's a decent chance this solves it. More testing would definitely be helpful

[oleglelchuk_gmail.com]

I tested your patch with vanilla drm-kmod today, and it didn't solve the problem that my github PR's code had solved for me. But thanks anyway for writing it.

[oleglelchuk_gmail.com]

I have some new results, but I am still confused about how D59436 in its original form relates to the GPU problems on my Meteor Lake machine. Could you help clarify its scope and a mapping-lifetime concern?

I have encountered two separate problems. The earlier one involved shmem backing-page ownership/reclamation, discussed in D59481 and D59883; I still use a local PFN fix for that. A newer CCS hang occurred with that fix already present. In the newer case, the LLM's analysis of saved captures found CPU command writes going to BAR2 stolen memory while the GPU fetched different system-RAM pages through the GGTT.

The LLM then modified drm-kmod, including the aperture eligibility check in gen8_gmch_probe() and framebuffer mapping, and implemented a modified version of D59436's cache-policy correction with additional mapping-lifetime handling. Since booting that combination, I have not seen the newer hang recur, and the live mapping checks and a bounded GPU compute/readback test passed. This looks promising, but several things changed together, so I cannot attribute the improvement to D59436 alone or claim a permanent fix.

What confuses me is that the LLM also reports a bug in the original diff 185902. It copied that diff's vmap()/vunmap() implementation into an isolated native kernel test module, renaming the entry points. Using a private page kept allocated and wired throughout, it tested this sequence:

a = vmap(&page, 1, 0, pgprot_writecombine(PAGE_KERNEL));
b = vmap(&page, 1, 0, pgprot_writecombine(PAGE_KERNEL));
vunmap(a);
/* b is still mapped here. */

After vunmap(a), the surviving mapping b was still write-combining (WC), but the page's direct mapping had become write-back (WB). The LLM traced this to vunmap() calling pmap_page_set_memattr(m, VM_MEMATTR_DEFAULT) without accounting for another surviving vmap. The local expanded implementation retained WC for both mappings until the last one was removed.

That overlap test did not call the local PFN cleanup code. It ran on my locally patched kernel based on f492ef8318f5, whose relevant native amd64 pmap and page-allocation routines are unchanged from that upstream base. It was an isolated mapping test, not a full boot/desktop test of the original D59436. No GPU hang was reproduced by this test, and the LLM has not established that ordinary i915 activity reaches this exact sequence.

I understand from your description that the original goal is to honor the requested cache policy for mappings such as GuC firmware buffers, and you mentioned GT1 initialization on Meteor Lake. Is the original patch expected to fix that initialization problem independently of the separate physical-mapping and ownership problems above? And is there a caller guarantee that rules out the overlapping-mapping sequence, or does vunmap() need additional lifetime handling before restoring the default attribute?

In other words, am I understanding correctly that D59436 can address its intended initialization problem while still having the separate cleanup bug reported by the LLM? Please correct either the LLM's analysis or my understanding if something has been missed.