* [PATCH v1] powerpc: Fix BUG_ON during memory unplug on radix
From: Bharata B Rao @ 2019-06-24 6:03 UTC (permalink / raw)
To: linuxppc-dev; +Cc: sraithal, aneesh.kumar, npiggin, Bharata B Rao
We hit the following BUG_ON when memory hotplugged before reboot
is unplugged after reboot:
kernel BUG at arch/powerpc/mm/pgtable-frag.c:113!
remove_pagetable+0x594/0x6a0
(unreliable)
remove_pagetable+0x94/0x6a0
vmemmap_free+0x394/0x410
sparse_remove_one_section+0x26c/0x2e8
__remove_pages+0x428/0x540
arch_remove_memory+0xd0/0x170
__remove_memory+0xd4/0x1a0
dlpar_remove_lmb+0xbc/0x110
dlpar_memory+0xa80/0xd20
handle_dlpar_errorlog+0xa8/0x160
pseries_hp_work_fn+0x2c/0x60
process_one_work+0x46c/0x860
worker_thread+0x364/0x5e0
kthread+0x1b0/0x1c0
ret_from_kernel_thread+0x5c/0x68
This occurs because, during reboot-after-hotplug, the hotplugged
memory range gets initialized as regular memory and page
tables are setup using memblock allocator. This means that we
wouldn't have initialized the PMD or PTE fragment count for
those PMD or PTE pages.
Fixing this includes 3 parts:
- Re-walk the init_mm page tables from mem_init() and initialize
the PMD and PTE fragment counts appropriately. So PMD and PTE
table pages allocated during early allocation will have a
fragment count of 1.
- Convert the pages from memblock pages to regular pages so that
they can be freed back to buddy allocator seamlessly. However
we do this for only PMD and PTE pages and not for PUD pages.
PUD pages are freed using kmem_cache_free() and we need to
identify memblock pages and free them differently.
- When we do early memblock based allocation of PMD and PUD pages,
allocate in PAGE_SIZE granularity so that we are sure the
complete page is used as pagetable page. PAGE_SIZE allocations will
have an implication on the amount of memory used for page tables,
an example of which is shown below:
Since we now do PAGE_SIZE allocations for both PUD table and
PMD table (Note that PTE table allocation is already of PAGE_SIZE),
we end up allocating more memory for the same amount of system RAM.
Here is an example of how much more we end up allocating for
page tables in case of 64T system RAM:
1. Mapping system RAM
With each PGD entry spanning 512G, 64TB RAM would need 128 entries
and hence 128 PUD tables. We use 1G mapping at PUD level (level 2)
With default PUD_TABLE_SIZE(4K), 128*4K=512K (8 64K pages)
With PAGE_SIZE(64K) allocations, 128*64K=8192K (128 64K pages)
2. Mapping struct pages (memmap)
64T RAM would need 64G for memmap with struct page size being 64B.
Since memmap array is mapped using 64K mappings, we would need
64 PUD entries or 64 PMD tables (level 3) in total.
With default PMD_TABLE_SIZE(4K), 64*4K=256K (4 64K pages)
With PAGE_SIZE(64K) allocations, 64*64K=4096K (64 64K pages)
There is no change in PTE table (level 4) allocation requirement as
early page table allocation is already using PAGE_SIZE PTE tables.
So essentially with this change we would use 180 64K pages
more for 64T system.
Reported-by: Srikanth Aithal <sraithal@linux.vnet.ibm.com>
Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
---
v0 - https://lists.ozlabs.org/pipermail/linuxppc-dev/2019-June/192242.html
Changes in v1:
- Handling PUD table freeing too.
- Added details about how much extra memory we use up with
this approach into the commit message
- A few cleanups and renames
arch/powerpc/include/asm/book3s/64/pgalloc.h | 7 +-
arch/powerpc/include/asm/book3s/64/radix.h | 1 +
arch/powerpc/include/asm/sparsemem.h | 1 +
arch/powerpc/mm/book3s64/pgtable.c | 15 +++-
arch/powerpc/mm/book3s64/radix_pgtable.c | 79 +++++++++++++++++++-
arch/powerpc/mm/mem.c | 5 ++
6 files changed, 104 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/pgalloc.h b/arch/powerpc/include/asm/book3s/64/pgalloc.h
index d5a44912902f..9ae134f260be 100644
--- a/arch/powerpc/include/asm/book3s/64/pgalloc.h
+++ b/arch/powerpc/include/asm/book3s/64/pgalloc.h
@@ -111,7 +111,12 @@ static inline pud_t *pud_alloc_one(struct mm_struct *mm, unsigned long addr)
static inline void pud_free(struct mm_struct *mm, pud_t *pud)
{
- kmem_cache_free(PGT_CACHE(PUD_CACHE_INDEX), pud);
+ struct page *page = virt_to_page(pud);
+
+ if (PageReserved(page))
+ free_reserved_page(page);
+ else
+ kmem_cache_free(PGT_CACHE(PUD_CACHE_INDEX), pud);
}
static inline void pud_populate(struct mm_struct *mm, pud_t *pud, pmd_t *pmd)
diff --git a/arch/powerpc/include/asm/book3s/64/radix.h b/arch/powerpc/include/asm/book3s/64/radix.h
index 574eca33f893..4320f2790e8d 100644
--- a/arch/powerpc/include/asm/book3s/64/radix.h
+++ b/arch/powerpc/include/asm/book3s/64/radix.h
@@ -285,6 +285,7 @@ static inline unsigned long radix__get_tree_size(void)
#ifdef CONFIG_MEMORY_HOTPLUG
int radix__create_section_mapping(unsigned long start, unsigned long end, int nid);
int radix__remove_section_mapping(unsigned long start, unsigned long end);
+void radix__fixup_pgtable_fragments(void);
#endif /* CONFIG_MEMORY_HOTPLUG */
#endif /* __ASSEMBLY__ */
#endif
diff --git a/arch/powerpc/include/asm/sparsemem.h b/arch/powerpc/include/asm/sparsemem.h
index 3192d454a733..e662f9232d35 100644
--- a/arch/powerpc/include/asm/sparsemem.h
+++ b/arch/powerpc/include/asm/sparsemem.h
@@ -15,6 +15,7 @@
#ifdef CONFIG_MEMORY_HOTPLUG
extern int create_section_mapping(unsigned long start, unsigned long end, int nid);
extern int remove_section_mapping(unsigned long start, unsigned long end);
+void fixup_pgtable_fragments(void);
#ifdef CONFIG_PPC_BOOK3S_64
extern int resize_hpt_for_hotplug(unsigned long new_mem_size);
diff --git a/arch/powerpc/mm/book3s64/pgtable.c b/arch/powerpc/mm/book3s64/pgtable.c
index 01bc9663360d..c8fa94802620 100644
--- a/arch/powerpc/mm/book3s64/pgtable.c
+++ b/arch/powerpc/mm/book3s64/pgtable.c
@@ -186,6 +186,13 @@ int __meminit remove_section_mapping(unsigned long start, unsigned long end)
return hash__remove_section_mapping(start, end);
}
+
+void fixup_pgtable_fragments(void)
+{
+ if (radix_enabled())
+ radix__fixup_pgtable_fragments();
+}
+
#endif /* CONFIG_MEMORY_HOTPLUG */
void __init mmu_partition_table_init(void)
@@ -326,6 +333,8 @@ void pmd_fragment_free(unsigned long *pmd)
static inline void pgtable_free(void *table, int index)
{
+ struct page *page;
+
switch (index) {
case PTE_INDEX:
pte_fragment_free(table, 0);
@@ -334,7 +343,11 @@ static inline void pgtable_free(void *table, int index)
pmd_fragment_free(table);
break;
case PUD_INDEX:
- kmem_cache_free(PGT_CACHE(PUD_CACHE_INDEX), table);
+ page = virt_to_page(table);
+ if (PageReserved(page))
+ free_reserved_page(page);
+ else
+ kmem_cache_free(PGT_CACHE(PUD_CACHE_INDEX), table);
break;
#if defined(CONFIG_PPC_4K_PAGES) && defined(CONFIG_HUGETLB_PAGE)
/* 16M hugepd directory at pud level */
diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
index 273ae66a9a45..4167e1ba1c58 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -32,6 +32,81 @@
unsigned int mmu_pid_bits;
unsigned int mmu_base_pid;
+/*
+ * Since we know that this page is a memblock-allocated page,
+ * convert it into a normal page in addition to fixing the fragment
+ * count.
+ */
+static void fix_fragment_count(struct page *page)
+{
+ ClearPageReserved(page);
+ init_page_count(page);
+ adjust_managed_page_count(page, 1);
+ atomic_inc(&page->pt_frag_refcount);
+}
+
+static void fixup_pte_fragments(pmd_t *pmd)
+{
+ int i;
+
+ for (i = 0; i < PTRS_PER_PMD; i++, pmd++) {
+ pte_t *pte;
+
+ if (pmd_none(*pmd))
+ continue;
+ if (pmd_huge(*pmd))
+ continue;
+
+ pte = pte_offset_kernel(pmd, 0);
+ if (!pte_none(*pte))
+ fix_fragment_count(virt_to_page(pte));
+ }
+}
+
+static void fixup_pmd_fragments(pud_t *pud)
+{
+ int i;
+
+ for (i = 0; i < PTRS_PER_PUD; i++, pud++) {
+ pmd_t *pmd;
+
+ if (pud_none(*pud))
+ continue;
+ if (pud_huge(*pud))
+ continue;
+
+ pmd = pmd_offset(pud, 0);
+ if (!pmd_none(*pmd))
+ fix_fragment_count(virt_to_page(pmd));
+ fixup_pte_fragments(pmd);
+ }
+}
+
+/*
+ * Walk the init_mm page tables and fixup the PMD and PTE fragment
+ * counts. This allows the PUD, PMD and PTE pages to be freed
+ * back to buddy allocator properly during memory unplug.
+ */
+void radix__fixup_pgtable_fragments(void)
+{
+ int i;
+ pgd_t *pgd = pgd_offset_k(0UL);
+
+ spin_lock(&init_mm.page_table_lock);
+ for (i = 0; i < PTRS_PER_PGD; i++, pgd++) {
+ pud_t *pud;
+
+ if (pgd_none(*pgd))
+ continue;
+ if (pgd_huge(*pgd))
+ continue;
+
+ pud = pud_offset(pgd, 0);
+ fixup_pmd_fragments(pud);
+ }
+ spin_unlock(&init_mm.page_table_lock);
+}
+
static int native_register_process_table(unsigned long base, unsigned long pg_sz,
unsigned long table_size)
{
@@ -80,7 +155,7 @@ static int early_map_kernel_page(unsigned long ea, unsigned long pa,
pgdp = pgd_offset_k(ea);
if (pgd_none(*pgdp)) {
- pudp = early_alloc_pgtable(PUD_TABLE_SIZE, nid,
+ pudp = early_alloc_pgtable(PAGE_SIZE, nid,
region_start, region_end);
pgd_populate(&init_mm, pgdp, pudp);
}
@@ -90,7 +165,7 @@ static int early_map_kernel_page(unsigned long ea, unsigned long pa,
goto set_the_pte;
}
if (pud_none(*pudp)) {
- pmdp = early_alloc_pgtable(PMD_TABLE_SIZE, nid,
+ pmdp = early_alloc_pgtable(PAGE_SIZE, nid,
region_start, region_end);
pud_populate(&init_mm, pudp, pmdp);
}
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index cba29131bccc..a8788b404266 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -51,6 +51,10 @@
#include <mm/mmu_decl.h>
+void __weak fixup_pgtable_fragments(void)
+{
+}
+
#ifndef CPU_FTR_COHERENT_ICACHE
#define CPU_FTR_COHERENT_ICACHE 0 /* XXX for now */
#define CPU_FTR_NOEXECUTE 0
@@ -276,6 +280,7 @@ void __init mem_init(void)
set_max_mapnr(max_pfn);
memblock_free_all();
+ fixup_pgtable_fragments();
#ifdef CONFIG_HIGHMEM
{
unsigned long pfn, highmem_mapnr;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 3/3] mm/vmalloc: fix vmalloc_to_page for huge vmap mappings
From: Anshuman Khandual @ 2019-06-24 6:52 UTC (permalink / raw)
To: Nicholas Piggin, linux-mm
Cc: Mark Rutland, Ard Biesheuvel, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <20190623094446.28722-4-npiggin@gmail.com>
On 06/23/2019 03:14 PM, Nicholas Piggin wrote:
> 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 <npiggin@gmail.com>
> ---
> include/asm-generic/4level-fixup.h | 1 +
> include/asm-generic/5level-fixup.h | 1 +
> mm/vmalloc.c | 37 +++++++++++++++++++-----------
> 3 files changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/include/asm-generic/4level-fixup.h b/include/asm-generic/4level-fixup.h
> index e3667c9a33a5..3cc65a4dd093 100644
> --- a/include/asm-generic/4level-fixup.h
> +++ b/include/asm-generic/4level-fixup.h
> @@ -20,6 +20,7 @@
> #define pud_none(pud) 0
> #define pud_bad(pud) 0
> #define pud_present(pud) 1
> +#define pud_large(pud) 0
> #define pud_ERROR(pud) do { } while (0)
> #define pud_clear(pud) pgd_clear(pud)
> #define pud_val(pud) pgd_val(pud)
> diff --git a/include/asm-generic/5level-fixup.h b/include/asm-generic/5level-fixup.h
> index bb6cb347018c..c4377db09a4f 100644
> --- a/include/asm-generic/5level-fixup.h
> +++ b/include/asm-generic/5level-fixup.h
> @@ -22,6 +22,7 @@
> #define p4d_none(p4d) 0
> #define p4d_bad(p4d) 0
> #define p4d_present(p4d) 1
> +#define p4d_large(p4d) 0
> #define p4d_ERROR(p4d) do { } while (0)
> #define p4d_clear(p4d) pgd_clear(p4d)
> #define p4d_val(p4d) pgd_val(p4d)
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 4c9e150e5ad3..4be98f700862 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -36,6 +36,7 @@
> #include <linux/rbtree_augmented.h>
>
> #include <linux/uaccess.h>
> +#include <asm/pgtable.h>
> #include <asm/tlbflush.h>
> #include <asm/shmparam.h>
>
> @@ -284,26 +285,36 @@ struct page *vmalloc_to_page(const void *vmalloc_addr)
>
> if (pgd_none(*pgd))
> return NULL;
> +
> p4d = p4d_offset(pgd, addr);
> if (p4d_none(*p4d))
> return NULL;
> - pud = pud_offset(p4d, addr);
> + if (WARN_ON_ONCE(p4d_bad(*p4d)))
> + return NULL;
The warning here is a required addition but it needs to be moved after p4d_large()
check. Please see the next comment below.
> +#ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
> + if (p4d_large(*p4d))
> + return p4d_page(*p4d) + ((addr & ~P4D_MASK) >> PAGE_SHIFT);
> +#endif
>
> - /*
> - * 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 (WARN_ON_ONCE(pud_bad(*pud)))
> return NULL;
> +#ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
> + if (pud_large(*pud))
> + return pud_page(*pud) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
> +#endif
> +
pud_bad() on arm64 returns true when the PUD does not point to a next page
table page implying the fact that it might be a large/huge entry. I am not
sure if the semantics holds good for other architectures too. But on arm64
if pud_large() is true, then pud_bad() will be true as well. So pud_bad()
check must happen after pud_large() check. So the sequence here should be
1. pud_none() --> Nothing is in here, return NULL
2. pud_large() --> Return offset page address from the huge page mapping
3. pud_bad() --> Return NULL as there is no more page table level left
Checking pud_bad() first can return NULL for a valid huge mapping.
> 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 (WARN_ON_ONCE(pmd_bad(*pmd)))
> return NULL;
> +#ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
> + if (pmd_large(*pmd))
> + return pmd_page(*pmd) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
> +#endif
Ditto.
I see that your previous proposal had this right which got changed in this
manner after my comments. Sorry about it.
It was recently when I learned (correctly) that expected semantics of pxx_bad()
is that - It does not point to the next page table page. Hence I wonder why is
this not renamed as pxx_table() instead to make it absolutely clear.
^ permalink raw reply
* [PATCH v5 2/4] crypto: talitos - move struct talitos_edesc into talitos.h
From: Christophe Leroy @ 2019-06-24 7:20 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, horia.geanta
Cc: linuxppc-dev, linux-crypto, linux-kernel
In-Reply-To: <cover.1561360551.git.christophe.leroy@c-s.fr>
Moves struct talitos_edesc into talitos.h so that it can be used
from any place in talitos.c
It will be required for next patch ("crypto: talitos - fix hash
on SEC1")
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: stable@vger.kernel.org
---
drivers/crypto/talitos.c | 30 ------------------------------
drivers/crypto/talitos.h | 30 ++++++++++++++++++++++++++++++
2 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index a7704b53529d..95dc3957f358 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -951,36 +951,6 @@ static int aead_des3_setkey(struct crypto_aead *authenc,
goto out;
}
-/*
- * talitos_edesc - s/w-extended descriptor
- * @src_nents: number of segments in input scatterlist
- * @dst_nents: number of segments in output scatterlist
- * @icv_ool: whether ICV is out-of-line
- * @iv_dma: dma address of iv for checking continuity and link table
- * @dma_len: length of dma mapped link_tbl space
- * @dma_link_tbl: bus physical address of link_tbl/buf
- * @desc: h/w descriptor
- * @link_tbl: input and output h/w link tables (if {src,dst}_nents > 1) (SEC2)
- * @buf: input and output buffeur (if {src,dst}_nents > 1) (SEC1)
- *
- * if decrypting (with authcheck), or either one of src_nents or dst_nents
- * is greater than 1, an integrity check value is concatenated to the end
- * of link_tbl data
- */
-struct talitos_edesc {
- int src_nents;
- int dst_nents;
- bool icv_ool;
- dma_addr_t iv_dma;
- int dma_len;
- dma_addr_t dma_link_tbl;
- struct talitos_desc desc;
- union {
- struct talitos_ptr link_tbl[0];
- u8 buf[0];
- };
-};
-
static void talitos_sg_unmap(struct device *dev,
struct talitos_edesc *edesc,
struct scatterlist *src,
diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index 32ad4fc679ed..95f78c6d9206 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -42,6 +42,36 @@ struct talitos_desc {
#define TALITOS_DESC_SIZE (sizeof(struct talitos_desc) - sizeof(__be32))
+/*
+ * talitos_edesc - s/w-extended descriptor
+ * @src_nents: number of segments in input scatterlist
+ * @dst_nents: number of segments in output scatterlist
+ * @icv_ool: whether ICV is out-of-line
+ * @iv_dma: dma address of iv for checking continuity and link table
+ * @dma_len: length of dma mapped link_tbl space
+ * @dma_link_tbl: bus physical address of link_tbl/buf
+ * @desc: h/w descriptor
+ * @link_tbl: input and output h/w link tables (if {src,dst}_nents > 1) (SEC2)
+ * @buf: input and output buffeur (if {src,dst}_nents > 1) (SEC1)
+ *
+ * if decrypting (with authcheck), or either one of src_nents or dst_nents
+ * is greater than 1, an integrity check value is concatenated to the end
+ * of link_tbl data
+ */
+struct talitos_edesc {
+ int src_nents;
+ int dst_nents;
+ bool icv_ool;
+ dma_addr_t iv_dma;
+ int dma_len;
+ dma_addr_t dma_link_tbl;
+ struct talitos_desc desc;
+ union {
+ struct talitos_ptr link_tbl[0];
+ u8 buf[0];
+ };
+};
+
/**
* talitos_request - descriptor submission request
* @desc: descriptor pointer (kernel virtual)
--
2.13.3
^ permalink raw reply related
* [PATCH v5 4/4] crypto: talitos - drop icv_ool
From: Christophe Leroy @ 2019-06-24 7:20 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, horia.geanta
Cc: linuxppc-dev, linux-crypto, linux-kernel
In-Reply-To: <cover.1561360551.git.christophe.leroy@c-s.fr>
icv_ool is not used anymore, drop it.
Fixes: e345177ded17 ("crypto: talitos - fix AEAD processing.")
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
drivers/crypto/talitos.c | 3 ---
drivers/crypto/talitos.h | 2 --
2 files changed, 5 deletions(-)
diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index ab6bd45addf7..c9d686a0e805 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1285,9 +1285,6 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
is_ipsec_esp && !encrypt);
tbl_off += ret;
- /* ICV data */
- edesc->icv_ool = !encrypt;
-
if (!encrypt && is_ipsec_esp) {
struct talitos_ptr *tbl_ptr = &edesc->link_tbl[tbl_off];
diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index 95f78c6d9206..1469b956948a 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -46,7 +46,6 @@ struct talitos_desc {
* talitos_edesc - s/w-extended descriptor
* @src_nents: number of segments in input scatterlist
* @dst_nents: number of segments in output scatterlist
- * @icv_ool: whether ICV is out-of-line
* @iv_dma: dma address of iv for checking continuity and link table
* @dma_len: length of dma mapped link_tbl space
* @dma_link_tbl: bus physical address of link_tbl/buf
@@ -61,7 +60,6 @@ struct talitos_desc {
struct talitos_edesc {
int src_nents;
int dst_nents;
- bool icv_ool;
dma_addr_t iv_dma;
int dma_len;
dma_addr_t dma_link_tbl;
--
2.13.3
^ permalink raw reply related
* [PATCH v5 1/4] lib/scatterlist: Fix mapping iterator when sg->offset is greater than PAGE_SIZE
From: Christophe Leroy @ 2019-06-24 7:20 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, horia.geanta
Cc: linuxppc-dev, linux-crypto, linux-kernel
In-Reply-To: <cover.1561360551.git.christophe.leroy@c-s.fr>
All mapping iterator logic is based on the assumption that sg->offset
is always lower than PAGE_SIZE.
But there are situations where sg->offset is such that the SG item
is on the second page. In that case sg_copy_to_buffer() fails
properly copying the data into the buffer. One of the reason is
that the data will be outside the kmapped area used to access that
data.
This patch fixes the issue by adjusting the mapping iterator
offset and pgoffset fields such that offset is always lower than
PAGE_SIZE.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Fixes: 4225fc8555a9 ("lib/scatterlist: use page iterator in the mapping iterator")
Cc: stable@vger.kernel.org
---
lib/scatterlist.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 739dc9fe2c55..f0757a67affe 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -678,17 +678,18 @@ static bool sg_miter_get_next_page(struct sg_mapping_iter *miter)
{
if (!miter->__remaining) {
struct scatterlist *sg;
- unsigned long pgoffset;
if (!__sg_page_iter_next(&miter->piter))
return false;
sg = miter->piter.sg;
- pgoffset = miter->piter.sg_pgoffset;
- miter->__offset = pgoffset ? 0 : sg->offset;
+ miter->__offset = miter->piter.sg_pgoffset ? 0 : sg->offset;
+ miter->piter.sg_pgoffset += miter->__offset >> PAGE_SHIFT;
+ miter->__offset &= PAGE_SIZE - 1;
miter->__remaining = sg->offset + sg->length -
- (pgoffset << PAGE_SHIFT) - miter->__offset;
+ (miter->piter.sg_pgoffset << PAGE_SHIFT) -
+ miter->__offset;
miter->__remaining = min_t(unsigned long, miter->__remaining,
PAGE_SIZE - miter->__offset);
}
--
2.13.3
^ permalink raw reply related
* [PATCH v5 0/4] *** SUBJECT HERE ***
From: Christophe Leroy @ 2019-06-24 7:20 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, horia.geanta
Cc: linuxppc-dev, linux-crypto, linux-kernel
This series is the last set of fixes for the Talitos driver.
We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
[ 3.385197] bus: 'platform': really_probe: probing driver talitos with device ff020000.crypto
[ 3.450982] random: fast init done
[ 12.252548] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos-hsna)
[ 12.262226] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos-hsna)
[ 43.310737] Bug in SEC1, padding ourself
[ 45.603318] random: crng init done
[ 54.612333] talitos ff020000.crypto: fsl,sec1.2 algorithms registered in /proc/crypto
[ 54.620232] driver: 'talitos': driver_bound: bound to device 'ff020000.crypto'
[ 1.193721] bus: 'platform': really_probe: probing driver talitos with device b0030000.crypto
[ 1.229197] random: fast init done
[ 2.714920] alg: No test for authenc(hmac(sha224),cbc(aes)) (authenc-hmac-sha224-cbc-aes-talitos)
[ 2.724312] alg: No test for authenc(hmac(sha224),cbc(aes)) (authenc-hmac-sha224-cbc-aes-talitos-hsna)
[ 4.482045] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos)
[ 4.490940] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos-hsna)
[ 4.500280] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos)
[ 4.509727] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos-hsna)
[ 6.631781] random: crng init done
[ 11.521795] talitos b0030000.crypto: fsl,sec2.2 algorithms registered in /proc/crypto
[ 11.529803] driver: 'talitos': driver_bound: bound to device 'b0030000.crypto'
v2: dropped patch 1 which was irrelevant due to a rebase weirdness. Added Cc to stable on the 2 first patches.
v3:
- removed stable reference in patch 1
- reworded patch 1 to include name of patch 2 for the dependency.
- mentionned this dependency in patch 2 as well.
- corrected the Fixes: sha1 in patch 4
v4:
- using scatterwalk_ffwd() instead of opencodying SG list forwarding.
- Added a patch to fix sg_copy_to_buffer() when sg->offset() is greater than PAGE_SIZE,
otherwise sg_copy_to_buffer() fails when the list has been forwarded with scatterwalk_ffwd().
- taken the patch "crypto: talitos - eliminate unneeded 'done' functions at build time"
out of the series because it is independent.
- added a helper to find the header field associated to a request in flush_channe()
v5:
- Replacing the while loop by a direct shift/mask operation, as suggested by Herbert in patch 1.
Christophe Leroy (4):
lib/scatterlist: Fix mapping iterator when sg->offset is greater than
PAGE_SIZE
crypto: talitos - move struct talitos_edesc into talitos.h
crypto: talitos - fix hash on SEC1.
crypto: talitos - drop icv_ool
drivers/crypto/talitos.c | 102 +++++++++++++++++++----------------------------
drivers/crypto/talitos.h | 28 +++++++++++++
lib/scatterlist.c | 9 +++--
3 files changed, 74 insertions(+), 65 deletions(-)
--
2.13.3
^ permalink raw reply
* [PATCH v5 3/4] crypto: talitos - fix hash on SEC1.
From: Christophe Leroy @ 2019-06-24 7:20 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, horia.geanta
Cc: linuxppc-dev, linux-crypto, linux-kernel
In-Reply-To: <cover.1561360551.git.christophe.leroy@c-s.fr>
On SEC1, hash provides wrong result when performing hashing in several
steps with input data SG list has more than one element. This was
detected with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
[ 44.185947] alg: hash: md5-talitos test failed (wrong result) on test vector 6, cfg="random: may_sleep use_finup src_divs=[<reimport>25.88%@+8063, <flush>24.19%@+9588, 28.63%@+16333, <reimport>4.60%@+6756, 16.70%@+16281] dst_divs=[71.61%@alignmask+16361, 14.36%@+7756, 14.3%@+"
[ 44.325122] alg: hash: sha1-talitos test failed (wrong result) on test vector 3, cfg="random: inplace use_final src_divs=[<flush,nosimd>16.56%@+16378, <reimport>52.0%@+16329, 21.42%@alignmask+16380, 10.2%@alignmask+16380] iv_offset=39"
[ 44.493500] alg: hash: sha224-talitos test failed (wrong result) on test vector 4, cfg="random: use_final nosimd src_divs=[<reimport>52.27%@+7401, <reimport>17.34%@+16285, <flush>17.71%@+26, 12.68%@+10644] iv_offset=43"
[ 44.673262] alg: hash: sha256-talitos test failed (wrong result) on test vector 4, cfg="random: may_sleep use_finup src_divs=[<reimport>60.6%@+12790, 17.86%@+1329, <reimport>12.64%@alignmask+16300, 8.29%@+15, 0.40%@+13506, <reimport>0.51%@+16322, <reimport>0.24%@+16339] dst_divs"
This is due to two issues:
- We have an overlap between the buffer used for copying the input
data (SEC1 doesn't do scatter/gather) and the chained descriptor.
- Data copy is wrong when the previous hash left less than one
blocksize of data to hash, implying a complement of the previous
block with a few bytes from the new request.
Fix it by:
- Moving the second descriptor after the buffer, as moving the buffer
after the descriptor would make it more complex for other cipher
operations (AEAD, ABLKCIPHER)
- Skip the bytes taken from the new request to complete the previous
one by moving the SG list forward.
Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
drivers/crypto/talitos.c | 69 ++++++++++++++++++++++++++++--------------------
1 file changed, 41 insertions(+), 28 deletions(-)
diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 95dc3957f358..ab6bd45addf7 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -320,6 +320,21 @@ static int talitos_submit(struct device *dev, int ch, struct talitos_desc *desc,
return -EINPROGRESS;
}
+static __be32 get_request_hdr(struct talitos_request *request, bool is_sec1)
+{
+ struct talitos_edesc *edesc;
+
+ if (!is_sec1)
+ return request->desc->hdr;
+
+ if (!request->desc->next_desc)
+ return request->desc->hdr1;
+
+ edesc = container_of(request->desc, struct talitos_edesc, desc);
+
+ return ((struct talitos_desc *)(edesc->buf + edesc->dma_len))->hdr1;
+}
+
/*
* process what was done, notify callback of error if not
*/
@@ -341,12 +356,7 @@ static void flush_channel(struct device *dev, int ch, int error, int reset_ch)
/* descriptors with their done bits set don't get the error */
rmb();
- if (!is_sec1)
- hdr = request->desc->hdr;
- else if (request->desc->next_desc)
- hdr = (request->desc + 1)->hdr1;
- else
- hdr = request->desc->hdr1;
+ hdr = get_request_hdr(request, is_sec1);
if ((hdr & DESC_HDR_DONE) == DESC_HDR_DONE)
status = 0;
@@ -476,8 +486,14 @@ static u32 current_desc_hdr(struct device *dev, int ch)
}
}
- if (priv->chan[ch].fifo[iter].desc->next_desc == cur_desc)
- return (priv->chan[ch].fifo[iter].desc + 1)->hdr;
+ if (priv->chan[ch].fifo[iter].desc->next_desc == cur_desc) {
+ struct talitos_edesc *edesc;
+
+ edesc = container_of(priv->chan[ch].fifo[iter].desc,
+ struct talitos_edesc, desc);
+ return ((struct talitos_desc *)
+ (edesc->buf + edesc->dma_len))->hdr;
+ }
return priv->chan[ch].fifo[iter].desc->hdr;
}
@@ -1402,15 +1418,11 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
edesc->dst_nents = dst_nents;
edesc->iv_dma = iv_dma;
edesc->dma_len = dma_len;
- if (dma_len) {
- void *addr = &edesc->link_tbl[0];
-
- if (is_sec1 && !dst)
- addr += sizeof(struct talitos_desc);
- edesc->dma_link_tbl = dma_map_single(dev, addr,
+ if (dma_len)
+ edesc->dma_link_tbl = dma_map_single(dev, &edesc->link_tbl[0],
edesc->dma_len,
DMA_BIDIRECTIONAL);
- }
+
return edesc;
}
@@ -1722,14 +1734,16 @@ static void common_nonsnoop_hash_unmap(struct device *dev,
struct talitos_private *priv = dev_get_drvdata(dev);
bool is_sec1 = has_ftr_sec1(priv);
struct talitos_desc *desc = &edesc->desc;
- struct talitos_desc *desc2 = desc + 1;
+ struct talitos_desc *desc2 = (struct talitos_desc *)
+ (edesc->buf + edesc->dma_len);
unmap_single_talitos_ptr(dev, &edesc->desc.ptr[5], DMA_FROM_DEVICE);
if (desc->next_desc &&
desc->ptr[5].ptr != desc2->ptr[5].ptr)
unmap_single_talitos_ptr(dev, &desc2->ptr[5], DMA_FROM_DEVICE);
- talitos_sg_unmap(dev, edesc, req_ctx->psrc, NULL, 0, 0);
+ if (req_ctx->psrc)
+ talitos_sg_unmap(dev, edesc, req_ctx->psrc, NULL, 0, 0);
/* When using hashctx-in, must unmap it. */
if (from_talitos_ptr_len(&edesc->desc.ptr[1], is_sec1))
@@ -1796,7 +1810,6 @@ static void talitos_handle_buggy_hash(struct talitos_ctx *ctx,
static int common_nonsnoop_hash(struct talitos_edesc *edesc,
struct ahash_request *areq, unsigned int length,
- unsigned int offset,
void (*callback) (struct device *dev,
struct talitos_desc *desc,
void *context, int error))
@@ -1835,9 +1848,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
sg_count = edesc->src_nents ?: 1;
if (is_sec1 && sg_count > 1)
- sg_pcopy_to_buffer(req_ctx->psrc, sg_count,
- edesc->buf + sizeof(struct talitos_desc),
- length, req_ctx->nbuf);
+ sg_copy_to_buffer(req_ctx->psrc, sg_count, edesc->buf, length);
else if (length)
sg_count = dma_map_sg(dev, req_ctx->psrc, sg_count,
DMA_TO_DEVICE);
@@ -1850,7 +1861,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
DMA_TO_DEVICE);
} else {
sg_count = talitos_sg_map(dev, req_ctx->psrc, length, edesc,
- &desc->ptr[3], sg_count, offset, 0);
+ &desc->ptr[3], sg_count, 0, 0);
if (sg_count > 1)
sync_needed = true;
}
@@ -1874,7 +1885,8 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
talitos_handle_buggy_hash(ctx, edesc, &desc->ptr[3]);
if (is_sec1 && req_ctx->nbuf && length) {
- struct talitos_desc *desc2 = desc + 1;
+ struct talitos_desc *desc2 = (struct talitos_desc *)
+ (edesc->buf + edesc->dma_len);
dma_addr_t next_desc;
memset(desc2, 0, sizeof(*desc2));
@@ -1895,7 +1907,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
DMA_TO_DEVICE);
copy_talitos_ptr(&desc2->ptr[2], &desc->ptr[2], is_sec1);
sg_count = talitos_sg_map(dev, req_ctx->psrc, length, edesc,
- &desc2->ptr[3], sg_count, offset, 0);
+ &desc2->ptr[3], sg_count, 0, 0);
if (sg_count > 1)
sync_needed = true;
copy_talitos_ptr(&desc2->ptr[5], &desc->ptr[5], is_sec1);
@@ -2006,7 +2018,6 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
struct device *dev = ctx->dev;
struct talitos_private *priv = dev_get_drvdata(dev);
bool is_sec1 = has_ftr_sec1(priv);
- int offset = 0;
u8 *ctx_buf = req_ctx->buf[req_ctx->buf_idx];
if (!req_ctx->last && (nbytes + req_ctx->nbuf <= blocksize)) {
@@ -2046,6 +2057,8 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
sg_chain(req_ctx->bufsl, 2, areq->src);
req_ctx->psrc = req_ctx->bufsl;
} else if (is_sec1 && req_ctx->nbuf && req_ctx->nbuf < blocksize) {
+ int offset;
+
if (nbytes_to_hash > blocksize)
offset = blocksize - req_ctx->nbuf;
else
@@ -2058,7 +2071,8 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
sg_copy_to_buffer(areq->src, nents,
ctx_buf + req_ctx->nbuf, offset);
req_ctx->nbuf += offset;
- req_ctx->psrc = areq->src;
+ req_ctx->psrc = scatterwalk_ffwd(req_ctx->bufsl, areq->src,
+ offset);
} else
req_ctx->psrc = areq->src;
@@ -2098,8 +2112,7 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
if (ctx->keylen && (req_ctx->first || req_ctx->last))
edesc->desc.hdr |= DESC_HDR_MODE0_MDEU_HMAC;
- return common_nonsnoop_hash(edesc, areq, nbytes_to_hash, offset,
- ahash_done);
+ return common_nonsnoop_hash(edesc, areq, nbytes_to_hash, ahash_done);
}
static int ahash_update(struct ahash_request *areq)
--
2.13.3
^ permalink raw reply related
* [PATCH v5 0/4] Additional fixes on Talitos driver
From: Christophe Leroy @ 2019-06-24 7:21 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, horia.geanta
Cc: linuxppc-dev, linux-crypto, linux-kernel
This series is the last set of fixes for the Talitos driver.
We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
[ 3.385197] bus: 'platform': really_probe: probing driver talitos with device ff020000.crypto
[ 3.450982] random: fast init done
[ 12.252548] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos-hsna)
[ 12.262226] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos-hsna)
[ 43.310737] Bug in SEC1, padding ourself
[ 45.603318] random: crng init done
[ 54.612333] talitos ff020000.crypto: fsl,sec1.2 algorithms registered in /proc/crypto
[ 54.620232] driver: 'talitos': driver_bound: bound to device 'ff020000.crypto'
[ 1.193721] bus: 'platform': really_probe: probing driver talitos with device b0030000.crypto
[ 1.229197] random: fast init done
[ 2.714920] alg: No test for authenc(hmac(sha224),cbc(aes)) (authenc-hmac-sha224-cbc-aes-talitos)
[ 2.724312] alg: No test for authenc(hmac(sha224),cbc(aes)) (authenc-hmac-sha224-cbc-aes-talitos-hsna)
[ 4.482045] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos)
[ 4.490940] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos-hsna)
[ 4.500280] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos)
[ 4.509727] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos-hsna)
[ 6.631781] random: crng init done
[ 11.521795] talitos b0030000.crypto: fsl,sec2.2 algorithms registered in /proc/crypto
[ 11.529803] driver: 'talitos': driver_bound: bound to device 'b0030000.crypto'
v2: dropped patch 1 which was irrelevant due to a rebase weirdness. Added Cc to stable on the 2 first patches.
v3:
- removed stable reference in patch 1
- reworded patch 1 to include name of patch 2 for the dependency.
- mentionned this dependency in patch 2 as well.
- corrected the Fixes: sha1 in patch 4
v4:
- using scatterwalk_ffwd() instead of opencodying SG list forwarding.
- Added a patch to fix sg_copy_to_buffer() when sg->offset() is greater than PAGE_SIZE,
otherwise sg_copy_to_buffer() fails when the list has been forwarded with scatterwalk_ffwd().
- taken the patch "crypto: talitos - eliminate unneeded 'done' functions at build time"
out of the series because it is independent.
- added a helper to find the header field associated to a request in flush_channe()
v5:
- Replacing the while loop by a direct shift/mask operation, as suggested by Herbert in patch 1.
Christophe Leroy (4):
lib/scatterlist: Fix mapping iterator when sg->offset is greater than
PAGE_SIZE
crypto: talitos - move struct talitos_edesc into talitos.h
crypto: talitos - fix hash on SEC1.
crypto: talitos - drop icv_ool
drivers/crypto/talitos.c | 102 +++++++++++++++++++----------------------------
drivers/crypto/talitos.h | 28 +++++++++++++
lib/scatterlist.c | 9 +++--
3 files changed, 74 insertions(+), 65 deletions(-)
--
2.13.3
^ permalink raw reply
* Re: remove asm-generic/ptrace.h v3
From: Thomas Gleixner @ 2019-06-24 7:23 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, Arnd Bergmann, linux-sh, linux-kernel, x86,
Oleg Nesterov, linux-mips, linuxppc-dev, linux-arm-kernel
In-Reply-To: <20190624054728.30966-1-hch@lst.de>
On Mon, 24 Jun 2019, Christoph Hellwig wrote:
>
> asm-generic/ptrace.h is a little weird in that it doesn't actually
> implement any functionality, but it provided multiple layers of macros
> that just implement trivial inline functions. We implement those
> directly in the few architectures and be off with a much simpler
> design.
>
> I'm not sure which tree is the right place, but may this can go through
> the asm-generic tree since it removes an asm-generic header?
Makes sense.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH 4/5] x86: don't use asm-generic/ptrace.h
From: Thomas Gleixner @ 2019-06-24 7:22 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, Arnd Bergmann, linux-sh, linux-kernel, x86,
Oleg Nesterov, linux-mips, linuxppc-dev, Ingo Molnar,
linux-arm-kernel
In-Reply-To: <20190624054728.30966-5-hch@lst.de>
On Mon, 24 Jun 2019, Christoph Hellwig wrote:
> Doing the indirection through macros for the regs accessors just
> makes them harder to read, so implement the helpers directly.
>
> Note that only the helpers actually used are implemented now.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Acked-by: Ingo Molnar <mingo@kernel.org>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/64s: Rename PPC_INVALIDATE_ERAT to PPC_ARCH_300_INVALIDATE_ERAT
From: Michael Ellerman @ 2019-06-24 11:39 UTC (permalink / raw)
To: Nicholas Piggin, Segher Boessenkool; +Cc: linuxppc-dev, kvm-ppc
In-Reply-To: <1561297021.pyb7y0yjt7.astroid@bobo.none>
Nicholas Piggin <npiggin@gmail.com> writes:
> Segher Boessenkool's on June 23, 2019 10:03 pm:
>> On Sun, Jun 23, 2019 at 08:41:51PM +1000, Nicholas Piggin wrote:
>>> This makes it clear to the caller that it can only be used on POWER9
>>> and later CPUs.
>>
>>> -#define PPC_INVALIDATE_ERAT PPC_SLBIA(7)
>>> +#define PPC_ARCH_300_INVALIDATE_ERAT PPC_SLBIA(7)
>>
>> The architecture version is 3.0 (or 3.0B), not "300". This will work on
>> implementations of later architecture versions as well, so maybe the name
>> isn't so great anyway?
>
> Yeah... this is kernel convention for better or worse. ISA v3.0B
> feature support is called CPU_FTR_ARCH_300, and later architectures
> will advertise that support. For the most part we can use architected
> features (incompatible changes would require additional code).
I'd rather we used 3_0B or something inside the kernel, but I'm not sure
it's worth the churn to rename the existing feature everywhere. And we
can't rename the user visible feature.
But if you're adding a new usage then I'd prefer: PPC_ISA_3_0B_INVALIDATE_ERAT
I dislike "300" because it implies we support ISA v3.0 but we actually
don't, we only support v3.0B.
cheers
^ permalink raw reply
* Re: [PATCH] KVM: PPC: Book3S HV: Fix CR0 setting in TM emulation
From: Michael Ellerman @ 2019-06-24 11:48 UTC (permalink / raw)
To: Michael Neuling; +Cc: mikey, linuxppc-dev, sjitindarsingh, kvm-ppc
In-Reply-To: <20190620060040.26945-1-mikey@neuling.org>
Michael Neuling <mikey@neuling.org> writes:
> When emulating tsr, treclaim and trechkpt, we incorrectly set CR0. The
> code currently sets:
> CR0 <- 00 || MSR[TS]
> but according to the ISA it should be:
> CR0 <- 0 || MSR[TS] || 0
Seems bad, what's the worst case impact?
Do we have a test case for this?
> This fixes the bit shift to put the bits in the correct location.
Fixes: ?
cheers
> diff --git a/arch/powerpc/kvm/book3s_hv_tm.c b/arch/powerpc/kvm/book3s_hv_tm.c
> index 888e2609e3..31cd0f327c 100644
> --- a/arch/powerpc/kvm/book3s_hv_tm.c
> +++ b/arch/powerpc/kvm/book3s_hv_tm.c
> @@ -131,7 +131,7 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
> }
> /* Set CR0 to indicate previous transactional state */
> vcpu->arch.regs.ccr = (vcpu->arch.regs.ccr & 0x0fffffff) |
> - (((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 28);
> + (((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 29);
> /* L=1 => tresume, L=0 => tsuspend */
> if (instr & (1 << 21)) {
> if (MSR_TM_SUSPENDED(msr))
> @@ -175,7 +175,7 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
>
> /* Set CR0 to indicate previous transactional state */
> vcpu->arch.regs.ccr = (vcpu->arch.regs.ccr & 0x0fffffff) |
> - (((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 28);
> + (((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 29);
> vcpu->arch.shregs.msr &= ~MSR_TS_MASK;
> return RESUME_GUEST;
>
> @@ -205,7 +205,7 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
>
> /* Set CR0 to indicate previous transactional state */
> vcpu->arch.regs.ccr = (vcpu->arch.regs.ccr & 0x0fffffff) |
> - (((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 28);
> + (((msr & MSR_TS_MASK) >> MSR_TS_S_LG) << 29);
> vcpu->arch.shregs.msr = msr | MSR_TS_S;
> return RESUME_GUEST;
> }
> --
> 2.21.0
^ permalink raw reply
* [next][PowerPC] RCU stalls while booting linux-next on PowerVM LPAR
From: Sachin Sant @ 2019-06-24 14:09 UTC (permalink / raw)
To: linuxppc-dev, david, Stephen Rothwell; +Cc: linux-next, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 3443 bytes --]
Latest -next fails to boot on POWER9 PowerVM LPAR due to RCU stalls.
This problem was introduced with next-20190620 (dc636f5d78).
next-20190619 was last good kernel.
Reverting following commit allows the kernel to boot.
2fd4aeea6b603 : mm/memory_hotplug: move and simplify walk_memory_blocks()
[ 0.014409] Using shared cache scheduler topology
[ 0.016302] devtmpfs: initialized
[ 0.031022] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns
[ 0.031034] futex hash table entries: 16384 (order: 5, 2097152 bytes, linear)
[ 0.031575] NET: Registered protocol family 16
[ 0.031724] audit: initializing netlink subsys (disabled)
[ 0.031796] audit: type=2000 audit(1561344029.030:1): state=initialized audit_enabled=0 res=1
[ 0.032249] cpuidle: using governor menu
[ 0.032403] pstore: Registered nvram as persistent store backend
[ 60.061246] rcu: INFO: rcu_sched self-detected stall on CPU
[ 60.061254] rcu: 0-....: (5999 ticks this GP) idle=1ea/1/0x4000000000000002 softirq=5/5 fqs=2999
[ 60.061261] (t=6000 jiffies g=-1187 q=0)
[ 60.061265] NMI backtrace for cpu 0
[ 60.061269] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc5-next-20190621-autotest-autotest #1
[ 60.061275] Call Trace:
[ 60.061280] [c0000018ee85f380] [c000000000b624ec] dump_stack+0xb0/0xf4 (unreliable)
[ 60.061287] [c0000018ee85f3c0] [c000000000b6d464] nmi_cpu_backtrace+0x144/0x150
[ 60.061293] [c0000018ee85f450] [c000000000b6d61c] nmi_trigger_cpumask_backtrace+0x1ac/0x1f0
[ 60.061300] [c0000018ee85f4f0] [c0000000000692c8] arch_trigger_cpumask_backtrace+0x28/0x40
[ 60.061306] [c0000018ee85f510] [c0000000001c5f90] rcu_dump_cpu_stacks+0x10c/0x16c
[ 60.061313] [c0000018ee85f560] [c0000000001c4fe4] rcu_sched_clock_irq+0x744/0x990
[ 60.061318] [c0000018ee85f630] [c0000000001d5b58] update_process_times+0x48/0x90
[ 60.061325] [c0000018ee85f660] [c0000000001ea03c] tick_periodic+0x4c/0x120
[ 60.061330] [c0000018ee85f690] [c0000000001ea150] tick_handle_periodic+0x40/0xe0
[ 60.061336] [c0000018ee85f6d0] [c00000000002b5cc] timer_interrupt+0x10c/0x2e0
[ 60.061342] [c0000018ee85f730] [c000000000009204] decrementer_common+0x134/0x140
[ 60.061350] --- interrupt: 901 at replay_interrupt_return+0x0/0x4
[ 60.061350] LR = arch_local_irq_restore+0x84/0x90
[ 60.061357] [c0000018ee85fa30] [c0000018ee85fbac] 0xc0000018ee85fbac (unreliable)
[ 60.061364] [c0000018ee85fa50] [c000000000b88300] _raw_spin_unlock_irqrestore+0x50/0x80
[ 60.061369] [c0000018ee85fa70] [c000000000b69da4] klist_next+0xb4/0x150
[ 60.061376] [c0000018ee85fac0] [c000000000766ea0] subsys_find_device_by_id+0xf0/0x1a0
[ 60.061382] [c0000018ee85fb20] [c000000000797a94] walk_memory_blocks+0x84/0x100
[ 60.061388] [c0000018ee85fb80] [c000000000795ea0] link_mem_sections+0x40/0x60
[ 60.061395] [c0000018ee85fbb0] [c000000000f28c28] topology_init+0xa0/0x268
[ 60.061400] [c0000018ee85fc10] [c000000000010448] do_one_initcall+0x68/0x2c0
[ 60.061406] [c0000018ee85fce0] [c000000000f247dc] kernel_init_freeable+0x318/0x47c
[ 60.061411] [c0000018ee85fdb0] [c0000000000107c4] kernel_init+0x24/0x150
[ 60.061417] [c0000018ee85fe20] [c00000000000ba54] ret_from_kernel_thread+0x5c/0x68
[ 88.016563] watchdog: BUG: soft lockup - CPU#0 stuck for 22s! [swapper/0:1]
[ 88.016569] Modules linked in:
Thanks
-Sachin
[-- Attachment #2: boot.log --]
[-- Type: application/octet-stream, Size: 11416 bytes --]
[ 0.000000] hash-mmu: Page sizes from device-tree:
[ 0.000000] hash-mmu: base_shift=12: shift=12, sllp=0x0000, avpnm=0x00000000, tlbiel=1, penc=0
[ 0.000000] hash-mmu: base_shift=12: shift=16, sllp=0x0000, avpnm=0x00000000, tlbiel=1, penc=7
[ 0.000000] hash-mmu: base_shift=12: shift=24, sllp=0x0000, avpnm=0x00000000, tlbiel=1, penc=56
[ 0.000000] hash-mmu: base_shift=16: shift=16, sllp=0x0110, avpnm=0x00000000, tlbiel=1, penc=1
[ 0.000000] hash-mmu: base_shift=16: shift=24, sllp=0x0110, avpnm=0x00000000, tlbiel=1, penc=8
[ 0.000000] hash-mmu: base_shift=24: shift=24, sllp=0x0100, avpnm=0x00000001, tlbiel=0, penc=0
[ 0.000000] hash-mmu: base_shift=34: shift=34, sllp=0x0120, avpnm=0x000007ff, tlbiel=0, penc=3
[ 0.000000] Using 1TB segments
[ 0.000000] hash-mmu: Initializing hash mmu with SLB
[ 0.000000] Linux version 5.2.0-rc5-next-20190620-autotest (root@ltczep10-lp3.aus.stglabs.ibm.com) (gcc version 4.8.5 20150623 (Red Hat 4.8.5-36) (GCC)) #1 SMP Mon Jun 24 06:38:12 PDT 2019
[ 0.000000] Found initrd at 0xc0000000032b0000:0xc0000000044c4efc
[ 0.000000] Using pSeries machine description
[ 0.000000] printk: bootconsole [udbg0] enabled
[ 0.000000] Partition configured for 40 cpus.
[ 0.000000] CPU maps initialized for 8 threads per core
[ 0.000000] -----------------------------------------------------
[ 0.000000] phys_mem_size = 0x1900000000
[ 0.000000] dcache_bsize = 0x80
[ 0.000000] icache_bsize = 0x80
[ 0.000000] cpu_features = 0x0000c07f8f5f91a7
[ 0.000000] possible = 0x0000fbffcf5fb1a7
[ 0.000000] always = 0x00000003800081a1
[ 0.000000] cpu_user_features = 0xdc0065c2 0xefe00000
[ 0.000000] mmu_features = 0x7c006001
[ 0.000000] firmware_features = 0x00000013c45bfc57
[ 0.000000] hash-mmu: ppc64_pft_size = 0x1e
[ 0.000000] hash-mmu: htab_hash_mask = 0x7fffff
[ 0.000000] hash-mmu: kernel vmalloc start = 0xc008000000000000
[ 0.000000] hash-mmu: kernel IO start = 0xc00a000000000000
[ 0.000000] hash-mmu: kernel vmemmap start = 0xc00c000000000000
[ 0.000000] -----------------------------------------------------
[ 0.000000] numa: NODE_DATA [mem 0x18ffe98980-0x18ffe9ffff]
[ 0.000000] numa: NODE_DATA(0) on node 3
[ 0.000000] numa: NODE_DATA [mem 0x18ffe91300-0x18ffe9897f]
[ 0.000000] rfi-flush: fallback displacement flush available
[ 0.000000] rfi-flush: mttrig type flush available
[ 0.000000] count-cache-flush: full software flush sequence enabled.
[ 0.000000] stf-barrier: eieio barrier available
[ 0.000000] PPC64 nvram contains 15360 bytes
[ 0.000000] barrier-nospec: using ORI speculation barrier
[ 0.000000] Zone ranges:
[ 0.000000] Normal [mem 0x0000000000000000-0x00000018ffffffff]
[ 0.000000] Device empty
[ 0.000000] Movable zone start for each node
[ 0.000000] Early memory node ranges
[ 0.000000] node 3: [mem 0x0000000000000000-0x00000018ffffffff]
[ 0.000000] Initmem setup node 0 [mem 0x0000000000000000-0x0000000000000000]
[ 0.000000] Initmem setup node 3 [mem 0x0000000000000000-0x00000018ffffffff]
[ 0.000000] percpu: Embedded 11 pages/cpu s631960 r0 d88936 u1048576
[ 0.000000] node[0] zonelist: 3:Normal
[ 0.000000] node[3] zonelist: 3:Normal
[ 0.000000] Built 2 zonelists, mobility grouping on. Total pages: 1636800
[ 0.000000] Policy zone: Normal
[ 0.000000] Kernel command line: root=UUID=5b503143-3264-4a73-bd1b-2b1cd7d250f6
[ 0.000000] Dentry cache hash table entries: 8388608 (order: 10, 67108864 bytes, linear)
[ 0.000000] Inode-cache hash table entries: 4194304 (order: 9, 33554432 bytes, linear)
[ 0.000000] mem auto-init: stack:off, heap alloc:off, heap free:off
[ 0.000000] Memory: 104547200K/104857600K available (11840K kernel code, 1792K rwdata, 3584K rodata, 4992K init, 3850K bss, 310400K reserved, 0K cma-reserved)
[ 0.000000] random: get_random_u64 called from cache_random_seq_create+0xa4/0x1c0 with crng_init=0
[ 0.000000] SLUB: HWalign=128, Order=0-3, MinObjects=0, CPUs=40, Nodes=32
[ 0.000000] ftrace: allocating 31746 entries in 12 pages
[ 0.000000] rcu: Hierarchical RCU implementation.
[ 0.000000] rcu: RCU restricting CPUs from NR_CPUS=2048 to nr_cpu_ids=40.
[ 0.000000] rcu: RCU calculated value of scheduler-enlistment delay is 10 jiffies.
[ 0.000000] rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=40
[ 0.000000] NR_IRQS: 512, nr_irqs: 512, preallocated irqs: 16
[ 0.000000] rcu: Offload RCU callbacks from CPUs: (none).
[ 0.000002] time_init: 56 bit decrementer (max: 7fffffffffffff)
[ 0.000067] clocksource: timebase: mask: 0xffffffffffffffff max_cycles: 0x761537d007, max_idle_ns: 440795202126 ns
[ 0.000178] clocksource: timebase mult[1f40000] shift[24] registered
[ 0.000312] Console: colour dummy device 80x25
[ 0.000363] printk: console [hvc0] enabled
[ 0.000363] printk: console [hvc0] enabled
[ 0.000411] printk: bootconsole [udbg0] disabled
[ 0.000411] printk: bootconsole [udbg0] disabled
[ 0.000508] mempolicy: Enabling automatic NUMA balancing. Configure with numa_balancing= or the kernel.numa_balancing sysctl
[ 0.000521] pid_max: default: 40960 minimum: 320
[ 0.000673] LSM: Security Framework initializing
[ 0.000759] Yama: becoming mindful.
[ 0.000781] SELinux: Initializing.
[ 0.000944] *** VALIDATE SELinux ***
[ 0.001035] Mount-cache hash table entries: 131072 (order: 4, 1048576 bytes, linear)
[ 0.001073] Mountpoint-cache hash table entries: 131072 (order: 4, 1048576 bytes, linear)
[ 0.001370] *** VALIDATE proc ***
[ 0.001623] *** VALIDATE cgroup1 ***
[ 0.001629] *** VALIDATE cgroup2 ***
[ 0.002225] EEH: pSeries platform initialized
[ 0.002233] POWER9 performance monitor hardware support registered
[ 0.002272] rcu: Hierarchical SRCU implementation.
[ 0.003078] smp: Bringing up secondary CPUs ...
[ 0.014555] smp: Brought up 2 nodes, 40 CPUs
[ 0.014564] numa: Node 0 CPUs:
[ 0.014567] numa: Node 3 CPUs: 0-39
[ 0.014571] Using small cores at SMT level
[ 0.014575] Using shared cache scheduler topology
[ 0.016465] devtmpfs: initialized
[ 0.031046] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns
[ 0.031058] futex hash table entries: 16384 (order: 5, 2097152 bytes, linear)
[ 0.031602] NET: Registered protocol family 16
[ 0.031751] audit: initializing netlink subsys (disabled)
[ 0.031822] audit: type=2000 audit(1561384091.030:1): state=initialized audit_enabled=0 res=1
[ 0.032275] cpuidle: using governor menu
[ 0.032429] pstore: Registered nvram as persistent store backend
[ 60.061325] rcu: INFO: rcu_sched self-detected stall on CPU
[ 60.061333] rcu: 0-....: (5999 ticks this GP) idle=1ca/1/0x4000000000000002 softirq=5/5 fqs=2999
[ 60.061340] (t=6000 jiffies g=-1187 q=0)
[ 60.061343] NMI backtrace for cpu 0
[ 60.061349] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc5-next-20190620-autotest #1
[ 60.061354] Call Trace:
[ 60.061358] [c0000018ee813360] [c000000000b625cc] dump_stack+0xb0/0xf4 (unreliable)
[ 60.061365] [c0000018ee8133a0] [c000000000b6d544] nmi_cpu_backtrace+0x144/0x150
[ 60.061371] [c0000018ee813430] [c000000000b6d6fc] nmi_trigger_cpumask_backtrace+0x1ac/0x1f0
[ 60.061378] [c0000018ee8134d0] [c0000000000692c8] arch_trigger_cpumask_backtrace+0x28/0x40
[ 60.061385] [c0000018ee8134f0] [c0000000001c6210] rcu_dump_cpu_stacks+0x10c/0x16c
[ 60.061391] [c0000018ee813540] [c0000000001c5264] rcu_sched_clock_irq+0x744/0x990
[ 60.061397] [c0000018ee813610] [c0000000001d5e38] update_process_times+0x48/0x90
[ 60.061403] [c0000018ee813640] [c0000000001ea31c] tick_periodic+0x4c/0x120
[ 60.061408] [c0000018ee813670] [c0000000001ea430] tick_handle_periodic+0x40/0xe0
[ 60.061414] [c0000018ee8136b0] [c00000000002b5cc] timer_interrupt+0x10c/0x2e0
[ 60.061420] [c0000018ee813710] [c000000000009204] decrementer_common+0x134/0x140
[ 60.061427] --- interrupt: 901 at replay_interrupt_return+0x0/0x4
[ 60.061427] LR = arch_local_irq_restore+0x84/0x90
[ 60.061434] [c0000018ee813a10] [c0000018ee813bac] 0xc0000018ee813bac (unreliable)
[ 60.061441] [c0000018ee813a30] [c000000000b883e0] _raw_spin_unlock_irqrestore+0x50/0x80
[ 60.061447] [c0000018ee813a50] [c000000000b69e84] klist_next+0xb4/0x150
[ 60.061453] [c0000018ee813aa0] [c000000000766f60] subsys_find_device_by_id+0xf0/0x1a0
[ 60.061459] [c0000018ee813b00] [c000000000796464] find_memory_block_by_id+0x94/0xb0
[ 60.061466] [c0000018ee813b30] [c000000000797b1c] walk_memory_blocks+0x7c/0xf0
[ 60.061472] [c0000018ee813b80] [c000000000795f60] link_mem_sections+0x40/0x60
[ 60.061478] [c0000018ee813bb0] [c000000000f28c28] topology_init+0xa0/0x268
[ 60.061483] [c0000018ee813c10] [c000000000010448] do_one_initcall+0x68/0x2c0
[ 60.061489] [c0000018ee813ce0] [c000000000f247dc] kernel_init_freeable+0x318/0x47c
[ 60.061495] [c0000018ee813db0] [c0000000000107c4] kernel_init+0x24/0x150
[ 60.061500] [c0000018ee813e20] [c00000000000ba54] ret_from_kernel_thread+0x5c/0x68
[ 88.016703] watchdog: BUG: soft lockup - CPU#0 stuck for 22s! [swapper/0:1]
[ 88.016709] Modules linked in:
[ 88.016713] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc5-next-20190620-autotest #1
[ 88.016718] NIP: c00000000000ab8c LR: c00000000001b434 CTR: 0000000000000000
[ 88.016724] REGS: c0000018ee813780 TRAP: 0901 Not tainted (5.2.0-rc5-next-20190620-autotest)
[ 88.016730] MSR: 8000000002009033 <SF,VEC,EE,ME,IR,DR,RI,LE> CR: 44008484 XER: 00000005
[ 88.016738] CFAR: c000000000b69e30 IRQMASK: 0
[ 88.016738] GPR00: c000000000b883e0 c0000018ee813a10 c000000001544d00 0000000000000900
[ 88.016738] GPR04: 0000000000000000 0000000000000000 0000000000000001 0000000000000004
[ 88.016738] GPR08: 0000000000000000 c0000018eb436d80 0000000000000001 0000000000000220
[ 88.016738] GPR12: 0000000000000000 c000000001990000
[ 88.016761] NIP [c00000000000ab8c] replay_interrupt_return+0x0/0x4
[ 88.016767] LR [c00000000001b434] arch_local_irq_restore+0x84/0x90
[ 88.016771] Call Trace:
[ 88.016774] [c0000018ee813a10] [c0000018ee813bac] 0xc0000018ee813bac (unreliable)
[ 88.016780] [c0000018ee813a30] [c000000000b883e0] _raw_spin_unlock_irqrestore+0x50/0x80
[ 88.016786] [c0000018ee813a50] [c000000000b69e84] klist_next+0xb4/0x150
[ 88.016792] [c0000018ee813aa0] [c000000000766f60] subsys_find_device_by_id+0xf0/0x1a0
[ 88.016798] [c0000018ee813b00] [c000000000796464] find_memory_block_by_id+0x94/0xb0
[ 88.016804] [c0000018ee813b30] [c000000000797b1c] walk_memory_blocks+0x7c/0xf0
[ 88.016810] [c0000018ee813b80] [c000000000795f60] link_mem_sections+0x40/0x60
[ 88.016816] [c0000018ee813bb0] [c000000000f28c28] topology_init+0xa0/0x268
[ 88.016821] [c0000018ee813c10] [c000000000010448] do_one_initcall+0x68/0x2c0
[ 88.016827] [c0000018ee813ce0] [c000000000f247dc] kernel_init_freeable+0x318/0x47c
[ 88.016833] [c0000018ee813db0] [c0000000000107c4] kernel_init+0x24/0x150
[ 88.016838] [c0000018ee813e20] [c00000000000ba54] ret_from_kernel_thread+0x5c/0x68
[ 88.016843] Instruction dump:
[ 88.016847] 7d200026 618c8000 2c030900 4182e568 2c030500 4182e010 2c030f00 4182f2d8
[ 88.016854] 2c030a00 4182ffb8 60000000 60000000 <4e800020> 7c781b78 480003e1 480003f9
^ permalink raw reply
* [PATCH] ocxl: Fix concurrent AFU open and device removal
From: Frederic Barrat @ 2019-06-24 14:41 UTC (permalink / raw)
To: linuxppc-dev, andrew.donnellan, clombard, groug, alastair
If an ocxl device is unbound through sysfs at the same time its AFU is
being opened by a user process, the open code may dereference freed
stuctures, which can lead to kernel oops messages. You'd have to hit a
tiny time window, but it's possible. It's fairly easy to test by
making the time window bigger artificially.
Fix it with a combination of 2 changes:
- when an AFU device is found in the IDR by looking for the device
minor number, we should hold a reference on the device until after the
context is allocated. A reference on the AFU structure is kept when
the context is allocated, so we can release the reference on the
device after the context allocation.
- with the fix above, there's still another even tinier window,
between the time the AFU device is found in the IDR and the reference
on the device is taken. We can fix this one by removing the IDR entry
earlier, when the device setup is removed, instead of waiting for the
'release' device callback. With proper locking around the IDR.
Fixes: 75ca758adbaf ("ocxl: Create a clear delineation between ocxl backend & frontend")
Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
---
mpe: this fixes a commit merged in v5.2-rc1. It's late, and I don't think it's that important. If it's for the next merge window, I would add:
Cc: stable@vger.kernel.org # v5.2
drivers/misc/ocxl/file.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
index 2870c25da166..4d1b44de1492 100644
--- a/drivers/misc/ocxl/file.c
+++ b/drivers/misc/ocxl/file.c
@@ -18,18 +18,15 @@ static struct class *ocxl_class;
static struct mutex minors_idr_lock;
static struct idr minors_idr;
-static struct ocxl_file_info *find_file_info(dev_t devno)
+static struct ocxl_file_info *find_and_get_file_info(dev_t devno)
{
struct ocxl_file_info *info;
- /*
- * We don't declare an RCU critical section here, as our AFU
- * is protected by a reference counter on the device. By the time the
- * info reference is removed from the idr, the ref count of
- * the device is already at 0, so no user API will access that AFU and
- * this function can't return it.
- */
+ mutex_lock(&minors_idr_lock);
info = idr_find(&minors_idr, MINOR(devno));
+ if (info)
+ get_device(&info->dev);
+ mutex_unlock(&minors_idr_lock);
return info;
}
@@ -58,14 +55,16 @@ static int afu_open(struct inode *inode, struct file *file)
pr_debug("%s for device %x\n", __func__, inode->i_rdev);
- info = find_file_info(inode->i_rdev);
+ info = find_and_get_file_info(inode->i_rdev);
if (!info)
return -ENODEV;
rc = ocxl_context_alloc(&ctx, info->afu, inode->i_mapping);
- if (rc)
+ if (rc) {
+ put_device(&info->dev);
return rc;
-
+ }
+ put_device(&info->dev);
file->private_data = ctx;
return 0;
}
@@ -487,7 +486,6 @@ static void info_release(struct device *dev)
{
struct ocxl_file_info *info = container_of(dev, struct ocxl_file_info, dev);
- free_minor(info);
ocxl_afu_put(info->afu);
kfree(info);
}
@@ -577,6 +575,7 @@ void ocxl_file_unregister_afu(struct ocxl_afu *afu)
ocxl_file_make_invisible(info);
ocxl_sysfs_unregister_afu(info);
+ free_minor(info);
device_unregister(&info->dev);
}
--
2.21.0
^ permalink raw reply related
* Re: [next][PowerPC] RCU stalls while booting linux-next on PowerVM LPAR
From: David Hildenbrand @ 2019-06-24 14:42 UTC (permalink / raw)
To: Sachin Sant, linuxppc-dev, Stephen Rothwell; +Cc: linux-next, linux-kernel
In-Reply-To: <0F3EAC10-0EA8-4097-99F2-BC42D92291FE@linux.vnet.ibm.com>
On 24.06.19 16:09, Sachin Sant wrote:
> Latest -next fails to boot on POWER9 PowerVM LPAR due to RCU stalls.
>
> This problem was introduced with next-20190620 (dc636f5d78).
> next-20190619 was last good kernel.
>
> Reverting following commit allows the kernel to boot.
> 2fd4aeea6b603 : mm/memory_hotplug: move and simplify walk_memory_blocks()
>
>
> [ 0.014409] Using shared cache scheduler topology
> [ 0.016302] devtmpfs: initialized
> [ 0.031022] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns
> [ 0.031034] futex hash table entries: 16384 (order: 5, 2097152 bytes, linear)
> [ 0.031575] NET: Registered protocol family 16
> [ 0.031724] audit: initializing netlink subsys (disabled)
> [ 0.031796] audit: type=2000 audit(1561344029.030:1): state=initialized audit_enabled=0 res=1
> [ 0.032249] cpuidle: using governor menu
> [ 0.032403] pstore: Registered nvram as persistent store backend
> [ 60.061246] rcu: INFO: rcu_sched self-detected stall on CPU
> [ 60.061254] rcu: 0-....: (5999 ticks this GP) idle=1ea/1/0x4000000000000002 softirq=5/5 fqs=2999
> [ 60.061261] (t=6000 jiffies g=-1187 q=0)
> [ 60.061265] NMI backtrace for cpu 0
> [ 60.061269] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc5-next-20190621-autotest-autotest #1
> [ 60.061275] Call Trace:
> [ 60.061280] [c0000018ee85f380] [c000000000b624ec] dump_stack+0xb0/0xf4 (unreliable)
> [ 60.061287] [c0000018ee85f3c0] [c000000000b6d464] nmi_cpu_backtrace+0x144/0x150
> [ 60.061293] [c0000018ee85f450] [c000000000b6d61c] nmi_trigger_cpumask_backtrace+0x1ac/0x1f0
> [ 60.061300] [c0000018ee85f4f0] [c0000000000692c8] arch_trigger_cpumask_backtrace+0x28/0x40
> [ 60.061306] [c0000018ee85f510] [c0000000001c5f90] rcu_dump_cpu_stacks+0x10c/0x16c
> [ 60.061313] [c0000018ee85f560] [c0000000001c4fe4] rcu_sched_clock_irq+0x744/0x990
> [ 60.061318] [c0000018ee85f630] [c0000000001d5b58] update_process_times+0x48/0x90
> [ 60.061325] [c0000018ee85f660] [c0000000001ea03c] tick_periodic+0x4c/0x120
> [ 60.061330] [c0000018ee85f690] [c0000000001ea150] tick_handle_periodic+0x40/0xe0
> [ 60.061336] [c0000018ee85f6d0] [c00000000002b5cc] timer_interrupt+0x10c/0x2e0
> [ 60.061342] [c0000018ee85f730] [c000000000009204] decrementer_common+0x134/0x140
> [ 60.061350] --- interrupt: 901 at replay_interrupt_return+0x0/0x4
> [ 60.061350] LR = arch_local_irq_restore+0x84/0x90
> [ 60.061357] [c0000018ee85fa30] [c0000018ee85fbac] 0xc0000018ee85fbac (unreliable)
> [ 60.061364] [c0000018ee85fa50] [c000000000b88300] _raw_spin_unlock_irqrestore+0x50/0x80
> [ 60.061369] [c0000018ee85fa70] [c000000000b69da4] klist_next+0xb4/0x150
> [ 60.061376] [c0000018ee85fac0] [c000000000766ea0] subsys_find_device_by_id+0xf0/0x1a0
> [ 60.061382] [c0000018ee85fb20] [c000000000797a94] walk_memory_blocks+0x84/0x100
> [ 60.061388] [c0000018ee85fb80] [c000000000795ea0] link_mem_sections+0x40/0x60
> [ 60.061395] [c0000018ee85fbb0] [c000000000f28c28] topology_init+0xa0/0x268
> [ 60.061400] [c0000018ee85fc10] [c000000000010448] do_one_initcall+0x68/0x2c0
> [ 60.061406] [c0000018ee85fce0] [c000000000f247dc] kernel_init_freeable+0x318/0x47c
> [ 60.061411] [c0000018ee85fdb0] [c0000000000107c4] kernel_init+0x24/0x150
> [ 60.061417] [c0000018ee85fe20] [c00000000000ba54] ret_from_kernel_thread+0x5c/0x68
> [ 88.016563] watchdog: BUG: soft lockup - CPU#0 stuck for 22s! [swapper/0:1]
> [ 88.016569] Modules linked in:
>
Hi, thanks! Please see
https://lkml.org/lkml/2019/6/21/600
and especially
https://lkml.org/lkml/2019/6/21/908
Does this fix your problem? The fix is on its way to next.
Cheers!
--
Thanks,
David / dhildenb
^ permalink raw reply
* CVE-2019-12817: Linux kernel: powerpc: Unrelated processes may be able to read/write to each other's virtual memory
From: Michael Ellerman @ 2019-06-24 14:44 UTC (permalink / raw)
To: oss-security; +Cc: linuxppc-dev, linux-kernel, linuxppc-users
[-- Attachment #1: Type: text/plain, Size: 4251 bytes --]
The Linux kernel for powerpc since 4.17 has a bug where unrelated processes may
be able to read/write to each other's virtual memory under certain conditions.
This bug only affects machines using 64-bit CPUs with the hash page table MMU,
see below for more detail on affected CPUs.
To trigger the bug a process must allocate memory above 512TB. That only happens
if userspace explicitly requests it with mmap(). That process must then fork(),
at this point the child incorrectly inherits the "context id" of the parent
associated with the mapping above 512TB. It may then be possible for the
parent/child to write to each other's mappings above 512TB, which should not be
possible, and constitutes memory corruption.
If instead the child process exits, all its context ids are freed, including the
context id that is still in use by the parent for the mapping above 512TB. That
id can then be reallocated to a third process, that process can then read/write
to the parent's mapping above 512TB. Additionally if the freed id is used for
the third process's primary context id, then the parent is able to read/write to
the third process's mappings *below* 512TB.
If the parent and child both exit before another process is allocated the freed
context id, the kernel will notice the double free of the id and print a warning
such as:
ida_free called for id=103 which is not allocated.
WARNING: CPU: 8 PID: 7293 at lib/idr.c:520 ida_free_rc+0x1b4/0x1d0
The bug was introduced in commit:
f384796c40dc ("powerpc/mm: Add support for handling > 512TB address in SLB miss")
Which was originally merged in v4.17.
Only machines using the hash page table (HPT) MMU are affected, eg. PowerPC 970
(G5), PA6T, Power5/6/7/8/9. By default Power9 bare metal machines (powernv) use
the Radix MMU and are not affected, unless the machine has been explicitly
booted in HPT mode (using disable_radix on the kernel command line). KVM guests
on Power9 may be affected if the host or guest is configured to use the HPT MMU.
LPARs under PowerVM on Power9 are affected as they always use the HPT MMU.
Kernels built with PAGE_SIZE=4K are not affected.
The upstream fix is here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ca72d88378b2f2444d3ec145dd442d449d3fefbc
There's also a kernel selftest to verify the fix:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=16391bfc862342f285195013b73c1394fab28b97
Or a similar standalone version is included below.
cheers
cat > test.c <<EOF
#undef NDEBUG
#include <assert.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/mman.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>
#ifndef MAP_FIXED_NOREPLACE
#define MAP_FIXED_NOREPLACE MAP_FIXED // "Should be safe" above 512TB
#endif
int main(void)
{
int p2c[2], c2p[2], rc, status, c, *p;
unsigned long page_size;
pid_t pid;
page_size = sysconf(_SC_PAGESIZE);
if (page_size != 65536) {
printf("Unsupported page size - not affected\n");
return 1;
}
// Create a mapping at 512TB to allocate an extended_id
p = mmap((void *)(512ul << 40), page_size, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED_NOREPLACE, -1, 0);
if (p == MAP_FAILED) {
perror("mmap");
printf("Error: couldn't mmap(), confirm kernel has 4TB support\n");
return 1;
}
printf("parent writing %p = 1\n", p);
*p = 1;
assert(pipe(p2c) != -1 && pipe(c2p) != -1);
pid = fork();
if (pid == 0) {
close(p2c[1]);
close(c2p[0]);
assert(read(p2c[0], &c, 1) == 1);
pid = getpid();
printf("child writing %p = %d\n", p, pid);
*p = pid;
assert(write(c2p[1], &c, 1) == 1);
assert(read(p2c[0], &c, 1) == 1);
exit(0);
}
close(p2c[0]);
close(c2p[1]);
c = 0;
assert(write(p2c[1], &c, 1) == 1);
assert(read(c2p[0], &c, 1) == 1);
// Prevent compiler optimisation
asm volatile("" : : : "memory");
rc = 0;
printf("parent reading %p = %d\n", p, *p);
if (*p != 1) {
printf("Error: BUG! parent saw child's write! *p = %d\n", *p);
rc = 1;
}
assert(write(p2c[1], &c, 1) == 1);
assert(waitpid(pid, &status, 0) != -1);
assert(WIFEXITED(status) && WEXITSTATUS(status) == 0);
if (rc == 0)
printf("success: test completed OK\n");
return rc;
}
EOF
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* [PATCH 1/2] powerpc/papr_scm: Update drc_pmem_unbind() to use H_SCM_UNBIND_ALL
From: Vaibhav Jain @ 2019-06-24 14:59 UTC (permalink / raw)
To: linuxppc-dev
Cc: Aneesh Kumar K . V, Oliver O'Halloran, Vaibhav Jain,
Laurent Dufour, David Gibson
In-Reply-To: <20190624145913.20122-1-vaibhav@linux.ibm.com>
The new hcall named H_SCM_UNBIND_ALL has been introduce that can
unbind all the memory drc memory-blocks assigned to an lpar. This is
more efficient than using H_SCM_UNBIND_MEM as currently we don't
support partial unbind of drc memory-blocks.
Hence this patch proposes following changes to drc_pmem_unbind():
* Update drc_pmem_unbind() to replace hcall H_SCM_UNBIND_MEM to
H_SCM_UNBIND_ALL.
* Update drc_pmem_unbind() to handles cases when PHYP asks the guest
kernel to wait for specific amount of time before retrying the
hcall via the 'LONG_BUSY' return value. In case it takes more
than 1 second to unbind the memory log a warning.
* Ensure appropriate error code is returned back from the function
in case of an error.
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
arch/powerpc/include/asm/hvcall.h | 2 +-
arch/powerpc/platforms/pseries/papr_scm.c | 37 ++++++++++++++++++++---
2 files changed, 33 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
index 463c63a9fcf1..bb56fa0f976c 100644
--- a/arch/powerpc/include/asm/hvcall.h
+++ b/arch/powerpc/include/asm/hvcall.h
@@ -302,7 +302,7 @@
#define H_SCM_UNBIND_MEM 0x3F0
#define H_SCM_QUERY_BLOCK_MEM_BINDING 0x3F4
#define H_SCM_QUERY_LOGICAL_MEM_BINDING 0x3F8
-#define H_SCM_MEM_QUERY 0x3FC
+#define H_SCM_UNBIND_ALL 0x3FC
#define H_SCM_BLOCK_CLEAR 0x400
#define MAX_HCALL_OPCODE H_SCM_BLOCK_CLEAR
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 96c53b23e58f..d790e4e4ffb3 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -11,11 +11,16 @@
#include <linux/sched.h>
#include <linux/libnvdimm.h>
#include <linux/platform_device.h>
+#include <linux/delay.h>
#include <asm/plpar_wrappers.h>
#define BIND_ANY_ADDR (~0ul)
+/* Scope args for H_SCM_UNBIND_ALL */
+#define H_UNBIND_SCOPE_ALL (0x1)
+#define H_UNBIND_SCOPE_DRC (0x2)
+
#define PAPR_SCM_DIMM_CMD_MASK \
((1ul << ND_CMD_GET_CONFIG_SIZE) | \
(1ul << ND_CMD_GET_CONFIG_DATA) | \
@@ -78,21 +83,43 @@ static int drc_pmem_unbind(struct papr_scm_priv *p)
{
unsigned long ret[PLPAR_HCALL_BUFSIZE];
uint64_t rc, token;
+ unsigned long starttime;
token = 0;
- /* NB: unbind has the same retry requirements mentioned above */
+ dev_dbg(&p->pdev->dev, "unbind drc %x\n", p->drc_index);
+
+ /* NB: unbind_all has the same retry requirements as drc_pmem_bind() */
+ starttime = HZ;
do {
- rc = plpar_hcall(H_SCM_UNBIND_MEM, ret, p->drc_index,
- p->bound_addr, p->blocks, token);
+ /* If this is taking too much time then log warning */
+ if (jiffies_to_msecs(HZ - starttime) > 1000) {
+ dev_warn(&p->pdev->dev,
+ "unbind taking > 1s to complete\n");
+ starttime = HZ;
+ }
+
+ /* Unbind of all SCM resources associated with drcIndex */
+ rc = plpar_hcall(H_SCM_UNBIND_ALL, ret, H_UNBIND_SCOPE_DRC,
+ p->drc_index, token);
token = ret[0];
- cond_resched();
+
+ /* Check if we are stalled for some time */
+ if (H_IS_LONG_BUSY(rc)) {
+ msleep(get_longbusy_msecs(rc));
+ rc = H_BUSY;
+ token = 0;
+
+ } else if (rc == H_BUSY) {
+ cond_resched();
+ }
+
} while (rc == H_BUSY);
if (rc)
dev_err(&p->pdev->dev, "unbind error: %lld\n", rc);
- return !!rc;
+ return rc == H_SUCCESS ? 0 : -ENXIO;
}
static int papr_scm_meta_get(struct papr_scm_priv *p,
--
2.21.0
^ permalink raw reply related
* [PATCH 0/2] powerpc/papr_scm: Workaround for failure of drc bind after kexec
From: Vaibhav Jain @ 2019-06-24 14:59 UTC (permalink / raw)
To: linuxppc-dev
Cc: Aneesh Kumar K . V, Oliver O'Halloran, Vaibhav Jain,
Laurent Dufour, David Gibson
Presently an error is returned in response to hcall H_SCM_BIND_MEM when a
new kernel boots on lpar via kexec. This prevents papr_scm from registering
drc memory regions with nvdimm. The error reported is of the form below:
"papr_scm ibm,persistent-memory:ibm,pmemory@44100002: bind err: -68"
On investigation it was revealed that phyp returns this error as previous
kernel did not completely release bindings for drc scm-memory blocks and
hence phyp rejected request for re-binding these block to lpar with error
H_OVERLAP. Also support for a new H_SCM_UNBIND_ALL is recently added which
is better suited for releasing all the bound scm-memory block from an lpar.
So leveraging new hcall H_SCM_UNBIND_ALL, we can workaround H_OVERLAP issue
during kexec by forcing an unbind of all drm scm-memory blocks and issuing
H_SCM_BIND_MEM to re-bind the drc scm-memory blocks to lpar. This sequence
will also be needed when a new kernel boot on lpar after previous kernel
panicked and it never got an opportunity to call H_SCM_UNBIND_MEM/ALL.
Hence this patch-set implements following changes to papr_scm module:
* Update it to use H_SCM_UNBIND_ALL instead of H_SCM_UNBIND_MEM
* In case hcall H_SCM_BIND_MEM fails with error H_OVERLAP, force
H_SCM_UNBIND_ALL and retry the bind operation again.
With the patch-set applied re-bind of drc scm-memory to lpar succeeds after
a kexec to new kernel as illustrated below:
# Old kernel
$ sudo ndctl list -R
[
{
"dev":"region0",
<snip>
....
}
]
# kexec to new kernel
$ sudo kexec --initrd=... vmlinux
...
...
I'm in purgatory
...
papr_scm ibm,persistent-memory:ibm,pmemory@44100002: Un-binding and retrying
...
# New kernel
$ sudo ndctl list -R
$ sudo ndctl list -R
[
{
"dev":"region0",
<snip>
....
}
]
Vaibhav Jain (2):
powerpc/papr_scm: Update drc_pmem_unbind() to use H_SCM_UNBIND_ALL
powerpc/papr_scm: Force a scm-unbind if initial scm-bind fails
arch/powerpc/include/asm/hvcall.h | 2 +-
arch/powerpc/platforms/pseries/papr_scm.c | 60 ++++++++++++++++++++---
2 files changed, 53 insertions(+), 9 deletions(-)
--
2.21.0
^ permalink raw reply
* [PATCH 2/2] powerpc/papr_scm: Force a scm-unbind if initial scm-bind fails
From: Vaibhav Jain @ 2019-06-24 14:59 UTC (permalink / raw)
To: linuxppc-dev
Cc: Aneesh Kumar K . V, Oliver O'Halloran, Vaibhav Jain,
Laurent Dufour, David Gibson
In-Reply-To: <20190624145913.20122-1-vaibhav@linux.ibm.com>
In some cases initial bind of scm memory for an lpar can fail if
previously it wasn't released using a scm-unbind hcall. This situation
can arise due to panic of the previous kernel or forced lpar reset. In
such cases the H_SCM_BIND_MEM return a H_OVERLAP error.
To mitigate such cases the patch updates drc_pmem_bind() to force a
call to drc_pmem_unbind() in case the initial bind of scm memory fails
with H_OVERLAP error. In case scm-bind operation again fails after the
forced scm-unbind then we follow the existing error path.
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
arch/powerpc/platforms/pseries/papr_scm.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index d790e4e4ffb3..049d7927c0a4 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -44,19 +44,26 @@ struct papr_scm_priv {
struct nd_interleave_set nd_set;
};
+/* Forward declaration */
+static int drc_pmem_unbind(struct papr_scm_priv *);
+
static int drc_pmem_bind(struct papr_scm_priv *p)
{
unsigned long ret[PLPAR_HCALL_BUFSIZE];
uint64_t rc, token;
- uint64_t saved = 0;
+ uint64_t saved;
+ bool tried_unbind = false;
+ dev_dbg(&p->pdev->dev, "bind drc %x\n", p->drc_index);
/*
* When the hypervisor cannot map all the requested memory in a single
* hcall it returns H_BUSY and we call again with the token until
* we get H_SUCCESS. Aborting the retry loop before getting H_SUCCESS
* leave the system in an undefined state, so we wait.
*/
+retry:
token = 0;
+ saved = 0;
do {
rc = plpar_hcall(H_SCM_BIND_MEM, ret, p->drc_index, 0,
@@ -68,8 +75,18 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
} while (rc == H_BUSY);
if (rc) {
- dev_err(&p->pdev->dev, "bind err: %lld\n", rc);
- return -ENXIO;
+ /* retry after unbinding */
+ if (rc == H_OVERLAP && !tried_unbind) {
+ dev_warn(&p->pdev->dev, "Un-binding and retrying\n");
+ /* Try unbind and ignore any errors */
+ tried_unbind = true;
+ drc_pmem_unbind(p);
+ goto retry;
+
+ } else {
+ dev_err(&p->pdev->dev, "bind err: %lld\n", rc);
+ return -ENXIO;
+ }
}
p->bound_addr = saved;
--
2.21.0
^ permalink raw reply related
* Re: [PATCH] ocxl: Fix concurrent AFU open and device removal
From: Frederic Barrat @ 2019-06-24 15:39 UTC (permalink / raw)
To: Greg Kurz; +Cc: clombard, linuxppc-dev, alastair, andrew.donnellan
In-Reply-To: <20190624172452.7e217596@bahia.lan>
Le 24/06/2019 à 17:24, Greg Kurz a écrit :
> On Mon, 24 Jun 2019 16:41:48 +0200
> Frederic Barrat <fbarrat@linux.ibm.com> wrote:
>
>> If an ocxl device is unbound through sysfs at the same time its AFU is
>> being opened by a user process, the open code may dereference freed
>> stuctures, which can lead to kernel oops messages. You'd have to hit a
>> tiny time window, but it's possible. It's fairly easy to test by
>> making the time window bigger artificially.
>>
>> Fix it with a combination of 2 changes:
>> - when an AFU device is found in the IDR by looking for the device
>> minor number, we should hold a reference on the device until after the
>> context is allocated. A reference on the AFU structure is kept when
>> the context is allocated, so we can release the reference on the
>> device after the context allocation.
>> - with the fix above, there's still another even tinier window,
>> between the time the AFU device is found in the IDR and the reference
>> on the device is taken. We can fix this one by removing the IDR entry
>> earlier, when the device setup is removed, instead of waiting for the
>> 'release' device callback. With proper locking around the IDR.
>>
>> Fixes: 75ca758adbaf ("ocxl: Create a clear delineation between ocxl backend & frontend")
>> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
>> ---
>> mpe: this fixes a commit merged in v5.2-rc1. It's late, and I don't think it's that important. If it's for the next merge window, I would add:
>> Cc: stable@vger.kernel.org # v5.2
>>
>>
>> drivers/misc/ocxl/file.c | 23 +++++++++++------------
>> 1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
>> index 2870c25da166..4d1b44de1492 100644
>> --- a/drivers/misc/ocxl/file.c
>> +++ b/drivers/misc/ocxl/file.c
>> @@ -18,18 +18,15 @@ static struct class *ocxl_class;
>> static struct mutex minors_idr_lock;
>> static struct idr minors_idr;
>>
>> -static struct ocxl_file_info *find_file_info(dev_t devno)
>> +static struct ocxl_file_info *find_and_get_file_info(dev_t devno)
>> {
>> struct ocxl_file_info *info;
>>
>> - /*
>> - * We don't declare an RCU critical section here, as our AFU
>> - * is protected by a reference counter on the device. By the time the
>> - * info reference is removed from the idr, the ref count of
>> - * the device is already at 0, so no user API will access that AFU and
>> - * this function can't return it.
>> - */
>> + mutex_lock(&minors_idr_lock);
>> info = idr_find(&minors_idr, MINOR(devno));
>> + if (info)
>> + get_device(&info->dev);
>> + mutex_unlock(&minors_idr_lock);
>> return info;
>> }
>>
>> @@ -58,14 +55,16 @@ static int afu_open(struct inode *inode, struct file *file)
>>
>> pr_debug("%s for device %x\n", __func__, inode->i_rdev);
>>
>> - info = find_file_info(inode->i_rdev);
>> + info = find_and_get_file_info(inode->i_rdev);
>> if (!info)
>> return -ENODEV;
>>
>> rc = ocxl_context_alloc(&ctx, info->afu, inode->i_mapping);
>> - if (rc)
>> + if (rc) {
>> + put_device(&info->dev);
>
> You could have a single call site for put_device() since it's
> needed for both branches. No big deal.
Agreed. Will fix if I end up respinning, but won't if it's the only
complaint :-)
>> return rc;
>> -
>> + }
>> + put_device(&info->dev);
>> file->private_data = ctx;
>> return 0;
>> }
>> @@ -487,7 +486,6 @@ static void info_release(struct device *dev)
>> {
>> struct ocxl_file_info *info = container_of(dev, struct ocxl_file_info, dev);
>>
>> - free_minor(info);
>> ocxl_afu_put(info->afu);
>> kfree(info);
>> }
>> @@ -577,6 +575,7 @@ void ocxl_file_unregister_afu(struct ocxl_afu *afu)
>>
>> ocxl_file_make_invisible(info);
>> ocxl_sysfs_unregister_afu(info);
>> + free_minor(info);
>
> Since the IDR entry is added by ocxl_file_register_afu(), it seems to make
> sense to undo that in ocxl_file_unregister_afu(). Out of curiosity, was there
> any historical reason to do this in info_release() in the first place ?
Yeah, it makes a lot of sense to remove the IDR entry in
ocxl_file_unregister_afu(), that's where we undo the device. I wish I
had noticed during the code reviews.
I don't think there was any good reason to have it in info_release() in
the first place. I remember the code went through many iterations to get
the reference counting on the AFU structure and device done correctly,
but we let that one slip.
I now think the pre-5.2 ocxl code was also exposed to the 2nd window
mentioned in the commit log (but the first window is new with the
refactoring introduced in 5.2-rc1).
Fred
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
>
>> device_unregister(&info->dev);
>> }
>>
>
^ permalink raw reply
* Re: [PATCH] ocxl: Fix concurrent AFU open and device removal
From: Greg Kurz @ 2019-06-24 15:50 UTC (permalink / raw)
To: Frederic Barrat; +Cc: clombard, linuxppc-dev, alastair, andrew.donnellan
In-Reply-To: <ea1295fe-d8ad-1e5f-54f7-a72a7149c5b7@linux.ibm.com>
On Mon, 24 Jun 2019 17:39:26 +0200
Frederic Barrat <fbarrat@linux.ibm.com> wrote:
> Le 24/06/2019 à 17:24, Greg Kurz a écrit :
> > On Mon, 24 Jun 2019 16:41:48 +0200
> > Frederic Barrat <fbarrat@linux.ibm.com> wrote:
> >
> >> If an ocxl device is unbound through sysfs at the same time its AFU is
> >> being opened by a user process, the open code may dereference freed
> >> stuctures, which can lead to kernel oops messages. You'd have to hit a
> >> tiny time window, but it's possible. It's fairly easy to test by
> >> making the time window bigger artificially.
> >>
> >> Fix it with a combination of 2 changes:
> >> - when an AFU device is found in the IDR by looking for the device
> >> minor number, we should hold a reference on the device until after the
> >> context is allocated. A reference on the AFU structure is kept when
> >> the context is allocated, so we can release the reference on the
> >> device after the context allocation.
> >> - with the fix above, there's still another even tinier window,
> >> between the time the AFU device is found in the IDR and the reference
> >> on the device is taken. We can fix this one by removing the IDR entry
> >> earlier, when the device setup is removed, instead of waiting for the
> >> 'release' device callback. With proper locking around the IDR.
> >>
> >> Fixes: 75ca758adbaf ("ocxl: Create a clear delineation between ocxl backend & frontend")
> >> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> >> ---
> >> mpe: this fixes a commit merged in v5.2-rc1. It's late, and I don't think it's that important. If it's for the next merge window, I would add:
> >> Cc: stable@vger.kernel.org # v5.2
> >>
> >>
> >> drivers/misc/ocxl/file.c | 23 +++++++++++------------
> >> 1 file changed, 11 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
> >> index 2870c25da166..4d1b44de1492 100644
> >> --- a/drivers/misc/ocxl/file.c
> >> +++ b/drivers/misc/ocxl/file.c
> >> @@ -18,18 +18,15 @@ static struct class *ocxl_class;
> >> static struct mutex minors_idr_lock;
> >> static struct idr minors_idr;
> >>
> >> -static struct ocxl_file_info *find_file_info(dev_t devno)
> >> +static struct ocxl_file_info *find_and_get_file_info(dev_t devno)
> >> {
> >> struct ocxl_file_info *info;
> >>
> >> - /*
> >> - * We don't declare an RCU critical section here, as our AFU
> >> - * is protected by a reference counter on the device. By the time the
> >> - * info reference is removed from the idr, the ref count of
> >> - * the device is already at 0, so no user API will access that AFU and
> >> - * this function can't return it.
> >> - */
> >> + mutex_lock(&minors_idr_lock);
> >> info = idr_find(&minors_idr, MINOR(devno));
> >> + if (info)
> >> + get_device(&info->dev);
> >> + mutex_unlock(&minors_idr_lock);
> >> return info;
> >> }
> >>
> >> @@ -58,14 +55,16 @@ static int afu_open(struct inode *inode, struct file *file)
> >>
> >> pr_debug("%s for device %x\n", __func__, inode->i_rdev);
> >>
> >> - info = find_file_info(inode->i_rdev);
> >> + info = find_and_get_file_info(inode->i_rdev);
> >> if (!info)
> >> return -ENODEV;
> >>
> >> rc = ocxl_context_alloc(&ctx, info->afu, inode->i_mapping);
> >> - if (rc)
> >> + if (rc) {
> >> + put_device(&info->dev);
> >
> > You could have a single call site for put_device() since it's
> > needed for both branches. No big deal.
>
>
> Agreed. Will fix if I end up respinning, but won't if it's the only
> complaint :-)
>
>
>
> >> return rc;
> >> -
> >> + }
> >> + put_device(&info->dev);
> >> file->private_data = ctx;
> >> return 0;
> >> }
> >> @@ -487,7 +486,6 @@ static void info_release(struct device *dev)
> >> {
> >> struct ocxl_file_info *info = container_of(dev, struct ocxl_file_info, dev);
> >>
> >> - free_minor(info);
> >> ocxl_afu_put(info->afu);
> >> kfree(info);
> >> }
> >> @@ -577,6 +575,7 @@ void ocxl_file_unregister_afu(struct ocxl_afu *afu)
> >>
> >> ocxl_file_make_invisible(info);
> >> ocxl_sysfs_unregister_afu(info);
> >> + free_minor(info);
> >
> > Since the IDR entry is added by ocxl_file_register_afu(), it seems to make
> > sense to undo that in ocxl_file_unregister_afu(). Out of curiosity, was there
> > any historical reason to do this in info_release() in the first place ?
>
>
> Yeah, it makes a lot of sense to remove the IDR entry in
> ocxl_file_unregister_afu(), that's where we undo the device. I wish I
> had noticed during the code reviews.
> I don't think there was any good reason to have it in info_release() in
> the first place. I remember the code went through many iterations to get
> the reference counting on the AFU structure and device done correctly,
> but we let that one slip.
>
> I now think the pre-5.2 ocxl code was also exposed to the 2nd window
> mentioned in the commit log (but the first window is new with the
> refactoring introduced in 5.2-rc1).
>
This calls for two separate patches then IMHO.
> Fred
>
>
>
> >
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> >
> >> device_unregister(&info->dev);
> >> }
> >>
> >
>
^ permalink raw reply
* Re: [PATCH] ocxl: Fix concurrent AFU open and device removal
From: Greg Kurz @ 2019-06-24 15:24 UTC (permalink / raw)
To: Frederic Barrat; +Cc: clombard, linuxppc-dev, alastair, andrew.donnellan
In-Reply-To: <20190624144148.32022-1-fbarrat@linux.ibm.com>
On Mon, 24 Jun 2019 16:41:48 +0200
Frederic Barrat <fbarrat@linux.ibm.com> wrote:
> If an ocxl device is unbound through sysfs at the same time its AFU is
> being opened by a user process, the open code may dereference freed
> stuctures, which can lead to kernel oops messages. You'd have to hit a
> tiny time window, but it's possible. It's fairly easy to test by
> making the time window bigger artificially.
>
> Fix it with a combination of 2 changes:
> - when an AFU device is found in the IDR by looking for the device
> minor number, we should hold a reference on the device until after the
> context is allocated. A reference on the AFU structure is kept when
> the context is allocated, so we can release the reference on the
> device after the context allocation.
> - with the fix above, there's still another even tinier window,
> between the time the AFU device is found in the IDR and the reference
> on the device is taken. We can fix this one by removing the IDR entry
> earlier, when the device setup is removed, instead of waiting for the
> 'release' device callback. With proper locking around the IDR.
>
> Fixes: 75ca758adbaf ("ocxl: Create a clear delineation between ocxl backend & frontend")
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---
> mpe: this fixes a commit merged in v5.2-rc1. It's late, and I don't think it's that important. If it's for the next merge window, I would add:
> Cc: stable@vger.kernel.org # v5.2
>
>
> drivers/misc/ocxl/file.c | 23 +++++++++++------------
> 1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
> index 2870c25da166..4d1b44de1492 100644
> --- a/drivers/misc/ocxl/file.c
> +++ b/drivers/misc/ocxl/file.c
> @@ -18,18 +18,15 @@ static struct class *ocxl_class;
> static struct mutex minors_idr_lock;
> static struct idr minors_idr;
>
> -static struct ocxl_file_info *find_file_info(dev_t devno)
> +static struct ocxl_file_info *find_and_get_file_info(dev_t devno)
> {
> struct ocxl_file_info *info;
>
> - /*
> - * We don't declare an RCU critical section here, as our AFU
> - * is protected by a reference counter on the device. By the time the
> - * info reference is removed from the idr, the ref count of
> - * the device is already at 0, so no user API will access that AFU and
> - * this function can't return it.
> - */
> + mutex_lock(&minors_idr_lock);
> info = idr_find(&minors_idr, MINOR(devno));
> + if (info)
> + get_device(&info->dev);
> + mutex_unlock(&minors_idr_lock);
> return info;
> }
>
> @@ -58,14 +55,16 @@ static int afu_open(struct inode *inode, struct file *file)
>
> pr_debug("%s for device %x\n", __func__, inode->i_rdev);
>
> - info = find_file_info(inode->i_rdev);
> + info = find_and_get_file_info(inode->i_rdev);
> if (!info)
> return -ENODEV;
>
> rc = ocxl_context_alloc(&ctx, info->afu, inode->i_mapping);
> - if (rc)
> + if (rc) {
> + put_device(&info->dev);
You could have a single call site for put_device() since it's
needed for both branches. No big deal.
> return rc;
> -
> + }
> + put_device(&info->dev);
> file->private_data = ctx;
> return 0;
> }
> @@ -487,7 +486,6 @@ static void info_release(struct device *dev)
> {
> struct ocxl_file_info *info = container_of(dev, struct ocxl_file_info, dev);
>
> - free_minor(info);
> ocxl_afu_put(info->afu);
> kfree(info);
> }
> @@ -577,6 +575,7 @@ void ocxl_file_unregister_afu(struct ocxl_afu *afu)
>
> ocxl_file_make_invisible(info);
> ocxl_sysfs_unregister_afu(info);
> + free_minor(info);
Since the IDR entry is added by ocxl_file_register_afu(), it seems to make
sense to undo that in ocxl_file_unregister_afu(). Out of curiosity, was there
any historical reason to do this in info_release() in the first place ?
Reviewed-by: Greg Kurz <groug@kaod.org>
> device_unregister(&info->dev);
> }
>
^ permalink raw reply
* Re: [PATCH] powerpc/rtas: retry when cpu offline races with suspend/migration
From: mmc @ 2019-06-24 16:52 UTC (permalink / raw)
To: Nathan Lynch; +Cc: ego, linuxppc-dev, julietk
In-Reply-To: <20190621060518.29616-1-nathanl@linux.ibm.com>
On 2019-06-21 00:05, Nathan Lynch wrote:
> The protocol for suspending or migrating an LPAR requires all present
> processor threads to enter H_JOIN. So if we have threads offline, we
> have to temporarily bring them up. This can race with administrator
> actions such as SMT state changes. As of dfd718a2ed1f ("powerpc/rtas:
> Fix a potential race between CPU-Offline & Migration"),
> rtas_ibm_suspend_me() accounts for this, but errors out with -EBUSY
> for what almost certainly is a transient condition in any reasonable
> scenario.
>
> Callers of rtas_ibm_suspend_me() already retry when -EAGAIN is
> returned, and it is typical during a migration for that to happen
> repeatedly for several minutes polling the H_VASI_STATE hcall result
> before proceeding to the next stage.
>
> So return -EAGAIN instead of -EBUSY when this race is
> encountered. Additionally: logging this event is still appropriate but
> use pr_info instead of pr_err; and remove use of unlikely() while here
> as this is not a hot path at all.
>
Looks good, since it's not a hot path anyway, so unlikely() should
benefit from optimize compiler path, and should stay. No?
Mingming
> Fixes: dfd718a2ed1f ("powerpc/rtas: Fix a potential race between
> CPU-Offline & Migration")
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---
> arch/powerpc/kernel/rtas.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index fbc676160adf..9b4d2a2ffb4f 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -984,10 +984,9 @@ int rtas_ibm_suspend_me(u64 handle)
> cpu_hotplug_disable();
>
> /* Check if we raced with a CPU-Offline Operation */
> - if (unlikely(!cpumask_equal(cpu_present_mask, cpu_online_mask))) {
> - pr_err("%s: Raced against a concurrent CPU-Offline\n",
> - __func__);
> - atomic_set(&data.error, -EBUSY);
> + if (!cpumask_equal(cpu_present_mask, cpu_online_mask)) {
> + pr_info("%s: Raced against a concurrent CPU-Offline\n", __func__);
> + atomic_set(&data.error, -EAGAIN);
> goto out_hotplug_enable;
> }
^ permalink raw reply
* Re: [next][PowerPC] RCU stalls while booting linux-next on PowerVM LPAR
From: Sachin Sant @ 2019-06-24 17:09 UTC (permalink / raw)
To: David Hildenbrand
Cc: Stephen Rothwell, linux-next, linuxppc-dev, linux-kernel
In-Reply-To: <83a31209-68cb-3e60-6b50-322154e6dcea@redhat.com>
> On 24-Jun-2019, at 8:12 PM, David Hildenbrand <david@redhat.com> wrote:
>
> On 24.06.19 16:09, Sachin Sant wrote:
>> Latest -next fails to boot on POWER9 PowerVM LPAR due to RCU stalls.
>>
>> This problem was introduced with next-20190620 (dc636f5d78).
>> next-20190619 was last good kernel.
>>
>> Reverting following commit allows the kernel to boot.
>> 2fd4aeea6b603 : mm/memory_hotplug: move and simplify walk_memory_blocks()
>>
>>
>> [ 0.014409] Using shared cache scheduler topology
>> [ 0.016302] devtmpfs: initialized
>> [ 0.031022] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns
>> [ 0.031034] futex hash table entries: 16384 (order: 5, 2097152 bytes, linear)
>> [ 0.031575] NET: Registered protocol family 16
>> [ 0.031724] audit: initializing netlink subsys (disabled)
>> [ 0.031796] audit: type=2000 audit(1561344029.030:1): state=initialized audit_enabled=0 res=1
>> [ 0.032249] cpuidle: using governor menu
>> [ 0.032403] pstore: Registered nvram as persistent store backend
>> [ 60.061246] rcu: INFO: rcu_sched self-detected stall on CPU
>> [ 60.061254] rcu: 0-....: (5999 ticks this GP) idle=1ea/1/0x4000000000000002 softirq=5/5 fqs=2999
>> [ 60.061261] (t=6000 jiffies g=-1187 q=0)
>> [ 60.061265] NMI backtrace for cpu 0
>> [ 60.061269] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc5-next-20190621-autotest-autotest #1
>> [ 60.061275] Call Trace:
>> [ 60.061280] [c0000018ee85f380] [c000000000b624ec] dump_stack+0xb0/0xf4 (unreliable)
>> [ 60.061287] [c0000018ee85f3c0] [c000000000b6d464] nmi_cpu_backtrace+0x144/0x150
>> [ 60.061293] [c0000018ee85f450] [c000000000b6d61c] nmi_trigger_cpumask_backtrace+0x1ac/0x1f0
>> [ 60.061300] [c0000018ee85f4f0] [c0000000000692c8] arch_trigger_cpumask_backtrace+0x28/0x40
>> [ 60.061306] [c0000018ee85f510] [c0000000001c5f90] rcu_dump_cpu_stacks+0x10c/0x16c
>> [ 60.061313] [c0000018ee85f560] [c0000000001c4fe4] rcu_sched_clock_irq+0x744/0x990
>> [ 60.061318] [c0000018ee85f630] [c0000000001d5b58] update_process_times+0x48/0x90
>> [ 60.061325] [c0000018ee85f660] [c0000000001ea03c] tick_periodic+0x4c/0x120
>> [ 60.061330] [c0000018ee85f690] [c0000000001ea150] tick_handle_periodic+0x40/0xe0
>> [ 60.061336] [c0000018ee85f6d0] [c00000000002b5cc] timer_interrupt+0x10c/0x2e0
>> [ 60.061342] [c0000018ee85f730] [c000000000009204] decrementer_common+0x134/0x140
>> [ 60.061350] --- interrupt: 901 at replay_interrupt_return+0x0/0x4
>> [ 60.061350] LR = arch_local_irq_restore+0x84/0x90
>> [ 60.061357] [c0000018ee85fa30] [c0000018ee85fbac] 0xc0000018ee85fbac (unreliable)
>> [ 60.061364] [c0000018ee85fa50] [c000000000b88300] _raw_spin_unlock_irqrestore+0x50/0x80
>> [ 60.061369] [c0000018ee85fa70] [c000000000b69da4] klist_next+0xb4/0x150
>> [ 60.061376] [c0000018ee85fac0] [c000000000766ea0] subsys_find_device_by_id+0xf0/0x1a0
>> [ 60.061382] [c0000018ee85fb20] [c000000000797a94] walk_memory_blocks+0x84/0x100
>> [ 60.061388] [c0000018ee85fb80] [c000000000795ea0] link_mem_sections+0x40/0x60
>> [ 60.061395] [c0000018ee85fbb0] [c000000000f28c28] topology_init+0xa0/0x268
>> [ 60.061400] [c0000018ee85fc10] [c000000000010448] do_one_initcall+0x68/0x2c0
>> [ 60.061406] [c0000018ee85fce0] [c000000000f247dc] kernel_init_freeable+0x318/0x47c
>> [ 60.061411] [c0000018ee85fdb0] [c0000000000107c4] kernel_init+0x24/0x150
>> [ 60.061417] [c0000018ee85fe20] [c00000000000ba54] ret_from_kernel_thread+0x5c/0x68
>> [ 88.016563] watchdog: BUG: soft lockup - CPU#0 stuck for 22s! [swapper/0:1]
>> [ 88.016569] Modules linked in:
>>
>
> Hi, thanks! Please see
>
> https://lkml.org/lkml/2019/6/21/600
>
> and especially
>
> https://lkml.org/lkml/2019/6/21/908
>
> Does this fix your problem? The fix is on its way to next.
Yes, this patch fixes the problem for me.
Thanks
-Sachin
^ permalink raw reply
* Re: [PATCH] powerpc/rtas: retry when cpu offline races with suspend/migration
From: Nathan Lynch @ 2019-06-24 17:23 UTC (permalink / raw)
To: mmc; +Cc: ego, linuxppc-dev, julietk
In-Reply-To: <f3b54ef4394bdbf4887d2185bb951c80@linux.vnet.ibm.com>
Hi Mingming,
mmc <mmc@linux.vnet.ibm.com> writes:
> On 2019-06-21 00:05, Nathan Lynch wrote:
>> So return -EAGAIN instead of -EBUSY when this race is
>> encountered. Additionally: logging this event is still appropriate but
>> use pr_info instead of pr_err; and remove use of unlikely() while here
>> as this is not a hot path at all.
>
> Looks good, since it's not a hot path anyway, so unlikely() should
> benefit from optimize compiler path, and should stay. No?
The latency of this path in rtas_ibm_suspend_me() in the best case is
around 2-3 seconds.
So I think not -- this is such a heavyweight and relatively
seldom-executed path that the unlikely() cannot yield any discernible
performance benefit, and its presence imposes a readability cost.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox