On 1/27/22 6:08 PM, Mike Rapoport wrote: > Hi Anshuman, > > On Mon, Jan 24, 2022 at 06:26:37PM +0530, Anshuman Khandual wrote: >> protection_map[] is an array based construct that translates given vm_flags >> combination. This array contains page protection map, which is populated by >> the platform via [__S000 .. __S111] and [__P000 .. __P111] exported macros. >> Primary usage for protection_map[] is for vm_get_page_prot(), which is used >> to determine page protection value for a given vm_flags. vm_get_page_prot() >> implementation, could again call platform overrides arch_vm_get_page_prot() >> and arch_filter_pgprot(). Some platforms override protection_map[] that was >> originally built with __SXXX/__PXXX with different runtime values. >> >> Currently there are multiple layers of abstraction i.e __SXXX/__PXXX macros >> , protection_map[], arch_vm_get_page_prot() and arch_filter_pgprot() built >> between the platform and generic MM, finally defining vm_get_page_prot(). >> >> Hence this series proposes to drop all these abstraction levels and instead >> just move the responsibility of defining vm_get_page_prot() to the platform >> itself making it clean and simple. >> >> This first introduces ARCH_HAS_VM_GET_PAGE_PROT which enables the platforms >> to define custom vm_get_page_prot(). This starts converting platforms that >> either change protection_map[] or define the overrides arch_filter_pgprot() >> or arch_vm_get_page_prot() which enables for those constructs to be dropped >> off completely. This series then converts remaining platforms which enables >> for __SXXX/__PXXX constructs to be dropped off completely. Finally it drops >> the generic vm_get_page_prot() and then ARCH_HAS_VM_GET_PAGE_PROT as every >> platform now defines their own vm_get_page_prot(). > > I generally like the idea, I just think the conversion can be more straight > forward. Rather than adding ARCH_HAS_VM_GET_PAGE_PROT and then dropping it, > why won't me make the generic vm_get_page_prot() __weak, then add per-arch > implementation and in the end drop the generic one? > Is not the ARCH_HAS_ config based switch over a relatively better method ? IIUC some existing platform overrides could have been implemented via __weak identifier, although ARCH_HAS_ method was preferred. But I might be missing something here.
