[PATCH v31 04/12] arm64: mm: allow for unmapping part of kernel mapping
From: AKASHI Takahiro <hidden>
Date: 2017-02-02 14:55:54
Also in:
kexec
On Thu, Feb 02, 2017 at 02:35:35PM +0000, Mark Rutland wrote:
On Thu, Feb 02, 2017 at 11:01:03PM +0900, AKASHI Takahiro wrote:quoted
On Thu, Feb 02, 2017 at 11:44:38AM +0000, Mark Rutland wrote:quoted
On Thu, Feb 02, 2017 at 07:21:32PM +0900, AKASHI Takahiro wrote:quoted
On Wed, Feb 01, 2017 at 04:03:54PM +0000, Mark Rutland wrote:quoted
Hi, On Wed, Feb 01, 2017 at 09:46:23PM +0900, AKASHI Takahiro wrote:quoted
A new function, remove_pgd_mapping(), is added. It allows us to unmap a specific portion of kernel mapping later as far as the mapping is made using create_pgd_mapping() and unless we try to free a sub-set of memory range within a section mapping.I'm not keen on adding more page table modification code. It was painful enough to ensure that those worked in all configurations. Why can't we reuse create_pgd_mapping()? If we pass page_mappings_only, and use an invalid prot (i.e. 0), what is the problem?As I did in v30? (though my implementation in v30 should be improved.)Something like that. I wasn't entirely sure why we needed to change those functions so much, so I'm clearly missing something there. I'll go have another look.I would be much easier if you see my new code.Sure. FWIW, I took a look, and I understand why those changes were necessary.quoted
quoted
quoted
If we don't need to free unused page tables, that would make things much simple. There are still some minor problems on the merge, but we can sort it out.I'm not sure I follow what you mean by 'on merge' here. Could you elaborate?What I had in mind is some changes needed to handle "__prot(0)" properly in alloc_init_pxx(). For example, p[mu]d_set_huge() doesn't make a "zeroed" entry.I think that if we only allow ourselves to make PTEs invalid, we don't have to handle that case. If we use page_mappings_only, we should only check pgattr_change_is_safe() for the pte level, and the {pmd,pud,pgd} entries shouldn't change. Is the below sufficient to allow that, or have I missed something?
I think it will be OK, but will double-check tomorrow. However, is is acceptable that create_pgd_mapping( __prot(0) ) can only handle the cases of page-mapping-only? That would be fine to kdump, but in general? -Takahiro AKASHI
quoted hunk ↗ jump to hunk
Thanks, Mark. ---->8----diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index 17243e4..05bf7bf 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c@@ -105,6 +105,22 @@ static bool pgattr_change_is_safe(u64 old, u64 new) return old == 0 || new == 0 || ((old ^ new) & ~mask) == 0; } +static bool pte_change_is_valid(pte old, pte new) +{ + /* + * So long as we subsequently perform TLB invalidation, it is safe to + * change a PTE to an invalid, but non-zero value. We only allow this + * for PTEs since there's no complicated allocation/free issues to deal + * with. + * + * Otherwise, the usual attribute change rules apply. + */ + if (!pte_valid(old) || !pte_valid(new)) + return true; + + return pgattr_change_is_safe(pte_val(old), pte_val(new)); +} + static void alloc_init_pte(pmd_t *pmd, unsigned long addr, unsigned long end, unsigned long pfn, pgprot_t prot,@@ -143,11 +159,7 @@ static void alloc_init_pte(pmd_t *pmd, unsigned long addr, set_pte(pte, pfn_pte(pfn, __prot)); pfn++; - /* - * After the PTE entry has been populated once, we - * only allow updates to the permission attributes. - */ - BUG_ON(!pgattr_change_is_safe(pte_val(old_pte), pte_val(*pte))); + BUG_ON(!pte_change_is_valid(old_pte, pte)); } while (pte++, addr += PAGE_SIZE, addr != end);