Re: [PATCH v3 5/8] mm: HUGE_VMAP arch support cleanup
Hi Nicholas,
I love your patch! Yet something to improve:
[auto build test ERROR on hnaz-linux-mm/master]
[also build test ERROR on arm64/for-next/core powerpc/next tip/x86/mm
linus/master v5.8 next-20200807]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Nicholas-Piggin/huge-vmalloc-mappings/20200810-103105
base: https://github.com/hnaz/linux-mm master
config: i386-randconfig-s002-20200810 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-118-ge1578773-dirty
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
>> arch/x86/mm/ioremap.c:484:6: error: redefinition of 'arch_vmap_p4d_supported'
484 | bool arch_vmap_p4d_supported(pgprot_t prot)
| ^~~
In file included from include/asm-generic/io.h:911,
from arch/x86/include/asm/io.h:375,
from arch/x86/include/asm/dma.h:13,
from include/linux/memblock.h:14,
from arch/x86/mm/ioremap.c:10:
include/linux/vmalloc.h:92:20: note: previous definition of
'arch_vmap_p4d_supported' was here
92 | static inline bool arch_vmap_p4d_supported(pgprot_t prot) { return
false; }
|^~~
>> arch/x86/mm/ioremap.c:489:6: error: redefinition of 'arch_vmap_pud_supported'
489 | bool arch_vmap_pud_supported(pgprot_t prot)
| ^~~
In file included from include/asm-generic/io.h:911,
from arch/x86/include/asm/io.h:375,
from arch/x86/include/asm/dma.h:13,
from include/linux/memblock.h:14,
from arch/x86/mm/ioremap.c:10:
include/linux/vmalloc.h:93:20: note: previous definition of
'arch_vmap_pud_supported' was here
93 | static inline bool arch_vmap_pud_supported(pgprot_t prot) { return
false; }
|^~~
>> arch/x86/mm/ioremap.c:498:6: error: redefinition of 'arch_vmap_pmd_supported'
498 | bool arch_vmap_pmd_supported(pgprot_t prot)
| ^~~
In file included from include/asm-generic/io.h:911,
from arch/x86/include/asm/io.h:375,
from arch/x86/include/asm/dma.h:13,
from include/linux/memblock.h:14,
from arch/x86/mm/ioremap.c:10:
include/linux/vmalloc.h:94:20: note: previous definition of
'arch_vmap_pmd_supported' was here
94 | static inline bool arch_vmap_pmd_supported(pgprot_t prot) { return
false; }
|^~~
arch/x86/mm/ioremap.c:737:17: warning: no previous prototype for
'early_memremap_pgprot_adjust' [-Wmissing-prototypes]
737 | pgprot_t __init early_memremap_pgprot_adjust(resource_size_t
phys_addr,
| ^~~~
vim +/arch_vmap_p4d_supported +484 arch/x86/mm/ioremap.c
483
> 484 bool arch_vmap_p4d_supported(pgprot_t prot)
485 {
486 return false;
487 }
488
> 489 bool arch_vmap_pud_supported(pgprot_t prot)
490 {
491 #ifdef CONFIG_X86_64
492 return boot_cpu_has(X86_FEATURE_GBPAGES);
493 #else
494 return false;
495 #endif
496 }
497
> 498 bool arch_vmap_pmd_supported(pgprot_t prot)
499 {
500 return boot_cpu_has(X86_FEATURE_PSE);
501 }
502
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
[PATCH v3 8/8] mm/vmalloc: Hugepage vmalloc mappings
On platforms that define HAVE_ARCH_HUGE_VMAP and support PMD vmaps,
vmalloc will attempt to allocate PMD-sized pages first, before falling
back to small pages.
Allocations which use something other than PAGE_KERNEL protections are
not permitted to use huge pages yet, not all callers expect this (e.g.,
module allocations vs strict module rwx).
This reduces TLB misses by nearly 30x on a `git diff` workload on a
2-node POWER9 (59,800 -> 2,100) and reduces CPU cycles by 0.54%.
This can result in more internal fragmentation and memory overhead for a
given allocation, an option nohugevmap is added to disable at boot.
Signed-off-by: Nicholas Piggin
---
.../admin-guide/kernel-parameters.txt | 2 +
include/linux/vmalloc.h | 1 +
mm/vmalloc.c | 174 +-
3 files changed, 135 insertions(+), 42 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index 98ea67f27809..eaef176c597f 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3190,6 +3190,8 @@
nohugeiomap [KNL,x86,PPC] Disable kernel huge I/O mappings.
+ nohugevmap [KNL,x86,PPC] Disable kernel huge vmalloc mappings.
+
nosmt [KNL,S390] Disable symmetric multithreading (SMT).
Equivalent to smt=1.
diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index e3590e93bfff..8f25dbaca0a1 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -58,6 +58,7 @@ struct vm_struct {
unsigned long size;
unsigned long flags;
struct page **pages;
+ unsigned intpage_order;
unsigned intnr_pages;
phys_addr_t phys_addr;
const void *caller;
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 4e5cb7c7f780..a7728c7086bc 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -45,6 +45,19 @@
#include "internal.h"
#include "pgalloc-track.h"
+#ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
+static bool __ro_after_init vmap_allow_huge = true;
+
+static int __init set_nohugevmap(char *str)
+{
+ vmap_allow_huge = false;
+ return 0;
+}
+early_param("nohugevmap", set_nohugevmap);
+#else /* CONFIG_HAVE_ARCH_HUGE_VMAP */
+static const bool vmap_allow_huge = false;
+#endif /* CONFIG_HAVE_ARCH_HUGE_VMAP */
+
bool is_vmalloc_addr(const void *x)
{
unsigned long addr = (unsigned long)x;
@@ -468,31 +481,12 @@ static int vmap_pages_p4d_range(pgd_t *pgd, unsigned long
addr, unsigned long en
return 0;
}
-/**
- * map_kernel_range_noflush - map kernel VM area with the specified pages
- * @addr: start of the VM area to map
- * @size: size of the VM area to map
- * @prot: page protection flags to use
- * @pages: pages to map
- *
- * Map PFN_UP(@size) pages at @addr. The VM area @addr and @size specify
should
- * have been allocated using get_vm_area() and its friends.
- *
- * NOTE:
- * This function does NOT do any cache flushing. The caller is responsible for
- * calling flush_cache_vmap() on to-be-mapped areas before calling this
- * function.
- *
- * RETURNS:
- * 0 on success, -errno on failure.
- */
-int map_kernel_range_noflush(unsigned long addr, unsigned long size,
-pgprot_t prot, struct page **pages)
+static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long
end,
+ pgprot_t prot, struct page **pages)
{
unsigned long start = addr;
- unsigned long end = addr + size;
- unsigned long next;
pgd_t *pgd;
+ unsigned long next;
int err = 0;
int nr = 0;
pgtbl_mod_mask mask = 0;
@@ -514,6 +508,65 @@ int map_kernel_range_noflush(unsigned long addr, unsigned
long size,
return 0;
}
+static int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
+ pgprot_t prot, struct page **pages, unsigned int page_shift)
+{
+ WARN_ON(page_shift < PAGE_SHIFT);
+
+ if (page_shift == PAGE_SHIFT) {
+ return vmap_small_pages_range_noflush(addr, end, prot, pages);
+ } else {
+ unsigned int i, nr = (end - addr) >> page_shift;
+
+ for (i = 0; i < nr; i++) {
+ int err;
+
+ err = vmap_range_noflush(addr, addr + (1UL <<
page_shift),
+ __pa(page_address(pages[i])),
prot, page_shift);
+ if (err)
+ return err;
+
+ addr += 1UL << page_shift;
+ }
+
+ return 0;
+ }
+}
+
+static int vmap_pages_range(unsigned long addr, unsigned long end,
+ pgprot_t prot, struct page **pages, unsigned int page_shift)
+{
+ int err;
+
+ err =
[PATCH v3 7/8] mm/vmalloc: add vmap_range_noflush variant
As a side-effect, the order of flush_cache_vmap() and
arch_sync_kernel_mappings() calls are switched, but that now matches
the other callers in this file.
Signed-off-by: Nicholas Piggin
---
mm/vmalloc.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 129f10545bb1..4e5cb7c7f780 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -234,8 +234,8 @@ static int vmap_p4d_range(pgd_t *pgd, unsigned long addr,
unsigned long end,
return 0;
}
-int vmap_range(unsigned long addr, unsigned long end, phys_addr_t phys_addr,
pgprot_t prot,
- unsigned int max_page_shift)
+static int vmap_range_noflush(unsigned long addr, unsigned long end,
phys_addr_t phys_addr,
+ pgprot_t prot, unsigned int max_page_shift)
{
pgd_t *pgd;
unsigned long start;
@@ -255,14 +255,23 @@ int vmap_range(unsigned long addr, unsigned long end,
phys_addr_t phys_addr, pgp
break;
} while (pgd++, phys_addr += (next - addr), addr = next, addr != end);
- flush_cache_vmap(start, end);
-
if (mask & ARCH_PAGE_TABLE_SYNC_MASK)
arch_sync_kernel_mappings(start, end);
return err;
}
+int vmap_range(unsigned long addr, unsigned long end, phys_addr_t phys_addr,
pgprot_t prot,
+ unsigned int max_page_shift)
+{
+ int err;
+
+ err = vmap_range_noflush(addr, end, phys_addr, prot, max_page_shift);
+ flush_cache_vmap(addr, end);
+
+ return err;
+}
+
static void vunmap_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
pgtbl_mod_mask *mask)
{
--
2.23.0
[PATCH v3 6/8] mm: Move vmap_range from lib/ioremap.c to mm/vmalloc.c
This is a generic kernel virtual memory mapper, not specific to ioremap.
Signed-off-by: Nicholas Piggin
---
include/linux/vmalloc.h | 2 +
mm/ioremap.c| 192
mm/vmalloc.c| 191 +++
3 files changed, 193 insertions(+), 192 deletions(-)
diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index 787d77ad7536..e3590e93bfff 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -181,6 +181,8 @@ extern struct vm_struct *remove_vm_area(const void *addr);
extern struct vm_struct *find_vm_area(const void *addr);
#ifdef CONFIG_MMU
+extern int vmap_range(unsigned long addr, unsigned long end, phys_addr_t
phys_addr, pgprot_t prot,
+ unsigned int max_page_shift);
extern int map_kernel_range_noflush(unsigned long start, unsigned long size,
pgprot_t prot, struct page **pages);
int map_kernel_range(unsigned long start, unsigned long size, pgprot_t prot,
diff --git a/mm/ioremap.c b/mm/ioremap.c
index b0032dbadaf7..cdda0e022740 100644
--- a/mm/ioremap.c
+++ b/mm/ioremap.c
@@ -28,198 +28,6 @@ early_param("nohugeiomap", set_nohugeiomap);
static const bool iomap_allow_huge = false;
#endif /* CONFIG_HAVE_ARCH_HUGE_VMAP */
-static int vmap_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
- phys_addr_t phys_addr, pgprot_t prot, pgtbl_mod_mask
*mask)
-{
- pte_t *pte;
- u64 pfn;
-
- pfn = phys_addr >> PAGE_SHIFT;
- pte = pte_alloc_kernel_track(pmd, addr, mask);
- if (!pte)
- return -ENOMEM;
- do {
- BUG_ON(!pte_none(*pte));
- set_pte_at(_mm, addr, pte, pfn_pte(pfn, prot));
- pfn++;
- } while (pte++, addr += PAGE_SIZE, addr != end);
- *mask |= PGTBL_PTE_MODIFIED;
- return 0;
-}
-
-static int vmap_try_huge_pmd(pmd_t *pmd, unsigned long addr, unsigned long end,
- phys_addr_t phys_addr, pgprot_t prot, unsigned int
max_page_shift)
-{
- if (max_page_shift < PMD_SHIFT)
- return 0;
-
- if (!arch_vmap_pmd_supported(prot))
- return 0;
-
- if ((end - addr) != PMD_SIZE)
- return 0;
-
- if (!IS_ALIGNED(addr, PMD_SIZE))
- return 0;
-
- if (!IS_ALIGNED(phys_addr, PMD_SIZE))
- return 0;
-
- if (pmd_present(*pmd) && !pmd_free_pte_page(pmd, addr))
- return 0;
-
- return pmd_set_huge(pmd, phys_addr, prot);
-}
-
-static int vmap_pmd_range(pud_t *pud, unsigned long addr, unsigned long end,
- phys_addr_t phys_addr, pgprot_t prot, unsigned int
max_page_shift,
- pgtbl_mod_mask *mask)
-{
- pmd_t *pmd;
- unsigned long next;
-
- pmd = pmd_alloc_track(_mm, pud, addr, mask);
- if (!pmd)
- return -ENOMEM;
- do {
- next = pmd_addr_end(addr, end);
-
- if (vmap_try_huge_pmd(pmd, addr, next, phys_addr, prot,
max_page_shift)) {
- *mask |= PGTBL_PMD_MODIFIED;
- continue;
- }
-
- if (vmap_pte_range(pmd, addr, next, phys_addr, prot, mask))
- return -ENOMEM;
- } while (pmd++, phys_addr += (next - addr), addr = next, addr != end);
- return 0;
-}
-
-static int vmap_try_huge_pud(pud_t *pud, unsigned long addr, unsigned long end,
- phys_addr_t phys_addr, pgprot_t prot, unsigned int
max_page_shift)
-{
- if (max_page_shift < PUD_SHIFT)
- return 0;
-
- if (!arch_vmap_pud_supported(prot))
- return 0;
-
- if ((end - addr) != PUD_SIZE)
- return 0;
-
- if (!IS_ALIGNED(addr, PUD_SIZE))
- return 0;
-
- if (!IS_ALIGNED(phys_addr, PUD_SIZE))
- return 0;
-
- if (pud_present(*pud) && !pud_free_pmd_page(pud, addr))
- return 0;
-
- return pud_set_huge(pud, phys_addr, prot);
-}
-
-static int vmap_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end,
- phys_addr_t phys_addr, pgprot_t prot, unsigned int
max_page_shift,
- pgtbl_mod_mask *mask)
-{
- pud_t *pud;
- unsigned long next;
-
- pud = pud_alloc_track(_mm, p4d, addr, mask);
- if (!pud)
- return -ENOMEM;
- do {
- next = pud_addr_end(addr, end);
-
- if (vmap_try_huge_pud(pud, addr, next, phys_addr, prot,
max_page_shift)) {
- *mask |= PGTBL_PUD_MODIFIED;
- continue;
- }
-
- if (vmap_pmd_range(pud, addr, next, phys_addr, prot,
max_page_shift, mask))
- return -ENOMEM;
- } while (pud++, phys_addr += (next - addr), addr = next, addr != end);
- return 0;
-}
-
[PATCH v3 5/8] mm: HUGE_VMAP arch support cleanup
This changes the awkward approach where architectures provide init
functions to determine which levels they can provide large mappings for,
to one where the arch is queried for each call.
This removes code and indirection, and allows constant-folding of dead
code for unsupported levels.
This also adds a prot argument to the arch query. This is unused
currently but could help with some architectures (e.g., some powerpc
processors can't map uncacheable memory with large pages).
Signed-off-by: Nicholas Piggin
---
arch/arm64/mm/mmu.c | 10 +--
arch/powerpc/mm/book3s64/radix_pgtable.c | 8 +-
arch/x86/mm/ioremap.c| 10 +--
include/linux/io.h | 9 ---
include/linux/vmalloc.h | 10 +++
init/main.c | 1 -
mm/ioremap.c | 96 +++-
7 files changed, 67 insertions(+), 77 deletions(-)
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 75df62fea1b6..184a6e917417 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1304,12 +1304,12 @@ void *__init fixmap_remap_fdt(phys_addr_t dt_phys, int
*size, pgprot_t prot)
return dt_virt;
}
-int __init arch_ioremap_p4d_supported(void)
+bool arch_vmap_p4d_supported(pgprot_t prot)
{
- return 0;
+ return false;
}
-int __init arch_ioremap_pud_supported(void)
+bool arch_vmap_pud_supported(pgprot_t prot)
{
/*
* Only 4k granule supports level 1 block mappings.
@@ -1319,9 +1319,9 @@ int __init arch_ioremap_pud_supported(void)
!IS_ENABLED(CONFIG_PTDUMP_DEBUGFS);
}
-int __init arch_ioremap_pmd_supported(void)
+bool arch_vmap_pmd_supported(pgprot_t prot)
{
- /* See arch_ioremap_pud_supported() */
+ /* See arch_vmap_pud_supported() */
return !IS_ENABLED(CONFIG_PTDUMP_DEBUGFS);
}
diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c
b/arch/powerpc/mm/book3s64/radix_pgtable.c
index 28c784976bed..eca83a50bf2e 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -1134,13 +1134,13 @@ void radix__ptep_modify_prot_commit(struct
vm_area_struct *vma,
set_pte_at(mm, addr, ptep, pte);
}
-int __init arch_ioremap_pud_supported(void)
+bool arch_vmap_pud_supported(pgprot_t prot)
{
/* HPT does not cope with large pages in the vmalloc area */
return radix_enabled();
}
-int __init arch_ioremap_pmd_supported(void)
+bool arch_vmap_pmd_supported(pgprot_t prot)
{
return radix_enabled();
}
@@ -1234,7 +1234,7 @@ int pmd_free_pte_page(pmd_t *pmd, unsigned long addr)
return 1;
}
-int __init arch_ioremap_p4d_supported(void)
+bool arch_vmap_p4d_supported(pgprot_t prot)
{
- return 0;
+ return false;
}
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 84d85dbd1dad..159bfca757b9 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -481,21 +481,21 @@ void iounmap(volatile void __iomem *addr)
}
EXPORT_SYMBOL(iounmap);
-int __init arch_ioremap_p4d_supported(void)
+bool arch_vmap_p4d_supported(pgprot_t prot)
{
- return 0;
+ return false;
}
-int __init arch_ioremap_pud_supported(void)
+bool arch_vmap_pud_supported(pgprot_t prot)
{
#ifdef CONFIG_X86_64
return boot_cpu_has(X86_FEATURE_GBPAGES);
#else
- return 0;
+ return false;
#endif
}
-int __init arch_ioremap_pmd_supported(void)
+bool arch_vmap_pmd_supported(pgprot_t prot)
{
return boot_cpu_has(X86_FEATURE_PSE);
}
diff --git a/include/linux/io.h b/include/linux/io.h
index 8394c56babc2..f1effd4d7a3c 100644
--- a/include/linux/io.h
+++ b/include/linux/io.h
@@ -31,15 +31,6 @@ static inline int ioremap_page_range(unsigned long addr,
unsigned long end,
}
#endif
-#ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
-void __init ioremap_huge_init(void);
-int arch_ioremap_p4d_supported(void);
-int arch_ioremap_pud_supported(void);
-int arch_ioremap_pmd_supported(void);
-#else
-static inline void ioremap_huge_init(void) { }
-#endif
-
/*
* Managed iomap interface
*/
diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index 0221f852a7e1..787d77ad7536 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -84,6 +84,16 @@ struct vmap_area {
};
};
+#ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
+bool arch_vmap_p4d_supported(pgprot_t prot);
+bool arch_vmap_pud_supported(pgprot_t prot);
+bool arch_vmap_pmd_supported(pgprot_t prot);
+#else
+static inline bool arch_vmap_p4d_supported(pgprot_t prot) { return false; }
+static inline bool arch_vmap_pud_supported(pgprot_t prot) { return false; }
+static inline bool arch_vmap_pmd_supported(pgprot_t prot) { return false; }
+#endif
+
/*
* Highlevel APIs for driver use
*/
diff --git a/init/main.c b/init/main.c
index ae78fb68d231..1c89aa127b8f 100644
--- a/init/main.c
+++ b/init/main.c
@@ -820,7 +820,6 @@ static void __init mm_init(void)
pgtable_init();
[PATCH v3 4/8] lib/ioremap: rename ioremap_*_range to vmap_*_range
This will be moved to mm/ and used as a generic kernel virtual mapping
function, so re-name it in preparation.
Signed-off-by: Nicholas Piggin
---
mm/ioremap.c | 55 ++--
1 file changed, 23 insertions(+), 32 deletions(-)
diff --git a/mm/ioremap.c b/mm/ioremap.c
index 5fa1ab41d152..6016ae3227ad 100644
--- a/mm/ioremap.c
+++ b/mm/ioremap.c
@@ -61,9 +61,8 @@ static inline int ioremap_pud_enabled(void) { return 0; }
static inline int ioremap_pmd_enabled(void) { return 0; }
#endif /* CONFIG_HAVE_ARCH_HUGE_VMAP */
-static int ioremap_pte_range(pmd_t *pmd, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr, pgprot_t prot,
- pgtbl_mod_mask *mask)
+static int vmap_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
+ phys_addr_t phys_addr, pgprot_t prot, pgtbl_mod_mask
*mask)
{
pte_t *pte;
u64 pfn;
@@ -81,9 +80,8 @@ static int ioremap_pte_range(pmd_t *pmd, unsigned long addr,
return 0;
}
-static int ioremap_try_huge_pmd(pmd_t *pmd, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr,
- pgprot_t prot)
+static int vmap_try_huge_pmd(pmd_t *pmd, unsigned long addr, unsigned long end,
+ phys_addr_t phys_addr, pgprot_t prot)
{
if (!ioremap_pmd_enabled())
return 0;
@@ -103,9 +101,8 @@ static int ioremap_try_huge_pmd(pmd_t *pmd, unsigned long
addr,
return pmd_set_huge(pmd, phys_addr, prot);
}
-static inline int ioremap_pmd_range(pud_t *pud, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr, pgprot_t prot,
- pgtbl_mod_mask *mask)
+static int vmap_pmd_range(pud_t *pud, unsigned long addr, unsigned long end,
+ phys_addr_t phys_addr, pgprot_t prot, pgtbl_mod_mask
*mask)
{
pmd_t *pmd;
unsigned long next;
@@ -116,20 +113,19 @@ static inline int ioremap_pmd_range(pud_t *pud, unsigned
long addr,
do {
next = pmd_addr_end(addr, end);
- if (ioremap_try_huge_pmd(pmd, addr, next, phys_addr, prot)) {
+ if (vmap_try_huge_pmd(pmd, addr, next, phys_addr, prot)) {
*mask |= PGTBL_PMD_MODIFIED;
continue;
}
- if (ioremap_pte_range(pmd, addr, next, phys_addr, prot, mask))
+ if (vmap_pte_range(pmd, addr, next, phys_addr, prot, mask))
return -ENOMEM;
} while (pmd++, phys_addr += (next - addr), addr = next, addr != end);
return 0;
}
-static int ioremap_try_huge_pud(pud_t *pud, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr,
- pgprot_t prot)
+static int vmap_try_huge_pud(pud_t *pud, unsigned long addr, unsigned long end,
+ phys_addr_t phys_addr, pgprot_t prot)
{
if (!ioremap_pud_enabled())
return 0;
@@ -149,9 +145,8 @@ static int ioremap_try_huge_pud(pud_t *pud, unsigned long
addr,
return pud_set_huge(pud, phys_addr, prot);
}
-static inline int ioremap_pud_range(p4d_t *p4d, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr, pgprot_t prot,
- pgtbl_mod_mask *mask)
+static int vmap_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end,
+ phys_addr_t phys_addr, pgprot_t prot, pgtbl_mod_mask
*mask)
{
pud_t *pud;
unsigned long next;
@@ -162,20 +157,19 @@ static inline int ioremap_pud_range(p4d_t *p4d, unsigned
long addr,
do {
next = pud_addr_end(addr, end);
- if (ioremap_try_huge_pud(pud, addr, next, phys_addr, prot)) {
+ if (vmap_try_huge_pud(pud, addr, next, phys_addr, prot)) {
*mask |= PGTBL_PUD_MODIFIED;
continue;
}
- if (ioremap_pmd_range(pud, addr, next, phys_addr, prot, mask))
+ if (vmap_pmd_range(pud, addr, next, phys_addr, prot, mask))
return -ENOMEM;
} while (pud++, phys_addr += (next - addr), addr = next, addr != end);
return 0;
}
-static int ioremap_try_huge_p4d(p4d_t *p4d, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr,
- pgprot_t prot)
+static int vmap_try_huge_p4d(p4d_t *p4d, unsigned long addr, unsigned long end,
+ phys_addr_t phys_addr, pgprot_t prot)
{
if (!ioremap_p4d_enabled())
return 0;
@@ -195,9 +189,8 @@ static int ioremap_try_huge_p4d(p4d_t *p4d, unsigned long
addr,
return p4d_set_huge(p4d, phys_addr, prot);
}
-static inline int ioremap_p4d_range(pgd_t *pgd, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr, pgprot_t
[PATCH v3 3/8] mm/vmalloc: rename vmap_*_range vmap_pages_*_range
The vmalloc mapper operates on a struct page * array rather than a
linear physical address, re-name it to make this distinction clear.
Signed-off-by: Nicholas Piggin
---
mm/vmalloc.c | 28
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 49f225b0f855..3a1e45fd1626 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -190,9 +190,8 @@ void unmap_kernel_range_noflush(unsigned long start,
unsigned long size)
arch_sync_kernel_mappings(start, end);
}
-static int vmap_pte_range(pmd_t *pmd, unsigned long addr,
- unsigned long end, pgprot_t prot, struct page **pages, int *nr,
- pgtbl_mod_mask *mask)
+static int vmap_pages_pte_range(pmd_t *pmd, unsigned long addr, unsigned long
end,
+ pgprot_t prot, struct page **pages, int *nr, pgtbl_mod_mask
*mask)
{
pte_t *pte;
@@ -218,9 +217,8 @@ static int vmap_pte_range(pmd_t *pmd, unsigned long addr,
return 0;
}
-static int vmap_pmd_range(pud_t *pud, unsigned long addr,
- unsigned long end, pgprot_t prot, struct page **pages, int *nr,
- pgtbl_mod_mask *mask)
+static int vmap_pages_pmd_range(pud_t *pud, unsigned long addr, unsigned long
end,
+ pgprot_t prot, struct page **pages, int *nr, pgtbl_mod_mask
*mask)
{
pmd_t *pmd;
unsigned long next;
@@ -230,15 +228,14 @@ static int vmap_pmd_range(pud_t *pud, unsigned long addr,
return -ENOMEM;
do {
next = pmd_addr_end(addr, end);
- if (vmap_pte_range(pmd, addr, next, prot, pages, nr, mask))
+ if (vmap_pages_pte_range(pmd, addr, next, prot, pages, nr,
mask))
return -ENOMEM;
} while (pmd++, addr = next, addr != end);
return 0;
}
-static int vmap_pud_range(p4d_t *p4d, unsigned long addr,
- unsigned long end, pgprot_t prot, struct page **pages, int *nr,
- pgtbl_mod_mask *mask)
+static int vmap_pages_pud_range(p4d_t *p4d, unsigned long addr, unsigned long
end,
+ pgprot_t prot, struct page **pages, int *nr, pgtbl_mod_mask
*mask)
{
pud_t *pud;
unsigned long next;
@@ -248,15 +245,14 @@ static int vmap_pud_range(p4d_t *p4d, unsigned long addr,
return -ENOMEM;
do {
next = pud_addr_end(addr, end);
- if (vmap_pmd_range(pud, addr, next, prot, pages, nr, mask))
+ if (vmap_pages_pmd_range(pud, addr, next, prot, pages, nr,
mask))
return -ENOMEM;
} while (pud++, addr = next, addr != end);
return 0;
}
-static int vmap_p4d_range(pgd_t *pgd, unsigned long addr,
- unsigned long end, pgprot_t prot, struct page **pages, int *nr,
- pgtbl_mod_mask *mask)
+static int vmap_pages_p4d_range(pgd_t *pgd, unsigned long addr, unsigned long
end,
+ pgprot_t prot, struct page **pages, int *nr, pgtbl_mod_mask
*mask)
{
p4d_t *p4d;
unsigned long next;
@@ -266,7 +262,7 @@ static int vmap_p4d_range(pgd_t *pgd, unsigned long addr,
return -ENOMEM;
do {
next = p4d_addr_end(addr, end);
- if (vmap_pud_range(p4d, addr, next, prot, pages, nr, mask))
+ if (vmap_pages_pud_range(p4d, addr, next, prot, pages, nr,
mask))
return -ENOMEM;
} while (p4d++, addr = next, addr != end);
return 0;
@@ -307,7 +303,7 @@ int map_kernel_range_noflush(unsigned long addr, unsigned
long size,
next = pgd_addr_end(addr, end);
if (pgd_bad(*pgd))
mask |= PGTBL_PGD_MODIFIED;
- err = vmap_p4d_range(pgd, addr, next, prot, pages, , );
+ err = vmap_pages_p4d_range(pgd, addr, next, prot, pages, ,
);
if (err)
return err;
} while (pgd++, addr = next, addr != end);
--
2.23.0
[PATCH v3 2/8] mm: apply_to_pte_range warn and fail if a large pte is encountered
Signed-off-by: Nicholas Piggin
---
mm/memory.c | 60 +++--
1 file changed, 44 insertions(+), 16 deletions(-)
diff --git a/mm/memory.c b/mm/memory.c
index c39a13b09602..1d5f3093c249 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2260,13 +2260,20 @@ static int apply_to_pmd_range(struct mm_struct *mm,
pud_t *pud,
}
do {
next = pmd_addr_end(addr, end);
- if (create || !pmd_none_or_clear_bad(pmd)) {
- err = apply_to_pte_range(mm, pmd, addr, next, fn, data,
-create);
- if (err)
- break;
+ if (pmd_none(*pmd) && !create)
+ continue;
+ if (WARN_ON_ONCE(pmd_leaf(*pmd)))
+ return -EINVAL;
+ if (WARN_ON_ONCE(pmd_bad(*pmd))) {
+ if (!create)
+ continue;
+ pmd_clear_bad(pmd);
}
+ err = apply_to_pte_range(mm, pmd, addr, next, fn, data, create);
+ if (err)
+ break;
} while (pmd++, addr = next, addr != end);
+
return err;
}
@@ -2287,13 +2294,20 @@ static int apply_to_pud_range(struct mm_struct *mm,
p4d_t *p4d,
}
do {
next = pud_addr_end(addr, end);
- if (create || !pud_none_or_clear_bad(pud)) {
- err = apply_to_pmd_range(mm, pud, addr, next, fn, data,
-create);
- if (err)
- break;
+ if (pud_none(*pud) && !create)
+ continue;
+ if (WARN_ON_ONCE(pud_leaf(*pud)))
+ return -EINVAL;
+ if (WARN_ON_ONCE(pud_bad(*pud))) {
+ if (!create)
+ continue;
+ pud_clear_bad(pud);
}
+ err = apply_to_pmd_range(mm, pud, addr, next, fn, data, create);
+ if (err)
+ break;
} while (pud++, addr = next, addr != end);
+
return err;
}
@@ -2314,13 +2328,20 @@ static int apply_to_p4d_range(struct mm_struct *mm,
pgd_t *pgd,
}
do {
next = p4d_addr_end(addr, end);
- if (create || !p4d_none_or_clear_bad(p4d)) {
- err = apply_to_pud_range(mm, p4d, addr, next, fn, data,
-create);
- if (err)
- break;
+ if (p4d_none(*p4d) && !create)
+ continue;
+ if (WARN_ON_ONCE(p4d_leaf(*p4d)))
+ return -EINVAL;
+ if (WARN_ON_ONCE(p4d_bad(*p4d))) {
+ if (!create)
+ continue;
+ p4d_clear_bad(p4d);
}
+ err = apply_to_pud_range(mm, p4d, addr, next, fn, data, create);
+ if (err)
+ break;
} while (p4d++, addr = next, addr != end);
+
return err;
}
@@ -2339,8 +2360,15 @@ static int __apply_to_page_range(struct mm_struct *mm,
unsigned long addr,
pgd = pgd_offset(mm, addr);
do {
next = pgd_addr_end(addr, end);
- if (!create && pgd_none_or_clear_bad(pgd))
+ if (pgd_none(*pgd) && !create)
continue;
+ if (WARN_ON_ONCE(pgd_leaf(*pgd)))
+ return -EINVAL;
+ if (WARN_ON_ONCE(pgd_bad(*pgd))) {
+ if (!create)
+ continue;
+ pgd_clear_bad(pgd);
+ }
err = apply_to_p4d_range(mm, pgd, addr, next, fn, data, create);
if (err)
break;
--
2.23.0
[PATCH v3 1/8] mm/vmalloc: fix vmalloc_to_page for huge vmap mappings
vmalloc_to_page returns NULL for addresses mapped by larger pages[*].
Whether or not a vmap is huge depends on the architecture details,
alignments, boot options, etc., which the caller can not be expected
to know. Therefore HUGE_VMAP is a regression for vmalloc_to_page.
This change teaches vmalloc_to_page about larger pages, and returns
the struct page that corresponds to the offset within the large page.
This makes the API agnostic to mapping implementation details.
[*] As explained by commit 029c54b095995 ("mm/vmalloc.c: huge-vmap:
fail gracefully on unexpected huge vmap mappings")
Signed-off-by: Nicholas Piggin
---
mm/vmalloc.c | 40 ++--
1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index b482d240f9a2..49f225b0f855 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -38,6 +38,7 @@
#include
#include
+#include
#include
#include
@@ -343,7 +344,9 @@ int is_vmalloc_or_module_addr(const void *x)
}
/*
- * Walk a vmap address to the struct page it maps.
+ * Walk a vmap address to the struct page it maps. Huge vmap mappings will
+ * return the tail page that corresponds to the base page address, which
+ * matches small vmap mappings.
*/
struct page *vmalloc_to_page(const void *vmalloc_addr)
{
@@ -363,25 +366,33 @@ struct page *vmalloc_to_page(const void *vmalloc_addr)
if (pgd_none(*pgd))
return NULL;
+ if (WARN_ON_ONCE(pgd_leaf(*pgd)))
+ return NULL; /* XXX: no allowance for huge pgd */
+ if (WARN_ON_ONCE(pgd_bad(*pgd)))
+ return NULL;
+
p4d = p4d_offset(pgd, addr);
if (p4d_none(*p4d))
return NULL;
- pud = pud_offset(p4d, addr);
+ if (p4d_leaf(*p4d))
+ return p4d_page(*p4d) + ((addr & ~P4D_MASK) >> PAGE_SHIFT);
+ if (WARN_ON_ONCE(p4d_bad(*p4d)))
+ return NULL;
- /*
-* Don't dereference bad PUD or PMD (below) entries. This will also
-* identify huge mappings, which we may encounter on architectures
-* that define CONFIG_HAVE_ARCH_HUGE_VMAP=y. Such regions will be
-* identified as vmalloc addresses by is_vmalloc_addr(), but are
-* not [unambiguously] associated with a struct page, so there is
-* no correct value to return for them.
-*/
- WARN_ON_ONCE(pud_bad(*pud));
- if (pud_none(*pud) || pud_bad(*pud))
+ pud = pud_offset(p4d, addr);
+ if (pud_none(*pud))
+ return NULL;
+ if (pud_leaf(*pud))
+ return pud_page(*pud) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
+ if (WARN_ON_ONCE(pud_bad(*pud)))
return NULL;
+
pmd = pmd_offset(pud, addr);
- WARN_ON_ONCE(pmd_bad(*pmd));
- if (pmd_none(*pmd) || pmd_bad(*pmd))
+ if (pmd_none(*pmd))
+ return NULL;
+ if (pmd_leaf(*pmd))
+ return pmd_page(*pmd) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
+ if (WARN_ON_ONCE(pmd_bad(*pmd)))
return NULL;
ptep = pte_offset_map(pmd, addr);
@@ -389,6 +400,7 @@ struct page *vmalloc_to_page(const void *vmalloc_addr)
if (pte_present(pte))
page = pte_page(pte);
pte_unmap(ptep);
+
return page;
}
EXPORT_SYMBOL(vmalloc_to_page);
--
2.23.0
[PATCH v3 0/8] huge vmalloc mappings
Not tested on x86 or arm64, would appreciate a quick test there so I can ask Andrew to put it in -mm. Other option is I can disable huge vmallocs for them for the time being. Since v2: - Rebased on vmalloc cleanups, split series into simpler pieces. - Fixed several compile errors and warnings - Keep the page array and accounting in small page units because struct vm_struct is an interface (this should fix x86 vmap stack debug assert). [Thanks Zefan] Nicholas Piggin (8): mm/vmalloc: fix vmalloc_to_page for huge vmap mappings mm: apply_to_pte_range warn and fail if a large pte is encountered mm/vmalloc: rename vmap_*_range vmap_pages_*_range lib/ioremap: rename ioremap_*_range to vmap_*_range mm: HUGE_VMAP arch support cleanup mm: Move vmap_range from lib/ioremap.c to mm/vmalloc.c mm/vmalloc: add vmap_range_noflush variant mm/vmalloc: Hugepage vmalloc mappings .../admin-guide/kernel-parameters.txt | 2 + arch/arm64/mm/mmu.c | 10 +- arch/powerpc/mm/book3s64/radix_pgtable.c | 8 +- arch/x86/mm/ioremap.c | 10 +- include/linux/io.h| 9 - include/linux/vmalloc.h | 13 + init/main.c | 1 - mm/ioremap.c | 231 + mm/memory.c | 60 ++- mm/vmalloc.c | 442 +++--- 10 files changed, 453 insertions(+), 333 deletions(-) -- 2.23.0
Re: [PATCH] powerpc/legacy_serial: Use early_ioremap()
On 24/03/20 10:54 am, Chris Packham wrote:
> Hi Christophe,
>
> On Wed, 2020-02-05 at 12:03 +, Christophe Leroy wrote:
>> [0.00] ioremap() called early from
>> find_legacy_serial_ports+0x3cc/0x474. Use early_ioremap() instead
>>
> I was just about to dig into this error message and found you patch. I
> applied it to a v5.5 base.
>
>> find_legacy_serial_ports() is called early from setup_arch(), before
>> paging_init(). vmalloc is not available yet, ioremap shouldn't be
>> used that early.
>>
>> Use early_ioremap() and switch to a regular ioremap() later.
>>
>> Signed-off-by: Christophe Leroy
> On my system (Freescale T2080 SOC) this seems to cause a crash/hang in
> early boot. Unfortunately because this is affecting the boot console I
> don't get any earlyprintk output.
I've been doing a bit more digging into why Christophe's patch didn't
work for me. I noticed the powerpc specific early_ioremap_range()
returns addresses around ioremap_bot. Yet the generic early_ioremap()
uses addresses around FIXADDR_TOP. If I try the following hack I can
make Christophe's patch work
diff --git a/arch/powerpc/include/asm/fixmap.h
b/arch/powerpc/include/asm/fixmap.h
index 2ef155a3c821..7bc2f3f73c8b 100644
--- a/arch/powerpc/include/asm/fixmap.h
+++ b/arch/powerpc/include/asm/fixmap.h
@@ -27,7 +27,7 @@
#include
#define FIXADDR_TOP (KASAN_SHADOW_START - PAGE_SIZE)
#else
-#define FIXADDR_TOP ((unsigned long)(-PAGE_SIZE))
+#define FIXADDR_TOP (IOREMAP_END - PAGE_SIZE)
#endif
/*
I'll admit to being out of my depth. It seems that the generic
early_ioremap() is not quite correctly plumbed in for powerpc.
>> ---
>> arch/powerpc/kernel/legacy_serial.c | 33 +
>>
>> 1 file changed, 29 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/legacy_serial.c
>> b/arch/powerpc/kernel/legacy_serial.c
>> index f061e06e9f51..8b2c1a8553a0 100644
>> --- a/arch/powerpc/kernel/legacy_serial.c
>> +++ b/arch/powerpc/kernel/legacy_serial.c
>> @@ -15,6 +15,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #undef DEBUG
>>
>> @@ -34,6 +35,7 @@ static struct legacy_serial_info {
>> unsigned intclock;
>> int irq_check_parent;
>> phys_addr_t taddr;
>> +void __iomem*early_addr;
>> } legacy_serial_infos[MAX_LEGACY_SERIAL_PORTS];
>>
>> static const struct of_device_id legacy_serial_parents[] __initconst
>> = {
>> @@ -325,17 +327,16 @@ static void __init
>> setup_legacy_serial_console(int console)
>> {
>> struct legacy_serial_info *info =
>> _serial_infos[console];
>> struct plat_serial8250_port *port =
>> _serial_ports[console];
>> -void __iomem *addr;
>> unsigned int stride;
>>
>> stride = 1 << port->regshift;
>>
>> /* Check if a translated MMIO address has been found */
>> if (info->taddr) {
>> -addr = ioremap(info->taddr, 0x1000);
>> -if (addr == NULL)
>> +info->early_addr = early_ioremap(info->taddr, 0x1000);
>> +if (info->early_addr == NULL)
>> return;
>> -udbg_uart_init_mmio(addr, stride);
>> +udbg_uart_init_mmio(info->early_addr, stride);
>> } else {
>> /* Check if it's PIO and we support untranslated PIO */
>> if (port->iotype == UPIO_PORT && isa_io_special)
>> @@ -353,6 +354,30 @@ static void __init
>> setup_legacy_serial_console(int console)
>> udbg_uart_setup(info->speed, info->clock);
>> }
>>
>> +static int __init ioremap_legacy_serial_console(void)
>> +{
>> +struct legacy_serial_info *info =
>> _serial_infos[legacy_serial_console];
>> +struct plat_serial8250_port *port =
>> _serial_ports[legacy_serial_console];
>> +void __iomem *vaddr;
>> +
>> +if (legacy_serial_console < 0)
>> +return 0;
>> +
>> +if (!info->early_addr)
>> +return 0;
>> +
>> +vaddr = ioremap(info->taddr, 0x1000);
>> +if (WARN_ON(!vaddr))
>> +return -ENOMEM;
>> +
>> +udbg_uart_init_mmio(vaddr, 1 << port->regshift);
>> +early_iounmap(info->early_addr, 0x1000);
>> +info->early_addr = NULL;
>> +
>> +return 0;
>> +}
>> +early_initcall(ioremap_legacy_serial_console);
>> +
>> /*
>>* This is called very early, as part of setup_system() or
>> eventually
>>* setup_arch(), basically before anything else in this file. This
>> function
Re: [PATCH 2/9] macintosh/via-macii: Poll the device most likely to respond
Hi,
On 8/9/20 3:58 PM, Finn Thain wrote:
> On Sun, 9 Aug 2020, Guenter Roeck wrote:
>
>> Hi,
>>
>> On Sun, Jun 28, 2020 at 02:23:12PM +1000, Finn Thain wrote:
>>> Poll the most recently polled device by default, rather than the lowest
>>> device address that happens to be enabled in autopoll_devs. This improves
>>> input latency. Re-use macii_queue_poll() rather than duplicate that logic.
>>> This eliminates a static struct and function.
>>>
>>> Fixes: d95fd5fce88f0 ("m68k: Mac II ADB fixes") # v5.0+
>>> Tested-by: Stan Johnson
>>> Signed-off-by: Finn Thain
>>
>> With this patch applied, the qemu "q800" emulation no longer works and
>> is stuck in early boot. Any idea why that might be the case, and/or how
>> to debug it ?
>>
>
> The problem you're seeing was mentioned in the cover letter,
> https://lore.kernel.org/linux-m68k/cover.1593318192.git.fth...@telegraphics.com.au/
>
> Since this series was merged, Linus' tree is no longer compatible with
> long-standing QEMU bugs.
>
> The best way to fix this is to upgrade QEMU (latest is 5.1.0-rc3). Or use
> the serial console instead of the framebuffer console.
>
I have no problem with that. Actually, I had checked the qemu commit log,
but somehow I had missed missed the commits there.
> I regret the inconvenience but the alternative was worse: adding code to
> Linux to get compatibility with QEMU bugs (which were added to QEMU due to
> Linux bugs).
>
> My main concern is correct operation on actual hardware, as always. But
> some QEMU developers are working on support for operating systems besides
> Linux.
>
> Therefore, I believe that both QEMU and Linux should aim for compatibility
> with actual hardware and not bug compatibility with each other.
>
I absolutely agree.
I repeated the test on the mainline kernel with qemu v5.1-rc3, and it works.
I also made sure that older versions of Linux still work with the qemu
v5.1.0-rc3. So everything is good, and sorry for the noise.
Thanks,
Guenter
Re: [PATCH 1/9] macintosh/via-macii: Access autopoll_devs when inside lock
On Sun, 9 Aug 2020, Guenter Roeck wrote: > Hi, > > On Sun, Jun 28, 2020 at 02:23:12PM +1000, Finn Thain wrote: > > The interrupt handler should be excluded when accessing the > > autopoll_devs variable. > > > > I am quite baffled by this patch. Other than adding an unnecessary lock > / unlock sequence, The new lock/unlock sequence means that the expression (autopoll_devs && !current_req) can be understood to be atomic. That makes it easier for me to follow (being that both variables are shared state). > accessing a variable (which is derived from another variable) from > inside or outside a lock does not make a difference. If autopoll_devs = > devs & 0xfffe is 0 inside the lock, it will just as much be 0 outside > the lock, and vice versa. > > Can you explain this in some more detail ? Not that is matters much > since the change already made it into mainline, but I would like to > understand what if anything I am missing here. > I think the new code is more readable and is obviously free of race conditions. It's not obvious to me why the old code was free of race conditions but if you can easily establish that by inspection then you are a better auditor than I am. Regardless, I'll stick with "Keep It Simple, Stupid".
Re: [PATCH 2/9] macintosh/via-macii: Poll the device most likely to respond
On Sun, 9 Aug 2020, Guenter Roeck wrote:
> Hi,
>
> On Sun, Jun 28, 2020 at 02:23:12PM +1000, Finn Thain wrote:
> > Poll the most recently polled device by default, rather than the lowest
> > device address that happens to be enabled in autopoll_devs. This improves
> > input latency. Re-use macii_queue_poll() rather than duplicate that logic.
> > This eliminates a static struct and function.
> >
> > Fixes: d95fd5fce88f0 ("m68k: Mac II ADB fixes") # v5.0+
> > Tested-by: Stan Johnson
> > Signed-off-by: Finn Thain
>
> With this patch applied, the qemu "q800" emulation no longer works and
> is stuck in early boot. Any idea why that might be the case, and/or how
> to debug it ?
>
The problem you're seeing was mentioned in the cover letter,
https://lore.kernel.org/linux-m68k/cover.1593318192.git.fth...@telegraphics.com.au/
Since this series was merged, Linus' tree is no longer compatible with
long-standing QEMU bugs.
The best way to fix this is to upgrade QEMU (latest is 5.1.0-rc3). Or use
the serial console instead of the framebuffer console.
I regret the inconvenience but the alternative was worse: adding code to
Linux to get compatibility with QEMU bugs (which were added to QEMU due to
Linux bugs).
My main concern is correct operation on actual hardware, as always. But
some QEMU developers are working on support for operating systems besides
Linux.
Therefore, I believe that both QEMU and Linux should aim for compatibility
with actual hardware and not bug compatibility with each other.
Re: [PATCH 1/9] macintosh/via-macii: Access autopoll_devs when inside lock
Hi,
On Sun, Jun 28, 2020 at 02:23:12PM +1000, Finn Thain wrote:
> The interrupt handler should be excluded when accessing the autopoll_devs
> variable.
>
I am quite baffled by this patch. Other than adding an unnecessary lock /
unlock sequence, accessing a variable (which is derived from another
variable) from inside or outside a lock does not make a difference.
If autopoll_devs = devs & 0xfffe is 0 inside the lock, it will just
as much be 0 outside the lock, and vice versa.
Can you explain this in some more detail ? Not that is matters much since
the change already made it into mainline, but I would like to understand
what if anything I am missing here.
Thanks,
Guenter
> Fixes: d95fd5fce88f0 ("m68k: Mac II ADB fixes") # v5.0+
> Tested-by: Stan Johnson
> Signed-off-by: Finn Thain
> ---
> drivers/macintosh/via-macii.c | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
> index ac824d7b2dcfc..6aa903529570d 100644
> --- a/drivers/macintosh/via-macii.c
> +++ b/drivers/macintosh/via-macii.c
> @@ -270,15 +270,12 @@ static int macii_autopoll(int devs)
> unsigned long flags;
> int err = 0;
>
> + local_irq_save(flags);
> +
> /* bit 1 == device 1, and so on. */
> autopoll_devs = devs & 0xFFFE;
>
> - if (!autopoll_devs)
> - return 0;
> -
> - local_irq_save(flags);
> -
> - if (current_req == NULL) {
> + if (autopoll_devs && !current_req) {
> /* Send a Talk Reg 0. The controller will repeatedly transmit
>* this as long as it is idle.
>*/
Re: [PATCH 2/9] macintosh/via-macii: Poll the device most likely to respond
Hi,
On Sun, Jun 28, 2020 at 02:23:12PM +1000, Finn Thain wrote:
> Poll the most recently polled device by default, rather than the lowest
> device address that happens to be enabled in autopoll_devs. This improves
> input latency. Re-use macii_queue_poll() rather than duplicate that logic.
> This eliminates a static struct and function.
>
> Fixes: d95fd5fce88f0 ("m68k: Mac II ADB fixes") # v5.0+
> Tested-by: Stan Johnson
> Signed-off-by: Finn Thain
With this patch applied, the qemu "q800" emulation no longer works and is stuck
in early boot. Any idea why that might be the case, and/or how to debug it ?
Thanks,
Guenter
> ---
> drivers/macintosh/via-macii.c | 99 +++
> 1 file changed, 53 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
> index 6aa903529570d..d4f1a65c5f1fd 100644
> --- a/drivers/macintosh/via-macii.c
> +++ b/drivers/macintosh/via-macii.c
> @@ -77,6 +77,10 @@ static volatile unsigned char *via;
> #define ST_ODD 0x20/* ADB state: odd data byte */
> #define ST_IDLE 0x30/* ADB state: idle, nothing to
> send */
>
> +/* ADB command byte structure */
> +#define ADDR_MASK0xF0
> +#define CMD_MASK 0x0F
> +
> static int macii_init_via(void);
> static void macii_start(void);
> static irqreturn_t macii_interrupt(int irq, void *arg);
> @@ -117,7 +121,8 @@ static int reply_len; /* number of bytes received in
> reply_buf or req->reply */
> static int status; /* VIA's ADB status bits captured upon interrupt
> */
> static int last_status; /* status bits as at previous interrupt
> */
> static int srq_asserted; /* have to poll for the device that asserted it
> */
> -static int command_byte; /* the most recent command byte transmitted
> */
> +static u8 last_cmd; /* the most recent command byte transmitted
> */
> +static u8 last_poll_cmd; /* the most recent Talk R0 command byte transmitted
> */
> static int autopoll_devs; /* bits set are device addresses to be polled
> */
>
> /* Check for MacII style ADB */
> @@ -179,35 +184,49 @@ static int macii_init_via(void)
> /* Send an ADB poll (Talk Register 0 command prepended to the request queue)
> */
> static void macii_queue_poll(void)
> {
> - /* No point polling the active device as it will never assert SRQ, so
> - * poll the next device in the autopoll list. This could leave us
> - * stuck in a polling loop if an unprobed device is asserting SRQ.
> - * In theory, that could only happen if a device was plugged in after
> - * probing started. Unplugging it again will break the cycle.
> - * (Simply polling the next higher device often ends up polling almost
> - * every device (after wrapping around), which takes too long.)
> - */
> - int device_mask;
> - int next_device;
> static struct adb_request req;
> + unsigned char poll_command;
> + unsigned int poll_addr;
>
> + /* This only polls devices in the autopoll list, which assumes that
> + * unprobed devices never assert SRQ. That could happen if a device was
> + * plugged in after the adb bus scan. Unplugging it again will resolve
> + * the problem. This behaviour is similar to MacOS.
> + */
> if (!autopoll_devs)
> return;
>
> - device_mask = (1 << (((command_byte & 0xF0) >> 4) + 1)) - 1;
> - if (autopoll_devs & ~device_mask)
> - next_device = ffs(autopoll_devs & ~device_mask) - 1;
> - else
> - next_device = ffs(autopoll_devs) - 1;
> + /* The device most recently polled may not be the best device to poll
> + * right now. Some other device(s) may have signalled SRQ (the active
> + * device won't do that). Or the autopoll list may have been changed.
> + * Try polling the next higher address.
> + */
> + poll_addr = (last_poll_cmd & ADDR_MASK) >> 4;
> + if ((srq_asserted && last_cmd == last_poll_cmd) ||
> + !(autopoll_devs & (1 << poll_addr))) {
> + unsigned int higher_devs;
> +
> + higher_devs = autopoll_devs & -(1 << (poll_addr + 1));
> + poll_addr = ffs(higher_devs ? higher_devs : autopoll_devs) - 1;
> + }
>
> - adb_request(, NULL, ADBREQ_NOSEND, 1, ADB_READREG(next_device, 0));
> + /* Send a Talk Register 0 command */
> + poll_command = ADB_READREG(poll_addr, 0);
> +
> + /* No need to repeat this Talk command. The transceiver will do that
> + * as long as it is idle.
> + */
> + if (poll_command == last_cmd)
> + return;
> +
> + adb_request(, NULL, ADBREQ_NOSEND, 1, poll_command);
>
> req.sent = 0;
> req.complete = 0;
> req.reply_len = 0;
> req.next = current_req;
>
> - if (current_req != NULL) {
> + if (WARN_ON(current_req)) {
> current_req =
> } else {
>
Re: [RFC PATCH 1/2] powerpc/numa: Introduce logical numa id
"Aneesh Kumar K.V" writes:
> On 8/8/20 2:15 AM, Nathan Lynch wrote:
>> "Aneesh Kumar K.V" writes:
>>> On 8/7/20 9:54 AM, Nathan Lynch wrote:
"Aneesh Kumar K.V" writes:
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index e437a9ac4956..6c659aada55b 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -221,25 +221,51 @@ static void initialize_distance_lookup_table(int
> nid,
> }
>}
>
> +static u32 nid_map[MAX_NUMNODES] = {[0 ... MAX_NUMNODES - 1] =
> NUMA_NO_NODE};
It's odd to me to use MAX_NUMNODES for this array when it's going to be
indexed not by Linux's logical node IDs but by the platform-provided
domain number, which has no relation to MAX_NUMNODES.
>>>
>>>
>>> I didn't want to dynamically allocate this. We could fetch
>>> "ibm,max-associativity-domains" to find the size for that. The current
>>> code do assume firmware group id to not exceed MAX_NUMNODES. Hence kept
>>> the array size to be MAX_NUMNODEs. I do agree that it is confusing. May
>>> be we can do #define MAX_AFFINITY_DOMAIN MAX_NUMNODES?
>>
>> Well, consider:
>>
>> - ibm,max-associativity-domains can change at runtime with LPM. This
>>doesn't happen in practice yet, but we should probably start thinking
>>about how to support that.
>> - The domain numbering isn't clearly specified to have any particular
>>properties such as beginning at zero or a contiguous range.
>>
>> While the current code likely contains assumptions contrary to these
>> points, a change such as this is an opportunity to think about whether
>> those assumptions can be reduced or removed. In particular I think it
>> would be good to gracefully degrade when the number of NUMA affinity
>> domains can exceed MAX_NUMNODES. Using the platform-supplied domain
>> numbers to directly index Linux data structures will make that
>> impossible.
>>
>> So, maybe genradix or even xarray wouldn't actually be overengineering
>> here.
>>
>
> One of the challenges with such a data structure is that we initialize
> the nid_map before the slab is available. This means a memblock based
> allocation and we would end up implementing such a sparse data structure
> ourselves here.
>
> As you mentioned above, since we know that hypervisor as of now limits
> the max affinity domain id below ibm,max-associativity-domains we are
> good with an array-like nid_map we have here. This keeps the code simpler.
>
> This will also allow us to switch to a more sparse data structure as you
> requested here in the future because the main change that is pushed in
> this series is the usage of firmare_group_id_to_nid(). The details of
> the data structure we use to keep track of that mapping are pretty much
> internal to that function.
How about this? This makes it not a direct index. But it do limit the
search to max numa node on the system.
static int domain_id_map[MAX_NUMNODES] = {[0 ... MAX_NUMNODES - 1] = -1 };
static int __affinity_domain_to_nid(int domain_id, int max_nid)
{
int i;
for (i = 0; i < max_nid; i++) {
if (domain_id_map[i] == domain_id)
return i;
}
return NUMA_NO_NODE;
}
int affinity_domain_to_nid(struct affinity_domain *domain)
{
int nid, domain_id;
static int last_nid = 0;
static DEFINE_SPINLOCK(node_id_lock);
domain_id = domain->id;
/*
* For PowerNV we don't change the node id. This helps to avoid
* confusion w.r.t the expected node ids. On pseries, node numbers
* are virtualized. Hence do logical node id for pseries.
*/
if (!firmware_has_feature(FW_FEATURE_LPAR))
return domain_id;
if (domain_id == -1 || last_nid == MAX_NUMNODES)
return NUMA_NO_NODE;
nid = __affinity_domain_to_nid(domain_id, last_nid);
if (nid == NUMA_NO_NODE) {
spin_lock(_id_lock);
/* recheck with lock held */
nid = __affinity_domain_to_nid(domain_id, last_nid);
if (nid == NUMA_NO_NODE) {
nid = last_nid++;
domain_id_map[nid] = domain_id;
}
spin_unlock(_id_lock);
}
return nid;
}
Re: [PASEMI] Nemo board doesn't boot anymore after the commit "powerpc/book3s64/pkeys: Simplify pkey disable branch"
On 8/9/20 8:04 PM, Aneesh Kumar K.V wrote:
On 8/9/20 7:42 PM, Christian Zigotzky wrote:
Hello,
The Nemo board (A-EON AmigaOne X1000) [1] doesn't start with the
latest Git kernel anymore after the commit "powerpc/book3s64/pkeys:
Simplify pkey disable branch" [2].
I bisected today [3].
Result: powerpc/book3s64/pkeys: Simplify pkey disable branch
(a4678d4b477c3d2901f101986ca01406f3b7eaea) [2] is the first bad commit.
Unfortunately I wasn't able to revert the first bad commit. The first
bad commit depends on many other commits, which unfortunately I don't
know. I tried to remove the modifications of the files from the first
bad commit but without any success. There are just too many dependencies.
Additionally I reverted the commit "selftests/powerpc: Fix pkey
syscall redefinitions" [4] and compiled a new kernel but without any
success.
Could you please check the first bad commit?
Thanks,
Christian
Can you share a successful boot log of the system so that i can double
check the cpu_feature and mmu_feature reported ? I am looking for
details similar to below.
[ 0.00] cpu_features = 0x0001c07f8f5f91a7
[ 0.00] possible = 0x0001fbffcf5fb1a7
[ 0.00] always = 0x0003800081a1
[ 0.00] cpu_user_features = 0xdc0065c2 0xefe0
[ 0.00] mmu_features = 0x7c006001
[ 0.00] firmware_features = 0x001fc45bfc57
[ 0.00] vmalloc start = 0xc008
[ 0.00] IO start = 0xc00a
[ 0.00] vmemmap start = 0xc00c
IIUC this is P5+? (ISA 2.04). On that pkey should be marked disabled via
static int scan_pkey_feature(void)
{
int ret;
int pkeys_total = 0;
/*
* Only P7 and above supports SPRN_AMR update with MSR[PR] = 1
*/
if (!early_cpu_has_feature(CPU_FTR_ARCH_206))
return 0;
}
Can you boot with CONFIG_PPC_MEM_KEYS=n ?
Can you try this change on top of master?
modified arch/powerpc/mm/book3s64/pkeys.c
@@ -215,10 +215,6 @@ void __init pkey_early_init_devtree(void)
pr_info("Enabling pkeys with max key count %d\n", num_pkey);
out:
- /*
-* Setup uamor on boot cpu
-*/
- mtspr(SPRN_UAMOR, default_uamor);
return;
}
-aneesh
Re: [PASEMI] Nemo board doesn't boot anymore after the commit "powerpc/book3s64/pkeys: Simplify pkey disable branch"
On 8/9/20 7:42 PM, Christian Zigotzky wrote:
Hello,
The Nemo board (A-EON AmigaOne X1000) [1] doesn't start with the latest
Git kernel anymore after the commit "powerpc/book3s64/pkeys: Simplify
pkey disable branch" [2].
I bisected today [3].
Result: powerpc/book3s64/pkeys: Simplify pkey disable branch
(a4678d4b477c3d2901f101986ca01406f3b7eaea) [2] is the first bad commit.
Unfortunately I wasn't able to revert the first bad commit. The first
bad commit depends on many other commits, which unfortunately I don't
know. I tried to remove the modifications of the files from the first
bad commit but without any success. There are just too many dependencies.
Additionally I reverted the commit "selftests/powerpc: Fix pkey syscall
redefinitions" [4] and compiled a new kernel but without any success.
Could you please check the first bad commit?
Thanks,
Christian
Can you share a successful boot log of the system so that i can double
check the cpu_feature and mmu_feature reported ? I am looking for
details similar to below.
[0.00] cpu_features = 0x0001c07f8f5f91a7
[0.00] possible= 0x0001fbffcf5fb1a7
[0.00] always = 0x0003800081a1
[0.00] cpu_user_features = 0xdc0065c2 0xefe0
[0.00] mmu_features = 0x7c006001
[0.00] firmware_features = 0x001fc45bfc57
[0.00] vmalloc start = 0xc008
[0.00] IO start = 0xc00a
[0.00] vmemmap start = 0xc00c
IIUC this is P5+? (ISA 2.04). On that pkey should be marked disabled via
static int scan_pkey_feature(void)
{
int ret;
int pkeys_total = 0;
/*
* Only P7 and above supports SPRN_AMR update with MSR[PR] = 1
*/
if (!early_cpu_has_feature(CPU_FTR_ARCH_206))
return 0;
}
Can you boot with CONFIG_PPC_MEM_KEYS=n ?
-aneesh
Re: [RFC PATCH 1/2] powerpc/numa: Introduce logical numa id
On 8/8/20 2:15 AM, Nathan Lynch wrote:
"Aneesh Kumar K.V" writes:
On 8/7/20 9:54 AM, Nathan Lynch wrote:
"Aneesh Kumar K.V" writes:
diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index e437a9ac4956..6c659aada55b 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -221,25 +221,51 @@ static void initialize_distance_lookup_table(int nid,
}
}
+static u32 nid_map[MAX_NUMNODES] = {[0 ... MAX_NUMNODES - 1] = NUMA_NO_NODE};
It's odd to me to use MAX_NUMNODES for this array when it's going to be
indexed not by Linux's logical node IDs but by the platform-provided
domain number, which has no relation to MAX_NUMNODES.
I didn't want to dynamically allocate this. We could fetch
"ibm,max-associativity-domains" to find the size for that. The current
code do assume firmware group id to not exceed MAX_NUMNODES. Hence kept
the array size to be MAX_NUMNODEs. I do agree that it is confusing. May
be we can do #define MAX_AFFINITY_DOMAIN MAX_NUMNODES?
Well, consider:
- ibm,max-associativity-domains can change at runtime with LPM. This
doesn't happen in practice yet, but we should probably start thinking
about how to support that.
- The domain numbering isn't clearly specified to have any particular
properties such as beginning at zero or a contiguous range.
While the current code likely contains assumptions contrary to these
points, a change such as this is an opportunity to think about whether
those assumptions can be reduced or removed. In particular I think it
would be good to gracefully degrade when the number of NUMA affinity
domains can exceed MAX_NUMNODES. Using the platform-supplied domain
numbers to directly index Linux data structures will make that
impossible.
So, maybe genradix or even xarray wouldn't actually be overengineering
here.
One of the challenges with such a data structure is that we initialize
the nid_map before the slab is available. This means a memblock based
allocation and we would end up implementing such a sparse data structure
ourselves here.
As you mentioned above, since we know that hypervisor as of now limits
the max affinity domain id below ibm,max-associativity-domains we are
good with an array-like nid_map we have here. This keeps the code simpler.
This will also allow us to switch to a more sparse data structure as you
requested here in the future because the main change that is pushed in
this series is the usage of firmare_group_id_to_nid(). The details of
the data structure we use to keep track of that mapping are pretty much
internal to that function.
-aneesh
[PASEMI] Nemo board doesn't boot anymore after the commit "powerpc/book3s64/pkeys: Simplify pkey disable branch"
Hello, The Nemo board (A-EON AmigaOne X1000) [1] doesn't start with the latest Git kernel anymore after the commit "powerpc/book3s64/pkeys: Simplify pkey disable branch" [2]. I bisected today [3]. Result: powerpc/book3s64/pkeys: Simplify pkey disable branch (a4678d4b477c3d2901f101986ca01406f3b7eaea) [2] is the first bad commit. Unfortunately I wasn't able to revert the first bad commit. The first bad commit depends on many other commits, which unfortunately I don't know. I tried to remove the modifications of the files from the first bad commit but without any success. There are just too many dependencies. Additionally I reverted the commit "selftests/powerpc: Fix pkey syscall redefinitions" [4] and compiled a new kernel but without any success. Could you please check the first bad commit? Thanks, Christian [1] https://en.wikipedia.org/wiki/AmigaOne_X1000 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a4678d4b477c3d2901f101986ca01406f3b7eaea [3] https://forum.hyperion-entertainment.com/viewtopic.php?p=51340#p51340 [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a7aaa2f26bfd932a654706b19859e7adf802bee2
Re: linux-next: manual merge of the set_fs tree with the powerpc tree
Hi all,
On Fri, 17 Jul 2020 19:09:31 +1000 Stephen Rothwell
wrote:
>
> Today's linux-next merge of the set_fs tree got a conflict in:
>
> arch/powerpc/mm/numa.c
>
> between commit:
>
> c30f931e891e ("powerpc/numa: remove ability to enable topology updates")
>
> from the powerpc tree and commit:
>
> 16a04bde8169 ("proc: switch over direct seq_read method calls to
> seq_read_iter")
>
> from the set_fs tree.
>
> I fixed it up (the former removed the code updated by the latter, so I
> just did that) and can carry the fix as necessary. This is now fixed as
> far as linux-next is concerned, but any non trivial conflicts should be
> mentioned to your upstream maintainer when your tree is submitted for
> merging. You may also want to consider cooperating with the maintainer
> of the conflicting tree to minimise any particularly complex conflicts.
This is now a conflict between the set_fs tree and Linus' tree.
--
Cheers,
Stephen Rothwell
pgpgoL2PCd_XS.pgp
Description: OpenPGP digital signature
