From: Balbir Singh <bsingharora@gmail.com>
To: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>,
benh@kernel.crashing.org, paulus@samba.org, mpe@ellerman.id.au
Cc: linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH 02/65] powerpc/mm: use _PAGE_READ to indicate Read access
Date: Tue, 5 Apr 2016 13:25:41 +1000 [thread overview]
Message-ID: <57033035.8040904@gmail.com> (raw)
In-Reply-To: <87shz1xw2q.fsf@linux.vnet.ibm.com>
On 05/04/16 00:55, Aneesh Kumar K.V wrote:
> Balbir Singh <bsingharora@gmail.com> writes:
>
>> [ text/plain ]
>>
>>
>> On 27/03/16 19:23, Aneesh Kumar K.V wrote:
>>> This split _PAGE_RW bit to _PAGE_READ and _PAGE_WRITE. It also remove
>>> the dependency on _PAGE_USER for implying read only. Few things to note
>>> here is that, we have read implied with write and execute permission.
>>> Hence we should always find _PAGE_READ set on hash pte fault.
>>>
>>> We still can't switch PROT_NONE to !(_PAGE_RWX). Auto numa do depend
>>> on marking a prot none pte _PAGE_WRITE. (For more details look at
>>> b191f9b106ea "mm: numa: preserve PTE write permissions across a NUMA hinting fault")
>>>
> ......
>
>
>>> diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
>>> index 90dd9280894f..ea23403b3fc0 100644
>>> --- a/arch/powerpc/mm/hash_utils_64.c
>>> +++ b/arch/powerpc/mm/hash_utils_64.c
>>> @@ -175,8 +175,9 @@ unsigned long htab_convert_pte_flags(unsigned long pteflags)
>>> * or PP=0x3 for read-only (including writeable but clean pages).
>>> */
>>> if (pteflags & _PAGE_USER) {
>>> - rflags |= 0x2;
>>> - if (!((pteflags & _PAGE_RW) && (pteflags & _PAGE_DIRTY)))
>>> + if (pteflags & _PAGE_RWX)
>> Should this be pteflags & _PAGE_RW?
> That will work, because we always have _PAGE_READ set for _PAGE_EXEC.
> But what we really need to check here is READ/WRITE/EXEC.
Does READ/WRITE matter for pp bits?
>
>>> + rflags |= 0x2;
>>> + if (!((pteflags & _PAGE_WRITE) && (pteflags & _PAGE_DIRTY)))
>>> rflags |= 0x1;
>> if pteflags == _PAGE_USER | _PAGE_WRITE | _PAGE_DIRTY, what is rflags set to?
>>
>
> rflags will be 0x2. and for readlyonly 0x3. That is documented above.
Yep the comment says that, I think the comment can enhanced to add more information
For example
pp = 0x3 for PAGE_READ | PAGE_EXEC (read-only)
pp = 0x2 if the page is writable (read-write)
>
>
>>
>>> }
>>> /*
>>> @@ -1205,7 +1206,7 @@ EXPORT_SYMBOL_GPL(hash_page);
>>> int __hash_page(unsigned long ea, unsigned long msr, unsigned long trap,
>>> unsigned long dsisr)
>>> {
>>> - unsigned long access = _PAGE_PRESENT;
>>> + unsigned long access = _PAGE_PRESENT | _PAGE_READ;
>>> unsigned long flags = 0;
>>> struct mm_struct *mm = current->mm;
>
> ....
>
>>> diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
>>> index 83dfd7925c72..98b5c03e344d 100644
>>> --- a/arch/powerpc/mm/pgtable.c
>>> +++ b/arch/powerpc/mm/pgtable.c
>>> @@ -177,8 +177,8 @@ void set_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
>>> * _PAGE_PRESENT, but we can be sure that it is not in hpte.
>>> * Hence we can use set_pte_at for them.
>>> */
>>> - VM_WARN_ON((pte_val(*ptep) & (_PAGE_PRESENT | _PAGE_USER)) ==
>>> - (_PAGE_PRESENT | _PAGE_USER));
>>> + VM_WARN_ON(pte_present(*ptep) && !pte_protnone(*ptep));
>>> +
>> What was wrong with the previous check? The compiler will optimize it, but the new
>> check uses two pte_val() calls on *ptep
> That was confusing, even though we documented it clearly above, it does
> confuse, because checking for _PAGE_USER is an indirect hint for prot
> numa ptes.
>
>
>>> /*
>>> * Add the pte bit when tryint set a pte
>>> */
>>> diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c
>>> index aa742aa35b64..00d8d985bba3 100644
>>> --- a/arch/powerpc/mm/pgtable_64.c
>>> +++ b/arch/powerpc/mm/pgtable_64.c
>>> @@ -277,7 +277,7 @@ void __iomem * ioremap_prot(phys_addr_t addr, unsigned long size,
>>> void *caller = __builtin_return_address(0);
>>>
>>> /* writeable implies dirty for kernel addresses */
>>> - if (flags & _PAGE_RW)
>>> + if (flags & _PAGE_WRITE)
>>> flags |= _PAGE_DIRTY;
>>>
>>> /* we don't want to let _PAGE_USER and _PAGE_EXEC leak out */
>>> @@ -681,8 +681,7 @@ void set_pmd_at(struct mm_struct *mm, unsigned long addr,
>>> pmd_t *pmdp, pmd_t pmd)
>>> {
>>> #ifdef CONFIG_DEBUG_VM
>>> - WARN_ON((pmd_val(*pmdp) & (_PAGE_PRESENT | _PAGE_USER)) ==
>>> - (_PAGE_PRESENT | _PAGE_USER));
>>> + WARN_ON(pte_present(pmd_pte(*pmdp)) && !pte_protnone(pmd_pte(*pmdp)));
>> Same as above, plus I think we can move these to VM_WARN_ON just to be consistent
> We already have #ifdef CONFIG_DEBUG_VM around, hence we can avoid using
> VM_WARN_ON.
-just nit-picking
Balbir Singh
next prev parent reply other threads:[~2016-04-05 3:25 UTC|newest]
Thread overview: 92+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-27 8:23 [PATCH 01/65] powerpc/mm: Use big endian page table for book3s 64 Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 02/65] powerpc/mm: use _PAGE_READ to indicate Read access Aneesh Kumar K.V
2016-03-27 15:35 ` Ian Munsie
2016-03-31 0:57 ` Balbir Singh
2016-04-04 14:55 ` Aneesh Kumar K.V
2016-04-05 3:25 ` Balbir Singh [this message]
2016-03-27 8:23 ` [PATCH 03/65] powerpc/mm/subpage: Clear RWX bit to indicate no access Aneesh Kumar K.V
2016-03-31 1:42 ` Balbir Singh
2016-04-04 14:59 ` Aneesh Kumar K.V
2016-04-05 3:30 ` Balbir Singh
2016-03-27 8:23 ` [PATCH 04/65] powerpc/mm: Use pte_user instead of opencoding Aneesh Kumar K.V
2016-03-31 1:45 ` Balbir Singh
2016-04-04 15:00 ` Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 05/65] powerpc/mm: Replace _PAGE_USER with _PAGE_PRIVILEGED Aneesh Kumar K.V
2016-03-27 15:41 ` Ian Munsie
2016-03-27 8:23 ` [PATCH 06/65] powerpc/cxl: Use REGION_ID instead of opencoding Aneesh Kumar K.V
2016-03-27 15:48 ` Ian Munsie
2016-03-27 8:23 ` [PATCH 07/65] powerpc/mm: Remove RPN_SHIFT and RPN_SIZE Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 08/65] powerpc/mm: Update _PAGE_KERNEL_RO Aneesh Kumar K.V
2016-04-05 7:45 ` Balbir Singh
2016-03-27 8:23 ` [PATCH 09/65] powerpc/mm: Use helper for finding pte bits mapping I/O area Aneesh Kumar K.V
2016-04-05 11:47 ` Balbir Singh
2016-03-27 8:23 ` [PATCH 10/65] powerpc/mm: Drop WIMG in favour of new constants Aneesh Kumar K.V
2016-04-05 13:25 ` Balbir Singh
2016-03-27 8:23 ` [PATCH 11/65] powerpc/mm: Use generic version of pmdp_clear_flush_young Aneesh Kumar K.V
2016-04-05 23:51 ` Balbir Singh
2016-03-27 8:23 ` [PATCH 12/65] powerpc/mm: Use generic version of ptep_clear_flush_young Aneesh Kumar K.V
2016-04-05 23:53 ` Balbir Singh
2016-03-27 8:23 ` [PATCH 13/65] powerpc/mm: Move common data structure between radix and hash to book3s 64 generic headers Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 14/65] powerpc/mm/power9: Add partition table format Aneesh Kumar K.V
2016-04-06 6:11 ` Balbir Singh
2016-03-27 8:23 ` [PATCH 15/65] powerpc/mm/hash: Add support for POWER9 hash Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 16/65] powerpc/mm: Move hash and no hash code to separate files Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 17/65] powerpc/mm/book3s: Rename hash specific PTE bits to carry H_ prefix Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 18/65] powerpc/mm: Handle _PTE_NONE_MASK Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 19/65] powerpc/mm: Move common pte bits and accessors to book3s/64/pgtable.h Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 20/65] powerpc/mm: Move pte accessors that operate on common pte bits to pgtable.h Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 21/65] powerpc/mm: Make page table size a variable Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 22/65] powerpc/mm: Move page table index and and vaddr to pgtable.h Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 23/65] powerpc/mm: Move pte related function together Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 24/65] powerpc/mm/radix: Add radix pte defines Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 25/65] powerpc/mm/radix: Dummy radix_enabled() Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 26/65] powerpc/mm: Add radix callbacks to pte accessors Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 27/65] powerpc/mm: Move hugetlb and THP related pmd accessors to pgtable.h Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 28/65] powerpc/mm/radix: Add radix callback for pmd accessors Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 29/65] powerpc/mm: Abstraction for early init routines Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 30/65] powerpc/mm/radix: Add radix callback " Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 31/65] powerpc/mm: Abstraction for vmemmap and map_kernel_page Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 32/65] powerpc/mm/radix: Add radix callback for vmemmap and map_kernel page Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 33/65] powerpc/mm: Abstraction for switch_mmu_context Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 34/65] powerpc/mm/radix: Add mmu context handling callback for radix Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 35/65] powerpc/mm: Rename mmu_context_hash64.c to mmu_context_book3s64.c Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 36/65] powerpc/mm: Hash linux abstraction for tlbflush routines Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 37/65] powerpc/mm/radix: Add " Aneesh Kumar K.V
2016-03-27 10:00 ` kbuild test robot
2016-03-28 17:58 ` Aneesh Kumar K.V
2016-03-27 10:09 ` kbuild test robot
2016-03-27 10:11 ` kbuild test robot
2016-03-27 8:23 ` [PATCH 38/65] powerpc/mm/radix: Add MMU_FTR_RADIX Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 39/65] powerpc/mm/radix: Use STD_MMU_64 to properly isolate hash related code Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 40/65] powerpc/mm/radix: Isolate hash table function from pseries guest code Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 41/65] powerpc/mm/radix: Add checks in slice code to catch radix usage Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 42/65] powerpc/mm/radix: Limit paca allocation in radix Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 43/65] powerpc/mm/radix: Pick the address layout for radix config Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 44/65] powerpc/mm/radix: Update secondary PTCR Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 45/65] powerpc/mm: Make a copy of pgalloc.h for 32 and 64 book3s Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 46/65] powerpc/mm: revert changes made to generic pgalloc-64.h Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 47/65] powerpc/mm: Copy pgalloc (part 2) Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 48/65] powerpc/mm: Simplify the code dropping 4 level table #ifdef Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 49/65] powerpc/mm: Rename function to indicate we are allocating fragments Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 50/65] powerpc/mm: make 4k and 64k use pte_t for pgtable_t Aneesh Kumar K.V
2016-03-27 8:23 ` [PATCH 51/65] powerpc/mm: Add radix pgalloc details Aneesh Kumar K.V
2016-03-27 8:24 ` [PATCH 52/65] powerpc/mm: Update pte filter for radix Aneesh Kumar K.V
2016-03-27 8:24 ` [PATCH 53/65] powerpc/mm: VMALLOC abstraction Aneesh Kumar K.V
2016-03-27 8:24 ` [PATCH 54/65] powerpc/radix: update mmu cache Aneesh Kumar K.V
2016-03-27 8:24 ` [PATCH 55/65] powerpc/mm: pte_frag abstraction Aneesh Kumar K.V
2016-03-27 8:24 ` [PATCH 56/65] powerpc/mm: Fix vma_mmu_pagesize for radix Aneesh Kumar K.V
2016-03-27 8:24 ` [PATCH 57/65] powerpc/mm: Add radix support for hugetlb Aneesh Kumar K.V
2016-03-27 8:24 ` [PATCH 58/65] powerpc/mm/radix: Make sure swapper pgdir is properly aligned Aneesh Kumar K.V
2016-03-27 8:24 ` [PATCH 59/65] powerpc/mm/radix: Add hugetlb support 4K page size Aneesh Kumar K.V
2016-03-27 8:24 ` [PATCH 60/65] powerpc/mm: Drop PTE_ATOMIC_UPDATES from pmd_hugepage_update Aneesh Kumar K.V
2016-03-27 8:24 ` [PATCH 61/65] powerpc/mm: THP is only available on hash64 as of now Aneesh Kumar K.V
2016-03-27 8:24 ` [PATCH 62/65] powerpc/mm/thp: Abstraction for THP functions Aneesh Kumar K.V
2016-03-27 8:24 ` [PATCH 63/65] powerpc/mm/radix: Add radix THP callbacks Aneesh Kumar K.V
2016-03-27 8:24 ` [PATCH 64/65] powerpc/mm/radix: Add THP support for 4k linux page size Aneesh Kumar K.V
2016-03-27 8:24 ` [PATCH 65/65] powerpc/mm/radix: Cputable update for radix Aneesh Kumar K.V
2016-03-30 1:01 ` Michael Neuling
2016-04-01 6:25 ` Michael Ellerman
2016-04-01 9:34 ` Aneesh Kumar K.V
2018-06-28 23:55 ` Benjamin Herrenschmidt
2016-03-30 10:53 ` [PATCH 01/65] powerpc/mm: Use big endian page table for book3s 64 Balbir Singh
2016-04-04 15:03 ` Aneesh Kumar K.V
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=57033035.8040904@gmail.com \
--to=bsingharora@gmail.com \
--cc=aneesh.kumar@linux.vnet.ibm.com \
--cc=benh@kernel.crashing.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
--cc=paulus@samba.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: link
Be 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.