Re: Adding kmap_atomic_prot_pfn (was: [git pull] drm patches for 2.6.27-rc1)

[Keith Packard <keithp@keithp.com>]

[-- Attachment #1: Type: text/plain, Size: 3748 bytes --]

On Thu, 2008-10-23 at 19:48 -0700, Linus Torvalds wrote:

> I'm not entirely sure who wants to own up to owning that particular part 
> of code, and is willing to make kmap_atomic_prot_pfn() also work in the 
> absense of CONFIG_HIGHMEM.

All of the kmap_atomic functions *do* work without CONFIG_HIGHMEM, they
just don't do what we want in this case. Without knowing the history, it
seems fairly clear that the kmap functions are designed to map physical
memory pages into the kernel virtual address space. On small 32-bit
systems, that's trivial, you just use the direct map (as one does on
64-bit systems). The magic fixmap entries make this work with
CONFIG_HIGHMEM as well.

So, I fear touching the kmap API as it appears to have a specific and
useful purpose, irrespective of the memory size the kernel is configured
for.

What I've proposed is that we create a new io-space specific set of
fixmap APIs. On CONFIG_HIGHMEM, they'd just use the existing kmap_atomic
mechanism, but on small 32-bit systems, we'd enable the fixmaps and have
some for that environment as well.

> So I would suspect that if you guys actually write a patch, and make sure 
> that it works on x86-32 even _without_ CONFIG_HIGHMEM, and send it to 
> Ingo, good things will happen.

Ok, we can give this a try.

> and it probably should all work automatically. The kmap_atomic() stuff 
> really should be almost totally independent of CONFIG_HIGHMEM, since it's 
> really much more closely related to fixmaps than HIGHMEM. 

As above, I think kmap_atomic should be left alone as a way of quickly
mapping memory pages. There are a users of both kmap_atomic_prot (xen)
and kmap_atomic_pfn (crash dumps).

> I guess there may be some debugging code that depends on HIGHMEM (eg that 
> whole testing for whether a page is a highmem page or not), so it might be 
> a _bit_ more than just moving code around, but I really didn't look 
> closer.
> 
> Then, there's the issue of 64-bit, and just mapping everything there, and 
> the interface to that. I liked the trivial extension to "struct resource" 
> to have a "cached mapping" pointer. So if we can just make it pass 
> resources around and get a page that way (and not even need kmap() on 
> 64-bit architections), that would be good.

The io_mapping API I proposed does precisely this. On 32-bit systems, it
uses kmap_atomic for each page access while on 64-bit systems it uses
ioremap_wc at device init time and then just uses an offset for each
page access.

Hiding this detail behind an API leaves the driver code independent of
this particular choice.

> It's too late for -rc1 (which I'm planning on cutting within the hour), 
> and if it ends up being complex, I guess that it means this will eb a 
> 2.6.29 issue.

If we do end up pushing this out to 2.6.29, I'd like to see
kmap_atomic_prot_pfn in place as a stop-gap so that PAT performance on
32-bit systems is reasonable. I don't think too many people are running
desktop systems without CONFIG_HIGHMEM at this point, and if so, we can
just suggest that perhaps they change their configuration.

> But if it really is just a matter of mostly just some trivial code 
> movement and both the gfx and x86 people are all happy, I'll happily take 
> it for -rc2, and we can just leave this all behind us.

I'll try to get something working in the next day or so and see how it
looks. My plan at this point is to create new API for 32-bit systems:

void *io_map_atomic_wc(unsigned long pfn)
void io_unmap_atomic(void *addr);

With this, I can switch my existing io_mapping API over to an
io-specific interface and implement those using the fixmap code.

-- 
keith.packard@intel.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]