All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Brendan Jackman" <brendan.jackman@linux.dev>
To: "Mike Rapoport (Microsoft)" <rppt@kernel.org>,
	"Andrew Morton" <akpm@linux-foundation.org>
Cc: "Adrian Barnaś" <abarnas@google.com>,
	"Albert Ou" <aou@eecs.berkeley.edu>,
	"Alexander Gordeev" <agordeev@linux.ibm.com>,
	"Alexandre Ghiti" <alex@ghiti.fr>,
	"Andy Lutomirski" <luto@kernel.org>,
	"Borislav Petkov" <bp@alien8.de>,
	"Brendan Jackman" <brendan.jackman@linux.dev>,
	"Catalin Marinas" <catalin.marinas@arm.com>,
	"Christian Borntraeger" <borntraeger@linux.ibm.com>,
	"Dave Hansen" <dave.hansen@linux.intel.com>,
	"David Hildenbrand" <david@kernel.org>,
	"Gerald Schaefer" <gerald.schaefer@linux.ibm.com>,
	"Heiko Carstens" <hca@linux.ibm.com>,
	"Huacai Chen" <chenhuacai@kernel.org>,
	"Ingo Molnar" <mingo@redhat.com>, "Len Brown" <lenb@kernel.org>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Paul Walmsley" <pjw@kernel.org>,
	"Pavel Machek" <pavel@kernel.org>,
	"Peter Zijlstra" <peterz@infradead.org>,
	"H. Peter Anvin" <hpa@zytor.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	"Ryan Roberts" <ryan.roberts@arm.com>,
	"Sven Schnelle" <svens@linux.ibm.com>,
	"Thomas Gleixner" <tglx@kernel.org>,
	"Uladzislau Rezki" <urezki@gmail.com>,
	"Vasily Gorbik" <gor@linux.ibm.com>,
	"WANG Xuerui" <kernel@xen0n.name>,
	"Will Deacon" <will@kernel.org>,
	x86@kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	linux-pm@vger.kernel.org, linux-riscv@lists.infradead.org,
	linux-s390@vger.kernel.org, loongarch@lists.linux.dev,
	yosry@kernel.org
Subject: Re: [PATCH 6/6] Revert "arch: introduce set_direct_map_valid_noflush()"
Date: Mon, 17 Aug 2026 17:14:19 +0200	[thread overview]
Message-ID: <DKRBF6LQY1L7.13CIBHAU0A87D@linux.dev> (raw)
In-Reply-To: <20260816-execmem-set-vm-perms-v0-2-v1-6-90944a3ad43f@kernel.org>

On Sun Aug 16, 2026 at 12:59 PM CEST, Mike Rapoport (Microsoft) wrote:
> Commit 0c6378a71574 ("arch: introduce set_direct_map_valid_noflush()")
> added set_direct_map_valid_noflush() to allow updating the direct map
> for a physically contiguous range in execmem.
>
> As Brendan recently pointed out [1], this API is confusing because on
> arm64 it means that is sets VALID bit in ptes, while on other
> architectures it is an analog of set_direct_map_default_noflush().
>
> The only user of set_direct_map_valid_noflush() was execmem's ROX cache
> freeing path and it was switched to utilize VM_FLUSH_RESET_PERMS for
> resetting permissions of the direct map alias.
>
> With the last user gone and with set_direct_map_{invalid,default}_noflush()
> accepting number of pages as a parameter, set_direct_map_valid_noflush()
> become a copy of set_memory_valid() on arm64 and a duplicate of
> set_direct_map_{invalid,default}_noflush() on other architecture, it is
> safe to remove set_direct_map_valid_noflush().
>
> Also drop a stale comment in arm64::__kernel_map_pages() that Linus
> bothered to add when merging changes containing set_direct_map_valid_noflush()
> to his tree.
>
> This reverts commit 0c6378a71574daa6cd1534ad42a956e3262756c7.
>
> [1] https://lore.kernel.org/all/DJ69RCVRBO0Y.3JCYSW50IC4RC@linux.dev
>
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>

Quick dump of my understanding (ignoring the NG bit on arm64)

- set_direct_map_invalid_noflush():

    x86: clear P and RW (and DIRTY)
    arm64: clear VALID

  So these look out of sync to me

- set_direct_map_default_noflush():

    x86: set P and RW
    arm64: set VALID and WRITE, clear RDONLY

- set_direct_map_valid_noflush(..., true):

    x86: exactly the same as set_direct_map_default_noflush()
         (but with a `nr` arg)
    arm64: set VALID

- set_direct_map_valid_noflush(..., false):

    both archs: exactly the same as set_direct_map_invalid_noflush()
         (but with a `nr` arg)

So basically the big issue here is specifically that
set_direct_map_valid_noflush(..., true) is special on arm64 and not x86.

And we fix that by jut deleting the API. SGTM!

The other issue I can see here is that set_direct_map_invalid_noflush()
clears RW on x86 but doesn't set RDONLY on arm64. So if you unmap
something using set_direct_map_invalid_noflush(), then map it again
using something other than set_direct_map_default_noflush(), you get
different behaviour between the archs.

