Re: [PATCH v2] x86/sgx: Replace kmap/kunmap_atomic calls

[Thomas Gleixner]

On Wed, Nov 16 2022 at 11:16, Fabio M. De Francesco wrote:
> On martedì 15 novembre 2022 17:16:26 CET Kristen Carlson Accardi wrote:
>> The use of kmap_atomic() in the SGX code was not an explicit design
>> choice to disable page faults or preemption, and there is no compelling
>> design reason to using kmap_atomic() vs. kmap_local_page().
>
> This is at the core of the reasons why you are converting, that is to avoid
> the potential overhead (in 32 bit architectures) of switching in atomic
> context where it is not required.

No. The point is that kmap_atomic() is an historical artifact of 32bit
HIGHMEM support.

The back then chosen implementation was to disable preemption and
pagefaults and use a temporary per CPU mapping. Disabling preemption was
required to protect the temporary mapping against a context switch.

Disabling pagefaults was an implicit side effect of disabling
preemption. The separation of preempt_disable() and pagefault_disable()
happened way later.

On 64bit and on 32bit systems with CONFIG_HIGHMEM=n this is not required
at all because the pages are already in the direct map.

That means support for 32bit highmem forces code to accomodate with the
preemption disabled section, where in the most cases this is absolutely
not required. That results often in suboptimal and horrible code:

again:
    kmap_atomic();
    remaining = copy_to_user_inatomic();
    kunmap_atomic();
    if (remaining) {
    	if (try_to_handle_fault())
            goto again;
        ret = -EFAULT;
    }

instead of:

    kmap_local();
    ret = copy_to_user();
    kunmap_local();
    
It obsiously does not allow to sleep or take sleeping locks in the
kmap_atomic() section which puts further restrictions on code just to
accomodate 32bit highmem.

So a few years ago we implemented kmap_local() and related interfaces to
replace kmap_atomic() completely, but we could not do a scripted
wholesale conversion because there are a few code pathes which rely on
the implicit preemption disable and of course something like the above
monstrosity needs manual conversion.

kmap_local() puts a penalty exclusively on 32bit highmem systems. The
temporary mapping is still strict per CPU, which is guaranteed by an
implicit migrate_disable(), and it is context switched in case of
[un]voluntary scheduling.

On plain 32bit and 64bit systems kmap_local() is pretty much a NOP. All
it does is to return the page address. It does not disable migration in
that case either. kunmap_local() is a complete NOP.

The goal is to eliminate _all_ use cases of kmap_atomic*() and replace
them with kmap_local*(). This is a prerequisite for system protection
keys and other things.

Thanks,

        tglx