* Re: [RFC PATCH 11/11] selftests/powerpc: Adapt the test
From: Breno Leitao @ 2018-09-27 20:57 UTC (permalink / raw)
To: Michael Neuling, linuxppc-dev; +Cc: ldufour, gromero
In-Reply-To: <b544029461059473cb7b8b6c66815a3ff31ec540.camel@neuling.org>
Hi Mikey,
On 09/18/2018 03:36 AM, Michael Neuling wrote:
> On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
>> The Documentation/powerpc/transactional_memory.txt says:
>>
>> "Syscalls made from within a suspended transaction are performed as normal
>> and the transaction is not explicitly doomed by the kernel. However,
>> what the kernel does to perform the syscall may result in the transaction
>> being doomed by the hardware."
>>
>> With this new TM mechanism, the syscall will continue to be executed if the
>> syscall happens on a suspended syscall, but, the syscall will *fail* if the
>> transaction is still active during the syscall invocation.
>
> Not sure I get this. This doesn't seem any different to before.
>
> An active (not suspended) transaction *will* result in the syscall failing and
> the transaction being doomed.
>
> A syscall in a suspended transaction should succeed and the transaction.
I understand that a transaction will always be doomed if there is a
reclaim/checkpoint, thus, if the you make a system call inside a suspended
transaction, it will reclaim and recheckpoint, thus, dooming the transaction,
and failing it on the next RFID.
So, the syscall executed in suspended mode may succeed, because it will not
be skipped (as in active mode), but the transaction will *always* fail,
because there was a reclaim and recheckpoint.
> You might need to clean up the language. I try to use:
>
> Active == transactional but not suspended (ie MSR[TS] = T)
> Suspended == suspended (ie MSR [TS] = S)
> Doomed == transaction to be rolled back at next opportinity (ie tcheck returns doomed)
>
> (note: the kernel MSR_TM_ACTIVE() macro is not consistent with this since it's
> MSR[TS] == S or T).
Ok, I agree with this language as well. I might want to improve the code to
follow the same language all across the code.
>> On the syscall path, if the transaction is active and not suspended, it
>> will call TM_KERNEL_ENTRY which will reclaim and recheckpoint the
>> transaction, thus, dooming the transaction on userspace return, with
>> failure code TM_CAUSE_SYSCALL.
>
> But the test below is on a suspend transaction?
Sorry, I meant "suspended transaction" above instead of "transaction is
active and not suspended".
>
>> This new model will break part of this test, but I understand that that the
>> documentation above didn't guarantee that the syscall would succeed, and it
>> will never succeed anymore now on.
>
> The syscall should pass in suspend (modulo the normal syscall checks). The
> transaction may fail as a result.
Perfect, and if the transaction fail, the CPU will rollback the changes and
restore the checkpoint registers (replacing the r3 that contains the pid
value), thus, it will be like "getpid" system call didn't execute.
For this test specifically, it assumes the syscall didn't execute if the
transaction failed. Take a look:
FUNC_START(getppid_tm_suspended)
tbegin.
beq 1f
li r0, __NR_getppid
tsuspend.
sc
tresume.
tend.
blr
1:
li r3, -1
blr
So, the tests thinks the syscall failed because the transaction aborted.
Anyway, I can try to improve this test other than removing this test, but I
am not sure how.
Thank you
^ permalink raw reply
* Re: [RFC PATCH 05/11] powerpc/tm: Function that updates the failure code
From: Breno Leitao @ 2018-09-27 20:58 UTC (permalink / raw)
To: Michael Neuling, linuxppc-dev; +Cc: ldufour, gromero
In-Reply-To: <53c7daa6af9a502827e877d04b4a45b7eea5efbe.camel@neuling.org>
Hi Mikey,
On 09/17/2018 10:29 PM, Michael Neuling wrote:
> On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
>> Now the transaction reclaims happens very earlier in the trap handler, and
>> it is impossible to know precisely, at that early time, what should be set
>> as the failure cause for some specific cases, as, if the task will be
>> rescheduled, thus, the transaction abort case should be updated from
>> TM_CAUSE_MISC to TM_CAUSE_RESCHED, for example.
>
> Please add comments to where this is used (in EXCEPTION_COMMON macro I think)
> that say this might happen.
Is it OK if I comment it in TM_KERNEL_ENTRY macro, since the failure cause
could be updated independently of the exception being execute, so, every call
to TM_KERNEL_ENTRY can have the cause overwritten.
I.e. it does not matter if the exception is a systemcall or a page fault,
the failure cause will be updated if there is a process reschedule after the
exception/syscall is handled.
Thank you
^ permalink raw reply
* Re: [RFC PATCH 08/11] powerpc/tm: Do not reclaim on ptrace
From: Breno Leitao @ 2018-09-27 21:03 UTC (permalink / raw)
To: Michael Neuling, linuxppc-dev; +Cc: ldufour, gromero
In-Reply-To: <743b29dab736700c1b284fabb8fccf7c83ddb3c2.camel@neuling.org>
Hi Mikey,
On 09/18/2018 02:36 AM, Michael Neuling wrote:
> On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
>> Make sure that we are not suspended on ptrace and that the registers were
>> already reclaimed.
>>
>> Since the data was already reclaimed, there is nothing to be done here
>> except to restore the SPRs.
>>
>> Signed-off-by: Breno Leitao <leitao@debian.org>
>> ---
>> arch/powerpc/kernel/ptrace.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
>> index 9667666eb18e..cf6ee9154b11 100644
>> --- a/arch/powerpc/kernel/ptrace.c
>> +++ b/arch/powerpc/kernel/ptrace.c
>> @@ -136,12 +136,10 @@ static void flush_tmregs_to_thread(struct task_struct
>> *tsk)
>> if ((!cpu_has_feature(CPU_FTR_TM)) || (tsk != current))
>> return;
>>
>> - if (MSR_TM_SUSPENDED(mfmsr())) {
>> - tm_reclaim_current(TM_CAUSE_SIGNAL);
>> - } else {
>> - tm_enable();
>> - tm_save_sprs(&(tsk->thread));
>> - }
>> + WARN_ON(MSR_TM_SUSPENDED(mfmsr()));
>> +
>> + tm_enable();
>> + tm_save_sprs(&(tsk->thread));
>
> Do we need to check if TM was enabled in the task before saving the TM SPRs?
>
> What happens if TM was lazily off and hence we had someone else's TM SPRs in the
> CPU currently? Wouldn't this flush the wrong values to the task_struct?
>
> I think we need to check the processes MSR before doing this.
Yes, it is a *very* good point, and I think we are vulnerable even before
this patch (in the current kernel). Take a look above, we are calling
tm_save_sprs() if MSR is not TM suspended independently if TM is lazily off.
It shouldn't be hard to create a test case for it. I can try to call
ptrace(PTRACE_GETVRREGS) on a task that sleeps until TM is lazily disabled,
compare if the TM SPR changed in this mean time. (while doing HTM in parallel
to keep HTM SPR changing). Let's see if I can read others task TM SPRs.
Thank you.
^ permalink raw reply
* drivers binding to device node with multiple compatible strings
From: Li Yang @ 2018-09-27 22:25 UTC (permalink / raw)
To: Rob Herring, Grant Likely,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, lkml,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linuxppc-dev
Hi Rob and Grant,
Various device tree specs are recommending to include all the
potential compatible strings in the device node, with the order from
most specific to most general. But it looks like Linux kernel doesn't
provide a way to bind the device to the most specific driver, however,
the first registered compatible driver will be bound.
As more and more generic drivers are added to the Linux kernel, they
are competing with the more specific vendor drivers and causes problem
when both are built into the kernel. I'm wondering if there is a
generic solution (or in plan) to make the most specific driver bound
to the device. Or we have to disable the more general driver or
remove the more general compatible string from the device tree?
Regards,
Leo
^ permalink raw reply
* [PATCH 2/5] dma-direct: add an explicit dma_direct_get_required_mask
From: Christoph Hellwig @ 2018-09-27 22:35 UTC (permalink / raw)
To: iommu
Cc: Robin Murphy, linux-kernel, Greg Kroah-Hartman, linuxppc-dev,
Marek Szyprowski
In-Reply-To: <20180927223539.28449-1-hch@lst.de>
This is somewhat modelled after the powerpc version, and differs from
the legacy fallback in use fls64 instead of pointlessly splitting up the
address into low and high dwords and in that it takes (__)phys_to_dma
into account.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
include/linux/dma-direct.h | 1 +
kernel/dma/direct.c | 22 +++++++++++++++++++---
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
index 86a59ba5a7f3..b79496d8c75b 100644
--- a/include/linux/dma-direct.h
+++ b/include/linux/dma-direct.h
@@ -55,6 +55,7 @@ static inline void dma_mark_clean(void *addr, size_t size)
}
#endif /* CONFIG_ARCH_HAS_DMA_MARK_CLEAN */
+u64 dma_direct_get_required_mask(struct device *dev);
void *dma_direct_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle,
gfp_t gfp, unsigned long attrs);
void dma_direct_free(struct device *dev, size_t size, void *cpu_addr,
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index c954f0a6dc62..f32b33cfa331 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -4,6 +4,7 @@
*
* DMA operations that map physical memory directly without using an IOMMU.
*/
+#include <linux/bootmem.h> /* for max_pfn */
#include <linux/export.h>
#include <linux/mm.h>
#include <linux/dma-direct.h>
@@ -53,11 +54,25 @@ check_addr(struct device *dev, dma_addr_t dma_addr, size_t size,
return true;
}
+static inline dma_addr_t phys_to_dma_direct(struct device *dev,
+ phys_addr_t phys)
+{
+ if (force_dma_unencrypted())
+ return __phys_to_dma(dev, phys);
+ return phys_to_dma(dev, phys);
+}
+
+u64 dma_direct_get_required_mask(struct device *dev)
+{
+ u64 max_dma = phys_to_dma_direct(dev, (max_pfn - 1) << PAGE_SHIFT);
+
+ return (1ULL << (fls64(max_dma) - 1)) * 2 - 1;
+}
+
static bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
{
- dma_addr_t addr = force_dma_unencrypted() ?
- __phys_to_dma(dev, phys) : phys_to_dma(dev, phys);
- return addr + size - 1 <= dev->coherent_dma_mask;
+ return phys_to_dma_direct(dev, phys) + size - 1 <=
+ dev->coherent_dma_mask;
}
void *dma_direct_alloc_pages(struct device *dev, size_t size,
@@ -296,6 +311,7 @@ const struct dma_map_ops dma_direct_ops = {
.unmap_page = dma_direct_unmap_page,
.unmap_sg = dma_direct_unmap_sg,
#endif
+ .get_required_mask = dma_direct_get_required_mask,
.dma_supported = dma_direct_supported,
.mapping_error = dma_direct_mapping_error,
.cache_sync = arch_dma_cache_sync,
--
2.19.0
^ permalink raw reply related
* [PATCH 3/5] dma-direct: refine dma_direct_alloc zone selection
From: Christoph Hellwig @ 2018-09-27 22:35 UTC (permalink / raw)
To: iommu
Cc: Robin Murphy, linux-kernel, Greg Kroah-Hartman, linuxppc-dev,
Marek Szyprowski
In-Reply-To: <20180927223539.28449-1-hch@lst.de>
We need to take the DMA offset and encryption bit into account when
selecting a zone. User the opportunity to factor out the zone
selection into a helper for reuse.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
kernel/dma/direct.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index f32b33cfa331..e78548397a92 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -69,6 +69,22 @@ u64 dma_direct_get_required_mask(struct device *dev)
return (1ULL << (fls64(max_dma) - 1)) * 2 - 1;
}
+static gfp_t __dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask,
+ u64 *phys_mask)
+{
+ if (force_dma_unencrypted())
+ *phys_mask = __dma_to_phys(dev, dma_mask);
+ else
+ *phys_mask = dma_to_phys(dev, dma_mask);
+
+ /* GFP_DMA32 and GFP_DMA are no ops without the corresponding zones: */
+ if (*phys_mask <= DMA_BIT_MASK(ARCH_ZONE_DMA_BITS))
+ return GFP_DMA;
+ if (*phys_mask <= DMA_BIT_MASK(32))
+ return GFP_DMA32;
+ return 0;
+}
+
static bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
{
return phys_to_dma_direct(dev, phys) + size - 1 <=
@@ -81,17 +97,13 @@ void *dma_direct_alloc_pages(struct device *dev, size_t size,
unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
int page_order = get_order(size);
struct page *page = NULL;
+ u64 phys_mask;
void *ret;
/* we always manually zero the memory once we are done: */
gfp &= ~__GFP_ZERO;
-
- /* GFP_DMA32 and GFP_DMA are no ops without the corresponding zones: */
- if (dev->coherent_dma_mask <= DMA_BIT_MASK(ARCH_ZONE_DMA_BITS))
- gfp |= GFP_DMA;
- if (dev->coherent_dma_mask <= DMA_BIT_MASK(32) && !(gfp & GFP_DMA))
- gfp |= GFP_DMA32;
-
+ gfp |= __dma_direct_optimal_gfp_mask(dev, dev->coherent_dma_mask,
+ &phys_mask);
again:
/* CMA can be used only in the context which permits sleeping */
if (gfpflags_allow_blocking(gfp)) {
@@ -110,15 +122,14 @@ void *dma_direct_alloc_pages(struct device *dev, size_t size,
page = NULL;
if (IS_ENABLED(CONFIG_ZONE_DMA32) &&
- dev->coherent_dma_mask < DMA_BIT_MASK(64) &&
+ phys_mask < DMA_BIT_MASK(64) &&
!(gfp & (GFP_DMA32 | GFP_DMA))) {
gfp |= GFP_DMA32;
goto again;
}
if (IS_ENABLED(CONFIG_ZONE_DMA) &&
- dev->coherent_dma_mask < DMA_BIT_MASK(32) &&
- !(gfp & GFP_DMA)) {
+ phys_mask < DMA_BIT_MASK(32) && !(gfp & GFP_DMA)) {
gfp = (gfp & ~GFP_DMA32) | GFP_DMA;
goto again;
}
--
2.19.0
^ permalink raw reply related
* [PATCH 4/5] dma-direct: implement complete bus_dma_mask handling
From: Christoph Hellwig @ 2018-09-27 22:35 UTC (permalink / raw)
To: iommu
Cc: Robin Murphy, linux-kernel, Greg Kroah-Hartman, linuxppc-dev,
Marek Szyprowski
In-Reply-To: <20180927223539.28449-1-hch@lst.de>
Instead of rejecting devices with a too small bus_dma_mask we can handle
by taking the bus dma_mask into account for allocations and bounce
buffering decisions.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
include/linux/dma-direct.h | 3 ++-
kernel/dma/direct.c | 21 +++++++++++----------
2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
index b79496d8c75b..fbca184ff5a0 100644
--- a/include/linux/dma-direct.h
+++ b/include/linux/dma-direct.h
@@ -27,7 +27,8 @@ static inline bool dma_capable(struct device *dev, dma_addr_t addr, size_t size)
if (!dev->dma_mask)
return false;
- return addr + size - 1 <= *dev->dma_mask;
+ return addr + size - 1 <=
+ min_not_zero(*dev->dma_mask, dev->bus_dma_mask);
}
#endif /* !CONFIG_ARCH_HAS_PHYS_TO_DMA */
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index e78548397a92..60c433b880e0 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -44,10 +44,11 @@ check_addr(struct device *dev, dma_addr_t dma_addr, size_t size,
return false;
}
- if (*dev->dma_mask >= DMA_BIT_MASK(32)) {
+ if (*dev->dma_mask >= DMA_BIT_MASK(32) || dev->bus_dma_mask) {
dev_err(dev,
- "%s: overflow %pad+%zu of device mask %llx\n",
- caller, &dma_addr, size, *dev->dma_mask);
+ "%s: overflow %pad+%zu of device mask %llx bus mask %llx\n",
+ caller, &dma_addr, size,
+ *dev->dma_mask, dev->bus_dma_mask);
}
return false;
}
@@ -66,12 +67,18 @@ u64 dma_direct_get_required_mask(struct device *dev)
{
u64 max_dma = phys_to_dma_direct(dev, (max_pfn - 1) << PAGE_SHIFT);
+ if (dev->bus_dma_mask && dev->bus_dma_mask < max_dma)
+ max_dma = dev->bus_dma_mask;
+
return (1ULL << (fls64(max_dma) - 1)) * 2 - 1;
}
static gfp_t __dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask,
u64 *phys_mask)
{
+ if (dev->bus_dma_mask && dev->bus_dma_mask < dma_mask)
+ dma_mask = dev->bus_dma_mask;
+
if (force_dma_unencrypted())
*phys_mask = __dma_to_phys(dev, dma_mask);
else
@@ -88,7 +95,7 @@ static gfp_t __dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask,
static bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
{
return phys_to_dma_direct(dev, phys) + size - 1 <=
- dev->coherent_dma_mask;
+ min_not_zero(dev->coherent_dma_mask, dev->bus_dma_mask);
}
void *dma_direct_alloc_pages(struct device *dev, size_t size,
@@ -292,12 +299,6 @@ int dma_direct_supported(struct device *dev, u64 mask)
if (mask < phys_to_dma(dev, DMA_BIT_MASK(32)))
return 0;
#endif
- /*
- * Upstream PCI/PCIe bridges or SoC interconnects may not carry
- * as many DMA address bits as the device itself supports.
- */
- if (dev->bus_dma_mask && mask > dev->bus_dma_mask)
- return 0;
return 1;
}
--
2.19.0
^ permalink raw reply related
* [PATCH 5/5] dma-direct: always allow dma mask <= physiscal memory size
From: Christoph Hellwig @ 2018-09-27 22:35 UTC (permalink / raw)
To: iommu
Cc: Robin Murphy, linux-kernel, Greg Kroah-Hartman, linuxppc-dev,
Marek Szyprowski
In-Reply-To: <20180927223539.28449-1-hch@lst.de>
This way an architecture with less than 4G of RAM can support dma_mask
smaller than 32-bit without a ZONE_DMA. Apparently that is a common
case on powerpc.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
kernel/dma/direct.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 60c433b880e0..170bd322a94a 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -284,21 +284,25 @@ int dma_direct_map_sg(struct device *dev, struct scatterlist *sgl, int nents,
return nents;
}
+/*
+ * Because 32-bit DMA masks are so common we expect every architecture to be
+ * able to satisfy them - either by not supporting more physical memory, or by
+ * providing a ZONE_DMA32. If neither is the case, the architecture needs to
+ * use an IOMMU instead of the direct mapping.
+ */
int dma_direct_supported(struct device *dev, u64 mask)
{
-#ifdef CONFIG_ZONE_DMA
- if (mask < phys_to_dma(dev, DMA_BIT_MASK(ARCH_ZONE_DMA_BITS)))
- return 0;
-#else
- /*
- * Because 32-bit DMA masks are so common we expect every architecture
- * to be able to satisfy them - either by not supporting more physical
- * memory, or by providing a ZONE_DMA32. If neither is the case, the
- * architecture needs to use an IOMMU instead of the direct mapping.
- */
- if (mask < phys_to_dma(dev, DMA_BIT_MASK(32)))
+ u64 min_mask;
+
+ if (IS_ENABLED(CONFIG_ZONE_DMA))
+ min_mask = DMA_BIT_MASK(ARCH_ZONE_DMA_BITS);
+ else
+ min_mask = DMA_BIT_MASK(32);
+
+ min_mask = min_t(u64, min_mask, (max_pfn - 1) << PAGE_SHIFT);
+
+ if (mask >= phys_to_dma(dev, min_mask))
return 0;
-#endif
return 1;
}
--
2.19.0
^ permalink raw reply related
* dma mask related fixups (including full bus_dma_mask support) v2
From: Christoph Hellwig @ 2018-09-27 22:35 UTC (permalink / raw)
To: iommu
Cc: Robin Murphy, linux-kernel, Greg Kroah-Hartman, linuxppc-dev,
Marek Szyprowski
Hi all,
the dma_get_required_mask dma API implementation has always been a little
odd, in that we by default don't wire it up struct dma_map_ops, but
instead hard code a default implementation. powerpc and ia64 override
this default and either call a method or otherwise duplicate the default.
This series always enabled the method and just falls back to the previous
default implementation when it is not available, as well as fixing up
a few bits in the default implementations. This already allows removing
the ia64 override of the implementation, and will also allow to remove
the powerpc one together with a few additional cleanups in the powerpc
code, but those will be sent separately with other powerpc DMA API
patches. Last but not least the method will allow us to return a more
sensible value for typical iommu dma_ops eventually, but that is left
to another series as well.
Additionally the dma-direct code has been a bit sloppy in when it was
using phys_to_dma in a few places, so this gets fixed up as well as
actually respecting the bus_dma_mask everywhere instead of just
rejecting dma mask that don't fit into it.
Alltogether this should be all core dma-direct changes required to
move powerpc over to the generic code.
Changes since v2:
- clean up a mask check
- add various acks
^ permalink raw reply
* [PATCH 1/5] dma-mapping: make the get_required_mask method available unconditionally
From: Christoph Hellwig @ 2018-09-27 22:35 UTC (permalink / raw)
To: iommu
Cc: Robin Murphy, linux-kernel, Greg Kroah-Hartman, linuxppc-dev,
Marek Szyprowski
In-Reply-To: <20180927223539.28449-1-hch@lst.de>
This save some duplication for ia64, and makes the interface more
general. In the long run we want each dma_map_ops instance to fill this
out, but this will take a little more prep work.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> [powerpc bits]
---
arch/ia64/include/asm/dma-mapping.h | 2 --
arch/ia64/include/asm/machvec.h | 7 -------
arch/ia64/include/asm/machvec_init.h | 1 -
arch/ia64/include/asm/machvec_sn2.h | 2 --
arch/ia64/pci/pci.c | 26 --------------------------
arch/ia64/sn/pci/pci_dma.c | 4 ++--
drivers/base/platform.c | 13 +++++++++++--
drivers/pci/controller/vmd.c | 4 ----
include/linux/dma-mapping.h | 2 --
9 files changed, 13 insertions(+), 48 deletions(-)
diff --git a/arch/ia64/include/asm/dma-mapping.h b/arch/ia64/include/asm/dma-mapping.h
index 76e4d6632d68..522745ae67bb 100644
--- a/arch/ia64/include/asm/dma-mapping.h
+++ b/arch/ia64/include/asm/dma-mapping.h
@@ -10,8 +10,6 @@
#include <linux/scatterlist.h>
#include <linux/dma-debug.h>
-#define ARCH_HAS_DMA_GET_REQUIRED_MASK
-
extern const struct dma_map_ops *dma_ops;
extern struct ia64_machine_vector ia64_mv;
extern void set_iommu_machvec(void);
diff --git a/arch/ia64/include/asm/machvec.h b/arch/ia64/include/asm/machvec.h
index 267f4f170191..5133739966bc 100644
--- a/arch/ia64/include/asm/machvec.h
+++ b/arch/ia64/include/asm/machvec.h
@@ -44,7 +44,6 @@ typedef void ia64_mv_kernel_launch_event_t(void);
/* DMA-mapping interface: */
typedef void ia64_mv_dma_init (void);
-typedef u64 ia64_mv_dma_get_required_mask (struct device *);
typedef const struct dma_map_ops *ia64_mv_dma_get_ops(struct device *);
/*
@@ -127,7 +126,6 @@ extern void machvec_tlb_migrate_finish (struct mm_struct *);
# define platform_global_tlb_purge ia64_mv.global_tlb_purge
# define platform_tlb_migrate_finish ia64_mv.tlb_migrate_finish
# define platform_dma_init ia64_mv.dma_init
-# define platform_dma_get_required_mask ia64_mv.dma_get_required_mask
# define platform_dma_get_ops ia64_mv.dma_get_ops
# define platform_irq_to_vector ia64_mv.irq_to_vector
# define platform_local_vector_to_irq ia64_mv.local_vector_to_irq
@@ -171,7 +169,6 @@ struct ia64_machine_vector {
ia64_mv_global_tlb_purge_t *global_tlb_purge;
ia64_mv_tlb_migrate_finish_t *tlb_migrate_finish;
ia64_mv_dma_init *dma_init;
- ia64_mv_dma_get_required_mask *dma_get_required_mask;
ia64_mv_dma_get_ops *dma_get_ops;
ia64_mv_irq_to_vector *irq_to_vector;
ia64_mv_local_vector_to_irq *local_vector_to_irq;
@@ -211,7 +208,6 @@ struct ia64_machine_vector {
platform_global_tlb_purge, \
platform_tlb_migrate_finish, \
platform_dma_init, \
- platform_dma_get_required_mask, \
platform_dma_get_ops, \
platform_irq_to_vector, \
platform_local_vector_to_irq, \
@@ -286,9 +282,6 @@ extern const struct dma_map_ops *dma_get_ops(struct device *);
#ifndef platform_dma_get_ops
# define platform_dma_get_ops dma_get_ops
#endif
-#ifndef platform_dma_get_required_mask
-# define platform_dma_get_required_mask ia64_dma_get_required_mask
-#endif
#ifndef platform_irq_to_vector
# define platform_irq_to_vector __ia64_irq_to_vector
#endif
diff --git a/arch/ia64/include/asm/machvec_init.h b/arch/ia64/include/asm/machvec_init.h
index 2b32fd06b7c6..2aafb69a3787 100644
--- a/arch/ia64/include/asm/machvec_init.h
+++ b/arch/ia64/include/asm/machvec_init.h
@@ -4,7 +4,6 @@
extern ia64_mv_send_ipi_t ia64_send_ipi;
extern ia64_mv_global_tlb_purge_t ia64_global_tlb_purge;
-extern ia64_mv_dma_get_required_mask ia64_dma_get_required_mask;
extern ia64_mv_irq_to_vector __ia64_irq_to_vector;
extern ia64_mv_local_vector_to_irq __ia64_local_vector_to_irq;
extern ia64_mv_pci_get_legacy_mem_t ia64_pci_get_legacy_mem;
diff --git a/arch/ia64/include/asm/machvec_sn2.h b/arch/ia64/include/asm/machvec_sn2.h
index ece9fa85be88..b5153d300289 100644
--- a/arch/ia64/include/asm/machvec_sn2.h
+++ b/arch/ia64/include/asm/machvec_sn2.h
@@ -55,7 +55,6 @@ extern ia64_mv_readb_t __sn_readb_relaxed;
extern ia64_mv_readw_t __sn_readw_relaxed;
extern ia64_mv_readl_t __sn_readl_relaxed;
extern ia64_mv_readq_t __sn_readq_relaxed;
-extern ia64_mv_dma_get_required_mask sn_dma_get_required_mask;
extern ia64_mv_dma_init sn_dma_init;
extern ia64_mv_migrate_t sn_migrate;
extern ia64_mv_kernel_launch_event_t sn_kernel_launch_event;
@@ -100,7 +99,6 @@ extern ia64_mv_pci_fixup_bus_t sn_pci_fixup_bus;
#define platform_pci_get_legacy_mem sn_pci_get_legacy_mem
#define platform_pci_legacy_read sn_pci_legacy_read
#define platform_pci_legacy_write sn_pci_legacy_write
-#define platform_dma_get_required_mask sn_dma_get_required_mask
#define platform_dma_init sn_dma_init
#define platform_migrate sn_migrate
#define platform_kernel_launch_event sn_kernel_launch_event
diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
index 7ccc64d5fe3e..5d71800df431 100644
--- a/arch/ia64/pci/pci.c
+++ b/arch/ia64/pci/pci.c
@@ -568,32 +568,6 @@ static void __init set_pci_dfl_cacheline_size(void)
pci_dfl_cache_line_size = (1 << cci.pcci_line_size) / 4;
}
-u64 ia64_dma_get_required_mask(struct device *dev)
-{
- u32 low_totalram = ((max_pfn - 1) << PAGE_SHIFT);
- u32 high_totalram = ((max_pfn - 1) >> (32 - PAGE_SHIFT));
- u64 mask;
-
- if (!high_totalram) {
- /* convert to mask just covering totalram */
- low_totalram = (1 << (fls(low_totalram) - 1));
- low_totalram += low_totalram - 1;
- mask = low_totalram;
- } else {
- high_totalram = (1 << (fls(high_totalram) - 1));
- high_totalram += high_totalram - 1;
- mask = (((u64)high_totalram) << 32) + 0xffffffff;
- }
- return mask;
-}
-EXPORT_SYMBOL_GPL(ia64_dma_get_required_mask);
-
-u64 dma_get_required_mask(struct device *dev)
-{
- return platform_dma_get_required_mask(dev);
-}
-EXPORT_SYMBOL_GPL(dma_get_required_mask);
-
static int __init pcibios_init(void)
{
set_pci_dfl_cacheline_size();
diff --git a/arch/ia64/sn/pci/pci_dma.c b/arch/ia64/sn/pci/pci_dma.c
index 74c934a997bb..96eb2567718a 100644
--- a/arch/ia64/sn/pci/pci_dma.c
+++ b/arch/ia64/sn/pci/pci_dma.c
@@ -344,11 +344,10 @@ static int sn_dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
return 0;
}
-u64 sn_dma_get_required_mask(struct device *dev)
+static u64 sn_dma_get_required_mask(struct device *dev)
{
return DMA_BIT_MASK(64);
}
-EXPORT_SYMBOL_GPL(sn_dma_get_required_mask);
char *sn_pci_get_legacy_mem(struct pci_bus *bus)
{
@@ -473,6 +472,7 @@ static struct dma_map_ops sn_dma_ops = {
.sync_sg_for_device = sn_dma_sync_sg_for_device,
.mapping_error = sn_dma_mapping_error,
.dma_supported = sn_dma_supported,
+ .get_required_mask = sn_dma_get_required_mask,
};
void sn_dma_init(void)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index dff82a3c2caa..cfe22fded980 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1179,8 +1179,7 @@ int __init platform_bus_init(void)
return error;
}
-#ifndef ARCH_HAS_DMA_GET_REQUIRED_MASK
-u64 dma_get_required_mask(struct device *dev)
+static u64 dma_default_get_required_mask(struct device *dev)
{
u32 low_totalram = ((max_pfn - 1) << PAGE_SHIFT);
u32 high_totalram = ((max_pfn - 1) >> (32 - PAGE_SHIFT));
@@ -1198,6 +1197,16 @@ u64 dma_get_required_mask(struct device *dev)
}
return mask;
}
+
+#ifndef ARCH_HAS_DMA_GET_REQUIRED_MASK
+u64 dma_get_required_mask(struct device *dev)
+{
+ const struct dma_map_ops *ops = get_dma_ops(dev);
+
+ if (ops->get_required_mask)
+ return ops->get_required_mask(dev);
+ return dma_default_get_required_mask(dev);
+}
EXPORT_SYMBOL_GPL(dma_get_required_mask);
#endif
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index fd2dbd7eed7b..f31ed62d518c 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -404,12 +404,10 @@ static int vmd_dma_supported(struct device *dev, u64 mask)
return vmd_dma_ops(dev)->dma_supported(to_vmd_dev(dev), mask);
}
-#ifdef ARCH_HAS_DMA_GET_REQUIRED_MASK
static u64 vmd_get_required_mask(struct device *dev)
{
return vmd_dma_ops(dev)->get_required_mask(to_vmd_dev(dev));
}
-#endif
static void vmd_teardown_dma_ops(struct vmd_dev *vmd)
{
@@ -450,9 +448,7 @@ static void vmd_setup_dma_ops(struct vmd_dev *vmd)
ASSIGN_VMD_DMA_OPS(source, dest, sync_sg_for_device);
ASSIGN_VMD_DMA_OPS(source, dest, mapping_error);
ASSIGN_VMD_DMA_OPS(source, dest, dma_supported);
-#ifdef ARCH_HAS_DMA_GET_REQUIRED_MASK
ASSIGN_VMD_DMA_OPS(source, dest, get_required_mask);
-#endif
add_dma_domain(domain);
}
#undef ASSIGN_VMD_DMA_OPS
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index d23fc45c8208..562af6b45f23 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -130,9 +130,7 @@ struct dma_map_ops {
enum dma_data_direction direction);
int (*mapping_error)(struct device *dev, dma_addr_t dma_addr);
int (*dma_supported)(struct device *dev, u64 mask);
-#ifdef ARCH_HAS_DMA_GET_REQUIRED_MASK
u64 (*get_required_mask)(struct device *dev);
-#endif
};
extern const struct dma_map_ops dma_direct_ops;
--
2.19.0
^ permalink raw reply related
* [PATCH 3/3] powerpc/powernv/npu: Remove atsd_threshold debugfs setting
From: Mark Hairgrove @ 2018-09-27 23:23 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alistair Popple, Mark Hairgrove, Reza Arbab
In-Reply-To: <1538090591-28519-1-git-send-email-mhairgrove@nvidia.com>
This threshold is no longer used now that all invalidates issue a single
ATSD to each active NPU.
Signed-off-by: Mark Hairgrove <mhairgrove@nvidia.com>
---
arch/powerpc/platforms/powernv/npu-dma.c | 13 -------------
1 files changed, 0 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
index e471a1a..426df1b 100644
--- a/arch/powerpc/platforms/powernv/npu-dma.c
+++ b/arch/powerpc/platforms/powernv/npu-dma.c
@@ -42,14 +42,6 @@
static DEFINE_SPINLOCK(npu_context_lock);
/*
- * When an address shootdown range exceeds this threshold we invalidate the
- * entire TLB on the GPU for the given PID rather than each specific address in
- * the range.
- */
-static uint64_t atsd_threshold = 2 * 1024 * 1024;
-static struct dentry *atsd_threshold_dentry;
-
-/*
* Other types of TCE cache invalidation are not functional in the
* hardware.
*/
@@ -968,11 +960,6 @@ int pnv_npu2_init(struct pnv_phb *phb)
static int npu_index;
uint64_t rc = 0;
- if (!atsd_threshold_dentry) {
- atsd_threshold_dentry = debugfs_create_x64("atsd_threshold",
- 0600, powerpc_debugfs_root, &atsd_threshold);
- }
-
phb->npu.nmmu_flush =
of_property_read_bool(phb->hose->dn, "ibm,nmmu-flush");
for_each_child_of_node(phb->hose->dn, dn) {
--
1.7.2.5
^ permalink raw reply related
* [PATCH 2/3] powerpc/powernv/npu: Use size-based ATSD invalidates
From: Mark Hairgrove @ 2018-09-27 23:23 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alistair Popple, Mark Hairgrove, Reza Arbab
In-Reply-To: <1538090591-28519-1-git-send-email-mhairgrove@nvidia.com>
Prior to this change only two types of ATSDs were issued to the NPU:
invalidates targeting a single page and invalidates targeting the whole
address space. The crossover point happened at the configurable
atsd_threshold which defaulted to 2M. Invalidates that size or smaller
would issue per-page invalidates for the whole range.
The NPU supports more invalidation sizes however: 64K, 2M, 1G, and all.
These invalidates target addresses aligned to their size. 2M is a common
invalidation size for GPU-enabled applications because that is a GPU
page size, so reducing the number of invalidates by 32x in that case is a
clear improvement.
ATSD latency is high in general so now we always issue a single invalidate
rather than multiple. This will over-invalidate in some cases, but for any
invalidation size over 2M it matches or improves the prior behavior.
There's also an improvement for single-page invalidates since the prior
version issued two invalidates for that case instead of one.
To show the benefit here are some performance numbers from a
microbenchmark which creates a 1G allocation then uses mprotect with
PROT_NONE to trigger invalidates in strides across the allocation.
One NPU (1 GPU):
mprotect rate (GB/s)
Stride Before After Speedup
64K 5.3 5.6 5%
1M 39.3 57.4 46%
2M 49.7 82.6 66%
4M 286.6 285.7 0%
Two NPUs (6 GPUs):
mprotect rate (GB/s)
Stride Before After Speedup
64K 6.5 7.4 13%
1M 33.4 67.9 103%
2M 38.7 93.1 141%
4M 356.7 354.6 -1%
Anything over 2M is roughly the same as before since both cases issue a
single ATSD.
Signed-off-by: Mark Hairgrove <mhairgrove@nvidia.com>
---
arch/powerpc/platforms/powernv/npu-dma.c | 71 +++++++++++++++++-------------
1 files changed, 40 insertions(+), 31 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
index c8f438a..e471a1a 100644
--- a/arch/powerpc/platforms/powernv/npu-dma.c
+++ b/arch/powerpc/platforms/powernv/npu-dma.c
@@ -509,15 +509,14 @@ static void mmio_invalidate_pid(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_LAUNCH, launch);
}
-static void mmio_invalidate_va(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
- unsigned long va, unsigned long pid, bool flush)
+static void mmio_invalidate_range(struct mmio_atsd_reg
+ mmio_atsd_reg[NV_MAX_NPUS], unsigned long pid,
+ unsigned long start, unsigned long psize, bool flush)
{
- unsigned long launch;
-
- launch = get_atsd_launch_val(pid, mmu_virtual_psize, flush);
+ unsigned long launch = get_atsd_launch_val(pid, psize, flush);
/* Write all VAs first */
- mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_AVA, va);
+ mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_AVA, start);
/* Issue one barrier for all address writes */
eieio();
@@ -608,15 +607,38 @@ static void release_atsd_reg(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS])
}
}
+#define PAGE_64K (64UL * 1024)
+#define PAGE_2M (2UL * 1024 * 1024)
+#define PAGE_1G (1UL * 1024 * 1024 * 1024)
+
/*
- * Invalidate either a single address or an entire PID depending on
- * the value of va.
+ * Invalidate a virtual address range
*/
-static void mmio_invalidate(struct npu_context *npu_context, int va,
- unsigned long address, bool flush)
+static void mmio_invalidate(struct npu_context *npu_context,
+ unsigned long start, unsigned long size, bool flush)
{
struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS];
unsigned long pid = npu_context->mm->context.id;
+ unsigned long atsd_start = 0;
+ unsigned long end = start + size - 1;
+ int atsd_psize = MMU_PAGE_COUNT;
+
+ /*
+ * Convert the input range into one of the supported sizes. If the range
+ * doesn't fit, use the next larger supported size. Invalidation latency
+ * is high, so over-invalidation is preferred to issuing multiple
+ * invalidates.
+ */
+ if (size == PAGE_64K) {
+ atsd_start = start;
+ atsd_psize = MMU_PAGE_64K;
+ } else if (ALIGN_DOWN(start, PAGE_2M) == ALIGN_DOWN(end, PAGE_2M)) {
+ atsd_start = ALIGN_DOWN(start, PAGE_2M);
+ atsd_psize = MMU_PAGE_2M;
+ } else if (ALIGN_DOWN(start, PAGE_1G) == ALIGN_DOWN(end, PAGE_1G)) {
+ atsd_start = ALIGN_DOWN(start, PAGE_1G);
+ atsd_psize = MMU_PAGE_1G;
+ }
if (npu_context->nmmu_flush)
/*
@@ -631,10 +653,12 @@ static void mmio_invalidate(struct npu_context *npu_context, int va,
* an invalidate.
*/
acquire_atsd_reg(npu_context, mmio_atsd_reg);
- if (va)
- mmio_invalidate_va(mmio_atsd_reg, address, pid, flush);
- else
+
+ if (atsd_psize == MMU_PAGE_COUNT)
mmio_invalidate_pid(mmio_atsd_reg, pid, flush);
+ else
+ mmio_invalidate_range(mmio_atsd_reg, pid, atsd_start,
+ atsd_psize, flush);
mmio_invalidate_wait(mmio_atsd_reg);
if (flush) {
@@ -664,7 +688,7 @@ static void pnv_npu2_mn_release(struct mmu_notifier *mn,
* There should be no more translation requests for this PID, but we
* need to ensure any entries for it are removed from the TLB.
*/
- mmio_invalidate(npu_context, 0, 0, true);
+ mmio_invalidate(npu_context, 0, ~0UL, true);
}
static void pnv_npu2_mn_change_pte(struct mmu_notifier *mn,
@@ -673,8 +697,7 @@ static void pnv_npu2_mn_change_pte(struct mmu_notifier *mn,
pte_t pte)
{
struct npu_context *npu_context = mn_to_npu_context(mn);
-
- mmio_invalidate(npu_context, 1, address, true);
+ mmio_invalidate(npu_context, address, PAGE_SIZE, true);
}
static void pnv_npu2_mn_invalidate_range(struct mmu_notifier *mn,
@@ -682,21 +705,7 @@ static void pnv_npu2_mn_invalidate_range(struct mmu_notifier *mn,
unsigned long start, unsigned long end)
{
struct npu_context *npu_context = mn_to_npu_context(mn);
- unsigned long address;
-
- if (end - start > atsd_threshold) {
- /*
- * Just invalidate the entire PID if the address range is too
- * large.
- */
- mmio_invalidate(npu_context, 0, 0, true);
- } else {
- for (address = start; address < end; address += PAGE_SIZE)
- mmio_invalidate(npu_context, 1, address, false);
-
- /* Do the flush only on the final addess == end */
- mmio_invalidate(npu_context, 1, address, true);
- }
+ mmio_invalidate(npu_context, start, end - start, true);
}
static const struct mmu_notifier_ops nv_nmmu_notifier_ops = {
--
1.7.2.5
^ permalink raw reply related
* [PATCH 1/3] powerpc/powernv/npu: Reduce eieio usage when issuing ATSD invalidates
From: Mark Hairgrove @ 2018-09-27 23:23 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alistair Popple, Mark Hairgrove, Reza Arbab
In-Reply-To: <1538090591-28519-1-git-send-email-mhairgrove@nvidia.com>
There are two types of ATSDs issued to the NPU: invalidates targeting a
specific virtual address and invalidates targeting the whole address
space. In both cases prior to this change, the sequence was:
for each NPU
- Write the target address to the XTS_ATSD_AVA register
- EIEIO
- Write the launch value to issue the ATSD
First, a target address is not required when invalidating the whole
address space, so that write and the EIEIO have been removed. The AP
(size) field in the launch is not needed either.
Second, for per-address invalidates the above sequence is inefficient in
the common case of multiple NPUs because an EIEIO is issued per NPU. This
unnecessarily forces the launches of later ATSDs to be ordered with the
launches of earlier ones. The new sequence only issues a single EIEIO:
for each NPU
- Write the target address to the XTS_ATSD_AVA register
EIEIO
for each NPU
- Write the launch value to issue the ATSD
Performance results were gathered using a microbenchmark which creates a
1G allocation then uses mprotect with PROT_NONE to trigger invalidates in
strides across the allocation.
With only a single NPU active (one GPU) the difference is in the noise for
both types of invalidates (+/-1%).
With two NPUs active (on a 6-GPU system) the effect is more noticeable:
mprotect rate (GB/s)
Stride Before After Speedup
64K 5.9 6.5 10%
1M 31.2 33.4 7%
2M 36.3 38.7 7%
4M 322.6 356.7 11%
Signed-off-by: Mark Hairgrove <mhairgrove@nvidia.com>
---
arch/powerpc/platforms/powernv/npu-dma.c | 99 ++++++++++++++---------------
1 files changed, 48 insertions(+), 51 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
index 8006c54..c8f438a 100644
--- a/arch/powerpc/platforms/powernv/npu-dma.c
+++ b/arch/powerpc/platforms/powernv/npu-dma.c
@@ -454,79 +454,76 @@ static void put_mmio_atsd_reg(struct npu *npu, int reg)
}
/* MMIO ATSD register offsets */
-#define XTS_ATSD_AVA 1
-#define XTS_ATSD_STAT 2
+#define XTS_ATSD_LAUNCH 0
+#define XTS_ATSD_AVA 1
+#define XTS_ATSD_STAT 2
-static void mmio_launch_invalidate(struct mmio_atsd_reg *mmio_atsd_reg,
- unsigned long launch, unsigned long va)
+static unsigned long get_atsd_launch_val(unsigned long pid, unsigned long psize,
+ bool flush)
{
- struct npu *npu = mmio_atsd_reg->npu;
- int reg = mmio_atsd_reg->reg;
+ unsigned long launch = 0;
- __raw_writeq_be(va, npu->mmio_atsd_regs[reg] + XTS_ATSD_AVA);
- eieio();
- __raw_writeq_be(launch, npu->mmio_atsd_regs[reg]);
+ if (psize == MMU_PAGE_COUNT) {
+ /* IS set to invalidate entire matching PID */
+ launch |= PPC_BIT(12);
+ } else {
+ /* AP set to invalidate region of psize */
+ launch |= (u64)mmu_get_ap(psize) << PPC_BITLSHIFT(17);
+ }
+
+ /* PRS set to process-scoped */
+ launch |= PPC_BIT(13);
+
+ /* PID */
+ launch |= pid << PPC_BITLSHIFT(38);
+
+ /* No flush */
+ launch |= !flush << PPC_BITLSHIFT(39);
+
+ return launch;
}
-static void mmio_invalidate_pid(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
- unsigned long pid, bool flush)
+static void mmio_atsd_regs_write(struct mmio_atsd_reg
+ mmio_atsd_reg[NV_MAX_NPUS], unsigned long offset,
+ unsigned long val)
{
- int i;
- unsigned long launch;
+ struct npu *npu;
+ int i, reg;
for (i = 0; i <= max_npu2_index; i++) {
- if (mmio_atsd_reg[i].reg < 0)
+ reg = mmio_atsd_reg[i].reg;
+ if (reg < 0)
continue;
- /* IS set to invalidate matching PID */
- launch = PPC_BIT(12);
-
- /* PRS set to process-scoped */
- launch |= PPC_BIT(13);
-
- /* AP */
- launch |= (u64)
- mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
-
- /* PID */
- launch |= pid << PPC_BITLSHIFT(38);
+ npu = mmio_atsd_reg[i].npu;
+ __raw_writeq_be(val, npu->mmio_atsd_regs[reg] + offset);
+ }
+}
- /* No flush */
- launch |= !flush << PPC_BITLSHIFT(39);
+static void mmio_invalidate_pid(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
+ unsigned long pid, bool flush)
+{
+ unsigned long launch = get_atsd_launch_val(pid, MMU_PAGE_COUNT, flush);
- /* Invalidating the entire process doesn't use a va */
- mmio_launch_invalidate(&mmio_atsd_reg[i], launch, 0);
- }
+ /* Invalidating the entire process doesn't use a va */
+ mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_LAUNCH, launch);
}
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;
- for (i = 0; i <= max_npu2_index; i++) {
- if (mmio_atsd_reg[i].reg < 0)
- continue;
-
- /* IS set to invalidate target VA */
- launch = 0;
+ launch = get_atsd_launch_val(pid, mmu_virtual_psize, flush);
- /* PRS set to process scoped */
- launch |= PPC_BIT(13);
+ /* Write all VAs first */
+ mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_AVA, va);
- /* AP */
- launch |= (u64)
- mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
-
- /* PID */
- launch |= pid << PPC_BITLSHIFT(38);
-
- /* No flush */
- launch |= !flush << PPC_BITLSHIFT(39);
+ /* Issue one barrier for all address writes */
+ eieio();
- mmio_launch_invalidate(&mmio_atsd_reg[i], launch, va);
- }
+ /* Launch */
+ mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_LAUNCH, launch);
}
#define mn_to_npu_context(x) container_of(x, struct npu_context, mn)
--
1.7.2.5
^ permalink raw reply related
* [PATCH 0/3] powerpc/powernv/npu: Improve ATSD invalidation overhead
From: Mark Hairgrove @ 2018-09-27 23:23 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alistair Popple, Mark Hairgrove, Reza Arbab
When ATS is used in a process, all CPU TLB invalidates in that process
also trigger ATSD invalidates via mmu_notifiers. This additional overhead
is noticeable in applications which do heavy memory allocation or page
migration among nodes, particularly to and from GPUs.
This patch set reduces that overhead by rearranging how the ATSDs are
issued and by using size-based ATSD invalidates.
Mark Hairgrove (3):
powerpc/powernv/npu: Reduce eieio usage when issuing ATSD invalidates
powerpc/powernv/npu: Use size-based ATSD invalidates
powerpc/powernv/npu: Remove atsd_threshold debugfs setting
arch/powerpc/platforms/powernv/npu-dma.c | 177 ++++++++++++++---------------
1 files changed, 85 insertions(+), 92 deletions(-)
--
1.7.2.5
^ permalink raw reply
* Re: [PATCH 3/5] dma-direct: refine dma_direct_alloc zone selection
From: Benjamin Herrenschmidt @ 2018-09-28 0:06 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Greg Kroah-Hartman, Robin Murphy, linux-kernel, iommu,
linuxppc-dev, Marek Szyprowski
In-Reply-To: <20180927134922.GA8281@lst.de>
On Thu, 2018-09-27 at 15:49 +0200, Christoph Hellwig wrote:
> On Thu, Sep 27, 2018 at 11:45:15AM +1000, Benjamin Herrenschmidt wrote:
> > I'm not sure this is entirely right.
> >
> > Let's say the mask is 30 bits. You will return GFP_DMA32, which will
> > fail if you allocate something above 1G (which is legit for
> > ZONE_DMA32).
>
> And then we will try GFP_DMA further down in the function:
>
> if (IS_ENABLED(CONFIG_ZONE_DMA) &&
> dev->coherent_dma_mask < DMA_BIT_MASK(32) &&
> !(gfp & GFP_DMA)) {
> gfp = (gfp & ~GFP_DMA32) | GFP_DMA;
> goto again;
> }
>
> This is and old optimization from x86, because chances are high that
> GFP_DMA32 will give you suitable memory for the infamous 31-bit
> dma mask devices (at least at boot time) and thus we don't have
> to deplete the tiny ZONE_DMA pool.
I see, it's rather confusing :-) Wouldn't it be better to check against
top of 32-bit memory instead here too ?
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH v4] powerpc/64s: reimplement book3s idle code in C
From: Nicholas Piggin @ 2018-09-28 0:16 UTC (permalink / raw)
To: Akshay Adiga
Cc: Gautham R . Shenoy, linuxppc-dev, Aneesh Kumar K.V, kvm-ppc,
Mahesh Jagannath Salgaonkar
In-Reply-To: <20180926140914.GA2442@aks.ibm>
On Wed, 26 Sep 2018 19:39:14 +0530
Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> wrote:
> On Fri, Sep 14, 2018 at 11:52:40AM +1000, Nicholas Piggin wrote:
> > +
> > + /*
> > + * On POWER9, SRR1 bits do not match exactly as expected.
> > + * SRR1_WS_GPRLOSS (10b) can also result in SPR loss, so
> > + * always test PSSCR if there is any state loss.
> > + */
> > + if (likely((psscr & PSSCR_RL_MASK) < pnv_first_hv_loss_level)) {
>
> Shouldn't we check PLS field to see if the cpu/core woke up from hv loss ?
>
> Currently, a cpu requested stop4 (RL=4) and exited from a shallower state
> (PLS=2), SPR's are unecessarily restored.
>
> We can do something like :
>
> #define PSSCR_PLS_SHIFT 60
> if (likely((psscr & PSSCR_PLS) >> PSSCR_PLS_SHIFT) < pnv_first_hv_loss_level)
Ah, that corresponds with the following existing code?
/*
* POWER ISA 3. Use PSSCR to determine if we
* are waking up from deep idle state
*/
LOAD_REG_ADDRBASE(r5,pnv_first_deep_stop_state)
ld r4,ADDROFF(pnv_first_deep_stop_state)(r5)
/*
* 0-3 bits correspond to Power-Saving Level Status
* which indicates the idle state we are waking up from
*/
mfspr r5, SPRN_PSSCR
rldicl r5,r5,4,60
li r0, 0 /* clear requested_psscr to say we're awake */
std r0, PACA_REQ_PSSCR(r13)
cmpd cr4,r5,r4
bge cr4,pnv_wakeup_tb_loss /* returns to caller */
Yes I didn't get that right, good catch.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] powerpc: wire up memtest
From: Michael Ellerman @ 2018-09-28 3:41 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <b3f2074b89ef0e3352fb11b69be30caeaa823fa0.1535360497.git.christophe.leroy@c-s.fr>
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Add call to early_memtest() so that kernel compiled with
> CONFIG_MEMTEST really perform memtest at startup when requested
> via 'memtest' boot parameter.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/kernel/setup-common.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> index 93fa0c99681e..904b728eb20d 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -33,6 +33,7 @@
> #include <linux/serial_8250.h>
> #include <linux/percpu.h>
> #include <linux/memblock.h>
> +#include <linux/bootmem.h>
> #include <linux/of_platform.h>
> #include <linux/hugetlb.h>
> #include <asm/debugfs.h>
> @@ -917,6 +918,8 @@ void __init setup_arch(char **cmdline_p)
> /* Parse memory topology */
> mem_topology_setup();
>
> + early_memtest(min_low_pfn << PAGE_SHIFT, max_low_pfn << PAGE_SHIFT);
On a ppc64le VM this boils down to early_memtest(0, 0) for me.
I think it's too early, we don't set up max_low_pfn until
initmem_init().
If I move it after initmem_init() then it does something more useful:
early_memtest: # of tests: 17
0x0000000001450580 - 0x0000000001450800 pattern 4c494e5558726c7a
0x0000000001450c00 - 0x0000000003600000 pattern 4c494e5558726c7a
0x00000000047c0000 - 0x000000002fff0000 pattern 4c494e5558726c7a
0x0000000030000000 - 0x000000003ff24000 pattern 4c494e5558726c7a
0x000000003fff4000 - 0x000000003fff4c00 pattern 4c494e5558726c7a
0x000000003fff5000 - 0x000000003fff5300 pattern 4c494e5558726c7a
0x000000003fff5c00 - 0x000000003fff5f00 pattern 4c494e5558726c7a
0x000000003fff6800 - 0x000000003fff6b00 pattern 4c494e5558726c7a
0x000000003fff7400 - 0x000000003fff7700 pattern 4c494e5558726c7a
0x000000003fff8000 - 0x000000003fff8300 pattern 4c494e5558726c7a
0x000000003fff8c00 - 0x000000003fff8f00 pattern 4c494e5558726c7a
0x000000003fff9800 - 0x000000003fff9b00 pattern 4c494e5558726c7a
0x000000003fffa400 - 0x000000003fffa700 pattern 4c494e5558726c7a
0x000000003fffb000 - 0x000000003fffb300 pattern 4c494e5558726c7a
0x000000003fffbc00 - 0x000000003fffbf00 pattern 4c494e5558726c7a
0x000000003fffc800 - 0x000000003fffcb00 pattern 4c494e5558726c7a
0x000000003fffd400 - 0x000000003fffd700 pattern 4c494e5558726c7a
0x000000003fffe000 - 0x000000003fffe100 pattern 4c494e5558726c7a
0x0000000040000000 - 0x00000000ffc10000 pattern 4c494e5558726c7a
0x00000000fffa0000 - 0x00000000fffa5b00 pattern 4c494e5558726c7a
0x0000000100000000 - 0x00000001ffbe0000 pattern 4c494e5558726c7a
0x00000001fff60000 - 0x00000001fff61b00 pattern 4c494e5558726c7a
0x00000001fffec000 - 0x00000001fffec4b8 pattern 4c494e5558726c7a
0x00000001fffec524 - 0x00000001fffec528 pattern 4c494e5558726c7a
cheers
^ permalink raw reply
* [PATCH] powerpc/numa: Skip onlining a offline node in kdump path
From: Srikar Dronamraju @ 2018-09-28 3:47 UTC (permalink / raw)
To: Michael Ellerman
Cc: Pavithra Prakash, Srikar Dronamraju, Mahesh J Salgaonkar,
Hari Bathini, linuxppc-dev, Michael Bringmann
With Commit 2ea626306810 ("powerpc/topology: Get topology for shared
processors at boot"), kdump kernel on shared lpar may crash.
The necessary conditions are
- Shared Lpar with atleast 2 nodes having memory and CPUs.
- Memory requirement for kdump kernel must be met by the first N-1 nodes
where there are atleast N nodes with memory and CPUs.
Example numactl of such a machine.
numactl -H
available: 5 nodes (0,2,5-7)
node 0 cpus:
node 0 size: 0 MB
node 0 free: 0 MB
node 2 cpus:
node 2 size: 255 MB
node 2 free: 189 MB
node 5 cpus: 24 25 26 27 28 29 30 31
node 5 size: 4095 MB
node 5 free: 4024 MB
node 6 cpus: 0 1 2 3 4 5 6 7 16 17 18 19 20 21 22 23
node 6 size: 6353 MB
node 6 free: 5998 MB
node 7 cpus: 8 9 10 11 12 13 14 15 32 33 34 35 36 37 38 39
node 7 size: 7640 MB
node 7 free: 7164 MB
node distances:
node 0 2 5 6 7
0: 10 40 40 40 40
2: 40 10 40 40 40
5: 40 40 10 40 40
6: 40 40 40 10 20
7: 40 40 40 20 10
Steps to reproduce.
1. Load / start kdump service.
2. Trigger a kdump (for example : echo c > /proc/sysrq-trigger)
When booting a kdump kernel with 2048M
kexec: Starting switchover sequence.
I'm in purgatory
Using 1TB segments
hash-mmu: Initializing hash mmu with SLB
Linux version 4.19.0-rc5-master+ (srikar@linux-xxu6) (gcc version 4.8.5 (SUSE Linux)) #1 SMP Thu Sep 27 19:45:00 IST 2018
Found initrd at 0xc000000009e70000:0xc00000000ae554b4
Using pSeries machine description
-----------------------------------------------------
ppc64_pft_size = 0x1e
phys_mem_size = 0x88000000
dcache_bsize = 0x80
icache_bsize = 0x80
cpu_features = 0x000000ff8f5d91a7
possible = 0x0000fbffcf5fb1a7
always = 0x0000006f8b5c91a1
cpu_user_features = 0xdc0065c2 0xef000000
mmu_features = 0x7c006001
firmware_features = 0x00000007c45bfc57
htab_hash_mask = 0x7fffff
physical_start = 0x8000000
-----------------------------------------------------
numa: NODE_DATA [mem 0x87d5e300-0x87d67fff]
numa: NODE_DATA(0) on node 6
numa: NODE_DATA [mem 0x87d54600-0x87d5e2ff]
Top of RAM: 0x88000000, Total RAM: 0x88000000
Memory hole size: 0MB
Zone ranges:
DMA [mem 0x0000000000000000-0x0000000087ffffff]
DMA32 empty
Normal empty
Movable zone start for each node
Early memory node ranges
node 6: [mem 0x0000000000000000-0x0000000087ffffff]
Could not find start_pfn for node 0
Initmem setup node 0 [mem 0x0000000000000000-0x0000000000000000]
On node 0 totalpages: 0
Initmem setup node 6 [mem 0x0000000000000000-0x0000000087ffffff]
On node 6 totalpages: 34816
Unable to handle kernel paging request for data at address 0x00000060
Faulting instruction address: 0xc000000008703a54
Oops: Kernel access of bad area, sig: 11 [#1]
LE SMP NR_CPUS=2048 NUMA pSeries
Modules linked in:
CPU: 11 PID: 1 Comm: swapper/11 Not tainted 4.19.0-rc5-master+ #1
NIP: c000000008703a54 LR: c000000008703a38 CTR: 0000000000000000
REGS: c00000000b673440 TRAP: 0380 Not tainted (4.19.0-rc5-master+)
MSR: 8000000002009033 <SF,VEC,EE,ME,IR,DR,RI,LE> CR: 24022022 XER: 20000002
CFAR: c0000000086fc238 IRQMASK: 0
GPR00: c000000008703a38 c00000000b6736c0 c000000009281900 0000000000000000
GPR04: 0000000000000000 0000000000000000 fffffffffffff001 c00000000b660080
GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000220
GPR12: 0000000000002200 c000000009e51400 0000000000000000 0000000000000008
GPR16: 0000000000000000 c000000008c152e8 c000000008c152a8 0000000000000000
GPR20: c000000009422fd8 c000000009412fd8 c000000009426040 0000000000000008
GPR24: 0000000000000000 0000000000000000 c000000009168bc8 c000000009168c78
GPR28: c00000000b126410 0000000000000000 c00000000916a0b8 c00000000b126400
NIP [c000000008703a54] bus_add_device+0x84/0x1e0
LR [c000000008703a38] bus_add_device+0x68/0x1e0
Call Trace:
[c00000000b6736c0] [c000000008703a38] bus_add_device+0x68/0x1e0 (unreliable)
[c00000000b673740] [c000000008700194] device_add+0x454/0x7c0
[c00000000b673800] [c00000000872e660] __register_one_node+0xb0/0x240
[c00000000b673860] [c00000000839a6bc] __try_online_node+0x12c/0x180
[c00000000b673900] [c00000000839b978] try_online_node+0x58/0x90
[c00000000b673930] [c0000000080846d8] find_and_online_cpu_nid+0x158/0x190
[c00000000b673a10] [c0000000080848a0] numa_update_cpu_topology+0x190/0x580
[c00000000b673c00] [c000000008d3f2e4] smp_cpus_done+0x94/0x108
[c00000000b673c70] [c000000008d5c00c] smp_init+0x174/0x19c
[c00000000b673d00] [c000000008d346b8] kernel_init_freeable+0x1e0/0x450
[c00000000b673dc0] [c0000000080102e8] kernel_init+0x28/0x160
[c00000000b673e30] [c00000000800b65c] ret_from_kernel_thread+0x5c/0x80
Instruction dump:
60000000 60000000 e89e0020 7fe3fb78 4bff87d5 60000000 7c7d1b79 4082008c
e8bf0050 e93e0098 3b9f0010 2fa50000 <e8690060> 38630018 419e0114 7f84e378
---[ end trace 593577668c2daa65 ]---
However a regular kernel with 4096M (2048 gets reserved for
crash kernel) boots properly.
Unlike regular kernels, which mark all available nodes as online, kdump
kernel only marks just enough nodes as online and marks the rest as
offline at boot. However kdump kernel boots with all available CPUs.
With Commit 2ea626306810 ("powerpc/topology: Get topology for shared
processors at boot"), all CPUs are onlined on their respective nodes at
boot time. try_online_node() tries to online the offline nodes but fails
as all needed subsystems are not yet initialized.
As part of fix, detect and skip early onlining of a offline node.
Fixes: 2ea626306810 ("powerpc/topology: Get topology for shared processors at boot")
Reported-by: Pavithra Prakash <pavrampu@in.ibm.com>
Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
arch/powerpc/mm/numa.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index e94148a1d7e4..d88139acdfe6 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -1217,9 +1217,10 @@ int find_and_online_cpu_nid(int cpu)
* Need to ensure that NODE_DATA is initialized for a node from
* available memory (see memblock_alloc_try_nid). If unable to
* init the node, then default to nearest node that has memory
- * installed.
+ * installed. Skip onlining a node if the subsystems are not
+ * yet initialized.
*/
- if (try_online_node(new_nid))
+ if (!topology_inited || try_online_node(new_nid))
new_nid = first_online_node;
#else
/*
--
2.12.3
^ permalink raw reply related
* Re: [PATCH v6] selftests: add headers_install to lib.mk
From: Michael Ellerman @ 2018-09-28 4:52 UTC (permalink / raw)
To: Anders Roxell, yamada.masahiro, michal.lkml, shuah, bamv2005,
brgl, pbonzini, akpm, rppt, aarcange
Cc: Anders Roxell, linux-kbuild, netdev, linux-kernel,
linux-kselftest, linuxppc-dev
In-Reply-To: <20180904104721.27535-1-anders.roxell@linaro.org>
[ + linuxppc-dev ]
Anders Roxell <anders.roxell@linaro.org> writes:
> If the kernel headers aren't installed we can't build all the tests.
> Add a new make target rule 'khdr' in the file lib.mk to generate the
> kernel headers and that gets include for every test-dir Makefile that
> includes lib.mk If the testdir in turn have its own sub-dirs the
> top_srcdir needs to be set to the linux-rootdir to be able to generate
> the kernel headers.
>
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> Reviewed-by: Fathi Boudra <fathi.boudra@linaro.org>
> ---
>
> I sent this (v5) a month ago and wondered if it got lost. Resending
> unchanged.
>
> Cheers,
> Anders
>
> Makefile | 14 +-------------
> scripts/subarch.include | 13 +++++++++++++
> tools/testing/selftests/android/Makefile | 2 +-
> tools/testing/selftests/android/ion/Makefile | 2 ++
> tools/testing/selftests/futex/functional/Makefile | 1 +
> tools/testing/selftests/gpio/Makefile | 7 ++-----
> tools/testing/selftests/kvm/Makefile | 7 ++-----
> tools/testing/selftests/lib.mk | 12 ++++++++++++
> tools/testing/selftests/net/Makefile | 1 +
> .../selftests/networking/timestamping/Makefile | 1 +
> tools/testing/selftests/vm/Makefile | 4 ----
> 11 files changed, 36 insertions(+), 28 deletions(-)
> create mode 100644 scripts/subarch.include
This broke all the powerpc selftests :(
Why did it go in at rc5?
cheers
^ permalink raw reply
* Re: [RFC PATCH 09/11] powerpc/tm: Do not restore default DSCR
From: Michael Neuling @ 2018-09-28 5:03 UTC (permalink / raw)
To: Breno Leitao, linuxppc-dev; +Cc: ldufour, gromero
In-Reply-To: <11f2155f-eff8-2aac-ab47-1ab343d52107@debian.org>
On Thu, 2018-09-27 at 17:51 -0300, Breno Leitao wrote:
> Hi Mikey,
>
> On 09/18/2018 02:41 AM, Michael Neuling wrote:
> > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> > > In the previous TM code, trecheckpoint was being executed in the middle of
> > > an exception, thus, DSCR was being restored to default kernel DSCR value
> > > after trecheckpoint was done.
> > >
> > > With this current patchset, trecheckpoint is executed just before getting
> > > to userspace, at ret_from_except_lite, for example. Thus, we do not need
> > > to
> > > set default kernel DSCR value anymore, as we are leaving kernel space. It
> > > is OK to keep the checkpointed DSCR value into the live SPR, mainly
> > > because
> > > the transaction is doomed and it will fail soon (after RFID),
> >
> > What if we are going back to a suspended transaction? It will remain live
> > until
> > userspace does a tresume
>
> Hmm, I understand that once we get in kernel space, and call
> treclaim/trecheckpoint, the transaction will be doomed and it will abort and
> rollback when we leave kernel space. I.e., if we can treclaim/trecheckpointn
> in kernel space, the transaction will *always* abort just after RFID, so, a
> possible tresume will never be executed. Is my understanding wrong?
Your understanding is wrong. We don't roll back until MSR[TS] = T.
There are two cases for the RFID.
1) if you RFID back to userspace that is transactional (ie MSR[TS] = T), then it
will immediately rollback as soon as the RFID happens since we are going
directly to T.
2) If you RFID back to userspace that is suspended (ie MSR[TS] = S), then it
won't roll back until userspace does a tresume. It doesn't rollback until we go
MSR[TS] = S -> T).
> >
> > > so,
> > > continuing with the pre-checkpointed DSCR value is what seems correct.
> >
> > Reading this description suggests this patch isn't really needed. Right?
>
> Maybe the description is not clear, but I understand this patch is required,
> otherwise we will leave userspace with a default DSCR value.
>
> By the way, do you know if there is a change in DSCR inside a transaction,
> will it be reverted if the transaction is aborted?
Yes it will be reverted. We even have a selftest for it in
tools/testing/selftests/powerpc/tm/tm-resched-dscr.c
There are a bunch of checkpointed SPRs. From the arch:
Checkpointed registers: The set of registers that are
saved to the “checkpoint area” when a transaction is
initiated, and restored upon transaction failure, is a
subset of the architected register state, consisting of
the General Purpose Registers, Floating-Point Regis-
ters, Vector Registers, Vector-Scalar Registers, and the
following Special Registers and fields: CR fields other
than CR0, XER, LR, CTR, FPSCR, AMR, PPR,
VRSAVE, VSCR, DSCR, and TAR.
Mikey
^ permalink raw reply
* Re: [RFC PATCH 10/11] powerpc/tm: Set failure summary
From: Michael Neuling @ 2018-09-28 5:17 UTC (permalink / raw)
To: Breno Leitao, linuxppc-dev; +Cc: ldufour, gromero
In-Reply-To: <85a2de6d-6cb5-6052-252e-858e61eb00ab@debian.org>
On Thu, 2018-09-27 at 17:52 -0300, Breno Leitao wrote:
> Hi Mikey,
>
> On 09/18/2018 02:50 AM, Michael Neuling wrote:
> > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> > > Since the transaction will be doomed with treckpt., the TEXASR[FS]
> > > should be set, to reflect that the transaction is a failure. This patch
> > > ensures it before recheckpointing, and remove changes from other places
> > > that were calling recheckpoint.
> >
> > TEXASR[FS] should be set by the reclaim.
>
> Do you mean that the CPU should set TEXASR[FS] when treclaim is called, or,
> that the tm_reclaim?
>
> Looking at the ISA, I didn't see TEXASR[FS] being set automatically when a
> reclaim happens,
treclaim in ISA talks about "TMRecordFailure(cause)" and that macro sets
TEXASR[FS]=1.
So yes treclaim always sets TEXASR[FS]=1.
> although, I see it needs TEXASR[FS] to be set when executing a
> trecheckpoint, otherwise it will cause a TM Bad Thing.
Yep
> That is why I am forcing TEXASR[FS]=1 to doom the transaction so we can
> recheckpoint it, but it seems I understood this wrongly.
You shouldn't need to. We do a bug_on() just before the trecheckpoint to make
sure it's set, but you shouldn't need to set it (other than the signals case).
> > I don't know why you'd need to set this
> > explicitly in process.c. The only case is when the user supplies a bad
> > signal
> > context, but we should check that in the signals code, not process.c
> >
> > Hence I think this patch is wrong.
> >
> > Also, according to the architecture, TEXASR[FS] HAS TO BE SET on
> > trecheckpoint
> > otherwise you'll get a TM Bad Thing. You should say that rather than
> > suggesting
> > it's because the transaction is doomed. It's ilqlegal to not do it. That's
> > why we
> > have this check in arch/powerpc/kernel/tm.S.
>
> When you say "HAS TO BE SET", do you mean that the hardware will set it and
> we shouldn't care about this flag? Thus, if I am hitting EMIT_BUG_ENTRY, it
> means my TEXASR was messed somehow?
I'm just saying what you said before about the tm bad thing. We have to make
sure it's set before we do the trecheckpoint otherwise we end up in the TM bad
thing. The treclaim should handle this for us (but again the signals case need
to be checked).
Mikey
^ permalink raw reply
* Re: [RFC PATCH 11/11] selftests/powerpc: Adapt the test
From: Michael Neuling @ 2018-09-28 5:25 UTC (permalink / raw)
To: Breno Leitao, linuxppc-dev; +Cc: ldufour, gromero
In-Reply-To: <434af377-492f-358a-b759-ab68eaeccb3c@debian.org>
On Thu, 2018-09-27 at 17:57 -0300, Breno Leitao wrote:
> Hi Mikey,
>
> On 09/18/2018 03:36 AM, Michael Neuling wrote:
> > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> > > The Documentation/powerpc/transactional_memory.txt says:
> > >
> > > "Syscalls made from within a suspended transaction are performed as
> > > normal
> > > and the transaction is not explicitly doomed by the kernel. However,
> > > what the kernel does to perform the syscall may result in the
> > > transaction
> > > being doomed by the hardware."
> > >
> > > With this new TM mechanism, the syscall will continue to be executed if
> > > the
> > > syscall happens on a suspended syscall, but, the syscall will *fail* if
> > > the
> > > transaction is still active during the syscall invocation.
> >
> > Not sure I get this. This doesn't seem any different to before.
> >
> > An active (not suspended) transaction *will* result in the syscall failing
> > and
> > the transaction being doomed.
> >
> > A syscall in a suspended transaction should succeed and the transaction.
>
> I understand that a transaction will always be doomed if there is a
> reclaim/checkpoint, thus, if the you make a system call inside a suspended
> transaction, it will reclaim and recheckpoint, thus, dooming the transaction,
> and failing it on the next RFID.
So currently a syscall won't explicitly treclaim/trecheckpoint so it won't
necessarily be doomed.
With your new patches it will be.
> So, the syscall executed in suspended mode may succeed, because it will not
> be skipped (as in active mode), but the transaction will *always* fail,
> because there was a reclaim and recheckpoint.
>
> > You might need to clean up the language. I try to use:
> >
> > Active == transactional but not suspended (ie MSR[TS] = T)
> > Suspended == suspended (ie MSR [TS] = S)
> > Doomed == transaction to be rolled back at next opportinity (ie tcheck
> > returns doomed)
> >
> > (note: the kernel MSR_TM_ACTIVE() macro is not consistent with this since
> > it's
> > MSR[TS] == S or T).
>
> Ok, I agree with this language as well. I might want to improve the code to
> follow the same language all across the code.
>
> > > On the syscall path, if the transaction is active and not suspended, it
> > > will call TM_KERNEL_ENTRY which will reclaim and recheckpoint the
> > > transaction, thus, dooming the transaction on userspace return, with
> > > failure code TM_CAUSE_SYSCALL.
> >
> > But the test below is on a suspend transaction?
>
> Sorry, I meant "suspended transaction" above instead of "transaction is
> active and not suspended".
>
> >
> > > This new model will break part of this test, but I understand that that
> > > the
> > > documentation above didn't guarantee that the syscall would succeed, and
> > > it
> > > will never succeed anymore now on.
> >
> > The syscall should pass in suspend (modulo the normal syscall checks). The
> > transaction may fail as a result.
>
> Perfect, and if the transaction fail, the CPU will rollback the changes and
> restore the checkpoint registers (replacing the r3 that contains the pid
> value), thus, it will be like "getpid" system call didn't execute.
No. If we are suspended, then we go back right after the sc. We don't get
rolled back till the tresume.
Mikey
> For this test specifically, it assumes the syscall didn't execute if the
> transaction failed. Take a look:
>
> FUNC_START(getppid_tm_suspended)
> tbegin.
> beq 1f
> li r0, __NR_getppid
> tsuspend.
> sc
> tresume.
> tend.
> blr
> 1:
> li r3, -1
> blr
>
> So, the tests thinks the syscall failed because the transaction aborted.
>
> Anyway, I can try to improve this test other than removing this test, but I
> am not sure how.
>
> Thank you
>
^ permalink raw reply
* Re: [RFC PATCH 05/11] powerpc/tm: Function that updates the failure code
From: Michael Neuling @ 2018-09-28 5:27 UTC (permalink / raw)
To: Breno Leitao, linuxppc-dev; +Cc: ldufour, gromero
In-Reply-To: <205eceb0-1f6f-340c-fea4-97b31297272d@debian.org>
On Thu, 2018-09-27 at 17:58 -0300, Breno Leitao wrote:
> Hi Mikey,
>
> On 09/17/2018 10:29 PM, Michael Neuling wrote:
> > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> > > Now the transaction reclaims happens very earlier in the trap handler, and
> > > it is impossible to know precisely, at that early time, what should be set
> > > as the failure cause for some specific cases, as, if the task will be
> > > rescheduled, thus, the transaction abort case should be updated from
> > > TM_CAUSE_MISC to TM_CAUSE_RESCHED, for example.
> >
> > Please add comments to where this is used (in EXCEPTION_COMMON macro I
> > think)
> > that say this might happen.
>
> Is it OK if I comment it in TM_KERNEL_ENTRY macro, since the failure cause
> could be updated independently of the exception being execute, so, every call
> to TM_KERNEL_ENTRY can have the cause overwritten.
Sure.
> I.e. it does not matter if the exception is a systemcall or a page fault,
> the failure cause will be updated if there is a process reschedule after the
> exception/syscall is handled.
>
> Thank you
>
^ permalink raw reply
* Re: [RFC PATCH 08/11] powerpc/tm: Do not reclaim on ptrace
From: Michael Neuling @ 2018-09-28 5:36 UTC (permalink / raw)
To: Breno Leitao, linuxppc-dev; +Cc: ldufour, gromero
In-Reply-To: <18dcf8b7-1001-d6cc-1847-af9c679f9435@debian.org>
On Thu, 2018-09-27 at 18:03 -0300, Breno Leitao wrote:
> Hi Mikey,
>
> On 09/18/2018 02:36 AM, Michael Neuling wrote:
> > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> > > Make sure that we are not suspended on ptrace and that the registers were
> > > already reclaimed.
> > >
> > > Since the data was already reclaimed, there is nothing to be done here
> > > except to restore the SPRs.
> > >
> > > Signed-off-by: Breno Leitao <leitao@debian.org>
> > > ---
> > > arch/powerpc/kernel/ptrace.c | 10 ++++------
> > > 1 file changed, 4 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> > > index 9667666eb18e..cf6ee9154b11 100644
> > > --- a/arch/powerpc/kernel/ptrace.c
> > > +++ b/arch/powerpc/kernel/ptrace.c
> > > @@ -136,12 +136,10 @@ static void flush_tmregs_to_thread(struct
> > > task_struct
> > > *tsk)
> > > if ((!cpu_has_feature(CPU_FTR_TM)) || (tsk != current))
> > > return;
> > >
> > > - if (MSR_TM_SUSPENDED(mfmsr())) {
> > > - tm_reclaim_current(TM_CAUSE_SIGNAL);
> > > - } else {
> > > - tm_enable();
> > > - tm_save_sprs(&(tsk->thread));
> > > - }
> > > + WARN_ON(MSR_TM_SUSPENDED(mfmsr()));
> > > +
> > > + tm_enable();
> > > + tm_save_sprs(&(tsk->thread));
> >
> > Do we need to check if TM was enabled in the task before saving the TM SPRs?
> >
> > What happens if TM was lazily off and hence we had someone else's TM SPRs in
> > the
> > CPU currently? Wouldn't this flush the wrong values to the task_struct?
> >
> > I think we need to check the processes MSR before doing this.
>
> Yes, it is a *very* good point, and I think we are vulnerable even before
> this patch (in the current kernel). Take a look above, we are calling
> tm_save_sprs() if MSR is not TM suspended independently if TM is lazily off.
I think you're right, we might already have an issue. There are some paths in
here that don't check the userspace msr or any of the lazy tm enable. :(
Mikey
> It shouldn't be hard to create a test case for it. I can try to call
> ptrace(PTRACE_GETVRREGS) on a task that sleeps until TM is lazily disabled,
> compare if the TM SPR changed in this mean time. (while doing HTM in parallel
> to keep HTM SPR changing). Let's see if I can read others task TM SPRs.
>
> Thank you.
>
^ permalink raw reply
* [PATCH kernel] cxl: Remove unused include
From: Alexey Kardashevskiy @ 2018-09-28 6:38 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alexey Kardashevskiy, Frederic Barrat, Andrew Donnellan
The included opal.h gives a wrong idea that CXL makes PPC OPAL calls
while it does not so let's remote it.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
drivers/misc/cxl/pci.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index b66d832..8cbcbb7 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -17,7 +17,6 @@
#include <linux/pci.h>
#include <linux/of.h>
#include <linux/delay.h>
-#include <asm/opal.h>
#include <asm/msi_bitmap.h>
#include <asm/pnv-pci.h>
#include <asm/io.h>
--
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