Re: [PATCH v3] powerpc: Use PFN_PHYS() to avoid truncating the physical address
On Fri, 2015-03-27 at 13:07 -0500, Scott Wood wrote: > On Fri, 2015-03-27 at 10:45 +1100, Michael Ellerman wrote: > > On Thu, 2015-03-26 at 10:31 -0500, Emil Medve wrote: > > > Hello Kumar, > > > > > > > > > On 03/26/2015 10:18 AM, Kumar Gala wrote: > > > > Why no commit message with what issue this change was trying to fix? > > > > > > A while back, when I attempted to remove bootmem (in favor of just plain > > > memblock as in powerpc land bootmem was just a wrapper to memblock > > > anyway) I run at some point into a problem with an intermediate address > > > value because of this '<< PAGE_SHIFT' on the wrong width variable. Using > > > PFN_PHYS() took care of it (it has a cast) so I decided to get this > > > defensive patch applied. Since, I dropped my bootmem/memblock patches in > > > favor to Anton's (Blanchard) work so my concrete issue example is > > > somewhat gone > > > > I'm not a big fan of it unless it's actually fixing an issue. It's a lot of > > churn and the end result is less readable IMHO. > > It is fixing an issue -- the issue is that there are overflow errors in > the code. Some of the places Emil fixed are only for platforms that > don't have physical addresses larger than pointers, or have the needed > casts, or are known to be dealing with lowmem, but others aren't. E.g. > page_is_ram() and devmem_is_allowed() are buggy on 32-bit with 64-bit > physical. Right. So obviously I'm fine with all the cases where it fixes an actual bug. > flush_dcache_icache_page() is buggy on mpc86xx with more than 4 GiB RAM > -- though that would still be buggy even with this change, due to > __flush_dcache_icache_phys taking unsigned long. The entire concept of > that function doesn't work for sizeof(phys_addr_t) > sizeof(void *), so > in this case 86xx should be using the booke code instead. > > Even in the places where overflow can't happen due to the above > circumstances (other than having the needed cast), it's setting a bad > example that can be copied to places where it will break, or the > circumstances of the code could change (e.g. currently 64-bit-only code > being used on 32-bit). I agree with that in principle, but it looks like in a bunch of places this patch ends up assigning the result to unsigned long anyway. So those cases are just churn IMHO. The end result doesn't work with 64-bit phys if the code is ever used there, even though it looks like maybe it should, and it's also not setting a good example for other code. Those cases should either be left alone, or fixed properly to use phys_addr_t. cheers ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3] powerpc: Use PFN_PHYS() to avoid truncating the physical address
On Fri, 2015-03-27 at 10:45 +1100, Michael Ellerman wrote: > On Thu, 2015-03-26 at 10:31 -0500, Emil Medve wrote: > > Hello Kumar, > > > > > > On 03/26/2015 10:18 AM, Kumar Gala wrote: > > > Why no commit message with what issue this change was trying to fix? > > > > A while back, when I attempted to remove bootmem (in favor of just plain > > memblock as in powerpc land bootmem was just a wrapper to memblock > > anyway) I run at some point into a problem with an intermediate address > > value because of this '<< PAGE_SHIFT' on the wrong width variable. Using > > PFN_PHYS() took care of it (it has a cast) so I decided to get this > > defensive patch applied. Since, I dropped my bootmem/memblock patches in > > favor to Anton's (Blanchard) work so my concrete issue example is > > somewhat gone > > I'm not a big fan of it unless it's actually fixing an issue. It's a lot of > churn and the end result is less readable IMHO. It is fixing an issue -- the issue is that there are overflow errors in the code. Some of the places Emil fixed are only for platforms that don't have physical addresses larger than pointers, or have the needed casts, or are known to be dealing with lowmem, but others aren't. E.g. page_is_ram() and devmem_is_allowed() are buggy on 32-bit with 64-bit physical. flush_dcache_icache_page() is buggy on mpc86xx with more than 4 GiB RAM -- though that would still be buggy even with this change, due to __flush_dcache_icache_phys taking unsigned long. The entire concept of that function doesn't work for sizeof(phys_addr_t) > sizeof(void *), so in this case 86xx should be using the booke code instead. Even in the places where overflow can't happen due to the above circumstances (other than having the needed cast), it's setting a bad example that can be copied to places where it will break, or the circumstances of the code could change (e.g. currently 64-bit-only code being used on 32-bit). -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3] powerpc: Use PFN_PHYS() to avoid truncating the physical address
On Thu, 2015-03-26 at 10:31 -0500, Emil Medve wrote: > Hello Kumar, > > > On 03/26/2015 10:18 AM, Kumar Gala wrote: > > Why no commit message with what issue this change was trying to fix? > > A while back, when I attempted to remove bootmem (in favor of just plain > memblock as in powerpc land bootmem was just a wrapper to memblock > anyway) I run at some point into a problem with an intermediate address > value because of this '<< PAGE_SHIFT' on the wrong width variable. Using > PFN_PHYS() took care of it (it has a cast) so I decided to get this > defensive patch applied. Since, I dropped my bootmem/memblock patches in > favor to Anton's (Blanchard) work so my concrete issue example is > somewhat gone I'm not a big fan of it unless it's actually fixing an issue. It's a lot of churn and the end result is less readable IMHO. cheers ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3] powerpc: Use PFN_PHYS() to avoid truncating the physical address
Hello Kumar, On 03/26/2015 10:18 AM, Kumar Gala wrote: > Why no commit message with what issue this change was trying to fix? A while back, when I attempted to remove bootmem (in favor of just plain memblock as in powerpc land bootmem was just a wrapper to memblock anyway) I run at some point into a problem with an intermediate address value because of this '<< PAGE_SHIFT' on the wrong width variable. Using PFN_PHYS() took care of it (it has a cast) so I decided to get this defensive patch applied. Since, I dropped my bootmem/memblock patches in favor to Anton's (Blanchard) work so my concrete issue example is somewhat gone Cheers, > - k > > On Mar 25, 2015, at 8:49 AM, Emil Medve wrote: > >> Signed-off-by: Emil Medve >> --- >> >> v3: Rebased and updated due to upstream changes since v2 >> >> v2: Rebased and updated due to upstream changes since v1 >> >> arch/powerpc/include/asm/io.h | 2 +- >> arch/powerpc/include/asm/page.h| 2 +- >> arch/powerpc/include/asm/pgalloc-32.h | 2 +- >> arch/powerpc/include/asm/rtas.h| 3 ++- >> arch/powerpc/kernel/crash_dump.c | 2 +- >> arch/powerpc/kernel/eeh.c | 4 +--- >> arch/powerpc/kernel/io-workarounds.c | 2 +- >> arch/powerpc/kernel/pci-common.c | 2 +- >> arch/powerpc/kernel/vdso.c | 6 +++--- >> arch/powerpc/kvm/book3s_64_mmu_host.c | 2 +- >> arch/powerpc/kvm/book3s_64_mmu_hv.c| 2 +- >> arch/powerpc/kvm/book3s_hv_rm_mmu.c| 4 ++-- >> arch/powerpc/kvm/e500_mmu_host.c | 5 ++--- >> arch/powerpc/mm/hugepage-hash64.c | 2 +- >> arch/powerpc/mm/hugetlbpage-book3e.c | 2 +- >> arch/powerpc/mm/hugetlbpage-hash64.c | 2 +- >> arch/powerpc/mm/mem.c | 9 - >> arch/powerpc/mm/numa.c | 5 ++--- >> arch/powerpc/platforms/powernv/opal.c | 2 +- >> arch/powerpc/platforms/pseries/iommu.c | 8 >> 20 files changed, 32 insertions(+), 36 deletions(-) ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3] powerpc: Use PFN_PHYS() to avoid truncating the physical address
Why no commit message with what issue this change was trying to fix? - k On Mar 25, 2015, at 8:49 AM, Emil Medve wrote: > Signed-off-by: Emil Medve > --- > > v3: Rebased and updated due to upstream changes since v2 > > v2: Rebased and updated due to upstream changes since v1 > > arch/powerpc/include/asm/io.h | 2 +- > arch/powerpc/include/asm/page.h| 2 +- > arch/powerpc/include/asm/pgalloc-32.h | 2 +- > arch/powerpc/include/asm/rtas.h| 3 ++- > arch/powerpc/kernel/crash_dump.c | 2 +- > arch/powerpc/kernel/eeh.c | 4 +--- > arch/powerpc/kernel/io-workarounds.c | 2 +- > arch/powerpc/kernel/pci-common.c | 2 +- > arch/powerpc/kernel/vdso.c | 6 +++--- > arch/powerpc/kvm/book3s_64_mmu_host.c | 2 +- > arch/powerpc/kvm/book3s_64_mmu_hv.c| 2 +- > arch/powerpc/kvm/book3s_hv_rm_mmu.c| 4 ++-- > arch/powerpc/kvm/e500_mmu_host.c | 5 ++--- > arch/powerpc/mm/hugepage-hash64.c | 2 +- > arch/powerpc/mm/hugetlbpage-book3e.c | 2 +- > arch/powerpc/mm/hugetlbpage-hash64.c | 2 +- > arch/powerpc/mm/mem.c | 9 - > arch/powerpc/mm/numa.c | 5 ++--- > arch/powerpc/platforms/powernv/opal.c | 2 +- > arch/powerpc/platforms/pseries/iommu.c | 8 > 20 files changed, 32 insertions(+), 36 deletions(-) ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v3] powerpc: Use PFN_PHYS() to avoid truncating the physical address
Signed-off-by: Emil Medve
---
v3: Rebased and updated due to upstream changes since v2
v2: Rebased and updated due to upstream changes since v1
arch/powerpc/include/asm/io.h | 2 +-
arch/powerpc/include/asm/page.h| 2 +-
arch/powerpc/include/asm/pgalloc-32.h | 2 +-
arch/powerpc/include/asm/rtas.h| 3 ++-
arch/powerpc/kernel/crash_dump.c | 2 +-
arch/powerpc/kernel/eeh.c | 4 +---
arch/powerpc/kernel/io-workarounds.c | 2 +-
arch/powerpc/kernel/pci-common.c | 2 +-
arch/powerpc/kernel/vdso.c | 6 +++---
arch/powerpc/kvm/book3s_64_mmu_host.c | 2 +-
arch/powerpc/kvm/book3s_64_mmu_hv.c| 2 +-
arch/powerpc/kvm/book3s_hv_rm_mmu.c| 4 ++--
arch/powerpc/kvm/e500_mmu_host.c | 5 ++---
arch/powerpc/mm/hugepage-hash64.c | 2 +-
arch/powerpc/mm/hugetlbpage-book3e.c | 2 +-
arch/powerpc/mm/hugetlbpage-hash64.c | 2 +-
arch/powerpc/mm/mem.c | 9 -
arch/powerpc/mm/numa.c | 5 ++---
arch/powerpc/platforms/powernv/opal.c | 2 +-
arch/powerpc/platforms/pseries/iommu.c | 8
20 files changed, 32 insertions(+), 36 deletions(-)
diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index 9eaf301..d6454f5 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h
@@ -794,7 +794,7 @@ static inline void * phys_to_virt(unsigned long address)
/*
* Change "struct page" to physical address.
*/
-#define page_to_phys(page) ((phys_addr_t)page_to_pfn(page) << PAGE_SHIFT)
+#define page_to_phys(page) PFN_PHYS(page_to_pfn(page))
/*
* 32 bits still uses virt_to_bus() for it's implementation of DMA
diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
index 69c0598..30f33ed 100644
--- a/arch/powerpc/include/asm/page.h
+++ b/arch/powerpc/include/asm/page.h
@@ -128,7 +128,7 @@ extern long long virt_phys_offset;
#endif
#define virt_to_page(kaddr)pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
-#define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT)
+#define pfn_to_kaddr(pfn) __va(PFN_PHYS(pfn))
#define virt_addr_valid(kaddr) pfn_valid(__pa(kaddr) >> PAGE_SHIFT)
/*
diff --git a/arch/powerpc/include/asm/pgalloc-32.h
b/arch/powerpc/include/asm/pgalloc-32.h
index 842846c..3d19a8e 100644
--- a/arch/powerpc/include/asm/pgalloc-32.h
+++ b/arch/powerpc/include/asm/pgalloc-32.h
@@ -24,7 +24,7 @@ extern void pgd_free(struct mm_struct *mm, pgd_t *pgd);
#define pmd_populate_kernel(mm, pmd, pte) \
(pmd_val(*(pmd)) = __pa(pte) | _PMD_PRESENT)
#define pmd_populate(mm, pmd, pte) \
- (pmd_val(*(pmd)) = (page_to_pfn(pte) << PAGE_SHIFT) |
_PMD_PRESENT)
+ (pmd_val(*(pmd)) = PFN_PHYS(page_to_pfn(pte)) | _PMD_PRESENT)
#define pmd_pgtable(pmd) pmd_page(pmd)
#else
#define pmd_populate_kernel(mm, pmd, pte) \
diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 2e23e92..2e430b6d 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -3,6 +3,7 @@
#ifdef __KERNEL__
#include
+#include
#include
/*
@@ -418,7 +419,7 @@ extern void rtas_take_timebase(void);
#ifdef CONFIG_PPC_RTAS
static inline int page_is_rtas_user_buf(unsigned long pfn)
{
- unsigned long paddr = (pfn << PAGE_SHIFT);
+ unsigned long paddr = PFN_PHYS(pfn);
if (paddr >= rtas_rmo_buf && paddr < (rtas_rmo_buf + RTAS_RMOBUF_MAX))
return 1;
return 0;
diff --git a/arch/powerpc/kernel/crash_dump.c b/arch/powerpc/kernel/crash_dump.c
index cfa0f81..b6578ee 100644
--- a/arch/powerpc/kernel/crash_dump.c
+++ b/arch/powerpc/kernel/crash_dump.c
@@ -104,7 +104,7 @@ ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
return 0;
csize = min_t(size_t, csize, PAGE_SIZE);
- paddr = pfn << PAGE_SHIFT;
+ paddr = PFN_PHYS(pfn);
if (memblock_is_region_memory(paddr, csize)) {
vaddr = __va(paddr);
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 3b2252e..119af20 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -326,7 +326,6 @@ void eeh_slot_error_detail(struct eeh_pe *pe, int severity)
static inline unsigned long eeh_token_to_phys(unsigned long token)
{
pte_t *ptep;
- unsigned long pa;
int hugepage_shift;
/*
@@ -336,9 +335,8 @@ static inline unsigned long eeh_token_to_phys(unsigned long
token)
if (!ptep)
return token;
WARN_ON(hugepage_shift);
- pa = pte_pfn(*ptep) << PAGE_SHIFT;
- return pa | (token & (PAGE_SIZE-1));
+ return PFN_PHYS(pte_pfn(*ptep)) | (token & (PAGE_SIZE - 1));
}
/*
diff --git a/arch/powerpc/kernel/io-workarounds.c
b/arch/powerpc/kernel/io-workarounds.c
index 24b968f..dd9a4a2 100644
--- a/arch/powerpc/kernel/io-workarounds.c
+++ b/arch/powerpc/kernel/io-workarounds.c
@@ -81,7 +81,7 @@ struct iowa_bus
