* Re: [PATCH kernel v2] powerpc/mm: Flush radix process translations when setting MMU type
From: Laurent Vivier @ 2018-02-13 11:25 UTC (permalink / raw)
To: Daniel Henrique Barboza, Alexey Kardashevskiy, linuxppc-dev
In-Reply-To: <f30971fd-a4a8-ceb6-bf0a-b293275c8d0b@linux.vnet.ibm.com>
On 07/02/2018 18:49, Daniel Henrique Barboza wrote:
>
>
> On 02/07/2018 12:33 PM, Laurent Vivier wrote:
>> On 01/02/2018 06:09, Alexey Kardashevskiy wrote:
>>> Radix guests do normally invalidate process-scoped translations when
>>> a new pid is allocated but migrated guests do not invalidate these so
>>> migrated guests crash sometime, especially easy to reproduce with
>>> migration happening within first 10 seconds after the guest boot
>>> start on
>>> the same machine.
>>>
>>> This adds the "Invalidate process-scoped translations" flush to fix
>>> radix guests migration.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> Changes:
>>> v2:
>>> * removed PPC_TLBIE_5() from the !(old&PATH_HR) case as it is pointless
>>> on hash
>>>
>>> ---
>>>
>>>
>>> Not so sure that "process-scoped translations" only require flushing
>>> at pid allocation and migration.
>>>
>>> ---
>>> arch/powerpc/mm/pgtable_64.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c
>>> index c9a623c..d75dd52 100644
>>> --- a/arch/powerpc/mm/pgtable_64.c
>>> +++ b/arch/powerpc/mm/pgtable_64.c
>>> @@ -471,6 +471,8 @@ void mmu_partition_table_set_entry(unsigned int
>>> lpid, unsigned long dw0,
>>> if (old & PATB_HR) {
>>> asm volatile(PPC_TLBIE_5(%0,%1,2,0,1) : :
>>> "r" (TLBIEL_INVAL_SET_LPID), "r" (lpid));
>>> + asm volatile(PPC_TLBIE_5(%0,%1,2,1,1) : :
>>> + "r" (TLBIEL_INVAL_SET_LPID), "r" (lpid));
>>> trace_tlbie(lpid, 0, TLBIEL_INVAL_SET_LPID, lpid, 2, 0, 1);
>>> } else {
>>> asm volatile(PPC_TLBIE_5(%0,%1,2,0,0) : :
>>>
>> This patch fixes for me a VM migration crash on POWER9.
>
> Same here.
>
> Tested-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
>
>>
>> Tested-by: Laurent Vivier <lvivier@redhat.com>
Any hope to have this patch merged soon?
It fixes a real problem and migration of VM is not reliable without it.
Thanks,
Laurent
^ permalink raw reply
* Re: [RFC PATCH 0/5] powerpc/mm/slice: improve slice speed and stack use
From: Christophe LEROY @ 2018-02-13 11:24 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev, Aneesh Kumar K . V, Michael Ellerman
In-Reply-To: <20180213184017.168c31f0@roar.ozlabs.ibm.com>
Le 13/02/2018 à 09:40, Nicholas Piggin a écrit :
> On Mon, 12 Feb 2018 18:42:21 +0100
> Christophe LEROY <christophe.leroy@c-s.fr> wrote:
>
>> Le 12/02/2018 à 16:24, Nicholas Piggin a écrit :
>>> On Mon, 12 Feb 2018 16:02:23 +0100
>>> Christophe LEROY <christophe.leroy@c-s.fr> wrote:
>>>
>>>> Le 10/02/2018 à 09:11, Nicholas Piggin a écrit :
>>>>> This series intends to improve performance and reduce stack
>>>>> consumption in the slice allocation code. It does it by keeping slice
>>>>> masks in the mm_context rather than compute them for each allocation,
>>>>> and by reducing bitmaps and slice_masks from stacks, using pointers
>>>>> instead where possible.
>>>>>
>>>>> checkstack.pl gives, before:
>>>>> 0x00000de4 slice_get_unmapped_area [slice.o]: 656
>>>>> 0x00001b4c is_hugepage_only_range [slice.o]: 512
>>>>> 0x0000075c slice_find_area_topdown [slice.o]: 416
>>>>> 0x000004c8 slice_find_area_bottomup.isra.1 [slice.o]: 272
>>>>> 0x00001aa0 slice_set_range_psize [slice.o]: 240
>>>>> 0x00000a64 slice_find_area [slice.o]: 176
>>>>> 0x00000174 slice_check_fit [slice.o]: 112
>>>>>
>>>>> after:
>>>>> 0x00000d70 slice_get_unmapped_area [slice.o]: 320
>>>>> 0x000008f8 slice_find_area [slice.o]: 144
>>>>> 0x00001860 slice_set_range_psize [slice.o]: 144
>>>>> 0x000018ec is_hugepage_only_range [slice.o]: 144
>>>>> 0x00000750 slice_find_area_bottomup.isra.4 [slice.o]: 128
>>>>>
>>>>> The benchmark in https://github.com/linuxppc/linux/issues/49 gives, before:
>>>>> $ time ./slicemask
>>>>> real 0m20.712s
>>>>> user 0m5.830s
>>>>> sys 0m15.105s
>>>>>
>>>>> after:
>>>>> $ time ./slicemask
>>>>> real 0m13.197s
>>>>> user 0m5.409s
>>>>> sys 0m7.779s
>>>>
>>>> Hi,
>>>>
>>>> I tested your serie on an 8xx, on top of patch
>>>> https://patchwork.ozlabs.org/patch/871675/
>>>>
>>>> I don't get a result as significant as yours, but there is some
>>>> improvment anyway:
>>>>
>>>> ITERATION 500000
>>>>
>>>> Before:
>>>>
>>>> root@vgoip:~# time ./slicemask
>>>> real 0m 33.26s
>>>> user 0m 1.94s
>>>> sys 0m 30.85s
>>>>
>>>> After:
>>>> root@vgoip:~# time ./slicemask
>>>> real 0m 29.69s
>>>> user 0m 2.11s
>>>> sys 0m 27.15s
>>>>
>>>> Most significant improvment is obtained with the first patch of your serie:
>>>> root@vgoip:~# time ./slicemask
>>>> real 0m 30.85s
>>>> user 0m 1.80s
>>>> sys 0m 28.57s
>>>
>>> Okay, thanks. Are you still spending significant time in the slice
>>> code?
>>
>> Do you mean am I still updating my patches ? No I hope we are at last
>
> Actually I was wondering about CPU time spent for the microbenchmark :)
Lol.
I've got the following perf report (functions over 0.50%)
# Overhead Command Shared Object Symbol
# ........ ......... ................. ..................................
#
7.13% slicemask [kernel.kallsyms] [k] do_brk_flags
6.19% slicemask [kernel.kallsyms] [k] DoSyscall
5.81% slicemask [kernel.kallsyms] [k] perf_event_mmap
5.55% slicemask [kernel.kallsyms] [k] do_munmap
4.55% slicemask [kernel.kallsyms] [k] sys_brk
4.43% slicemask [kernel.kallsyms] [k] find_vma
3.42% slicemask [kernel.kallsyms] [k] vma_compute_subtree_gap
3.08% slicemask libc-2.23.so [.] __brk
2.95% slicemask [kernel.kallsyms] [k] slice_get_unmapped_area
2.81% slicemask [kernel.kallsyms] [k] __vm_enough_memory
2.78% slicemask [kernel.kallsyms] [k] kmem_cache_free
2.51% slicemask [kernel.kallsyms] [k] perf_iterate_ctx.constprop.84
2.40% slicemask [kernel.kallsyms] [k] unmap_page_range
2.27% slicemask [kernel.kallsyms] [k] perf_iterate_sb
2.21% slicemask [kernel.kallsyms] [k] vmacache_find
2.04% slicemask [kernel.kallsyms] [k] vma_gap_update
1.91% slicemask [kernel.kallsyms] [k] unmap_region
1.81% slicemask [kernel.kallsyms] [k] memset_nocache_branch
1.59% slicemask [kernel.kallsyms] [k] kmem_cache_alloc
1.57% slicemask [kernel.kallsyms] [k] get_unmapped_area.part.7
1.55% slicemask [kernel.kallsyms] [k] up_write
1.44% slicemask [kernel.kallsyms] [k] vma_merge
1.28% slicemask slicemask [.] main
1.27% slicemask [kernel.kallsyms] [k] lru_add_drain
1.22% slicemask [kernel.kallsyms] [k] vma_link
1.19% slicemask [kernel.kallsyms] [k] tlb_gather_mmu
1.17% slicemask [kernel.kallsyms] [k] tlb_flush_mmu_free
1.15% slicemask libc-2.23.so [.] got_label
1.11% slicemask [kernel.kallsyms] [k] unlink_anon_vmas
1.06% slicemask [kernel.kallsyms] [k] lru_add_drain_cpu
1.02% slicemask [kernel.kallsyms] [k] free_pgtables
1.01% slicemask [kernel.kallsyms] [k] remove_vma
0.98% slicemask [kernel.kallsyms] [k] strlcpy
0.98% slicemask [kernel.kallsyms] [k] perf_event_mmap_output
0.95% slicemask [kernel.kallsyms] [k] may_expand_vm
0.90% slicemask [kernel.kallsyms] [k] unmap_vmas
0.86% slicemask [kernel.kallsyms] [k] down_write_killable
0.83% slicemask [kernel.kallsyms] [k] __vma_link_list
0.83% slicemask [kernel.kallsyms] [k] arch_vma_name
0.81% slicemask [kernel.kallsyms] [k] __vma_rb_erase
0.80% slicemask [kernel.kallsyms] [k] __rcu_read_unlock
0.71% slicemask [kernel.kallsyms] [k] tlb_flush_mmu
0.70% slicemask [kernel.kallsyms] [k] tlb_finish_mmu
0.68% slicemask [kernel.kallsyms] [k] __rb_insert_augmented
0.63% slicemask [kernel.kallsyms] [k] cap_capable
0.61% slicemask [kernel.kallsyms] [k] free_pgd_range
0.59% slicemask [kernel.kallsyms] [k] arch_tlb_finish_mmu
0.59% slicemask [kernel.kallsyms] [k] __vma_link_rb
0.56% slicemask [kernel.kallsyms] [k] __rcu_read_lock
0.55% slicemask [kernel.kallsyms] [k]
arch_get_unmapped_area_topdown
0.53% slicemask [kernel.kallsyms] [k] unlink_file_vma
0.51% slicemask [kernel.kallsyms] [k] vmacache_update
0.50% slicemask [kernel.kallsyms] [k] kfree
Unfortunatly I didn't run a perf report before applying the patch serie.
If you are interested for the comparison, I won't be able to do it
before next week.
>
>> run with v4 now that Aneesh has tagged all of them as reviewed-by himself.
>> Once the serie has been accepted, my next step will be to backport at
>> least the 3 first ones in kernel 4.14
>>
>>>
>>>>
>>>> Had to modify your serie a bit, if you are interested I can post it.
>>>>
>>>
>>> Sure, that would be good.
>>
>> Ok, lets share it. The patch are not 100% clean.
>
> Those look pretty good, thanks for doing that work.
You are welcome. I wanted to try your serie on the 8xx. It is untested
on the book3s64, not sure it even compiles.
Christophe
>
> Thanks,
> Nick
>
^ permalink raw reply
* [PATCH] cxl: Check if PSL data-cache is available before issue flush request
From: Vaibhav Jain @ 2018-02-13 11:10 UTC (permalink / raw)
To: linuxppc-dev, Frederic Barrat
Cc: Vaibhav Jain, Andrew Donnellan, Christophe Lombard,
Philippe Bergheaud, Alastair D'Silva
PSL9D doesn't have a data-cache that needs to be flushed before
resetting the card. However when cxl tries to flush data-cache on such
a card, it times-out as PSL_Control register never indicates flush
operation complete due to missing data-cache. This is usually
indicated in the kernel logs with this message:
"WARNING: cache flush timed out"
To fix this the patch checks PSL_Debug register CDC-Field(BIT:27)
which indicates the absence of a data-cache and sets a flag
'no_data_cache' in 'struct cxl_native' to indicate this. When
cxl_data_cache_flush() is called it checks the flag and if set bails
out early without requesting a data-cache flush operation to the PSL.
Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
---
drivers/misc/cxl/cxl.h | 4 ++++
drivers/misc/cxl/native.c | 11 ++++++++++-
drivers/misc/cxl/pci.c | 19 +++++++++++++------
3 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index 4f015da78f28..4949b8d5a748 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h
@@ -369,6 +369,9 @@ static const cxl_p2n_reg_t CXL_PSL_WED_An = {0x0A0};
#define CXL_PSL_TFC_An_AE (1ull << (63-30)) /* Restart PSL with address error */
#define CXL_PSL_TFC_An_R (1ull << (63-31)) /* Restart PSL transaction */
+/****** CXL_PSL_DEBUG *****************************************************/
+#define CXL_PSL_DEBUG_CDC (1ull << (63-27)) /* Coherent Data cache support */
+
/****** CXL_XSL9_IERAT_ERAT - CAIA 2 **********************************/
#define CXL_XSL9_IERAT_MLPID (1ull << (63-0)) /* Match LPID */
#define CXL_XSL9_IERAT_MPID (1ull << (63-1)) /* Match PID */
@@ -669,6 +672,7 @@ struct cxl_native {
irq_hw_number_t err_hwirq;
unsigned int err_virq;
u64 ps_off;
+ bool no_data_cache; /* set if no data cache on the card */
const struct cxl_service_layer_ops *sl_ops;
};
diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
index 1b3d7c65ea3f..98f867fcef24 100644
--- a/drivers/misc/cxl/native.c
+++ b/drivers/misc/cxl/native.c
@@ -353,8 +353,17 @@ int cxl_data_cache_flush(struct cxl *adapter)
u64 reg;
unsigned long timeout = jiffies + (HZ * CXL_TIMEOUT);
- pr_devel("Flushing data cache\n");
+ /*
+ * Do a datacache flush only if datacache is available.
+ * In case of PSL9D datacache absent hence flush operation.
+ * would timeout.
+ */
+ if (adapter->native->no_data_cache) {
+ pr_devel("No PSL data cache. Ignoring cache flush req.\n");
+ return 0;
+ }
+ pr_devel("Flushing data cache\n");
reg = cxl_p1_read(adapter, CXL_PSL_Control);
reg |= CXL_PSL_Control_Fr;
cxl_p1_write(adapter, CXL_PSL_Control, reg);
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 758842f65a1b..39ddf89c3c14 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -456,6 +456,7 @@ static int init_implementation_adapter_regs_psl9(struct cxl *adapter,
u64 chipid;
u32 phb_index;
u64 capp_unit_id;
+ u64 psl_debug;
int rc;
rc = cxl_calc_capp_routing(dev, &chipid, &phb_index, &capp_unit_id);
@@ -506,6 +507,16 @@ static int init_implementation_adapter_regs_psl9(struct cxl *adapter,
} else
cxl_p1_write(adapter, CXL_PSL9_DEBUG, 0x4000000000000000ULL);
+ /* Check if PSL has data-cache. We need to flush adapter datacache
+ * when as its about to be removed. But data-cache flush is not
+ * supported supported on P9-DD1 and
+ */
+ psl_debug = cxl_p1_read(adapter, CXL_PSL9_DEBUG);
+ if (cxl_is_power9_dd1() || (psl_debug & CXL_PSL_DEBUG_CDC)) {
+ dev_info(&dev->dev, "No data-cache present\n");
+ adapter->native->no_data_cache = true;
+ }
+
return 0;
}
@@ -1449,10 +1460,8 @@ int cxl_pci_reset(struct cxl *adapter)
/*
* The adapter is about to be reset, so ignore errors.
- * Not supported on P9 DD1
*/
- if ((cxl_is_power8()) || (!(cxl_is_power9_dd1())))
- cxl_data_cache_flush(adapter);
+ cxl_data_cache_flush(adapter);
/* pcie_warm_reset requests a fundamental pci reset which includes a
* PERST assert/deassert. PERST triggers a loading of the image
@@ -1936,10 +1945,8 @@ static void cxl_pci_remove_adapter(struct cxl *adapter)
/*
* Flush adapter datacache as its about to be removed.
- * Not supported on P9 DD1.
*/
- if ((cxl_is_power8()) || (!(cxl_is_power9_dd1())))
- cxl_data_cache_flush(adapter);
+ cxl_data_cache_flush(adapter);
cxl_deconfigure_adapter(adapter);
--
2.14.3
^ permalink raw reply related
* [PATCH V3] powerpc/mm/hash64: memset the pagetable pages on allocation.
From: Aneesh Kumar K.V @ 2018-02-13 11:09 UTC (permalink / raw)
To: benh, paulus, mpe, Ram Pai; +Cc: linuxppc-dev, Aneesh Kumar K.V
On powerpc we allocate page table pages from slab cache of different sizes. For
now we have a constructor that zero out the objects when we allocate then for
the first time. We expect the objects to be zeroed out when we free the the
object back to slab cache. This happens in the unmap path. For hugetlb pages
we call huge_pte_get_and_clear to do that. With the current configuration of
page table size, both pud and pgd level tables get allocated from the same slab
cache. At the pud level, we use the second half of the table to store the slot
information. But never clear that when unmapping. When such an freed object get
allocated at pgd level, we will have part of the page table page not initlaized
correctly. This result in kernel crash
Simplify this by calling the object initialization after kmem_cache_alloc
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/book3s/64/pgalloc.h | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/pgalloc.h b/arch/powerpc/include/asm/book3s/64/pgalloc.h
index 53df86d3cfce..e4d154a4d114 100644
--- a/arch/powerpc/include/asm/book3s/64/pgalloc.h
+++ b/arch/powerpc/include/asm/book3s/64/pgalloc.h
@@ -73,10 +73,13 @@ static inline void radix__pgd_free(struct mm_struct *mm, pgd_t *pgd)
static inline pgd_t *pgd_alloc(struct mm_struct *mm)
{
+ pgd_t *pgd;
if (radix_enabled())
return radix__pgd_alloc(mm);
- return kmem_cache_alloc(PGT_CACHE(PGD_INDEX_SIZE),
- pgtable_gfp_flags(mm, GFP_KERNEL));
+ pgd = kmem_cache_alloc(PGT_CACHE(PGD_INDEX_SIZE),
+ pgtable_gfp_flags(mm, GFP_KERNEL));
+ memset(pgd, 0, PGD_TABLE_SIZE);
+ return pgd;
}
static inline void pgd_free(struct mm_struct *mm, pgd_t *pgd)
--
2.14.3
^ permalink raw reply related
* Re: [PATCH 2/3] cxl: Introduce module parameter 'enable_psltrace'
From: Vaibhav Jain @ 2018-02-13 11:07 UTC (permalink / raw)
To: Frederic Barrat, christophe lombard, linuxppc-dev
Cc: Philippe Bergheaud, Alastair D'Silva, Christophe Lombard,
Andrew Donnellan
In-Reply-To: <51364d03-d7a5-471b-2531-cf4c17af8c8d@linux.vnet.ibm.com>
Frederic Barrat <fbarrat@linux.vnet.ibm.com> writes:
> Le 11/02/2018 =C3=A0 18:10, Vaibhav Jain a =C3=A9crit=C2=A0:
>> Thanks for reviewing the patch Christophe,
>>=20
>> christophe lombard <clombard@linux.vnet.ibm.com> writes:
>>>> +bool cxl_enable_psltrace =3D true;
>>>> +module_param_named(enable_psltrace, cxl_enable_psltrace, bool, 0600);
>>>> +MODULE_PARM_DESC(enable_psltrace, "Set PSL traces on probe. default: =
on");
>>>> +
>>> I am not too agree to add a new parameter. This can cause doubts.
>>> PSL team has confirmed that enabling traces has no impact.
>>> Do you see any reason to disable the traces ?
>>=20
>> Traces on PSL follow a 'set and fetch' model. So once the trace buffer f=
or
>> a specific array is full it will stop and switch to 'FIN' state and at
>> that point we need to fetch the trace-data and reinit the array to
>> re-arm it.
>
> If the PSL trace arrays don't wrap, is there anything to gain by=20
> enabling tracing by default instead of letting the developer handle it=20
> through sysfs? I was under the (now wrong) impression that the PSL would=
=20
> wrap.
Enabling the traces quickly enough should let AFU developers debug init
issues. Specifically AFU's that rely on cxl kernel-apis.
> I'm not a big fan of the module parameter. It seems we're giving a=20
> second way of activating traces on top of sysfs, more cumbersome and=20
> limited.
Yes, this indeed is providing a second way of activating traces on top
of sysfs. The way I see this that there are two ways PSL traces are
managed:
1. Let userspace handle state machine of the traces entirely via sysfs.
2. PSL trace machine is handled via cxl. It starts it when a card is
probed and stops it when the card is reset.
--=20
Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.
^ permalink raw reply
* Re: samples/seccomp/ broken when cross compiling s390, ppc allyesconfig
From: Michal Hocko @ 2018-02-13 10:32 UTC (permalink / raw)
To: Michael Ellerman; +Cc: Kees Cook, Will Drewry, linux-s390, PowerPC, LKML
In-Reply-To: <87d1192nzc.fsf@concordia.ellerman.id.au>
On Tue 13-02-18 21:16:55, Michael Ellerman wrote:
> Kees Cook <keescook@chromium.org> writes:
>
> > On Mon, Feb 12, 2018 at 7:25 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> >> Michal Hocko <mhocko@kernel.org> writes:
> >>> Hi,
> >>> my build test machinery chokes on samples/seccomp when cross compiling
> >>> s390 and ppc64 allyesconfig. This has been the case for quite some
> >>> time already but I never found time to look at the problem and report
> >>> it. It seems this is not new issue and similar thing happend for
> >>> MIPS e9107f88c985 ("samples/seccomp/Makefile: do not build tests if
> >>> cross-compiling for MIPS").
> >>>
> >>> The build logs are attached.
> >>>
> >>> What is the best way around this? Should we simply skip compilation on
> >>> cross compile or is actually anybody relying on that? Or should I simply
> >>> disable it for s390 and ppc?
> >>
> >> The whole thing seems very confused. It's not building for the target,
> >> it's building for the host, ie. the Makefile sets hostprogs-m and
> >> HOSTCFLAGS etc.
> >>
> >> So it can't possibly work with cross compiling as it's currently
> >> written.
> >>
> >> Either the Makefile needs some serious work to properly support cross
> >> compiling or it should just be disabled when cross compiling.
> >
> > Hrm, yeah, the goal was to entirely disable cross compiling, but I
> > guess we didn't hit it with a hard enough hammer. :)
>
> Do you know why it is written that way? Why doesn't it just try to cross
> compile like normal code?
No idea, sorry. All I know about this code is that it breaks my build
testing.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] powerpc/xive: use hw CPU ids when configuring the CPU queues
From: Michael Ellerman @ 2018-02-13 10:18 UTC (permalink / raw)
To: Cédric Le Goater, linuxppc-dev
Cc: Benjamin Herrenschmidt, Cédric Le Goater
In-Reply-To: <20180213084712.18351-1-clg@kaod.org>
C=C3=A9dric Le Goater <clg@kaod.org> writes:
> The CPU event notification queues on sPAPR should be configured using
> a hardware CPU identifier.
>
> The problem did not show up on the Power Hypervisor because pHyp
> supports 8 threads per core which keeps CPU number contiguous. This is
> not the case on all sPAPR virtual machines, some use SMT=3D1.
>
> Also improve error logging by adding the CPU number.
>
> Signed-off-by: C=C3=A9dric Le Goater <clg@kaod.org>
> ---
>
> I think we should send this one to stable also.
Fixes: eac1e731b59e ("powerpc/xive: guest exploitation of the XIVE interrup=
t controller")
Cc: stable@vger.kernel.org # v4.14+
?
cheers
> diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/=
spapr.c
> index d9c4c9366049..091f1d0d0af1 100644
> --- a/arch/powerpc/sysdev/xive/spapr.c
> +++ b/arch/powerpc/sysdev/xive/spapr.c
> @@ -356,7 +356,8 @@ static int xive_spapr_configure_queue(u32 target, str=
uct xive_q *q, u8 prio,
>=20=20
> rc =3D plpar_int_get_queue_info(0, target, prio, &esn_page, &esn_size);
> if (rc) {
> - pr_err("Error %lld getting queue info prio %d\n", rc, prio);
> + pr_err("Error %lld getting queue info CPU %d prio %d\n", rc,
> + target, prio);
> rc =3D -EIO;
> goto fail;
> }
> @@ -370,7 +371,8 @@ static int xive_spapr_configure_queue(u32 target, str=
uct xive_q *q, u8 prio,
> /* Configure and enable the queue in HW */
> rc =3D plpar_int_set_queue_config(flags, target, prio, qpage_phys, orde=
r);
> if (rc) {
> - pr_err("Error %lld setting queue for prio %d\n", rc, prio);
> + pr_err("Error %lld setting queue for CPU %d prio %d\n", rc,
> + target, prio);
> rc =3D -EIO;
> } else {
> q->qpage =3D qpage;
> @@ -389,8 +391,8 @@ static int xive_spapr_setup_queue(unsigned int cpu, s=
truct xive_cpu *xc,
> if (IS_ERR(qpage))
> return PTR_ERR(qpage);
>=20=20
> - return xive_spapr_configure_queue(cpu, q, prio, qpage,
> - xive_queue_shift);
> + return xive_spapr_configure_queue(get_hard_smp_processor_id(cpu),
> + q, prio, qpage, xive_queue_shift);
> }
>=20=20
> static void xive_spapr_cleanup_queue(unsigned int cpu, struct xive_cpu *=
xc,
> @@ -399,10 +401,12 @@ static void xive_spapr_cleanup_queue(unsigned int c=
pu, struct xive_cpu *xc,
> struct xive_q *q =3D &xc->queue[prio];
> unsigned int alloc_order;
> long rc;
> + int hw_cpu =3D get_hard_smp_processor_id(cpu);
>=20=20
> - rc =3D plpar_int_set_queue_config(0, cpu, prio, 0, 0);
> + rc =3D plpar_int_set_queue_config(0, hw_cpu, prio, 0, 0);
> if (rc)
> - pr_err("Error %ld setting queue for prio %d\n", rc, prio);
> + pr_err("Error %ld setting queue for CPU %d prio %d\n", rc,
> + hw_cpu, prio);
>=20=20
> alloc_order =3D xive_alloc_order(xive_queue_shift);
> free_pages((unsigned long)q->qpage, alloc_order);
> --=20
> 2.13.6
^ permalink raw reply
* Re: samples/seccomp/ broken when cross compiling s390, ppc allyesconfig
From: Michael Ellerman @ 2018-02-13 10:16 UTC (permalink / raw)
To: Kees Cook; +Cc: Michal Hocko, Will Drewry, linux-s390, PowerPC, LKML
In-Reply-To: <CAGXu5j+DnkUv9pZn93wxA6_2s+c3Ap_Q34OR0weY7Y9MJ1aP9Q@mail.gmail.com>
Kees Cook <keescook@chromium.org> writes:
> On Mon, Feb 12, 2018 at 7:25 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Michal Hocko <mhocko@kernel.org> writes:
>>> Hi,
>>> my build test machinery chokes on samples/seccomp when cross compiling
>>> s390 and ppc64 allyesconfig. This has been the case for quite some
>>> time already but I never found time to look at the problem and report
>>> it. It seems this is not new issue and similar thing happend for
>>> MIPS e9107f88c985 ("samples/seccomp/Makefile: do not build tests if
>>> cross-compiling for MIPS").
>>>
>>> The build logs are attached.
>>>
>>> What is the best way around this? Should we simply skip compilation on
>>> cross compile or is actually anybody relying on that? Or should I simply
>>> disable it for s390 and ppc?
>>
>> The whole thing seems very confused. It's not building for the target,
>> it's building for the host, ie. the Makefile sets hostprogs-m and
>> HOSTCFLAGS etc.
>>
>> So it can't possibly work with cross compiling as it's currently
>> written.
>>
>> Either the Makefile needs some serious work to properly support cross
>> compiling or it should just be disabled when cross compiling.
>
> Hrm, yeah, the goal was to entirely disable cross compiling, but I
> guess we didn't hit it with a hard enough hammer. :)
Do you know why it is written that way? Why doesn't it just try to cross
compile like normal code?
cheers
^ permalink raw reply
* Re: [PATCH] headers: untangle kmemleak.h from mm.h
From: Michael Ellerman @ 2018-02-13 10:09 UTC (permalink / raw)
To: Randy Dunlap, LKML, Linux MM, Andrew Morton, Fengguang Wu
Cc: linux-s390, John Johansen, netdev@vger.kernel.org, X86 ML,
linux-wireless, virtualization, iommu, Greg Kroah-Hartman,
sparclinux, linuxppc-dev, Dmitry Kasatkin
In-Reply-To: <f119a273-2e86-1b7f-346f-7627ad8b51ed@infradead.org>
Randy Dunlap <rdunlap@infradead.org> writes:
> On 02/12/2018 04:28 AM, Michael Ellerman wrote:
>> Randy Dunlap <rdunlap@infradead.org> writes:
>>
>>> From: Randy Dunlap <rdunlap@infradead.org>
>>>
>>> Currently <linux/slab.h> #includes <linux/kmemleak.h> for no obvious
>>> reason. It looks like it's only a convenience, so remove kmemleak.h
>>> from slab.h and add <linux/kmemleak.h> to any users of kmemleak_*
>>> that don't already #include it.
>>> Also remove <linux/kmemleak.h> from source files that do not use it.
>>>
>>> This is tested on i386 allmodconfig and x86_64 allmodconfig. It
>>> would be good to run it through the 0day bot for other $ARCHes.
>>> I have neither the horsepower nor the storage space for the other
>>> $ARCHes.
>>>
>>> [slab.h is the second most used header file after module.h; kernel.h
>>> is right there with slab.h. There could be some minor error in the
>>> counting due to some #includes having comments after them and I
>>> didn't combine all of those.]
>>>
>>> This is Lingchi patch #1 (death by a thousand cuts, applied to kernel
>>> header files).
>>>
>>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
>>
>> I threw it at a random selection of configs and so far the only failures
>> I'm seeing are:
>>
>> lib/test_firmware.c:134:2: error: implicit declaration of function 'vfree' [-Werror=implicit-function-declaration]
>> lib/test_firmware.c:620:25: error: implicit declaration of function 'vzalloc' [-Werror=implicit-function-declaration]
>> lib/test_firmware.c:620:2: error: implicit declaration of function 'vzalloc' [-Werror=implicit-function-declaration]
>> security/integrity/digsig.c:146:2: error: implicit declaration of function 'vfree' [-Werror=implicit-function-declaration]
>
> Both of those source files need to #include <linux/vmalloc.h>.
Yep, I added those and rebuilt. I don't see any more failures that look
related to your patch.
http://kisskb.ellerman.id.au/kisskb/head/13399/
I haven't gone through the defconfigs I have enabled for a while, so
it's possible I have some missing but it's still a reasonable cross
section.
cheers
^ permalink raw reply
* [PATCH] powerpc/xive: use hw CPU ids when configuring the CPU queues
From: Cédric Le Goater @ 2018-02-13 8:47 UTC (permalink / raw)
To: linuxppc-dev
Cc: Michael Ellerman, Benjamin Herrenschmidt, Cédric Le Goater
The CPU event notification queues on sPAPR should be configured using
a hardware CPU identifier.
The problem did not show up on the Power Hypervisor because pHyp
supports 8 threads per core which keeps CPU number contiguous. This is
not the case on all sPAPR virtual machines, some use SMT=1.
Also improve error logging by adding the CPU number.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
I think we should send this one to stable also.
arch/powerpc/sysdev/xive/spapr.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c
index d9c4c9366049..091f1d0d0af1 100644
--- a/arch/powerpc/sysdev/xive/spapr.c
+++ b/arch/powerpc/sysdev/xive/spapr.c
@@ -356,7 +356,8 @@ static int xive_spapr_configure_queue(u32 target, struct xive_q *q, u8 prio,
rc = plpar_int_get_queue_info(0, target, prio, &esn_page, &esn_size);
if (rc) {
- pr_err("Error %lld getting queue info prio %d\n", rc, prio);
+ pr_err("Error %lld getting queue info CPU %d prio %d\n", rc,
+ target, prio);
rc = -EIO;
goto fail;
}
@@ -370,7 +371,8 @@ static int xive_spapr_configure_queue(u32 target, struct xive_q *q, u8 prio,
/* Configure and enable the queue in HW */
rc = plpar_int_set_queue_config(flags, target, prio, qpage_phys, order);
if (rc) {
- pr_err("Error %lld setting queue for prio %d\n", rc, prio);
+ pr_err("Error %lld setting queue for CPU %d prio %d\n", rc,
+ target, prio);
rc = -EIO;
} else {
q->qpage = qpage;
@@ -389,8 +391,8 @@ static int xive_spapr_setup_queue(unsigned int cpu, struct xive_cpu *xc,
if (IS_ERR(qpage))
return PTR_ERR(qpage);
- return xive_spapr_configure_queue(cpu, q, prio, qpage,
- xive_queue_shift);
+ return xive_spapr_configure_queue(get_hard_smp_processor_id(cpu),
+ q, prio, qpage, xive_queue_shift);
}
static void xive_spapr_cleanup_queue(unsigned int cpu, struct xive_cpu *xc,
@@ -399,10 +401,12 @@ static void xive_spapr_cleanup_queue(unsigned int cpu, struct xive_cpu *xc,
struct xive_q *q = &xc->queue[prio];
unsigned int alloc_order;
long rc;
+ int hw_cpu = get_hard_smp_processor_id(cpu);
- rc = plpar_int_set_queue_config(0, cpu, prio, 0, 0);
+ rc = plpar_int_set_queue_config(0, hw_cpu, prio, 0, 0);
if (rc)
- pr_err("Error %ld setting queue for prio %d\n", rc, prio);
+ pr_err("Error %ld setting queue for CPU %d prio %d\n", rc,
+ hw_cpu, prio);
alloc_order = xive_alloc_order(xive_queue_shift);
free_pages((unsigned long)q->qpage, alloc_order);
--
2.13.6
^ permalink raw reply related
* Re: samples/seccomp/ broken when cross compiling s390, ppc allyesconfig
From: Michal Hocko @ 2018-02-13 8:59 UTC (permalink / raw)
To: Kees Cook; +Cc: Michael Ellerman, Will Drewry, linux-s390, PowerPC, LKML
In-Reply-To: <CAGXu5j+DnkUv9pZn93wxA6_2s+c3Ap_Q34OR0weY7Y9MJ1aP9Q@mail.gmail.com>
On Mon 12-02-18 21:54:39, Kees Cook wrote:
> On Mon, Feb 12, 2018 at 7:25 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> > Michal Hocko <mhocko@kernel.org> writes:
> >> Hi,
> >> my build test machinery chokes on samples/seccomp when cross compiling
> >> s390 and ppc64 allyesconfig. This has been the case for quite some
> >> time already but I never found time to look at the problem and report
> >> it. It seems this is not new issue and similar thing happend for
> >> MIPS e9107f88c985 ("samples/seccomp/Makefile: do not build tests if
> >> cross-compiling for MIPS").
> >>
> >> The build logs are attached.
> >>
> >> What is the best way around this? Should we simply skip compilation on
> >> cross compile or is actually anybody relying on that? Or should I simply
> >> disable it for s390 and ppc?
> >
> > The whole thing seems very confused. It's not building for the target,
> > it's building for the host, ie. the Makefile sets hostprogs-m and
> > HOSTCFLAGS etc.
> >
> > So it can't possibly work with cross compiling as it's currently
> > written.
> >
> > Either the Makefile needs some serious work to properly support cross
> > compiling or it should just be disabled when cross compiling.
>
> Hrm, yeah, the goal was to entirely disable cross compiling, but I
> guess we didn't hit it with a hard enough hammer. :)
Hammer like this?
diff --git a/samples/seccomp/Makefile b/samples/seccomp/Makefile
index 0e349b80686e..ba942e3ead89 100644
--- a/samples/seccomp/Makefile
+++ b/samples/seccomp/Makefile
@@ -1,4 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
+ifndef CROSS_COMPILE
hostprogs-$(CONFIG_SAMPLE_SECCOMP) := bpf-fancy dropper bpf-direct
HOSTCFLAGS_bpf-fancy.o += -I$(objtree)/usr/include
@@ -16,7 +17,6 @@ HOSTCFLAGS_bpf-direct.o += -idirafter $(objtree)/include
bpf-direct-objs := bpf-direct.o
# Try to match the kernel target.
-ifndef CROSS_COMPILE
ifndef CONFIG_64BIT
# s390 has -m31 flag to build 31 bit binaries
@@ -35,12 +35,4 @@ HOSTLOADLIBES_bpf-fancy += $(MFLAG)
HOSTLOADLIBES_dropper += $(MFLAG)
endif
always := $(hostprogs-m)
-else
-# MIPS system calls are defined based on the -mabi that is passed
-# to the toolchain which may or may not be a valid option
-# for the host toolchain. So disable tests if target architecture
-# is MIPS but the host isn't.
-ifndef CONFIG_MIPS
-always := $(hostprogs-m)
-endif
endif
--
Michal Hocko
SUSE Labs
^ permalink raw reply related
* Re: [RFC PATCH 0/5] powerpc/mm/slice: improve slice speed and stack use
From: Nicholas Piggin @ 2018-02-13 8:40 UTC (permalink / raw)
To: Christophe LEROY; +Cc: linuxppc-dev, Aneesh Kumar K . V, Michael Ellerman
In-Reply-To: <7364b502-83a7-8b40-3530-b160e3c80523@c-s.fr>
On Mon, 12 Feb 2018 18:42:21 +0100
Christophe LEROY <christophe.leroy@c-s.fr> wrote:
> Le 12/02/2018 à 16:24, Nicholas Piggin a écrit :
> > On Mon, 12 Feb 2018 16:02:23 +0100
> > Christophe LEROY <christophe.leroy@c-s.fr> wrote:
> >
> >> Le 10/02/2018 à 09:11, Nicholas Piggin a écrit :
> >>> This series intends to improve performance and reduce stack
> >>> consumption in the slice allocation code. It does it by keeping slice
> >>> masks in the mm_context rather than compute them for each allocation,
> >>> and by reducing bitmaps and slice_masks from stacks, using pointers
> >>> instead where possible.
> >>>
> >>> checkstack.pl gives, before:
> >>> 0x00000de4 slice_get_unmapped_area [slice.o]: 656
> >>> 0x00001b4c is_hugepage_only_range [slice.o]: 512
> >>> 0x0000075c slice_find_area_topdown [slice.o]: 416
> >>> 0x000004c8 slice_find_area_bottomup.isra.1 [slice.o]: 272
> >>> 0x00001aa0 slice_set_range_psize [slice.o]: 240
> >>> 0x00000a64 slice_find_area [slice.o]: 176
> >>> 0x00000174 slice_check_fit [slice.o]: 112
> >>>
> >>> after:
> >>> 0x00000d70 slice_get_unmapped_area [slice.o]: 320
> >>> 0x000008f8 slice_find_area [slice.o]: 144
> >>> 0x00001860 slice_set_range_psize [slice.o]: 144
> >>> 0x000018ec is_hugepage_only_range [slice.o]: 144
> >>> 0x00000750 slice_find_area_bottomup.isra.4 [slice.o]: 128
> >>>
> >>> The benchmark in https://github.com/linuxppc/linux/issues/49 gives, before:
> >>> $ time ./slicemask
> >>> real 0m20.712s
> >>> user 0m5.830s
> >>> sys 0m15.105s
> >>>
> >>> after:
> >>> $ time ./slicemask
> >>> real 0m13.197s
> >>> user 0m5.409s
> >>> sys 0m7.779s
> >>
> >> Hi,
> >>
> >> I tested your serie on an 8xx, on top of patch
> >> https://patchwork.ozlabs.org/patch/871675/
> >>
> >> I don't get a result as significant as yours, but there is some
> >> improvment anyway:
> >>
> >> ITERATION 500000
> >>
> >> Before:
> >>
> >> root@vgoip:~# time ./slicemask
> >> real 0m 33.26s
> >> user 0m 1.94s
> >> sys 0m 30.85s
> >>
> >> After:
> >> root@vgoip:~# time ./slicemask
> >> real 0m 29.69s
> >> user 0m 2.11s
> >> sys 0m 27.15s
> >>
> >> Most significant improvment is obtained with the first patch of your serie:
> >> root@vgoip:~# time ./slicemask
> >> real 0m 30.85s
> >> user 0m 1.80s
> >> sys 0m 28.57s
> >
> > Okay, thanks. Are you still spending significant time in the slice
> > code?
>
> Do you mean am I still updating my patches ? No I hope we are at last
Actually I was wondering about CPU time spent for the microbenchmark :)
> run with v4 now that Aneesh has tagged all of them as reviewed-by himself.
> Once the serie has been accepted, my next step will be to backport at
> least the 3 first ones in kernel 4.14
>
> >
> >>
> >> Had to modify your serie a bit, if you are interested I can post it.
> >>
> >
> > Sure, that would be good.
>
> Ok, lets share it. The patch are not 100% clean.
Those look pretty good, thanks for doing that work.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 5/5] mtd: Stop updating erase_info->state and calling mtd_erase_callback()
From: Miquel Raynal @ 2018-02-13 8:33 UTC (permalink / raw)
To: Boris Brezillon
Cc: David Woodhouse, Brian Norris, Marek Vasut, Richard Weinberger,
Cyrille Pitchen, linux-mtd, Joern Engel, Robert Jarzmik,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Kyungmin Park, Artem Bityutskiy, Solarflare linux maintainers,
Edward Cree, Bert Kenward, Greg Kroah-Hartman, linuxppc-dev,
netdev, devel
In-Reply-To: <20180213091714.5eca4f45@bbrezillon>
Hi Boris,
On Tue, 13 Feb 2018 09:17:14 +0100, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> On Tue, 13 Feb 2018 08:42:46 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> > Hi Boris,
> >
> > Just a few comments about the form.
> >
> > Otherwise:
> > Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >
> >
> > > diff --git a/drivers/mtd/devices/lart.c b/drivers/mtd/devices/lart.c
> > > index 555b94406e0b..3d6c8ffd351f 100644
> > > --- a/drivers/mtd/devices/lart.c
> > > +++ b/drivers/mtd/devices/lart.c
> > > @@ -415,7 +415,6 @@ static int flash_erase (struct mtd_info *mtd,struct erase_info *instr)
> > > {
> > > if (!erase_block (addr))
> > > {
> > > - instr->state = MTD_ERASE_FAILED;
> > > return (-EIO);
> > > }
> >
> > You can also safely remove these '{' '}'
>
> Well, this patch is not about fixing coding style issues, otherwise I'd
> have a lot more work on this driver :-)
Sure, I was not referring to the weird style but just that you switch
from two to one line in the block, thus the braces are not needed
anymore.
>
> >
> > >
> > > @@ -425,9 +424,6 @@ static int flash_erase (struct mtd_info *mtd,struct erase_info *instr)
> > > if (addr == mtd->eraseregions[i].offset + (mtd->eraseregions[i].erasesize * mtd->eraseregions[i].numblocks)) i++;
> > > }
> > >
> > > - instr->state = MTD_ERASE_DONE;
> > > - mtd_erase_callback(instr);
> > > -
> > > return (0);
> > > }
> > >
> > > diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
> > > index 5dc8bd042cc5..aaaeaae01e1d 100644
> > > --- a/drivers/mtd/devices/mtd_dataflash.c
> > > +++ b/drivers/mtd/devices/mtd_dataflash.c
> > > @@ -220,10 +220,6 @@ static int dataflash_erase(struct mtd_info *mtd, struct erase_info *instr)
> > > }
> > > mutex_unlock(&priv->lock);
> > >
> > > - /* Inform MTD subsystem that erase is complete */
> > > - instr->state = MTD_ERASE_DONE;
> > > - mtd_erase_callback(instr);
> > > -
> > > return 0;
> > > }
> > >
> > > diff --git a/drivers/mtd/devices/mtdram.c b/drivers/mtd/devices/mtdram.c
> > > index 0bf4aeaf0cb8..efef43c6684b 100644
> > > --- a/drivers/mtd/devices/mtdram.c
> > > +++ b/drivers/mtd/devices/mtdram.c
> > > @@ -60,8 +60,6 @@ static int ram_erase(struct mtd_info *mtd, struct erase_info *instr)
> > > if (check_offs_len(mtd, instr->addr, instr->len))
> > > return -EINVAL;
> > > memset((char *)mtd->priv + instr->addr, 0xff, instr->len);
> > > - instr->state = MTD_ERASE_DONE;
> > > - mtd_erase_callback(instr);
> >
> > Space ?
>
> I could add a blank line, but again, I'm just following the coding style
> in place in this file :-).
Ok.
>
> >
> > > return 0;
> > > }
> > >
> > > diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> > > index 7287696a21f9..a963c88d392d 100644
> > > --- a/drivers/mtd/devices/phram.c
> > > +++ b/drivers/mtd/devices/phram.c
> > > @@ -44,8 +44,6 @@ static int phram_erase(struct mtd_info *mtd, struct erase_info *instr)
> > > * I don't feel at all ashamed. This kind of thing is possible anyway
> > > * with flash, but unlikely.
> > > */
> >
> > Not sure this comment is still relevant? Maybe you could remove it
> > or at least change it?
> >
> > > - instr->state = MTD_ERASE_DONE;
> > > - mtd_erase_callback(instr);
> >
> > Space ?
> >
> > > return 0;
> > > }
> > >
> > > diff --git a/drivers/mtd/devices/pmc551.c b/drivers/mtd/devices/pmc551.c
> > > index cadea0620cd0..5d842cbca3de 100644
> > > --- a/drivers/mtd/devices/pmc551.c
> > > +++ b/drivers/mtd/devices/pmc551.c
> > > @@ -184,12 +184,10 @@ static int pmc551_erase(struct mtd_info *mtd, struct erase_info *instr)
> > > }
> > >
> > > out:
> > > - instr->state = MTD_ERASE_DONE;
> > > #ifdef CONFIG_MTD_PMC551_DEBUG
> > > printk(KERN_DEBUG "pmc551_erase() done\n");
> > > #endif
> > >
> > > - mtd_erase_callback(instr);
> > > return 0;
> > > }
> > >
> > > diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
> > > index 26f9feaa5d17..5f383630c16f 100644
> > > --- a/drivers/mtd/devices/powernv_flash.c
> > > +++ b/drivers/mtd/devices/powernv_flash.c
> > > @@ -175,19 +175,12 @@ static int powernv_flash_erase(struct mtd_info *mtd, struct erase_info *erase)
> > > {
> > > int rc;
> > >
> > > - erase->state = MTD_ERASING;
> > > -
> > > /* todo: register our own notifier to do a true async implementation */
> > > rc = powernv_flash_async_op(mtd, FLASH_OP_ERASE, erase->addr,
> > > erase->len, NULL, NULL);
> >
> > Are you sure this is still needed? Maybe this should go away in your
> > first patch?
>
> Hm, indeed. This comment should be dropped.
>
> >
> > > -
> > > - if (rc) {
> > > + if (rc)
> > > erase->fail_addr = erase->addr;
> > > - erase->state = MTD_ERASE_FAILED;
> > > - } else {
> > > - erase->state = MTD_ERASE_DONE;
> > > - }
> > > - mtd_erase_callback(erase);
> > > +
> > > return rc;
> > > }
> > >
> > > diff --git a/drivers/mtd/devices/slram.c b/drivers/mtd/devices/slram.c
> > > index 0ec85f316d24..2f05e1801047 100644
> > > --- a/drivers/mtd/devices/slram.c
> > > +++ b/drivers/mtd/devices/slram.c
> > > @@ -88,8 +88,6 @@ static int slram_erase(struct mtd_info *mtd, struct erase_info *instr)
> > > * I don't feel at all ashamed. This kind of thing is possible anyway
> > > * with flash, but unlikely.
> > > */
> >
> > Same with this comment.
>
> Actually, I'm not sure I understand that comment, but I guess it's
> talking about races between read/write and erase paths, so nothing
> related to the changed I'm doing here.
I thought that races could only happen because of the asynchronous
manner how erases were handled, but if you feel this is still relevant,
I trust you :)
>
> >
> > > - instr->state = MTD_ERASE_DONE;
> > > - mtd_erase_callback(instr);
> >
> > Space ?
> >
> > > return(0);
> > > }
> > >
> >
> >
> >
> >
>
>
>
--
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
^ permalink raw reply
* Re: [PATCH 5/5] mtd: Stop updating erase_info->state and calling mtd_erase_callback()
From: Boris Brezillon @ 2018-02-13 8:17 UTC (permalink / raw)
To: Miquel Raynal
Cc: David Woodhouse, Brian Norris, Marek Vasut, Richard Weinberger,
Cyrille Pitchen, linux-mtd, Joern Engel, Robert Jarzmik,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Kyungmin Park, Artem Bityutskiy, Solarflare linux maintainers,
Edward Cree, Bert Kenward, Greg Kroah-Hartman, linuxppc-dev,
netdev, devel
In-Reply-To: <20180213084246.4f626bec@xps13>
On Tue, 13 Feb 2018 08:42:46 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> Hi Boris,
>
> Just a few comments about the form.
>
> Otherwise:
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
>
>
> > diff --git a/drivers/mtd/devices/lart.c b/drivers/mtd/devices/lart.c
> > index 555b94406e0b..3d6c8ffd351f 100644
> > --- a/drivers/mtd/devices/lart.c
> > +++ b/drivers/mtd/devices/lart.c
> > @@ -415,7 +415,6 @@ static int flash_erase (struct mtd_info *mtd,struct erase_info *instr)
> > {
> > if (!erase_block (addr))
> > {
> > - instr->state = MTD_ERASE_FAILED;
> > return (-EIO);
> > }
>
> You can also safely remove these '{' '}'
Well, this patch is not about fixing coding style issues, otherwise I'd
have a lot more work on this driver :-)
>
> >
> > @@ -425,9 +424,6 @@ static int flash_erase (struct mtd_info *mtd,struct erase_info *instr)
> > if (addr == mtd->eraseregions[i].offset + (mtd->eraseregions[i].erasesize * mtd->eraseregions[i].numblocks)) i++;
> > }
> >
> > - instr->state = MTD_ERASE_DONE;
> > - mtd_erase_callback(instr);
> > -
> > return (0);
> > }
> >
> > diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
> > index 5dc8bd042cc5..aaaeaae01e1d 100644
> > --- a/drivers/mtd/devices/mtd_dataflash.c
> > +++ b/drivers/mtd/devices/mtd_dataflash.c
> > @@ -220,10 +220,6 @@ static int dataflash_erase(struct mtd_info *mtd, struct erase_info *instr)
> > }
> > mutex_unlock(&priv->lock);
> >
> > - /* Inform MTD subsystem that erase is complete */
> > - instr->state = MTD_ERASE_DONE;
> > - mtd_erase_callback(instr);
> > -
> > return 0;
> > }
> >
> > diff --git a/drivers/mtd/devices/mtdram.c b/drivers/mtd/devices/mtdram.c
> > index 0bf4aeaf0cb8..efef43c6684b 100644
> > --- a/drivers/mtd/devices/mtdram.c
> > +++ b/drivers/mtd/devices/mtdram.c
> > @@ -60,8 +60,6 @@ static int ram_erase(struct mtd_info *mtd, struct erase_info *instr)
> > if (check_offs_len(mtd, instr->addr, instr->len))
> > return -EINVAL;
> > memset((char *)mtd->priv + instr->addr, 0xff, instr->len);
> > - instr->state = MTD_ERASE_DONE;
> > - mtd_erase_callback(instr);
>
> Space ?
I could add a blank line, but again, I'm just following the coding style
in place in this file :-).
>
> > return 0;
> > }
> >
> > diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> > index 7287696a21f9..a963c88d392d 100644
> > --- a/drivers/mtd/devices/phram.c
> > +++ b/drivers/mtd/devices/phram.c
> > @@ -44,8 +44,6 @@ static int phram_erase(struct mtd_info *mtd, struct erase_info *instr)
> > * I don't feel at all ashamed. This kind of thing is possible anyway
> > * with flash, but unlikely.
> > */
>
> Not sure this comment is still relevant? Maybe you could remove it
> or at least change it?
>
> > - instr->state = MTD_ERASE_DONE;
> > - mtd_erase_callback(instr);
>
> Space ?
>
> > return 0;
> > }
> >
> > diff --git a/drivers/mtd/devices/pmc551.c b/drivers/mtd/devices/pmc551.c
> > index cadea0620cd0..5d842cbca3de 100644
> > --- a/drivers/mtd/devices/pmc551.c
> > +++ b/drivers/mtd/devices/pmc551.c
> > @@ -184,12 +184,10 @@ static int pmc551_erase(struct mtd_info *mtd, struct erase_info *instr)
> > }
> >
> > out:
> > - instr->state = MTD_ERASE_DONE;
> > #ifdef CONFIG_MTD_PMC551_DEBUG
> > printk(KERN_DEBUG "pmc551_erase() done\n");
> > #endif
> >
> > - mtd_erase_callback(instr);
> > return 0;
> > }
> >
> > diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
> > index 26f9feaa5d17..5f383630c16f 100644
> > --- a/drivers/mtd/devices/powernv_flash.c
> > +++ b/drivers/mtd/devices/powernv_flash.c
> > @@ -175,19 +175,12 @@ static int powernv_flash_erase(struct mtd_info *mtd, struct erase_info *erase)
> > {
> > int rc;
> >
> > - erase->state = MTD_ERASING;
> > -
> > /* todo: register our own notifier to do a true async implementation */
> > rc = powernv_flash_async_op(mtd, FLASH_OP_ERASE, erase->addr,
> > erase->len, NULL, NULL);
>
> Are you sure this is still needed? Maybe this should go away in your
> first patch?
Hm, indeed. This comment should be dropped.
>
> > -
> > - if (rc) {
> > + if (rc)
> > erase->fail_addr = erase->addr;
> > - erase->state = MTD_ERASE_FAILED;
> > - } else {
> > - erase->state = MTD_ERASE_DONE;
> > - }
> > - mtd_erase_callback(erase);
> > +
> > return rc;
> > }
> >
> > diff --git a/drivers/mtd/devices/slram.c b/drivers/mtd/devices/slram.c
> > index 0ec85f316d24..2f05e1801047 100644
> > --- a/drivers/mtd/devices/slram.c
> > +++ b/drivers/mtd/devices/slram.c
> > @@ -88,8 +88,6 @@ static int slram_erase(struct mtd_info *mtd, struct erase_info *instr)
> > * I don't feel at all ashamed. This kind of thing is possible anyway
> > * with flash, but unlikely.
> > */
>
> Same with this comment.
Actually, I'm not sure I understand that comment, but I guess it's
talking about races between read/write and erase paths, so nothing
related to the changed I'm doing here.
>
> > - instr->state = MTD_ERASE_DONE;
> > - mtd_erase_callback(instr);
>
> Space ?
>
> > return(0);
> > }
> >
>
>
>
>
--
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
^ permalink raw reply
* [bug report] ocxl: Add AFU interrupt support
From: Dan Carpenter @ 2018-02-13 8:12 UTC (permalink / raw)
To: fbarrat; +Cc: linuxppc-dev
Hello Frederic Barrat,
The patch aeddad1760ae: "ocxl: Add AFU interrupt support" from Jan
23, 2018, leads to the following static checker warning:
drivers/misc/ocxl/file.c:163 afu_ioctl()
warn: maybe return -EFAULT instead of the bytes remaining?
drivers/misc/ocxl/file.c
111 static long afu_ioctl(struct file *file, unsigned int cmd,
112 unsigned long args)
113 {
114 struct ocxl_context *ctx = file->private_data;
115 struct ocxl_ioctl_irq_fd irq_fd;
116 u64 irq_offset;
117 long rc;
118
119 pr_debug("%s for context %d, command %s\n", __func__, ctx->pasid,
120 CMD_STR(cmd));
121
122 if (ctx->status == CLOSED)
123 return -EIO;
124
125 switch (cmd) {
126 case OCXL_IOCTL_ATTACH:
127 rc = afu_ioctl_attach(ctx,
128 (struct ocxl_ioctl_attach __user *) args);
129 break;
130
131 case OCXL_IOCTL_IRQ_ALLOC:
132 rc = ocxl_afu_irq_alloc(ctx, &irq_offset);
133 if (!rc) {
134 rc = copy_to_user((u64 __user *) args, &irq_offset,
135 sizeof(irq_offset));
136 if (rc)
^^
copy_to_user() returns the number of bytes remaining but we want to
return -EFAULT on error.
137 ocxl_afu_irq_free(ctx, irq_offset);
138 }
139 break;
140
drivers/misc/ocxl/file.c:320 afu_read()
warn: unsigned 'used' is never less than zero.
drivers/misc/ocxl/file.c
279 ssize_t rc;
280 size_t used = 0;
^^^^^^
This should be ssize_t
281 DEFINE_WAIT(event_wait);
282
283 memset(&header, 0, sizeof(header));
284
285 /* Require offset to be 0 */
286 if (*off != 0)
287 return -EINVAL;
288
289 if (count < (sizeof(struct ocxl_kernel_event_header) +
290 AFU_EVENT_BODY_MAX_SIZE))
291 return -EINVAL;
292
293 for (;;) {
294 prepare_to_wait(&ctx->events_wq, &event_wait,
295 TASK_INTERRUPTIBLE);
296
297 if (afu_events_pending(ctx))
298 break;
299
300 if (ctx->status == CLOSED)
301 break;
302
303 if (file->f_flags & O_NONBLOCK) {
304 finish_wait(&ctx->events_wq, &event_wait);
305 return -EAGAIN;
306 }
307
308 if (signal_pending(current)) {
309 finish_wait(&ctx->events_wq, &event_wait);
310 return -ERESTARTSYS;
311 }
312
313 schedule();
314 }
315
316 finish_wait(&ctx->events_wq, &event_wait);
317
318 if (has_xsl_error(ctx)) {
319 used = append_xsl_error(ctx, &header, buf + sizeof(header));
320 if (used < 0)
^^^^^^^^
Impossible.
321 return used;
322 }
323
324 if (!afu_events_pending(ctx))
325 header.flags |= OCXL_KERNEL_EVENT_FLAG_LAST;
326
327 if (copy_to_user(buf, &header, sizeof(header)))
328 return -EFAULT;
329
330 used += sizeof(header);
331
332 rc = (ssize_t) used;
^^^^^^^^^^^^^^
You could remove the cast.
333 return rc;
334 }
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH v7 00/24] Speculative page faults
From: Laurent Dufour @ 2018-02-13 7:56 UTC (permalink / raw)
To: Andrew Morton
Cc: paulmck, peterz, kirill, ak, mhocko, dave, jack, Matthew Wilcox,
benh, mpe, paulus, Thomas Gleixner, Ingo Molnar, hpa, Will Deacon,
Sergey Senozhatsky, Andrea Arcangeli, Alexei Starovoitov,
kemi.wang, sergey.senozhatsky.work, Daniel Jordan, linux-kernel,
linux-mm, haren, khandual, npiggin, bsingharora, Tim Chen,
linuxppc-dev, x86
In-Reply-To: <20180208125301.99445c91979343756e4cca9b@linux-foundation.org>
On 08/02/2018 21:53, Andrew Morton wrote:
> On Tue, 6 Feb 2018 17:49:46 +0100 Laurent Dufour <ldufour@linux.vnet.ibm.com> wrote:
>
>> This is a port on kernel 4.15 of the work done by Peter Zijlstra to
>> handle page fault without holding the mm semaphore [1].
>>
>> The idea is to try to handle user space page faults without holding the
>> mmap_sem. This should allow better concurrency for massively threaded
>> process since the page fault handler will not wait for other threads memory
>> layout change to be done, assuming that this change is done in another part
>> of the process's memory space. This type page fault is named speculative
>> page fault. If the speculative page fault fails because of a concurrency is
>> detected or because underlying PMD or PTE tables are not yet allocating, it
>> is failing its processing and a classic page fault is then tried.
>>
>> The speculative page fault (SPF) has to look for the VMA matching the fault
>> address without holding the mmap_sem, this is done by introducing a rwlock
>> which protects the access to the mm_rb tree. Previously this was done using
>> SRCU but it was introducing a lot of scheduling to process the VMA's
>> freeing
>> operation which was hitting the performance by 20% as reported by Kemi Wang
>> [2].Using a rwlock to protect access to the mm_rb tree is limiting the
>> locking contention to these operations which are expected to be in a O(log
>> n)
>> order. In addition to ensure that the VMA is not freed in our back a
>> reference count is added and 2 services (get_vma() and put_vma()) are
>> introduced to handle the reference count. When a VMA is fetch from the RB
>> tree using get_vma() is must be later freeed using put_vma(). Furthermore,
>> to allow the VMA to be used again by the classic page fault handler a
>> service is introduced can_reuse_spf_vma(). This service is expected to be
>> called with the mmap_sem hold. It checked that the VMA is still matching
>> the specified address and is releasing its reference count as the mmap_sem
>> is hold it is ensure that it will not be freed in our back. In general, the
>> VMA's reference count could be decremented when holding the mmap_sem but it
>> should not be increased as holding the mmap_sem is ensuring that the VMA is
>> stable. I can't see anymore the overhead I got while will-it-scale
>> benchmark anymore.
>>
>> The VMA's attributes checked during the speculative page fault processing
>> have to be protected against parallel changes. This is done by using a per
>> VMA sequence lock. This sequence lock allows the speculative page fault
>> handler to fast check for parallel changes in progress and to abort the
>> speculative page fault in that case.
>>
>> Once the VMA is found, the speculative page fault handler would check for
>> the VMA's attributes to verify that the page fault has to be handled
>> correctly or not. Thus the VMA is protected through a sequence lock which
>> allows fast detection of concurrent VMA changes. If such a change is
>> detected, the speculative page fault is aborted and a *classic* page fault
>> is tried. VMA sequence lockings are added when VMA attributes which are
>> checked during the page fault are modified.
>>
>> When the PTE is fetched, the VMA is checked to see if it has been changed,
>> so once the page table is locked, the VMA is valid, so any other changes
>> leading to touching this PTE will need to lock the page table, so no
>> parallel change is possible at this time.
>>
>> The locking of the PTE is done with interrupts disabled, this allows to
>> check for the PMD to ensure that there is not an ongoing collapsing
>> operation. Since khugepaged is firstly set the PMD to pmd_none and then is
>> waiting for the other CPU to have catch the IPI interrupt, if the pmd is
>> valid at the time the PTE is locked, we have the guarantee that the
>> collapsing opertion will have to wait on the PTE lock to move foward. This
>> allows the SPF handler to map the PTE safely. If the PMD value is different
>> than the one recorded at the beginning of the SPF operation, the classic
>> page fault handler will be called to handle the operation while holding the
>> mmap_sem. As the PTE lock is done with the interrupts disabled, the lock is
>> done using spin_trylock() to avoid dead lock when handling a page fault
>> while a TLB invalidate is requested by an other CPU holding the PTE.
>>
>> Support for THP is not done because when checking for the PMD, we can be
>> confused by an in progress collapsing operation done by khugepaged. The
>> issue is that pmd_none() could be true either if the PMD is not already
>> populate or if the underlying PTE are in the way to be collapsed. So we
>> cannot safely allocate a PMD if pmd_none() is true.
>>
>> This series builds on top of v4.15-mmotm-2018-01-31-16-51 and is
>> functional on x86 and PowerPC.
>
> One question which people will want to answer is "is this thing
> working". ie, how frequently does the code fall back to the regular
> heavyweight fault path.
>
> I see that trace events have been added for this, but the overall
> changelog doesn't describe them. I think this material is important
> enough to justify including it here.
Got it, I'll detail the new perf and trace events here.
> Also, a few words to help people figure out how to gather these stats
> would be nice. And maybe helper scripts if appropriate?
I'll provide some command line examples detailing how to capture those events.
> I'm wondering if this info should even be presented via
> /proc/self/something, dunno.
My understanding is that this is part of the kernel ABI, so I was not comfortable
to touch it but if needed I could probably put some numbers there.
> And it would be interesting to present the fallback frequency in the
> benchmark results.
Yes these numbers are missing.
Here are numbers I captured during a kernbench run on a 80 CPUs Power node:
87549520 faults
0 spf
Which is expected as the kernbench's processes are not multithreaded.
When running ebizzy on the same node:
711589 faults
692649 spf
10579 pagefault:spf_pte_lock
7815 pagefault:spf_vma_changed
0 pagefault:spf_vma_noanon
417 pagefault:spf_vma_notsup
0 pagefault:spf_vma_access
0 pagefault:spf_pmd_changed
Here about 98% of the page faults where managed in a speculative way.
>
>> ------------------
>> Benchmarks results
>>
>> There is no functional change compared to the v6 so benchmark results are
>> the same.
>> Please see https://lkml.org/lkml/2018/1/12/515 for details.
>
> Please include this vitally important info in the [0/n], don't make
> people chase links.
Sorry, will do next time.
>
> And I'd really like to see some quantitative testing results for real
> workloads, not just a bunch of microbenchmarks. Help us understand how
> useful this patchset is to our users.
We did non official runs using a "popular in memory multithreaded database product" on
176 cores SMT8 Power system which showed a 30% improvements in the number of transaction
processed per second.
Here are the perf data captured during 2 of these runs :
vanilla spf
faults 89.418 101.364
spf n/a 97.989
With the SPF kernel, most of the page fault were processed in a speculative way.
Laurent.
^ permalink raw reply
* [RFC PATCH] powerpc/mm/hugetlb: initialize the pagetable cache correctly for hugetlb
From: Aneesh Kumar K.V @ 2018-02-13 7:47 UTC (permalink / raw)
To: benh, paulus, mpe, Christophe Leroy; +Cc: linuxppc-dev, Aneesh Kumar K.V
With 64k page size, we have hugetlb pte entries at the pmd and pud level for
book3s64. We don't need to create a separate page table cache for that. With 4k
we need to make sure hugepd page table cache for 16M is placed at PUD level
and 16G at the PGD level.
Simplify all these by not using HUGEPD_PD_SHIFT which is confusing for book3s64.
Without this patch, with 64k page size we create pagetable caches with shift
value 10 and 7 which are not used at all.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
arch/powerpc/mm/hugetlbpage.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index 876da2bc1796..3b509b268030 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c
@@ -122,9 +122,6 @@ static int __hugepte_alloc(struct mm_struct *mm, hugepd_t *hpdp,
#if defined(CONFIG_PPC_FSL_BOOK3E) || defined(CONFIG_PPC_8xx)
#define HUGEPD_PGD_SHIFT PGDIR_SHIFT
#define HUGEPD_PUD_SHIFT PUD_SHIFT
-#else
-#define HUGEPD_PGD_SHIFT PUD_SHIFT
-#define HUGEPD_PUD_SHIFT PMD_SHIFT
#endif
/*
@@ -669,12 +666,24 @@ static int __init hugetlbpage_init(void)
if (add_huge_page_size(1ULL << shift) < 0)
continue;
+
+#ifdef CONFIG_PPC_BOOK3S_64
+ if (shift > PGDIR_SHIFT)
+ BUG();
+ else if (shift > PUD_SHIFT)
+ pdshift = PGDIR_SHIFT;
+ else if (shift > PMD_SHIFT)
+ pdshift = PUD_SHIFT;
+ else
+ pdshift = PMD_SHIFT;
+#else
if (shift < HUGEPD_PUD_SHIFT)
pdshift = PMD_SHIFT;
else if (shift < HUGEPD_PGD_SHIFT)
pdshift = PUD_SHIFT;
else
pdshift = PGDIR_SHIFT;
+#endif
/*
* if we have pdshift and shift value same, we don't
* use pgt cache for hugepd.
--
2.14.3
^ permalink raw reply related
* [PATCH] powerpc/powernv: IMC fix out of bounds memory access at shutdown
From: Nicholas Piggin @ 2018-02-13 7:45 UTC (permalink / raw)
To: linuxppc-dev
Cc: Nicholas Piggin, Anju T Sudhakar, Hemant Kumar,
Madhavan Srinivasan
The OPAL IMC driver's shutdown handler disables nest PMU counters by
walking nodes and taking the first CPU out of their cpumask, which is
used to index into the paca (get_hard_smp_processor_id()). This does
not always do the right thing, and in particular for CPU-less nodes it
returns NR_CPUS and that overruns the paca and dereferences random
memory.
Fix it by being more careful about checking returned CPU, and only
using online CPUs. It's not clear this shutdown code makes sense
after commit 885dcd709b ("powerpc/perf: Add nest IMC PMU support"),
but this should not make things worse
Changing the way pacas are allocated to an array of pointers exposed
this bug:
Unable to handle kernel paging request for data at address 0x2a21af1eeb000076
Faulting instruction address: 0xc0000000000a5468
Oops: Kernel access of bad area, sig: 11 [#1]
LE SMP NR_CPUS=2048 NUMA PowerNV
Modules linked in: iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp tun bridge stp llc iptable_filter ib_ipoib ib_cm ib_core kvm_hv kvm binfmt_misc vmx_crypto dm_multipath scsi_dh_rdac scsi_dh_alua ip_tables x_tables autofs4 crc32c_vpmsum
CPU: 52 PID: 1 Comm: systemd-shutdow Not tainted 4.15.0-12636-g3f1ac76cdc8f-dirty #134
NIP: c0000000000a5468 LR: c0000000000a5454 CTR: 0000000000000000
REGS: c000200e58403870 TRAP: 0380 Not tainted (4.15.0-12636-g3f1ac76cdc8f-dirty)
MSR: 900000000280b033 <SF,HV,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 28288422 XER: 20040000
CFAR: c000000000152354 SOFTE: 0
GPR00: c0000000000a5454 c000200e58403af0 c000000001093f00 0000000000000001
GPR04: 0000000000000001 00000000000004dc c000200e609a0000 000000000001b3bc
GPR08: c0000000010d0b98 2a21af1eeb000046 c000200fff7fc000 0000000000000000
GPR12: 0000000000008000 c000000ffffeb800 0000000133f97b10 0000000000000000
GPR16: 00007ffff2e9dcc8 0000000133faf4a0 0000000133f97310 0000000000000000
GPR20: 0000000133f97e80 0000000133f97d80 0000000133f97470 0000000133f97aa8
GPR24: c0000000010cfb70 c000000000d20d68 c000000000d20d78 c000000000d30438
GPR28: c000000000d20d88 0000000000000800 c0000000010d10b8 00000000000000fc
NIP [c0000000000a5468] opal_imc_counters_shutdown+0x148/0x1d0
LR [c0000000000a5454] opal_imc_counters_shutdown+0x134/0x1d0
Call Trace:
[c000200e58403af0] [c0000000000a5454] opal_imc_counters_shutdown+0x134/0x1d0 (unreliable)
[c000200e58403b90] [c000000000723734] platform_drv_shutdown+0x44/0x60
[c000200e58403bb0] [c00000000071df58] device_shutdown+0x1f8/0x350
[c000200e58403c50] [c00000000010bbd4] kernel_restart_prepare+0x54/0x70
[c000200e58403c70] [c00000000010bd28] kernel_restart+0x28/0xc0
[c000200e58403ce0] [c00000000010c210] SyS_reboot+0x1d0/0x2c0
[c000200e58403e30] [c00000000000b920] system_call+0x58/0x6c
Instruction dump:
48512459 60000000 7fe4fb78 7c7d07b4 7f63db78 7fa5eb78 480acebd 60000000
e9580000 7ba91f24 38600001 7d2a482a <a8890030> 4bfe84a9 60000000 7fa5eb78
---[ end trace 8e58676c4eb8656a ]---
Cc: Anju T Sudhakar <anju@linux.vnet.ibm.com>
Cc: Hemant Kumar <hemant@linux.vnet.ibm.com>
Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/platforms/powernv/opal-imc.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/opal-imc.c b/arch/powerpc/platforms/powernv/opal-imc.c
index dd4c9b8b8a81..f6f55ab4980e 100644
--- a/arch/powerpc/platforms/powernv/opal-imc.c
+++ b/arch/powerpc/platforms/powernv/opal-imc.c
@@ -199,9 +199,11 @@ static void disable_nest_pmu_counters(void)
const struct cpumask *l_cpumask;
get_online_cpus();
- for_each_online_node(nid) {
+ for_each_node_with_cpus(nid) {
l_cpumask = cpumask_of_node(nid);
- cpu = cpumask_first(l_cpumask);
+ cpu = cpumask_first_and(l_cpumask, cpu_online_mask);
+ if (cpu >= nr_cpu_ids)
+ continue;
opal_imc_counters_stop(OPAL_IMC_COUNTERS_NEST,
get_hard_smp_processor_id(cpu));
}
--
2.16.1
^ permalink raw reply related
* Re: [PATCH 5/5] mtd: Stop updating erase_info->state and calling mtd_erase_callback()
From: Miquel Raynal @ 2018-02-13 7:42 UTC (permalink / raw)
To: Boris Brezillon
Cc: David Woodhouse, Brian Norris, Marek Vasut, Richard Weinberger,
Cyrille Pitchen, linux-mtd, Joern Engel, Robert Jarzmik,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Kyungmin Park, Artem Bityutskiy, Solarflare linux maintainers,
Edward Cree, Bert Kenward, Greg Kroah-Hartman, linuxppc-dev,
netdev, devel
In-Reply-To: <20180212210311.23244-6-boris.brezillon@bootlin.com>
Hi Boris,
Just a few comments about the form.
Otherwise:
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> diff --git a/drivers/mtd/devices/lart.c b/drivers/mtd/devices/lart.c
> index 555b94406e0b..3d6c8ffd351f 100644
> --- a/drivers/mtd/devices/lart.c
> +++ b/drivers/mtd/devices/lart.c
> @@ -415,7 +415,6 @@ static int flash_erase (struct mtd_info *mtd,struct erase_info *instr)
> {
> if (!erase_block (addr))
> {
> - instr->state = MTD_ERASE_FAILED;
> return (-EIO);
> }
You can also safely remove these '{' '}'
>
> @@ -425,9 +424,6 @@ static int flash_erase (struct mtd_info *mtd,struct erase_info *instr)
> if (addr == mtd->eraseregions[i].offset + (mtd->eraseregions[i].erasesize * mtd->eraseregions[i].numblocks)) i++;
> }
>
> - instr->state = MTD_ERASE_DONE;
> - mtd_erase_callback(instr);
> -
> return (0);
> }
>
> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
> index 5dc8bd042cc5..aaaeaae01e1d 100644
> --- a/drivers/mtd/devices/mtd_dataflash.c
> +++ b/drivers/mtd/devices/mtd_dataflash.c
> @@ -220,10 +220,6 @@ static int dataflash_erase(struct mtd_info *mtd, struct erase_info *instr)
> }
> mutex_unlock(&priv->lock);
>
> - /* Inform MTD subsystem that erase is complete */
> - instr->state = MTD_ERASE_DONE;
> - mtd_erase_callback(instr);
> -
> return 0;
> }
>
> diff --git a/drivers/mtd/devices/mtdram.c b/drivers/mtd/devices/mtdram.c
> index 0bf4aeaf0cb8..efef43c6684b 100644
> --- a/drivers/mtd/devices/mtdram.c
> +++ b/drivers/mtd/devices/mtdram.c
> @@ -60,8 +60,6 @@ static int ram_erase(struct mtd_info *mtd, struct erase_info *instr)
> if (check_offs_len(mtd, instr->addr, instr->len))
> return -EINVAL;
> memset((char *)mtd->priv + instr->addr, 0xff, instr->len);
> - instr->state = MTD_ERASE_DONE;
> - mtd_erase_callback(instr);
Space ?
> return 0;
> }
>
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index 7287696a21f9..a963c88d392d 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -44,8 +44,6 @@ static int phram_erase(struct mtd_info *mtd, struct erase_info *instr)
> * I don't feel at all ashamed. This kind of thing is possible anyway
> * with flash, but unlikely.
> */
Not sure this comment is still relevant? Maybe you could remove it
or at least change it?
> - instr->state = MTD_ERASE_DONE;
> - mtd_erase_callback(instr);
Space ?
> return 0;
> }
>
> diff --git a/drivers/mtd/devices/pmc551.c b/drivers/mtd/devices/pmc551.c
> index cadea0620cd0..5d842cbca3de 100644
> --- a/drivers/mtd/devices/pmc551.c
> +++ b/drivers/mtd/devices/pmc551.c
> @@ -184,12 +184,10 @@ static int pmc551_erase(struct mtd_info *mtd, struct erase_info *instr)
> }
>
> out:
> - instr->state = MTD_ERASE_DONE;
> #ifdef CONFIG_MTD_PMC551_DEBUG
> printk(KERN_DEBUG "pmc551_erase() done\n");
> #endif
>
> - mtd_erase_callback(instr);
> return 0;
> }
>
> diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
> index 26f9feaa5d17..5f383630c16f 100644
> --- a/drivers/mtd/devices/powernv_flash.c
> +++ b/drivers/mtd/devices/powernv_flash.c
> @@ -175,19 +175,12 @@ static int powernv_flash_erase(struct mtd_info *mtd, struct erase_info *erase)
> {
> int rc;
>
> - erase->state = MTD_ERASING;
> -
> /* todo: register our own notifier to do a true async implementation */
> rc = powernv_flash_async_op(mtd, FLASH_OP_ERASE, erase->addr,
> erase->len, NULL, NULL);
Are you sure this is still needed? Maybe this should go away in your
first patch?
> -
> - if (rc) {
> + if (rc)
> erase->fail_addr = erase->addr;
> - erase->state = MTD_ERASE_FAILED;
> - } else {
> - erase->state = MTD_ERASE_DONE;
> - }
> - mtd_erase_callback(erase);
> +
> return rc;
> }
>
> diff --git a/drivers/mtd/devices/slram.c b/drivers/mtd/devices/slram.c
> index 0ec85f316d24..2f05e1801047 100644
> --- a/drivers/mtd/devices/slram.c
> +++ b/drivers/mtd/devices/slram.c
> @@ -88,8 +88,6 @@ static int slram_erase(struct mtd_info *mtd, struct erase_info *instr)
> * I don't feel at all ashamed. This kind of thing is possible anyway
> * with flash, but unlikely.
> */
Same with this comment.
> - instr->state = MTD_ERASE_DONE;
> - mtd_erase_callback(instr);
Space ?
> return(0);
> }
>
--
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
^ permalink raw reply
* [PATCH] selftests/powerpc: Fix: use ucontext_t instead of struct ucontext
From: Harish @ 2018-02-13 6:32 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Harish
With glibc 2.26 'struct ucontext' is removed to improve POSIX
compliance, which breaks powerpc/alignment_handler selftest.
Fix the test by using ucontext_t. Tested on ppc, works with older
glibc versions as well.
Fixes the following:
alignment_handler.c: In function ‘sighandler’:
alignment_handler.c:68:5: error: dereferencing pointer to incomplete type ‘struct ucontext’
ucp->uc_mcontext.gp_regs[PT_NIP] += 4;
^~
Signed-off-by: Harish <harish@linux.vnet.ibm.com>
---
tools/testing/selftests/powerpc/alignment/alignment_handler.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/powerpc/alignment/alignment_handler.c b/tools/testing/selftests/powerpc/alignment/alignment_handler.c
index 39fd362..0f2698f 100644
--- a/tools/testing/selftests/powerpc/alignment/alignment_handler.c
+++ b/tools/testing/selftests/powerpc/alignment/alignment_handler.c
@@ -57,7 +57,7 @@ volatile int gotsig;
void sighandler(int sig, siginfo_t *info, void *ctx)
{
- struct ucontext *ucp = ctx;
+ ucontext_t *ucp = ctx;
if (!testing) {
signal(sig, SIG_DFL);
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] powerpc/npu-dma.c: Fix deadlock in mmio_invalidate
From: Balbir Singh @ 2018-02-13 6:06 UTC (permalink / raw)
To: Alistair Popple; +Cc: mpe, linuxppc-dev, Mark Hairgrove, Javier Cabezas
In-Reply-To: <20180213031734.19831-1-alistair@popple.id.au>
On Tue, 13 Feb 2018 14:17:34 +1100
Alistair Popple <alistair@popple.id.au> wrote:
> When sending TLB invalidates to the NPU we need to send extra flushes due
> to a hardware issue. The original implementation would lock the all the
> ATSD MMIO registers sequentially before unlocking and relocking each of
> them sequentially to do the extra flush.
>
> This introduced a deadlock as it is possible for one thread to hold one
> ATSD register whilst waiting for another register to be freed while the
> other thread is holding that register waiting for the one in the first
> thread to be freed.
>
> For example if there are two threads and two ATSD registers:
>
> Thread A Thread B
> Acquire 1
> Acquire 2
> Release 1 Acquire 1
> Wait 1 Wait 2
>
> Both threads will be stuck waiting to acquire a register resulting in an
> RCU stall warning or soft lockup.
>
> This patch solves the deadlock by refactoring the code to ensure registers
> are not released between flushes and to ensure all registers are either
> acquired or released together and in order.
>
> Fixes: bbd5ff50afff ("powerpc/powernv/npu-dma: Add explicit flush when sending an ATSD")
> Signed-off-by: Alistair Popple <alistair@popple.id.au>
> ---
>
> Michael,
>
> This should probalby go to stable as well, although it's bigger than the 100
> line limit mentioned in the stable kernel rules.
>
> - Alistair
>
> arch/powerpc/platforms/powernv/npu-dma.c | 195 +++++++++++++++++--------------
> 1 file changed, 109 insertions(+), 86 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
> index fb0a6dee9bce..5746b456dfa4 100644
> --- a/arch/powerpc/platforms/powernv/npu-dma.c
> +++ b/arch/powerpc/platforms/powernv/npu-dma.c
> @@ -408,6 +408,11 @@ struct npu_context {
> void *priv;
> };
>
> +struct mmio_atsd_reg {
> + struct npu *npu;
> + int reg;
> +};
> +
Is it just easier to move reg to inside of struct npu?
> /*
> * Find a free MMIO ATSD register and mark it in use. Return -ENOSPC
> * if none are available.
> @@ -433,79 +438,83 @@ static void put_mmio_atsd_reg(struct npu *npu, int reg)
> #define XTS_ATSD_AVA 1
> #define XTS_ATSD_STAT 2
>
> -static int mmio_launch_invalidate(struct npu *npu, unsigned long launch,
> - unsigned long va)
> +static void mmio_launch_invalidate(struct mmio_atsd_reg *mmio_atsd_reg,
> + unsigned long launch, unsigned long va)
> {
> - int mmio_atsd_reg;
> -
> - do {
> - mmio_atsd_reg = get_mmio_atsd_reg(npu);
> - cpu_relax();
> - } while (mmio_atsd_reg < 0);
> + struct npu *npu = mmio_atsd_reg->npu;
> + int reg = mmio_atsd_reg->reg;
>
> __raw_writeq(cpu_to_be64(va),
> - npu->mmio_atsd_regs[mmio_atsd_reg] + XTS_ATSD_AVA);
> + npu->mmio_atsd_regs[reg] + XTS_ATSD_AVA);
> eieio();
> - __raw_writeq(cpu_to_be64(launch), npu->mmio_atsd_regs[mmio_atsd_reg]);
> -
> - return mmio_atsd_reg;
> + __raw_writeq(cpu_to_be64(launch), npu->mmio_atsd_regs[reg]);
> }
>
> -static int mmio_invalidate_pid(struct npu *npu, unsigned long pid, bool flush)
> +static void mmio_invalidate_pid(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
> + unsigned long pid, bool flush)
> {
> + int i;
> unsigned long launch;
>
> - /* IS set to invalidate matching PID */
> - launch = PPC_BIT(12);
> + for (i = 0; i <= max_npu2_index; i++) {
> + if (mmio_atsd_reg[i].reg < 0)
> + continue;
>
> - /* PRS set to process-scoped */
> - launch |= PPC_BIT(13);
> + /* IS set to invalidate matching PID */
> + launch = PPC_BIT(12);
>
> - /* AP */
> - launch |= (u64) mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
> + /* PRS set to process-scoped */
> + launch |= PPC_BIT(13);
>
> - /* PID */
> - launch |= pid << PPC_BITLSHIFT(38);
> + /* AP */
> + launch |= (u64)
> + mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
>
> - /* No flush */
> - launch |= !flush << PPC_BITLSHIFT(39);
> + /* PID */
> + launch |= pid << PPC_BITLSHIFT(38);
>
> - /* Invalidating the entire process doesn't use a va */
> - return mmio_launch_invalidate(npu, launch, 0);
> + /* No flush */
> + launch |= !flush << PPC_BITLSHIFT(39);
> +
> + /* Invalidating the entire process doesn't use a va */
> + mmio_launch_invalidate(&mmio_atsd_reg[i], launch, 0);
> + }
> }
>
> -static int mmio_invalidate_va(struct npu *npu, unsigned long va,
> - unsigned long pid, bool flush)
> +static void mmio_invalidate_va(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
> + unsigned long va, unsigned long pid, bool flush)
> {
> + int i;
> unsigned long launch;
>
> - /* IS set to invalidate target VA */
> - launch = 0;
> + for (i = 0; i <= max_npu2_index; i++) {
> + if (mmio_atsd_reg[i].reg < 0)
> + continue;
> +
> + /* IS set to invalidate target VA */
> + launch = 0;
>
> - /* PRS set to process scoped */
> - launch |= PPC_BIT(13);
> + /* PRS set to process scoped */
> + launch |= PPC_BIT(13);
>
> - /* AP */
> - launch |= (u64) mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
> + /* AP */
> + launch |= (u64)
> + mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
>
> - /* PID */
> - launch |= pid << PPC_BITLSHIFT(38);
> + /* PID */
> + launch |= pid << PPC_BITLSHIFT(38);
>
> - /* No flush */
> - launch |= !flush << PPC_BITLSHIFT(39);
> + /* No flush */
> + launch |= !flush << PPC_BITLSHIFT(39);
>
> - return mmio_launch_invalidate(npu, launch, va);
> + mmio_launch_invalidate(&mmio_atsd_reg[i], launch, va);
> + }
> }
>
> #define mn_to_npu_context(x) container_of(x, struct npu_context, mn)
>
> -struct mmio_atsd_reg {
> - struct npu *npu;
> - int reg;
> -};
> -
> static void mmio_invalidate_wait(
> - struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS], bool flush)
> + struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS])
> {
> struct npu *npu;
> int i, reg;
> @@ -520,16 +529,46 @@ static void mmio_invalidate_wait(
> reg = mmio_atsd_reg[i].reg;
> while (__raw_readq(npu->mmio_atsd_regs[reg] + XTS_ATSD_STAT))
> cpu_relax();
> + }
> +}
>
> - put_mmio_atsd_reg(npu, reg);
> +static void acquire_atsd_reg(struct npu_context *npu_context,
> + struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS])
> +{
> + int i, j;
> + struct npu *npu;
> + struct pci_dev *npdev;
> + struct pnv_phb *nphb;
>
> - /*
> - * The GPU requires two flush ATSDs to ensure all entries have
> - * been flushed. We use PID 0 as it will never be used for a
> - * process on the GPU.
> - */
> - if (flush)
> - mmio_invalidate_pid(npu, 0, true);
> + for (i = 0; i <= max_npu2_index; i++) {
> + mmio_atsd_reg[i].reg = -1;
> + for (j = 0; j < NV_MAX_LINKS; j++) {
Is it safe to assume that npu_context->npdev will not change in this
loop? I guess it would need to be stronger than just this loop.
> + npdev = npu_context->npdev[i][j];
> + if (!npdev)
> + continue;
> +
> + nphb = pci_bus_to_host(npdev->bus)->private_data;
> + npu = &nphb->npu;
> + mmio_atsd_reg[i].npu = npu;
> + mmio_atsd_reg[i].reg = get_mmio_atsd_reg(npu);
> + while (mmio_atsd_reg[i].reg < 0) {
> + mmio_atsd_reg[i].reg = get_mmio_atsd_reg(npu);
> + cpu_relax();
A cond_resched() as well if we have too many tries?
Balbir
^ permalink raw reply
* Re: [PATCH v2 04/13] lpfc: Add push-to-adapter support to sli4
From: Michael Ellerman @ 2018-02-13 5:59 UTC (permalink / raw)
To: Johannes Thumshirn, James Smart
Cc: linux-scsi, Dick Kennedy, James Smart, Mark Rutland, linuxppc-dev
In-Reply-To: <20180207102759.qzzqleujqhxnenna@linux-x5ow.site>
Johannes Thumshirn <jthumshirn@suse.de> writes:
> On Wed, Feb 07, 2018 at 10:51:57AM +0100, Johannes Thumshirn wrote:
>> > + /* Enable combined writes for DPP aperture */
>> > + pg_addr = (unsigned long)(wq->dpp_regaddr) & PAGE_MASK;
>> > +#ifdef CONFIG_X86
>> > + rc = set_memory_wc(pg_addr, 1);
>> > + if (rc) {
>> > + lpfc_printf_log(phba, KERN_ERR, LOG_INIT,
>> > + "3272 Cannot setup Combined "
>> > + "Write on WQ[%d] - disable DPP\n",
>> > + wq->queue_id);
>> > + phba->cfg_enable_dpp = 0;
>> > + }
>> > +#else
>> > + phba->cfg_enable_dpp = 0;
>> > +#endif
>> > + } else
>> > + wq->db_regaddr = phba->sli4_hba.WQDBregaddr;
>>
>> I don't really like the set_memory_wc() call here. Neither do I like the ifdef
>> CONFIG_X86 special casing.
>>
>> If you really need write combining, can't you at least use ioremap_wc()?
>
> Coming back to this again (after talking to our ARM/POWER folks internally).
> Is this really x86 specific here? I know there are servers with other architectures
> using lpfcs out there.
>
> I _think_ write combining should be possible on other architectures (that have
> PCIe and aren't dead) as well.
>
> The ioremap_wc() I suggested is probably wrong.
>
> So can you please revisit this? I CCed Mark and Michael, maybe they can help
> here.
I'm not much of an I/O guy, but I do know that on powerpc we don't
implement set_memory_wc(). So if you're using that then you do need the
ifdef.
I couldn't easily find the rest of this thread, so I'm not sure if
ioremap_wc() is an option. We do implement that and on modern CPUs at
least it will give you something that's not just a plain uncached
mapping.
cheers
^ permalink raw reply
* Re: [PATCH v2] powerpc/npu: Cleanup MMIO ATSD flushing
From: Balbir Singh @ 2018-02-13 5:55 UTC (permalink / raw)
To: Alistair Popple, Michael Ellerman
Cc: open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), Aneesh Kumar KV
In-Reply-To: <5227513.IOhi8YYFsi@new-mexico>
On Wed, Feb 7, 2018 at 2:14 PM, Alistair Popple <alistair@popple.id.au> wrote:
> On Tuesday, 16 January 2018 3:15:05 PM AEDT Alistair Popple wrote:
>> Thanks Balbir, one question below. I have no way of testing this at present but
>> it looks ok to me. Thanks!
>
> The below are more future optimisations once we can test. So in the meantime:
>
> Acked-by: Alistair Popple <alistair@popple.id.au>
@aneesh can you please look at this? @mpe can we pick this up if there
are no objections?
Balbir Singh
^ permalink raw reply
* Re: samples/seccomp/ broken when cross compiling s390, ppc allyesconfig
From: Kees Cook @ 2018-02-13 5:54 UTC (permalink / raw)
To: Michael Ellerman; +Cc: Michal Hocko, Will Drewry, linux-s390, PowerPC, LKML
In-Reply-To: <87r2ppvaeq.fsf@concordia.ellerman.id.au>
On Mon, Feb 12, 2018 at 7:25 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Michal Hocko <mhocko@kernel.org> writes:
>> Hi,
>> my build test machinery chokes on samples/seccomp when cross compiling
>> s390 and ppc64 allyesconfig. This has been the case for quite some
>> time already but I never found time to look at the problem and report
>> it. It seems this is not new issue and similar thing happend for
>> MIPS e9107f88c985 ("samples/seccomp/Makefile: do not build tests if
>> cross-compiling for MIPS").
>>
>> The build logs are attached.
>>
>> What is the best way around this? Should we simply skip compilation on
>> cross compile or is actually anybody relying on that? Or should I simply
>> disable it for s390 and ppc?
>
> The whole thing seems very confused. It's not building for the target,
> it's building for the host, ie. the Makefile sets hostprogs-m and
> HOSTCFLAGS etc.
>
> So it can't possibly work with cross compiling as it's currently
> written.
>
> Either the Makefile needs some serious work to properly support cross
> compiling or it should just be disabled when cross compiling.
Hrm, yeah, the goal was to entirely disable cross compiling, but I
guess we didn't hit it with a hard enough hammer. :)
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* [PATCH kernel] powerpc/npu: Do not try invalidating 32bit table when 64bit table is enabled
From: Alexey Kardashevskiy @ 2018-02-13 5:51 UTC (permalink / raw)
To: linuxppc-dev
Cc: Alexey Kardashevskiy, David Gibson, Alistair Popple,
Russell Currey
GPUs and the corresponding NVLink bridges get different PEs as they have
separate translation validation entries (TVEs). We put these PEs to
the same IOMMU group so they cannot be passed through separately.
So the iommu_table_group_ops::set_window/unset_window for GPUs do set
tables to the NPU PEs as well which means that iommu_table's list of
attached PEs (iommu_table_group_link) has both GPU and NPU PEs linked.
This list is used for TCE cache invalidation.
The problem is that NPU PE has just a single TVE and can be programmed
to point to 32bit or 64bit windows while GPU PE has two (as any other PCI
device). So we end up having an 32bit iommu_table struct linked to both
PEs even though only the 64bit TCE table cache can be invalidated on NPU.
And a relatively recent skiboot detects this and prints errors.
This changes GPU's iommu_table_group_ops::set_window/unset_window to make
sure that NPU PE is only linked to the table actually used by the hardware.
If there are two tables used by an IOMMU group, the NPU PE will use
the last programmed one which with the current use scenarios is expected
to be a 64bit one.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Do we need BUG_ON(IOMMU_TABLE_GROUP_MAX_TABLES != 2)?
This is an example for:
0004:04:00.0 3D: NVIDIA Corporation Device 1db1 (rev a1)
0006:00:00.0 Bridge: IBM Device 04ea (rev 01)
0006:00:00.1 Bridge: IBM Device 04ea (rev 01)
Before the patch (npu2_tce_kill messages are from skiboot):
pci 0004:04 : [PE# 00] Setting up window#0 0..3fffffff pg=1000
pci 0006:00:00.0: [PE# 0d] Setting up window 0..3fffffff pg=1000
pci 0004:04 : [PE# 00] Setting up window#1 800000000000000..8000000ffffffff pg=10000
pci 0006:00:00.0: [PE# 0d] Setting up window 800000000000000..8000000ffffffff pg=10000
NPU6: npu2_tce_kill: Unexpected TCE size (got 0x1000 expected 0x10000)
NPU6: npu2_tce_kill: Unexpected TCE size (got 0x1000 expected 0x10000)
NPU6: npu2_tce_kill: Unexpected TCE size (got 0x1000 expected 0x10000)
NPU6: npu2_tce_kill: Unexpected TCE size (got 0x1000 expected 0x10000)
NPU6: npu2_tce_kill: Unexpected TCE size (got 0x1000 expected 0x10000)
...
pci 0004:04 : [PE# 00] Removing DMA window #0
pci 0006:00:00.0: [PE# 0d] Removing DMA window
pci 0004:04 : [PE# 00] Removing DMA window #1
pci 0006:00:00.0: [PE# 0d] Removing DMA window
pci 0004:04 : [PE# 00] Setting up window#0 0..3fffffff pg=1000
pci 0006:00:00.0: [PE# 0d] Setting up window 0..3fffffff pg=1000
pci 0004:04 : [PE# 00] Setting up window#1 800000000000000..8000000ffffffff pg=10000
pci 0006:00:00.0: [PE# 0d] Setting up window 800000000000000..8000000ffffffff pg=10000
After the patch (no errors here):
pci 0004:04 : [PE# 00] Setting up window#0 0..3fffffff pg=1000
pci 0006:00:00.0: [PE# 0d] Setting up window 0..3fffffff pg=1000
pci 0004:04 : [PE# 00] Setting up window#1 800000000000000..8000000ffffffff pg=10000
pci 0006:00:00.0: [PE# 0d] Removing DMA window
pci 0006:00:00.0: [PE# 0d] Setting up window 800000000000000..8000000ffffffff pg=10000
pci 0004:04 : [PE# 00] Removing DMA window #0
pci 0004:04 : [PE# 00] Removing DMA window #1
pci 0006:00:00.0: [PE# 0d] Removing DMA window
pci 0004:04 : [PE# 00] Setting up window#0 0..3fffffff pg=1000
pci 0006:00:00.0: [PE# 0d] Setting up window 0..3fffffff pg=1000
pci 0004:04 : [PE# 00] Setting up window#1 800000000000000..8000000ffffffff pg=10000
pci 0006:00:00.0: [PE# 0d] Removing DMA window
pci 0006:00:00.0: [PE# 0d] Setting up window 800000000000000..8000000ffffffff pg=10000
---
arch/powerpc/platforms/powernv/pci-ioda.c | 27 ++++++++++++++++++++++++---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 496e476..2f91815 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -2681,14 +2681,23 @@ static struct pnv_ioda_pe *gpe_table_group_to_npe(
static long pnv_pci_ioda2_npu_set_window(struct iommu_table_group *table_group,
int num, struct iommu_table *tbl)
{
+ struct pnv_ioda_pe *npe = gpe_table_group_to_npe(table_group);
+ int num2 = (num == 0) ? 1 : 0;
long ret = pnv_pci_ioda2_set_window(table_group, num, tbl);
if (ret)
return ret;
- ret = pnv_npu_set_window(gpe_table_group_to_npe(table_group), num, tbl);
- if (ret)
+ if (table_group->tables[num2])
+ pnv_npu_unset_window(npe, num2);
+
+ ret = pnv_npu_set_window(npe, num, tbl);
+ if (ret) {
pnv_pci_ioda2_unset_window(table_group, num);
+ if (table_group->tables[num2])
+ pnv_npu_set_window(npe, num2,
+ table_group->tables[num2]);
+ }
return ret;
}
@@ -2697,12 +2706,24 @@ static long pnv_pci_ioda2_npu_unset_window(
struct iommu_table_group *table_group,
int num)
{
+ struct pnv_ioda_pe *npe = gpe_table_group_to_npe(table_group);
+ int num2 = (num == 0) ? 1 : 0;
long ret = pnv_pci_ioda2_unset_window(table_group, num);
if (ret)
return ret;
- return pnv_npu_unset_window(gpe_table_group_to_npe(table_group), num);
+ if (!npe->table_group.tables[num])
+ return 0;
+
+ ret = pnv_npu_unset_window(npe, num);
+ if (ret)
+ return ret;
+
+ if (table_group->tables[num2])
+ ret = pnv_npu_set_window(npe, num2, table_group->tables[num2]);
+
+ return ret;
}
static void pnv_ioda2_npu_take_ownership(struct iommu_table_group *table_group)
--
2.11.0
^ permalink raw reply related
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