On 3/3/22 20:58, Catalin Marinas wrote:
> Hi Anshuman,
>
> On Mon, Feb 28, 2022 at 04:17:28PM +0530, Anshuman Khandual wrote:
>> +static inline pgprot_t __vm_get_page_prot(unsigned long vm_flags)
>> +{
>> + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) {
>> + case VM_NONE:
>> + return PAGE_NONE;
>> + case VM_READ:
>> + case VM_WRITE:
>> + case VM_WRITE | VM_READ:
>> + return PAGE_READONLY;
>> + case VM_EXEC:
>> + return PAGE_EXECONLY;
>> + case VM_EXEC | VM_READ:
>> + case VM_EXEC | VM_WRITE:
>> + case VM_EXEC | VM_WRITE | VM_READ:
>> + return PAGE_READONLY_EXEC;
>> + case VM_SHARED:
>> + return PAGE_NONE;
>> + case VM_SHARED | VM_READ:
>> + return PAGE_READONLY;
>> + case VM_SHARED | VM_WRITE:
>> + case VM_SHARED | VM_WRITE | VM_READ:
>> + return PAGE_SHARED;
>> + case VM_SHARED | VM_EXEC:
>> + return PAGE_EXECONLY;
>> + case VM_SHARED | VM_EXEC | VM_READ:
>> + return PAGE_READONLY_EXEC;
>> + case VM_SHARED | VM_EXEC | VM_WRITE:
>> + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ:
>> + return PAGE_SHARED_EXEC;
>> + default:
>> + BUILD_BUG();
>> + }
>> +}
>
> I'd say ack for trying to get of the extra arch_vm_get_page_prot() and
> arch_filter_pgprot() but, TBH, I'm not so keen on the outcome. I haven't
> built the code to see what's generated but I suspect it's no significant
> improvement. As for the code readability, the arm64 parts don't look
> much better either. The only advantage with this patch is that all
> functions have been moved under arch/arm64.
Got it.
>
> I'd keep most architectures that don't have own arch_vm_get_page_prot()
> or arch_filter_pgprot() unchanged and with a generic protection_map[]
> array. For architectures that need fancier stuff, add a
> CONFIG_ARCH_HAS_VM_GET_PAGE_PROT (as you do) and allow them to define
> vm_get_page_prot() while getting rid of arch_vm_get_page_prot() and
> arch_filter_pgprot(). I think you could also duplicate protection_map[]
> for architectures with own vm_get_page_prot() (make it static) and
> #ifdef it out in mm/mmap.c.
>
> If later you have more complex needs or a switch statement generates
> better code, go for it, but for this series I'd keep things simple, only
> focus on getting rid of arch_vm_get_page_prot() and
> arch_filter_pgprot().
Got it.
>
> If I grep'ed correctly, there are only 4 architectures that have own
> arch_vm_get_page_prot() (arm64, powerpc, sparc, x86) and 2 that have own
> arch_filter_pgprot() (arm64, x86). Try to only change these for the time
> being, together with the other generic mm cleanups you have in this
> series. I think there are a couple more that touch protection_map[]
> (arm, m68k). You can leave the generic protection_map[] global if the
> arch does not select ARCH_HAS_VM_GET_PAGE_PROT.
Okay, I will probably split the series into two parts.
- Drop arch_vm_get_page_prot() and arch_filter_pgprot() on relevant
platforms i.e arm64, powerpc, sparc and x86 via this new config
ARCH_HAS_VM_GET_PAGE_PROT, keeping the generic protection_map[]
since platform __SXXX/__PXX macros would be still around.
- Drop __SXXX/__PXXX across all platforms via just initializing
protection_map[] early during boot in the platform OR moving
both vm_get_page_prot() via ARCH_HAS_VM_GET_PAGE_PROT and the
generic protection_map[] inside the platform.
There were some objections with respect to switch case code in
comparison to the array based table look up.
>
>> +static pgprot_t arm64_arch_filter_pgprot(pgprot_t prot)
>> +{
>> + if (cpus_have_const_cap(ARM64_HAS_EPAN))
>> + return prot;
>> +
>> + if (pgprot_val(prot) != pgprot_val(PAGE_EXECONLY))
>> + return prot;
>> +
>> + return PAGE_READONLY_EXEC;
>> +}
>> +
>> +static pgprot_t arm64_arch_vm_get_page_prot(unsigned long vm_flags)
>> +{
>> + pteval_t prot = 0;
>> +
>> + if (vm_flags & VM_ARM64_BTI)
>> + prot |= PTE_GP;
>> +
>> + /*
>> + * There are two conditions required for returning a Normal Tagged
>> + * memory type: (1) the user requested it via PROT_MTE passed to
>> + * mmap() or mprotect() and (2) the corresponding vma supports MTE. We
>> + * register (1) as VM_MTE in the vma->vm_flags and (2) as
>> + * VM_MTE_ALLOWED. Note that the latter can only be set during the
>> + * mmap() call since mprotect() does not accept MAP_* flags.
>> + * Checking for VM_MTE only is sufficient since arch_validate_flags()
>> + * does not permit (VM_MTE & !VM_MTE_ALLOWED).
>> + */
>> + if (vm_flags & VM_MTE)
>> + prot |= PTE_ATTRINDX(MT_NORMAL_TAGGED);
>> +
>> + return __pgprot(prot);
>> +}
>> +
>> +pgprot_t vm_get_page_prot(unsigned long vm_flags)
>> +{
>> + pgprot_t ret = __pgprot(pgprot_val(__vm_get_page_prot(vm_flags)) |
>> + pgprot_val(arm64_arch_vm_get_page_prot(vm_flags)));
>> +
>> + return arm64_arch_filter_pgprot(ret);
>> +}
>
> If we kept the array, we can have everything in a single function
> (untested and with my own comments for future changes):
Got it.
>
> pgprot_t vm_get_page_prot(unsigned long vm_flags)
> {
> pgprot_t prot = __pgprot(pgprot_val(protection_map[vm_flags &
> (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]));
>
> /*
> * We could get rid of this test if we updated protection_map[]
> * to turn exec-only into read-exec during boot.
> */
> if (!cpus_have_const_cap(ARM64_HAS_EPAN) &&
> pgprot_val(prot) == pgprot_val(PAGE_EXECONLY))
> prot = PAGE_READONLY_EXEC;
>
> if (vm_flags & VM_ARM64_BTI)
> prot != PTE_GP;
>
> /*
> * We can get rid of the requirement for PROT_NORMAL to be 0
> * since here we can mask out PTE_ATTRINDX_MASK.
> */
> if (vm_flags & VM_MTE) {
> prot &= ~PTE_ATTRINDX_MASK;
> prot |= PTE_ATTRINDX(MT_NORMAL_TAGGED);
> }
>
> return prot;
> }
>
