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

From: Fabio M. De Francesco
Date: Thu Oct 06 2022 - 16:39:23 EST

On Thursday, September 29, 2022 6:06:46 PM CEST Kristen Carlson Accardi wrote:
> It is not necessary to disable page faults or preemption when
> using kmap calls

Do you refer to the page faults disabling that kmap_atomic() provides as a
side effect? Can you please clarify a little more? kmap_atomic() disables
always only page faults, instead it might not disable preemption; it depends
on CONFIG_PREEMPT_RT. Therefore, why are you also talking about preemption?

Are you converting code which runs in atomic context regardless kmap_atomic()?
If so, between kmap() and kmap_atomic(), the author(s) had only one choice, it
correctly was kmap_atomic(), otherwise we might end up with a perfect recipe
for triggering SAC bugs.

kmap() were not suited in those cases because it might sleep. If the intents
of the author are simply map a page while in atomic, so to avoid sleeping in
atomic bugs, your conversions looks good.

For the reasons above, can you please say something more about why this code
needed a kmap_atomic() instead of calling kmap()?

A different case is in choosing kmap_atomic() is there because of its side
effects, despite the code is running in non atomic context until the mapping,
but it needs to disable pagefaults only somewhere between the mapping and
unmapping. This is a trickier case than the above-mentioned one because along
with conversion developers should at least disable the pagefaults and
probably, although not necessarily, also disable preemption.

> , so replace kmap_atomic() and kunmap_atomic()
> calls with more the more appropriate kmap_local_page() and
> kunmap_local() calls.

Why is kmap_local_page() better suited in general and is safe in
this specific case?

I think that we should provide the maintainer with well supported reasons why
they should change any piece of code which is still doing what it is thought
for. A mere deprecation in favour of a newer API may not be enough to change
code that is still working properly (like in the old "if it's not broken,
don't fix it!", or something like this :)).



> Signed-off-by: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
> ---
> arch/x86/kernel/cpu/sgx/encl.c | 12 ++++++------
> arch/x86/kernel/cpu/sgx/ioctl.c | 4 ++--
> arch/x86/kernel/cpu/sgx/main.c | 8 ++++----
> 3 files changed, 12 insertions(+), 12 deletions(-)