I think the answer to that is probably: doing that is a bug, i.e.
_invalid_noflush() and _default_noflush() are a pair that you have to
use together. But I haven't checked if this is currently the case. Maybe
it would still make sense to just align these fully?

Anyway, aside from all this yapping, getting rid of _valid_noflush()
seems like an unambiguous win here so thanks for the cleanup!

Reviewed-by: Brendan Jackman <brendan.jackman@linux.dev>

WARNING: multiple messages have this Message-ID (diff)
From: "Brendan Jackman" <brendan.jackman@linux.dev>
To: "Mike Rapoport (Microsoft)" <rppt@kernel.org>,
	"Andrew Morton" <akpm@linux-foundation.org>
Cc: "Adrian Barnaś" <abarnas@google.com>,
	"Albert Ou" <aou@eecs.berkeley.edu>,
	"Alexander Gordeev" <agordeev@linux.ibm.com>,
	"Alexandre Ghiti" <alex@ghiti.fr>,
	"Andy Lutomirski" <luto@kernel.org>,
	"Borislav Petkov" <bp@alien8.de>,
	"Brendan Jackman" <brendan.jackman@linux.dev>,
	"Catalin Marinas" <catalin.marinas@arm.com>,
	"Christian Borntraeger" <borntraeger@linux.ibm.com>,
	"Dave Hansen" <dave.hansen@linux.intel.com>,
	"David Hildenbrand" <david@kernel.org>,
	"Gerald Schaefer" <gerald.schaefer@linux.ibm.com>,
	"Heiko Carstens" <hca@linux.ibm.com>,
	"Huacai Chen" <chenhuacai@kernel.org>,
	"Ingo Molnar" <mingo@redhat.com>, "Len Brown" <lenb@kernel.org>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Paul Walmsley" <pjw@kernel.org>,
	"Pavel Machek" <pavel@kernel.org>,
	"Peter Zijlstra" <peterz@infradead.org>,
	"H. Peter Anvin" <hpa@zytor.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	"Ryan Roberts" <ryan.roberts@arm.com>,
	"Sven Schnelle" <svens@linux.ibm.com>,
	"Thomas Gleixner" <tglx@kernel.org>,
	"Uladzislau Rezki" <urezki@gmail.com>,
	"Vasily Gorbik" <gor@linux.ibm.com>,
	"WANG Xuerui" <kernel@xen0n.name>,
	"Will Deacon" <will@kernel.org>,
	x86@kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	linux-pm@vger.kernel.org, linux-riscv@lists.infradead.org,
	linux-s390@vger.kernel.org, loongarch@lists.linux.dev,
	yosry@kernel.org
Subject: Re: [PATCH 6/6] Revert "arch: introduce set_direct_map_valid_noflush()"
Date: Mon, 17 Aug 2026 17:14:19 +0200	[thread overview]
Message-ID: <DKRBF6LQY1L7.13CIBHAU0A87D@linux.dev> (raw)
In-Reply-To: <20260816-execmem-set-vm-perms-v0-2-v1-6-90944a3ad43f@kernel.org>

On Sun Aug 16, 2026 at 12:59 PM CEST, Mike Rapoport (Microsoft) wrote:
> Commit 0c6378a71574 ("arch: introduce set_direct_map_valid_noflush()")
> added set_direct_map_valid_noflush() to allow updating the direct map
> for a physically contiguous range in execmem.
>
> As Brendan recently pointed out [1], this API is confusing because on
> arm64 it means that is sets VALID bit in ptes, while on other
> architectures it is an analog of set_direct_map_default_noflush().
>
> The only user of set_direct_map_valid_noflush() was execmem's ROX cache
> freeing path and it was switched to utilize VM_FLUSH_RESET_PERMS for
> resetting permissions of the direct map alias.
>
> With the last user gone and with set_direct_map_{invalid,default}_noflush()
> accepting number of pages as a parameter, set_direct_map_valid_noflush()
> become a copy of set_memory_valid() on arm64 and a duplicate of
> set_direct_map_{invalid,default}_noflush() on other architecture, it is
> safe to remove set_direct_map_valid_noflush().
>
> Also drop a stale comment in arm64::__kernel_map_pages() that Linus
> bothered to add when merging changes containing set_direct_map_valid_noflush()
> to his tree.
>
> This reverts commit 0c6378a71574daa6cd1534ad42a956e3262756c7.
>
> [1] https://lore.kernel.org/all/DJ69RCVRBO0Y.3JCYSW50IC4RC@linux.dev
>
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>

Quick dump of my understanding (ignoring the NG bit on arm64)

- set_direct_map_invalid_noflush():

    x86: clear P and RW (and DIRTY)
    arm64: clear VALID

  So these look out of sync to me

- set_direct_map_default_noflush():

    x86: set P and RW
    arm64: set VALID and WRITE, clear RDONLY

- set_direct_map_valid_noflush(..., true):

    x86: exactly the same as set_direct_map_default_noflush()
         (but with a `nr` arg)
    arm64: set VALID

