From: takahiro.akashi@linaro.org (AKASHI Takahiro) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v30 04/11] arm64: mm: allow for unmapping memory region from kernel mapping Date: Thu, 26 Jan 2017 17:08:08 +0900 [thread overview] Message-ID: <20170126080807.GF23406@linaro.org> (raw) In-Reply-To: <5888C924.9080100@arm.com> On Wed, Jan 25, 2017 at 03:49:56PM +0000, James Morse wrote: > Hi Akashi, > > On 24/01/17 08:49, AKASHI Takahiro wrote: > > The current implementation of create_mapping_late() is only allowed > > to modify permission attributes (read-only or read-write) against > > the existing kernel mapping. > > > > In this patch, PAGE_KERNEL_INVALID protection attribute is introduced. > > We will now be able to invalidate (or unmap) some part of the existing > > Can we stick to calling this 'unmap', otherwise 'invalidate this page range' > becomes ambiguous, cache maintenance or page-table manipulation?! Sure. I hesitated to use "unmap" because we don't free any of descriptor table pages here, but who notice the differences. > > > kernel mapping by specifying PAGE_KERNEL_INVALID to create_mapping_late(). > > > > This feature will be used in a suceeding kdump patch to protect > > the memory reserved for crash dump kernel once after loaded. > > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > > index 17243e43184e..9c7adcce8e4e 100644 > > --- a/arch/arm64/mm/mmu.c > > +++ b/arch/arm64/mm/mmu.c > > @@ -190,7 +192,8 @@ static void alloc_init_pmd(pud_t *pud, unsigned long addr, unsigned long end, > > * Set the contiguous bit for the subsequent group of > > * PMDs if its size and alignment are appropriate. > > */ > > - if (((addr | phys) & ~CONT_PMD_MASK) == 0) { > > > + if ((pgprot_val(prot) | PMD_VALID) && > > & PMD_VALID? Shame on me. > > > + ((addr | phys) & ~CONT_PMD_MASK) == 0) { > > if (end - addr >= CONT_PMD_SIZE) > > __prot = __pgprot(pgprot_val(prot) | > > PTE_CONT); > > @@ -203,7 +206,8 @@ static void alloc_init_pmd(pud_t *pud, unsigned long addr, unsigned long end, > > * After the PMD entry has been populated once, we > > * only allow updates to the permission attributes. > > */ > > - BUG_ON(!pgattr_change_is_safe(pmd_val(old_pmd), > > + BUG_ON(pmd_valid(old_pmd) && pmd_valid(*pmd) && > > + !pgattr_change_is_safe(pmd_val(old_pmd), > > pmd_val(*pmd))); > > Could you add PTE_VALID checks to pgattr_change_is_safe() instead of at every > call? I think (old == 0 || new == 0) is probably doing something similar. Good catch :) > > > } else { > > alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys), > > @@ -791,14 +796,20 @@ int __init arch_ioremap_pmd_supported(void) > > int pud_set_huge(pud_t *pud, phys_addr_t phys, pgprot_t prot) > > { > > BUG_ON(phys & ~PUD_MASK); > > - set_pud(pud, __pud(phys | PUD_TYPE_SECT | pgprot_val(mk_sect_prot(prot)))); > > + set_pud(pud, __pud(phys | > > + ((pgprot_val(prot) & PUD_VALID) ? > > + PUD_TYPE_SECT : 0) | > > + pgprot_val(mk_sect_prot(prot)))); > > This looks complicated. Is this equivalent?: > > > prot = mk_sect_prot(prot); > > if (pgprot_val(prot) & PUD_VALID) > > prot |= PUD_TYPE_SECT; > > > > set_pud(pud, __pud(phys | pgprot_val(prot))); Yeah, I just wanted to keep it simple, > > Given you defined PUD_VALID as being PUD_TYPE_SECT, I think you can then reduce > it to: > > set_pud(pud, __pud(phys | pgprot_val(mk_sect_prot(prot)))); > > It looks like mk_sect_prot() doesn't touch the valid bit so all this is doing is > clearing the table bit making it a section and keeping the valid bit from the > caller. but this seems to be too much optimized because, even without my patch, this change is applicable. The intention of the original code would be to make sure the entry be a pud-level descriptor whatever given "prot" is. -Takahiro AKASHI > > Thanks, > > James >
WARNING: multiple messages have this Message-ID (diff)
From: AKASHI Takahiro <takahiro.akashi@linaro.org> To: James Morse <james.morse@arm.com> Cc: mark.rutland@arm.com, geoff@infradead.org, catalin.marinas@arm.com, will.deacon@arm.com, bauerman@linux.vnet.ibm.com, dyoung@redhat.com, kexec@lists.infradead.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v30 04/11] arm64: mm: allow for unmapping memory region from kernel mapping Date: Thu, 26 Jan 2017 17:08:08 +0900 [thread overview] Message-ID: <20170126080807.GF23406@linaro.org> (raw) In-Reply-To: <5888C924.9080100@arm.com> On Wed, Jan 25, 2017 at 03:49:56PM +0000, James Morse wrote: > Hi Akashi, > > On 24/01/17 08:49, AKASHI Takahiro wrote: > > The current implementation of create_mapping_late() is only allowed > > to modify permission attributes (read-only or read-write) against > > the existing kernel mapping. > > > > In this patch, PAGE_KERNEL_INVALID protection attribute is introduced. > > We will now be able to invalidate (or unmap) some part of the existing > > Can we stick to calling this 'unmap', otherwise 'invalidate this page range' > becomes ambiguous, cache maintenance or page-table manipulation?! Sure. I hesitated to use "unmap" because we don't free any of descriptor table pages here, but who notice the differences. > > > kernel mapping by specifying PAGE_KERNEL_INVALID to create_mapping_late(). > > > > This feature will be used in a suceeding kdump patch to protect > > the memory reserved for crash dump kernel once after loaded. > > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > > index 17243e43184e..9c7adcce8e4e 100644 > > --- a/arch/arm64/mm/mmu.c > > +++ b/arch/arm64/mm/mmu.c > > @@ -190,7 +192,8 @@ static void alloc_init_pmd(pud_t *pud, unsigned long addr, unsigned long end, > > * Set the contiguous bit for the subsequent group of > > * PMDs if its size and alignment are appropriate. > > */ > > - if (((addr | phys) & ~CONT_PMD_MASK) == 0) { > > > + if ((pgprot_val(prot) | PMD_VALID) && > > & PMD_VALID? Shame on me. > > > + ((addr | phys) & ~CONT_PMD_MASK) == 0) { > > if (end - addr >= CONT_PMD_SIZE) > > __prot = __pgprot(pgprot_val(prot) | > > PTE_CONT); > > @@ -203,7 +206,8 @@ static void alloc_init_pmd(pud_t *pud, unsigned long addr, unsigned long end, > > * After the PMD entry has been populated once, we > > * only allow updates to the permission attributes. > > */ > > - BUG_ON(!pgattr_change_is_safe(pmd_val(old_pmd), > > + BUG_ON(pmd_valid(old_pmd) && pmd_valid(*pmd) && > > + !pgattr_change_is_safe(pmd_val(old_pmd), > > pmd_val(*pmd))); > > Could you add PTE_VALID checks to pgattr_change_is_safe() instead of at every > call? I think (old == 0 || new == 0) is probably doing something similar. Good catch :) > > > } else { > > alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys), > > @@ -791,14 +796,20 @@ int __init arch_ioremap_pmd_supported(void) > > int pud_set_huge(pud_t *pud, phys_addr_t phys, pgprot_t prot) > > { > > BUG_ON(phys & ~PUD_MASK); > > - set_pud(pud, __pud(phys | PUD_TYPE_SECT | pgprot_val(mk_sect_prot(prot)))); > > + set_pud(pud, __pud(phys | > > + ((pgprot_val(prot) & PUD_VALID) ? > > + PUD_TYPE_SECT : 0) | > > + pgprot_val(mk_sect_prot(prot)))); > > This looks complicated. Is this equivalent?: > > > prot = mk_sect_prot(prot); > > if (pgprot_val(prot) & PUD_VALID) > > prot |= PUD_TYPE_SECT; > > > > set_pud(pud, __pud(phys | pgprot_val(prot))); Yeah, I just wanted to keep it simple, > > Given you defined PUD_VALID as being PUD_TYPE_SECT, I think you can then reduce > it to: > > set_pud(pud, __pud(phys | pgprot_val(mk_sect_prot(prot)))); > > It looks like mk_sect_prot() doesn't touch the valid bit so all this is doing is > clearing the table bit making it a section and keeping the valid bit from the > caller. but this seems to be too much optimized because, even without my patch, this change is applicable. The intention of the original code would be to make sure the entry be a pud-level descriptor whatever given "prot" is. -Takahiro AKASHI > > Thanks, > > James > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
next prev parent reply other threads:[~2017-01-26 8:08 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-01-24 8:46 [PATCH v30 00/11] arm64: add kdump support AKASHI Takahiro 2017-01-24 8:46 ` AKASHI Takahiro 2017-01-24 8:49 ` [PATCH v30 01/11] memblock: add memblock_cap_memory_range() AKASHI Takahiro 2017-01-24 8:49 ` AKASHI Takahiro 2017-01-24 8:49 ` AKASHI Takahiro 2017-01-24 8:49 ` [PATCH v30 02/11] arm64: limit memory regions based on DT property, usable-memory-range AKASHI Takahiro 2017-01-24 8:49 ` AKASHI Takahiro 2017-01-24 8:49 ` [PATCH v30 03/11] arm64: kdump: reserve memory for crash dump kernel AKASHI Takahiro 2017-01-24 8:49 ` AKASHI Takahiro 2017-01-24 8:49 ` [PATCH v30 04/11] arm64: mm: allow for unmapping memory region from kernel mapping AKASHI Takahiro 2017-01-24 8:49 ` AKASHI Takahiro 2017-01-24 11:32 ` Pratyush Anand 2017-01-24 11:32 ` Pratyush Anand 2017-01-25 6:37 ` AKASHI Takahiro 2017-01-25 6:37 ` AKASHI Takahiro 2017-01-25 15:49 ` James Morse 2017-01-25 15:49 ` James Morse 2017-01-26 8:08 ` AKASHI Takahiro [this message] 2017-01-26 8:08 ` AKASHI Takahiro 2017-01-24 8:49 ` [PATCH v30 05/11] arm64: kdump: protect crash dump kernel memory AKASHI Takahiro 2017-01-24 8:49 ` AKASHI Takahiro 2017-01-25 17:37 ` James Morse 2017-01-25 17:37 ` James Morse 2017-01-26 11:28 ` AKASHI Takahiro 2017-01-26 11:28 ` AKASHI Takahiro 2017-01-27 11:19 ` James Morse 2017-01-27 11:19 ` James Morse 2017-01-27 17:15 ` AKASHI Takahiro 2017-01-27 17:15 ` AKASHI Takahiro 2017-01-27 18:56 ` Mark Rutland 2017-01-27 18:56 ` Mark Rutland 2017-01-30 8:42 ` AKASHI Takahiro 2017-01-30 8:42 ` AKASHI Takahiro 2017-01-30 8:27 ` AKASHI Takahiro 2017-01-30 8:27 ` AKASHI Takahiro 2017-01-27 13:59 ` James Morse 2017-01-27 13:59 ` James Morse 2017-01-27 15:42 ` AKASHI Takahiro 2017-01-27 15:42 ` AKASHI Takahiro 2017-01-27 19:41 ` Mark Rutland 2017-01-27 19:41 ` Mark Rutland 2017-01-24 8:50 ` [PATCH v30 06/11] arm64: kdump: implement machine_crash_shutdown() AKASHI Takahiro 2017-01-24 8:50 ` AKASHI Takahiro 2017-01-24 8:50 ` [PATCH v30 07/11] arm64: kdump: add VMCOREINFO's for user-space tools AKASHI Takahiro 2017-01-24 8:50 ` AKASHI Takahiro 2017-01-24 8:50 ` [PATCH v30 08/11] arm64: kdump: provide /proc/vmcore file AKASHI Takahiro 2017-01-24 8:50 ` AKASHI Takahiro 2017-01-24 8:50 ` [PATCH v30 09/11] arm64: kdump: enable kdump in defconfig AKASHI Takahiro 2017-01-24 8:50 ` AKASHI Takahiro 2017-01-24 8:50 ` [PATCH v30 10/11] Documentation: kdump: describe arm64 port AKASHI Takahiro 2017-01-24 8:50 ` AKASHI Takahiro [not found] ` <20170124084638.3770-1-takahiro.akashi-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 2017-01-24 8:53 ` [PATCH v30 11/11] Documentation: dt: chosen properties for arm64 kdump AKASHI Takahiro 2017-01-24 8:53 ` AKASHI Takahiro 2017-01-24 8:53 ` AKASHI Takahiro
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=20170126080807.GF23406@linaro.org \ --to=takahiro.akashi@linaro.org \ --cc=linux-arm-kernel@lists.infradead.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: linkBe 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.