| Message ID | 1643029028-12710-3-git-send-email-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/mmap: Drop protection_map[] and platform's __SXXX/__PXXX requirements | expand |
> + [VM_NONE] = __P000, > + [VM_READ] = __P001, > + [VM_WRITE] = __P010, > + [VM_READ|VM_WRITE] = __P011, > + [VM_EXEC] = __P100, > + [VM_EXEC|VM_READ] = __P101, > + [VM_EXEC|VM_WRITE] = __P110, > + [VM_EXEC|VM_READ|VM_WRITE] = __P111, > + [VM_SHARED] = __S000, > + [VM_SHARED|VM_READ] = __S001, > + [VM_SHARED|VM_WRITE] = __S010, > + [VM_SHARED|VM_READ|VM_WRITE] = __S011, > + [VM_SHARED|VM_EXEC] = __S100, > + [VM_SHARED|VM_READ|VM_EXEC] = __S101, > + [VM_SHARED|VM_WRITE|VM_EXEC] = __S110, > + [VM_SHARED|VM_READ|VM_WRITE|VM_EXEC] = __S111 Please add whitespaces around the | operators. Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On 1/26/22 12:46 PM, Christoph Hellwig wrote: >> + [VM_NONE] = __P000, >> + [VM_READ] = __P001, >> + [VM_WRITE] = __P010, >> + [VM_READ|VM_WRITE] = __P011, >> + [VM_EXEC] = __P100, >> + [VM_EXEC|VM_READ] = __P101, >> + [VM_EXEC|VM_WRITE] = __P110, >> + [VM_EXEC|VM_READ|VM_WRITE] = __P111, >> + [VM_SHARED] = __S000, >> + [VM_SHARED|VM_READ] = __S001, >> + [VM_SHARED|VM_WRITE] = __S010, >> + [VM_SHARED|VM_READ|VM_WRITE] = __S011, >> + [VM_SHARED|VM_EXEC] = __S100, >> + [VM_SHARED|VM_READ|VM_EXEC] = __S101, >> + [VM_SHARED|VM_WRITE|VM_EXEC] = __S110, >> + [VM_SHARED|VM_READ|VM_WRITE|VM_EXEC] = __S111 > > Please add whitespaces around the | operators. Sure, will add. > > Otherwise looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> >
On Mon, Jan 24, 2022 at 06:26:39PM +0530, Anshuman Khandual wrote: > protection_map[] maps vm_flags access combinations into page protection > value as defined by the platform via __PXXX and __SXXX macros. The array > indices in protection_map[], represents vm_flags access combinations but > it's not very intuitive to derive. This makes it clear and explicit. The protection_map is going to be removed in one of the next patches, why bother with this patch at all? > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: linux-mm@kvack.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > mm/mmap.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index 1e8fdb0b51ed..254d716220df 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -102,8 +102,22 @@ static void unmap_region(struct mm_struct *mm, > * x: (yes) yes > */ > pgprot_t protection_map[16] __ro_after_init = { > - __P000, __P001, __P010, __P011, __P100, __P101, __P110, __P111, > - __S000, __S001, __S010, __S011, __S100, __S101, __S110, __S111 > + [VM_NONE] = __P000, > + [VM_READ] = __P001, > + [VM_WRITE] = __P010, > + [VM_READ|VM_WRITE] = __P011, > + [VM_EXEC] = __P100, > + [VM_EXEC|VM_READ] = __P101, > + [VM_EXEC|VM_WRITE] = __P110, > + [VM_EXEC|VM_READ|VM_WRITE] = __P111, > + [VM_SHARED] = __S000, > + [VM_SHARED|VM_READ] = __S001, > + [VM_SHARED|VM_WRITE] = __S010, > + [VM_SHARED|VM_READ|VM_WRITE] = __S011, > + [VM_SHARED|VM_EXEC] = __S100, > + [VM_SHARED|VM_READ|VM_EXEC] = __S101, > + [VM_SHARED|VM_WRITE|VM_EXEC] = __S110, > + [VM_SHARED|VM_READ|VM_WRITE|VM_EXEC] = __S111 > }; > > #ifndef CONFIG_ARCH_HAS_FILTER_PGPROT > -- > 2.25.1 > >
On 1/27/22 6:09 PM, Mike Rapoport wrote: > On Mon, Jan 24, 2022 at 06:26:39PM +0530, Anshuman Khandual wrote: >> protection_map[] maps vm_flags access combinations into page protection >> value as defined by the platform via __PXXX and __SXXX macros. The array >> indices in protection_map[], represents vm_flags access combinations but >> it's not very intuitive to derive. This makes it clear and explicit. > > The protection_map is going to be removed in one of the next patches, why > bother with this patch at all? This makes the transition from protection_map[] into __vm_get_page_prot() more intuitive, where protection_map[] gets dropped. This helps platforms (first ones subscribing ARCH_HAS_VM_GET_PAGE_PROT before this drop) create /formulate the required switch case elements in their vm_get_page_prot(). The existing protection_map[] is not clear in demonstrating how exactly the vm_flags combination is mapped into page protection map. This helps clarify the underlying switch before we move on defining it on platforms. > >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: linux-mm@kvack.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> mm/mmap.c | 18 ++++++++++++++++-- >> 1 file changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/mm/mmap.c b/mm/mmap.c >> index 1e8fdb0b51ed..254d716220df 100644 >> --- a/mm/mmap.c >> +++ b/mm/mmap.c >> @@ -102,8 +102,22 @@ static void unmap_region(struct mm_struct *mm, >> * x: (yes) yes >> */ >> pgprot_t protection_map[16] __ro_after_init = { >> - __P000, __P001, __P010, __P011, __P100, __P101, __P110, __P111, >> - __S000, __S001, __S010, __S011, __S100, __S101, __S110, __S111 >> + [VM_NONE] = __P000, >> + [VM_READ] = __P001, >> + [VM_WRITE] = __P010, >> + [VM_READ|VM_WRITE] = __P011, >> + [VM_EXEC] = __P100, >> + [VM_EXEC|VM_READ] = __P101, >> + [VM_EXEC|VM_WRITE] = __P110, >> + [VM_EXEC|VM_READ|VM_WRITE] = __P111, >> + [VM_SHARED] = __S000, >> + [VM_SHARED|VM_READ] = __S001, >> + [VM_SHARED|VM_WRITE] = __S010, >> + [VM_SHARED|VM_READ|VM_WRITE] = __S011, >> + [VM_SHARED|VM_EXEC] = __S100, >> + [VM_SHARED|VM_READ|VM_EXEC] = __S101, >> + [VM_SHARED|VM_WRITE|VM_EXEC] = __S110, >> + [VM_SHARED|VM_READ|VM_WRITE|VM_EXEC] = __S111 >> }; >> >> #ifndef CONFIG_ARCH_HAS_FILTER_PGPROT >> -- >> 2.25.1 >> >> >
The 01/24/2022 18:26, Anshuman Khandual wrote: > protection_map[] maps vm_flags access combinations into page protection > value as defined by the platform via __PXXX and __SXXX macros. The array > indices in protection_map[], represents vm_flags access combinations but > it's not very intuitive to derive. This makes it clear and explicit. > > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: linux-mm@kvack.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > mm/mmap.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index 1e8fdb0b51ed..254d716220df 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -102,8 +102,22 @@ static void unmap_region(struct mm_struct *mm, > * x: (yes) yes > */ > pgprot_t protection_map[16] __ro_after_init = { > - __P000, __P001, __P010, __P011, __P100, __P101, __P110, __P111, > - __S000, __S001, __S010, __S011, __S100, __S101, __S110, __S111 > + [VM_NONE] = __P000, > + [VM_READ] = __P001, > + [VM_WRITE] = __P010, > + [VM_READ|VM_WRITE] = __P011, > + [VM_EXEC] = __P100, > + [VM_EXEC|VM_READ] = __P101, > + [VM_EXEC|VM_WRITE] = __P110, > + [VM_EXEC|VM_READ|VM_WRITE] = __P111, > + [VM_SHARED] = __S000, > + [VM_SHARED|VM_READ] = __S001, > + [VM_SHARED|VM_WRITE] = __S010, > + [VM_SHARED|VM_READ|VM_WRITE] = __S011, > + [VM_SHARED|VM_EXEC] = __S100, > + [VM_SHARED|VM_READ|VM_EXEC] = __S101, > + [VM_SHARED|VM_WRITE|VM_EXEC] = __S110, > + [VM_SHARED|VM_READ|VM_WRITE|VM_EXEC] = __S111 Just a little bit picky:) Would you mind rearranging vm_flags access commbination in the order as the access bits appear in __SXXX or __PXXX? For example, change the following: [VM_SHARED|VM_READ|VM_WRITE|VM_EXEC] = __S111 to [VM_SHARED | VM_EXEC | VM_WRITE | VM_READ] = __S111 I think it's would be more clear for looking. Best, // Firo
On 2/5/22 2:40 PM, Firo Yang wrote: > The 01/24/2022 18:26, Anshuman Khandual wrote: >> protection_map[] maps vm_flags access combinations into page protection >> value as defined by the platform via __PXXX and __SXXX macros. The array >> indices in protection_map[], represents vm_flags access combinations but >> it's not very intuitive to derive. This makes it clear and explicit. >> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: linux-mm@kvack.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> mm/mmap.c | 18 ++++++++++++++++-- >> 1 file changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/mm/mmap.c b/mm/mmap.c >> index 1e8fdb0b51ed..254d716220df 100644 >> --- a/mm/mmap.c >> +++ b/mm/mmap.c >> @@ -102,8 +102,22 @@ static void unmap_region(struct mm_struct *mm, >> * x: (yes) yes >> */ >> pgprot_t protection_map[16] __ro_after_init = { >> - __P000, __P001, __P010, __P011, __P100, __P101, __P110, __P111, >> - __S000, __S001, __S010, __S011, __S100, __S101, __S110, __S111 >> + [VM_NONE] = __P000, >> + [VM_READ] = __P001, >> + [VM_WRITE] = __P010, >> + [VM_READ|VM_WRITE] = __P011, >> + [VM_EXEC] = __P100, >> + [VM_EXEC|VM_READ] = __P101, >> + [VM_EXEC|VM_WRITE] = __P110, >> + [VM_EXEC|VM_READ|VM_WRITE] = __P111, >> + [VM_SHARED] = __S000, >> + [VM_SHARED|VM_READ] = __S001, >> + [VM_SHARED|VM_WRITE] = __S010, >> + [VM_SHARED|VM_READ|VM_WRITE] = __S011, >> + [VM_SHARED|VM_EXEC] = __S100, >> + [VM_SHARED|VM_READ|VM_EXEC] = __S101, >> + [VM_SHARED|VM_WRITE|VM_EXEC] = __S110, >> + [VM_SHARED|VM_READ|VM_WRITE|VM_EXEC] = __S111 > > Just a little bit picky:) > Would you mind rearranging vm_flags access commbination in the order as > the access bits appear in __SXXX or __PXXX? For example, change the following: > > [VM_SHARED|VM_READ|VM_WRITE|VM_EXEC] = __S111 > to > [VM_SHARED | VM_EXEC | VM_WRITE | VM_READ] = __S111 > > I think it's would be more clear for looking. So the vm_flags combination set here (and like in the platforms) should be like the following .. [VM_NONE] [VM_READ] [VM_WRITE] [VM_WRITE | VM_READ] [VM_EXEC] [VM_EXEC|VM_READ] [VM_EXEC|VM_WRITE] [VM_EXEC|VM_WRITE | VM_READ] [VM_SHARED] [VM_SHARED|VM_READ] [VM_SHARED|VM_WRITE] [VM_SHARED|VM_WRITE | VM_READ] [VM_SHARED|VM_EXEC] [VM_SHARED|VM_EXEC | VM_READ] [VM_SHARED|VM_EXEC | VM_WRITE] [VM_SHARED|VM_EXEC | VM_WRITE | VM_READ] Implying the relative position for these flags among each other. [VM_SHARED] [VM_EXEC] [VM_WRITE] [VM_WRITE] This makes sense, will change the series accordingly. - Anshuman
diff --git a/mm/mmap.c b/mm/mmap.c index 1e8fdb0b51ed..254d716220df 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -102,8 +102,22 @@ static void unmap_region(struct mm_struct *mm, * x: (yes) yes */ pgprot_t protection_map[16] __ro_after_init = { - __P000, __P001, __P010, __P011, __P100, __P101, __P110, __P111, - __S000, __S001, __S010, __S011, __S100, __S101, __S110, __S111 + [VM_NONE] = __P000, + [VM_READ] = __P001, + [VM_WRITE] = __P010, + [VM_READ|VM_WRITE] = __P011, + [VM_EXEC] = __P100, + [VM_EXEC|VM_READ] = __P101, + [VM_EXEC|VM_WRITE] = __P110, + [VM_EXEC|VM_READ|VM_WRITE] = __P111, + [VM_SHARED] = __S000, + [VM_SHARED|VM_READ] = __S001, + [VM_SHARED|VM_WRITE] = __S010, + [VM_SHARED|VM_READ|VM_WRITE] = __S011, + [VM_SHARED|VM_EXEC] = __S100, + [VM_SHARED|VM_READ|VM_EXEC] = __S101, + [VM_SHARED|VM_WRITE|VM_EXEC] = __S110, + [VM_SHARED|VM_READ|VM_WRITE|VM_EXEC] = __S111 }; #ifndef CONFIG_ARCH_HAS_FILTER_PGPROT
protection_map[] maps vm_flags access combinations into page protection value as defined by the platform via __PXXX and __SXXX macros. The array indices in protection_map[], represents vm_flags access combinations but it's not very intuitive to derive. This makes it clear and explicit. Cc: Andrew Morton <akpm@linux-foundation.org> Cc: linux-mm@kvack.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- mm/mmap.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)