- set_direct_map_valid_noflush(..., false):

    both archs: exactly the same as set_direct_map_invalid_noflush()
         (but with a `nr` arg)

So basically the big issue here is specifically that
set_direct_map_valid_noflush(..., true) is special on arm64 and not x86.

And we fix that by jut deleting the API. SGTM!

The other issue I can see here is that set_direct_map_invalid_noflush()
clears RW on x86 but doesn't set RDONLY on arm64. So if you unmap
something using set_direct_map_invalid_noflush(), then map it again
using something other than set_direct_map_default_noflush(), you get
different behaviour between the archs.

I think the answer to that is probably: doing that is a bug, i.e.
_invalid_noflush() and _default_noflush() are a pair that you have to
use together. But I haven't checked if this is currently the case. Maybe
it would still make sense to just align these fully?

Anyway, aside from all this yapping, getting rid of _valid_noflush()
seems like an unambiguous win here so thanks for the cleanup!

Reviewed-by: Brendan Jackman <brendan.jackman@linux.dev>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  parent reply	other threads:[~2026-08-17 15:14 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-16 10:59 [PATCH 0/6] arch, mm/execmem: resolve confusion about set_direct_map_valid_noflush() Mike Rapoport (Microsoft)
2026-08-16 10:59 ` Mike Rapoport (Microsoft)
2026-08-16 10:59 ` [PATCH 1/6] set_memory: add number of pages parameter to set_direct_map APIs Mike Rapoport (Microsoft)
2026-08-16 10:59   ` Mike Rapoport (Microsoft)
2026-08-16 11:12   ` sashiko-bot
2026-08-17 12:10   ` Brendan Jackman
2026-08-17 12:10     ` Brendan Jackman
2026-08-16 10:59 ` [PATCH 2/6] mm/vmalloc: set area's page_order after allocation succeeds Mike Rapoport (Microsoft)
2026-08-16 10:59   ` Mike Rapoport (Microsoft)
2026-08-16 11:08   ` sashiko-bot
2026-08-16 10:59 ` [PATCH 3/6] mm/vmalloc: constify vm parameter of get_vm_area_page_order() Mike Rapoport (Microsoft)
2026-08-16 10:59   ` Mike Rapoport (Microsoft)
2026-08-16 11:03   ` sashiko-bot
2026-08-16 10:59 ` [PATCH 4/6] mm/vmalloc: make set_area_direct_map HUGE_VMAP friendly Mike Rapoport (Microsoft)
2026-08-16 10:59   ` Mike Rapoport (Microsoft)
2026-08-16 11:10   ` sashiko-bot
2026-08-17 12:21   ` Brendan Jackman
2026-08-17 12:21     ` Brendan Jackman
2026-08-16 10:59 ` [PATCH 5/6] mm/execmem: use VM_FLUSH_RESET_PERMS for ROX cache allocations Mike Rapoport (Microsoft)
2026-08-16 10:59   ` Mike Rapoport (Microsoft)
2026-08-16 11:17   ` sashiko-bot
2026-08-16 10:59 ` [PATCH 6/6] Revert "arch: introduce set_direct_map_valid_noflush()" Mike Rapoport (Microsoft)
2026-08-16 10:59   ` Mike Rapoport (Microsoft)
2026-08-16 11:03   ` sashiko-bot
2026-08-17 15:14   ` Brendan Jackman [this message]
2026-08-17 15:14     ` Brendan Jackman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DKRBF6LQY1L7.13CIBHAU0A87D@linux.dev \
    --to=brendan.jackman@linux.dev \
    --cc=abarnas@google.com \
    --cc=agordeev@linux.ibm.com \
    --cc=akpm@linux-foundation.org \
    --cc=alex@ghiti.fr \
    --cc=aou@eecs.berkeley.edu \
    --cc=borntraeger@linux.ibm.com \
    --cc=bp@alien8.de \
    --cc=catalin.marinas@arm.com \
    --cc=chenhuacai@kernel.org \
    --cc=dave.hansen@linux.intel.com \
    --cc=david@kernel.org \
    --cc=gerald.schaefer@linux.ibm.com \
    --cc=gor@linux.ibm.com \
    --cc=hca@linux.ibm.com \
    --cc=hpa@zytor.com \
    --cc=kernel@xen0n.name \
    --cc=lenb@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=loongarch@lists.linux.dev \
    --cc=luto@kernel.org \
    --cc=mingo@redhat.com \
    --cc=palmer@dabbelt.com \
    --cc=pavel@kernel.org \
    --cc=peterz@infradead.org \
    --cc=pjw@kernel.org \
    --cc=rafael@kernel.org \
    --cc=rppt@kernel.org \
    --cc=ryan.roberts@arm.com \
    --cc=svens@linux.ibm.com \
    --cc=tglx@kernel.org \
    --cc=urezki@gmail.com \
    --cc=will@kernel.org \
    --cc=x86@kernel.org \
    --cc=yosry@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.