* Re: [RESEND PATCH v2] powerpc/mce: Fix SLB rebolting during MCE recovery path.
From: Mahesh Jagannath Salgaonkar @ 2018-08-23 8:01 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: Nicholas Piggin, Aneesh Kumar K.V
In-Reply-To: <87d0u9lgyt.fsf@concordia.ellerman.id.au>
On 08/23/2018 12:14 PM, Michael Ellerman wrote:
> Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com> writes:
>
>> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>>
>> With the powerpc next commit e7e81847478 (powerpc/mce: Fix SLB rebolting
>> during MCE recovery path.),
>
> That commit description is wrong, I'll fix it up.
Ouch.. My bad.. :-(
>
> cheers
>
>> the SLB error recovery is broken. The new
>> change now does not add index value to RB[52-63] that selects the SLB
>> entry while rebolting, instead it assumes that the shadow save area
>> already have index embeded correctly in esid field. While all valid bolted
>> save areas do contain index value set correctly, there is a case where
>> 3rd (KSTACK_INDEX) entry for kernel stack does not embed index for NULL
>> esid entry. This patch fixes that.
>>
>> Without this patch the SLB rebolt code overwrites the 1st entry of kernel
>> linear mapping and causes SLB recovery to fail.
>>
>> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/mm/slb.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
>> index 0b095fa54049..9f574e59d178 100644
>> --- a/arch/powerpc/mm/slb.c
>> +++ b/arch/powerpc/mm/slb.c
>> @@ -70,7 +70,7 @@ static inline void slb_shadow_update(unsigned long ea, int ssize,
>>
>> static inline void slb_shadow_clear(enum slb_index index)
>> {
>> - WRITE_ONCE(get_slb_shadow()->save_area[index].esid, 0);
>> + WRITE_ONCE(get_slb_shadow()->save_area[index].esid, cpu_to_be64(index));
>> }
>>
>> static inline void create_shadowed_slbe(unsigned long ea, int ssize,
>
^ permalink raw reply
* [PATCH 4.4 60/79] net/ethernet/freescale/fman: fix cross-build error
From: Greg Kroah-Hartman @ 2018-08-23 7:53 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Randy Dunlap, Madalin Bucur, netdev,
linuxppc-dev, David S. Miller, Sasha Levin
In-Reply-To: <20180823074918.641878835@linuxfoundation.org>
4.4-stable review patch. If anyone has any objections, please let me know.
------------------
From: Randy Dunlap <rdunlap@infradead.org>
[ Upstream commit c133459765fae249ba482f62e12f987aec4376f0 ]
CC [M] drivers/net/ethernet/freescale/fman/fman.o
In file included from ../drivers/net/ethernet/freescale/fman/fman.c:35:
../include/linux/fsl/guts.h: In function 'guts_set_dmacr':
../include/linux/fsl/guts.h:165:2: error: implicit declaration of function 'clrsetbits_be32' [-Werror=implicit-function-declaration]
clrsetbits_be32(&guts->dmacr, 3 << shift, device << shift);
^~~~~~~~~~~~~~~
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Cc: Madalin Bucur <madalin.bucur@nxp.com>
Cc: netdev@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
include/linux/fsl/guts.h | 1 +
1 file changed, 1 insertion(+)
--- a/include/linux/fsl/guts.h
+++ b/include/linux/fsl/guts.h
@@ -16,6 +16,7 @@
#define __FSL_GUTS_H__
#include <linux/types.h>
+#include <linux/io.h>
/**
* Global Utility Registers.
^ permalink raw reply
* Re: Infinite looping observed in __offline_pages
From: Michal Hocko @ 2018-08-23 7:25 UTC (permalink / raw)
To: Mike Kravetz
Cc: Aneesh Kumar K.V, Haren Myneni, n-horiguchi, linuxppc-dev,
linux-kernel, kamezawa.hiroyu, mgorman
In-Reply-To: <54c72a22-a921-fc64-460d-f66985d0df4e@oracle.com>
On Wed 22-08-18 11:58:02, Mike Kravetz wrote:
> On 08/22/2018 02:30 AM, Aneesh Kumar K.V wrote:
[...]
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index 4eb6e824a80c..f9bdea685cf4 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -1338,7 +1338,8 @@ static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
> > return pfn;
> > if (__PageMovable(page))
> > return pfn;
> > - if (PageHuge(page)) {
> > + if (IS_ENABLED(CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION) &&
> > + PageHuge(page)) {
>
> How about using hugepage_migration_supported instead? It would automatically
> catch those non-migratable huge page sizes. Something like:
>
> if (PageHuge(page) &&
> hugepage_migration_supported(page_hstate(page))) {
Ohh, definitely, this is much better.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RESEND PATCH v2] powerpc/mce: Fix SLB rebolting during MCE recovery path.
From: Nicholas Piggin @ 2018-08-23 7:02 UTC (permalink / raw)
To: Mahesh J Salgaonkar; +Cc: linuxppc-dev, Michael Ellerman, Aneesh Kumar K.V
In-Reply-To: <153500619258.20614.8965724795728734200.stgit@jupiter.in.ibm.com>
On Thu, 23 Aug 2018 12:06:53 +0530
Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com> wrote:
> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>
> With the powerpc next commit e7e81847478 (powerpc/mce: Fix SLB rebolting
> during MCE recovery path.), the SLB error recovery is broken. The new
> change now does not add index value to RB[52-63] that selects the SLB
> entry while rebolting, instead it assumes that the shadow save area
> already have index embeded correctly in esid field. While all valid bolted
> save areas do contain index value set correctly, there is a case where
> 3rd (KSTACK_INDEX) entry for kernel stack does not embed index for NULL
> esid entry. This patch fixes that.
>
> Without this patch the SLB rebolt code overwrites the 1st entry of kernel
> linear mapping and causes SLB recovery to fail.
>
> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Changelog just needs a little more work, maybe this?
The commit e7e81847478 ("powerpc/64s: move machine check SLB flushing
to mm/slb.c") introduced a bug in reloading bolted SLB entries. Unused
bolted entries are stored with .esid=0 in the slb_shadow area, and
that value is now used directly as the RB input to slbmte, which means
the RB[52:63] index field is set to 0, which causes SLB entry 0 to be
cleared.
Fix this by storing the index bits in the unused bolted entries, which
directs the slbmte to the right place.
The SLB shadow area is also used by the hypervisor, but PAPR is okay
with that, from LoPAPR v1.1, 14.11.1.3 SLB Shadow Buffer:
Note: SLB is filled sequentially starting at index 0
from the shadow buffer ignoring the contents of
RB field bits 52-63
Fixes: e7e81847478 ("powerpc/64s: move machine check SLB flushing to
mm/slb.c")
Thanks,
Nick
> ---
> arch/powerpc/mm/slb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
> index 0b095fa54049..9f574e59d178 100644
> --- a/arch/powerpc/mm/slb.c
> +++ b/arch/powerpc/mm/slb.c
> @@ -70,7 +70,7 @@ static inline void slb_shadow_update(unsigned long ea, int ssize,
>
> static inline void slb_shadow_clear(enum slb_index index)
> {
> - WRITE_ONCE(get_slb_shadow()->save_area[index].esid, 0);
> + WRITE_ONCE(get_slb_shadow()->save_area[index].esid, cpu_to_be64(index));
> }
>
> static inline void create_shadowed_slbe(unsigned long ea, int ssize,
>
^ permalink raw reply
* Re: [RESEND PATCH v2] powerpc/mce: Fix SLB rebolting during MCE recovery path.
From: Michael Ellerman @ 2018-08-23 6:44 UTC (permalink / raw)
To: Mahesh J Salgaonkar, linuxppc-dev
Cc: Nicholas Piggin, Nicholas Piggin, Aneesh Kumar K.V,
Nicholas Piggin
In-Reply-To: <153500619258.20614.8965724795728734200.stgit@jupiter.in.ibm.com>
Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com> writes:
> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>
> With the powerpc next commit e7e81847478 (powerpc/mce: Fix SLB rebolting
> during MCE recovery path.),
That commit description is wrong, I'll fix it up.
cheers
> the SLB error recovery is broken. The new
> change now does not add index value to RB[52-63] that selects the SLB
> entry while rebolting, instead it assumes that the shadow save area
> already have index embeded correctly in esid field. While all valid bolted
> save areas do contain index value set correctly, there is a case where
> 3rd (KSTACK_INDEX) entry for kernel stack does not embed index for NULL
> esid entry. This patch fixes that.
>
> Without this patch the SLB rebolt code overwrites the 1st entry of kernel
> linear mapping and causes SLB recovery to fail.
>
> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/mm/slb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
> index 0b095fa54049..9f574e59d178 100644
> --- a/arch/powerpc/mm/slb.c
> +++ b/arch/powerpc/mm/slb.c
> @@ -70,7 +70,7 @@ static inline void slb_shadow_update(unsigned long ea, int ssize,
>
> static inline void slb_shadow_clear(enum slb_index index)
> {
> - WRITE_ONCE(get_slb_shadow()->save_area[index].esid, 0);
> + WRITE_ONCE(get_slb_shadow()->save_area[index].esid, cpu_to_be64(index));
> }
>
> static inline void create_shadowed_slbe(unsigned long ea, int ssize,
^ permalink raw reply
* [RESEND PATCH v2] powerpc/mce: Fix SLB rebolting during MCE recovery path.
From: Mahesh J Salgaonkar @ 2018-08-23 6:36 UTC (permalink / raw)
To: linuxppc-dev, Michael Ellerman
Cc: Nicholas Piggin, Nicholas Piggin, Aneesh Kumar K.V,
Nicholas Piggin
From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
With the powerpc next commit e7e81847478 (powerpc/mce: Fix SLB rebolting
during MCE recovery path.), the SLB error recovery is broken. The new
change now does not add index value to RB[52-63] that selects the SLB
entry while rebolting, instead it assumes that the shadow save area
already have index embeded correctly in esid field. While all valid bolted
save areas do contain index value set correctly, there is a case where
3rd (KSTACK_INDEX) entry for kernel stack does not embed index for NULL
esid entry. This patch fixes that.
Without this patch the SLB rebolt code overwrites the 1st entry of kernel
linear mapping and causes SLB recovery to fail.
Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/mm/slb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
index 0b095fa54049..9f574e59d178 100644
--- a/arch/powerpc/mm/slb.c
+++ b/arch/powerpc/mm/slb.c
@@ -70,7 +70,7 @@ static inline void slb_shadow_update(unsigned long ea, int ssize,
static inline void slb_shadow_clear(enum slb_index index)
{
- WRITE_ONCE(get_slb_shadow()->save_area[index].esid, 0);
+ WRITE_ONCE(get_slb_shadow()->save_area[index].esid, cpu_to_be64(index));
}
static inline void create_shadowed_slbe(unsigned long ea, int ssize,
^ permalink raw reply related
* Re: [PATCH v2] poewrpc/mce: Fix SLB rebolting during MCE recovery path.
From: Mahesh Jagannath Salgaonkar @ 2018-08-23 6:32 UTC (permalink / raw)
To: linuxppc-dev, Michael Ellerman; +Cc: Aneesh Kumar K.V, Nicholas Piggin
In-Reply-To: <153500011126.9482.8367068862900987587.stgit@jupiter.in.ibm.com>
On 08/23/2018 10:26 AM, Mahesh J Salgaonkar wrote:
> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>
> With the powrpc next commit e7e81847478 (poewrpc/mce: Fix SLB rebolting
> during MCE recovery path.), the SLB error recovery is broken. The new
> change now does not add index value to RB[52-63] that selects the SLB
> entry while rebolting, instead it assumes that the shadow save area
> already have index embeded correctly in esid field. While all valid bolted
> save areas do contain index value set correctly, there is a case where
> 3rd (KSTACK_INDEX) entry for kernel stack does not embed index for NULL
> esid entry. This patch fixes that.
>
> Without this patch the SLB rebolt code overwirtes the 1st entry of kernel
> linear mapping and causes SLB recovery to fail.
>
> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Ignore this patch.. There are few spelling mistakes in this patch.. will
resend v2 again after fixing those.
Thanks,
-Mahesh.
^ permalink raw reply
* Re: [PATCH 01/20] kernel/dma/direct: take DMA offset into account in dma_direct_supported
From: Benjamin Herrenschmidt @ 2018-08-23 5:24 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Paul Mackerras, Michael Ellerman, Tony Luck, Fenghua Yu,
Konrad Rzeszutek Wilk, Robin Murphy, linuxppc-dev, iommu,
linux-ia64
In-Reply-To: <20180823052403.GA14388@lst.de>
On Thu, 2018-08-23 at 07:24 +0200, Christoph Hellwig wrote:
> > Well, iommus can have bypass regions, which we also use for
> > performance, so we do at dma_set_mask() time "swap" the ops around, and
> > in that case, we do want to check the mask against the actual top of
> > memory...
>
> That is a bit of a powerpc special case (we also had one other arch
> doing that, but it got removed in the great purge, can't rember which
> one right now). Everyone else has one set of ops, and they just switch
> to the direct mapping inside the iommu ops.
We more or less do that too in some of ours these days bcs of the whole
coherent_mask vs mask where a given device might need either depending
on the type of mapping.
Ben.
^ permalink raw reply
* Re: [PATCH 02/20] kernel/dma/direct: refine dma_direct_alloc zone selection
From: Christoph Hellwig @ 2018-08-23 5:26 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Christoph Hellwig, Paul Mackerras, Michael Ellerman, Tony Luck,
Fenghua Yu, Konrad Rzeszutek Wilk, Robin Murphy, linuxppc-dev,
iommu, linux-ia64
In-Reply-To: <d9866d44704073ecc418f62da0ba7587ce7c9af8.camel@kernel.crashing.org>
On Thu, Aug 23, 2018 at 10:01:45AM +1000, Benjamin Herrenschmidt wrote:
> > The general scheme that architectures should implement is:
> >
> > ZONE_DMA: Any memory below a magic threshold that is lower than
> > 32-bit. Only enabled if actually required (usually
> > either 24-bit for ISA, or some other weird architecture
> > specific value like 32-bit for S/390)
>
> It should have been ZONE_ISA_DMA :-)
For most of these use cases it should have been indeed, and that
would avoid a lot of confusion where people use GFP_DMA just because
they do DMA.
Anyway, switching powerpc to this scheme would be great, but I don't
think it is required - GFP_KERNEL will silently fall back to ZONE_DMA,
so except for an additional GFP_DMA fallback allocation when the
GFP_KERNEL one fails the code should just work.
^ permalink raw reply
* Re: [PATCH 01/20] kernel/dma/direct: take DMA offset into account in dma_direct_supported
From: Christoph Hellwig @ 2018-08-23 5:24 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Christoph Hellwig, Paul Mackerras, Michael Ellerman, Tony Luck,
Fenghua Yu, Konrad Rzeszutek Wilk, Robin Murphy, linuxppc-dev,
iommu, linux-ia64
In-Reply-To: <079a961eaf548644250719df83930d3d72e34cac.camel@kernel.crashing.org>
On Thu, Aug 23, 2018 at 09:59:18AM +1000, Benjamin Herrenschmidt wrote:
> > Yeah, the other platforms that support these devices support ZONE_DMA
> > to reliably handle these devices. But there is two other ways the
> > current code would actually handle these fine despite the dma_direct
> > checks:
> >
> > 1) if the device only has physical addresses up to 31-bit anyway
> > 2) by trying again to find a lower address. But this only works
> > for coherent allocations and not streaming maps (unless we have
> > swiotlb with a buffer below 31-bits).
> >
> > It seems powerpc can have ZONE_DMA, though and we will cover these
> > devices just fine. If it didn't have that the current powerpc
> > code would not work either.
>
> Not exactly. powerpc has ZONE_DMA covering all of system memory.
>
> What happens in ppc32 is that we somewhat "know" that none of the
> systems with those stupid 31-bit limited pieces of HW is capable of
> having more than 2GB of memory anyway.
>
> So we get away with just returning "1".
I think I can up with a proper way of handling that by checking
the actual amount of physical memory present instead of the hard coded
32-bit.
> > If your PCI bridge / PCIe root port doesn't support dma to addresses
> > larger than 32-bit the device capabilities above that don't matter, it
> > just won't work. We have this case at least for some old VIA x86 chipsets
> > and some relatively modern Xilinx FPGAs with PCIe.
>
> Hrm... that's the usual confusion dma_capable() vs. dma_set_mask().
>
> It's always been perfectly fine for a driver to do a dma_set_mask(64-
> bit) on a system where the bridge can only do 32-bits ...
No, it hasn't. That's why we have this pattern of trying a 64-bit
mask first and then setting a 32-bit mask if that fails all over
drivers/. However with all the work we've done over the last month
we are getting really close to a world where:
- the driver just does one dma_set_mask for the capabilities and
stores that in the dma_mask
- other limitations go elsewhere and will be automatically taken
into account.
Which is I guess what you always wanted, but which wasn't how things
actually worked before.
> We shouldn't fail there, we should instead "clamp" the mask to 32-bit,
> see what I mean ? It doesn't matter that the device itself is capable
> of issuing >32 addresses, I agree, but what we need to express is that
> the combination device+bridge doesn't want addresses above 32-bit, so
> it's equivalent to making the device do a set_mask(32-bit).
As said, we'll get there (but with the new separate bus_dma_mask in 4.19),
but this is not how things currently work.
> > Your observation is right, but there always has been the implicit
> > assumption that architectures with more than 4GB of physical address
> > space must either support and iommu or swiotlb and use that. It's
> > never been document anywhere, but I'm working on integrating all
> > this code to make more sense.
>
> Well, iommus can have bypass regions, which we also use for
> performance, so we do at dma_set_mask() time "swap" the ops around, and
> in that case, we do want to check the mask against the actual top of
> memory...
That is a bit of a powerpc special case (we also had one other arch
doing that, but it got removed in the great purge, can't rember which
one right now). Everyone else has one set of ops, and they just switch
to the direct mapping inside the iommu ops.
^ permalink raw reply
* [PATCH v2] poewrpc/mce: Fix SLB rebolting during MCE recovery path.
From: Mahesh J Salgaonkar @ 2018-08-23 4:56 UTC (permalink / raw)
To: linuxppc-dev, Michael Ellerman
Cc: Nicholas Piggin, Nicholas Piggin, Aneesh Kumar K.V,
Nicholas Piggin
From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
With the powrpc next commit e7e81847478 (poewrpc/mce: Fix SLB rebolting
during MCE recovery path.), the SLB error recovery is broken. The new
change now does not add index value to RB[52-63] that selects the SLB
entry while rebolting, instead it assumes that the shadow save area
already have index embeded correctly in esid field. While all valid bolted
save areas do contain index value set correctly, there is a case where
3rd (KSTACK_INDEX) entry for kernel stack does not embed index for NULL
esid entry. This patch fixes that.
Without this patch the SLB rebolt code overwirtes the 1st entry of kernel
linear mapping and causes SLB recovery to fail.
Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/mm/slb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
index 0b095fa54049..9f574e59d178 100644
--- a/arch/powerpc/mm/slb.c
+++ b/arch/powerpc/mm/slb.c
@@ -70,7 +70,7 @@ static inline void slb_shadow_update(unsigned long ea, int ssize,
static inline void slb_shadow_clear(enum slb_index index)
{
- WRITE_ONCE(get_slb_shadow()->save_area[index].esid, 0);
+ WRITE_ONCE(get_slb_shadow()->save_area[index].esid, cpu_to_be64(index));
}
static inline void create_shadowed_slbe(unsigned long ea, int ssize,
^ permalink raw reply related
* Re: [PATCH] poewrpc/mce: Fix SLB rebolting during MCE recovery path.
From: Nicholas Piggin @ 2018-08-23 4:36 UTC (permalink / raw)
To: Mahesh Jagannath Salgaonkar
Cc: linuxppc-dev, Michael Ellerman, Aneesh Kumar K.V
In-Reply-To: <4dc90537-0fde-ab1a-8372-aba2d82ebd8c@linux.vnet.ibm.com>
On Thu, 23 Aug 2018 09:58:31 +0530
Mahesh Jagannath Salgaonkar <mahesh@linux.vnet.ibm.com> wrote:
> On 08/21/2018 03:57 PM, Nicholas Piggin wrote:
> > On Fri, 17 Aug 2018 14:51:47 +0530
> > Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com> wrote:
> >
> >> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> >>
> >> With the powrpc next commit e7e81847478 (poewrpc/mce: Fix SLB rebolting
> >> during MCE recovery path.), the SLB error recovery is broken. The
> >> commit missed a crucial change of OR-ing index value to RB[52-63] which
> >> selects the SLB entry while rebolting. This patch fixes that.
> >>
> >> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> >> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> >> ---
> >> arch/powerpc/mm/slb.c | 5 ++++-
> >> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
> >> index 0b095fa54049..6dd9913425bc 100644
> >> --- a/arch/powerpc/mm/slb.c
> >> +++ b/arch/powerpc/mm/slb.c
> >> @@ -101,9 +101,12 @@ void __slb_restore_bolted_realmode(void)
> >>
> >> /* No isync needed because realmode. */
> >> for (index = 0; index < SLB_NUM_BOLTED; index++) {
> >> + unsigned long rb = be64_to_cpu(p->save_area[index].esid);
> >> +
> >> + rb = (rb & ~0xFFFul) | index;
> >> asm volatile("slbmte %0,%1" :
> >> : "r" (be64_to_cpu(p->save_area[index].vsid)),
> >> - "r" (be64_to_cpu(p->save_area[index].esid)));
> >> + "r" (rb));
> >> }
> >> }
> >>
> >>
> >
> > I'm just looking at this again. The bolted save areas do have the
> > index field set. So for the OS, your patch should be equivalent to
> > this, right?
> >
> > static inline void slb_shadow_clear(enum slb_index index)
> > {
> > - WRITE_ONCE(get_slb_shadow()->save_area[index].esid, 0);
> > + WRITE_ONCE(get_slb_shadow()->save_area[index].esid, index);
> > }
> >
> > Which seems like a better fix.
>
> Yeah this also fixes the issue. The only additional change required is
> cpu_to_be64(index).
Ah yep.
> As long as we maintain index in bolted save areas
> (for valid/invalid entries) we should be ok. Will respin v2 with this
> change.
Cool, Reviewed-by: Nicholas Piggin <npiggin@gmail.com> in that case :)
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] poewrpc/mce: Fix SLB rebolting during MCE recovery path.
From: Mahesh Jagannath Salgaonkar @ 2018-08-23 4:28 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev, Michael Ellerman, Aneesh Kumar K.V
In-Reply-To: <20180821202702.4198d426@roar.ozlabs.ibm.com>
On 08/21/2018 03:57 PM, Nicholas Piggin wrote:
> On Fri, 17 Aug 2018 14:51:47 +0530
> Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com> wrote:
>
>> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>>
>> With the powrpc next commit e7e81847478 (poewrpc/mce: Fix SLB rebolting
>> during MCE recovery path.), the SLB error recovery is broken. The
>> commit missed a crucial change of OR-ing index value to RB[52-63] which
>> selects the SLB entry while rebolting. This patch fixes that.
>>
>> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/mm/slb.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
>> index 0b095fa54049..6dd9913425bc 100644
>> --- a/arch/powerpc/mm/slb.c
>> +++ b/arch/powerpc/mm/slb.c
>> @@ -101,9 +101,12 @@ void __slb_restore_bolted_realmode(void)
>>
>> /* No isync needed because realmode. */
>> for (index = 0; index < SLB_NUM_BOLTED; index++) {
>> + unsigned long rb = be64_to_cpu(p->save_area[index].esid);
>> +
>> + rb = (rb & ~0xFFFul) | index;
>> asm volatile("slbmte %0,%1" :
>> : "r" (be64_to_cpu(p->save_area[index].vsid)),
>> - "r" (be64_to_cpu(p->save_area[index].esid)));
>> + "r" (rb));
>> }
>> }
>>
>>
>
> I'm just looking at this again. The bolted save areas do have the
> index field set. So for the OS, your patch should be equivalent to
> this, right?
>
> static inline void slb_shadow_clear(enum slb_index index)
> {
> - WRITE_ONCE(get_slb_shadow()->save_area[index].esid, 0);
> + WRITE_ONCE(get_slb_shadow()->save_area[index].esid, index);
> }
>
> Which seems like a better fix.
Yeah this also fixes the issue. The only additional change required is
cpu_to_be64(index). As long as we maintain index in bolted save areas
(for valid/invalid entries) we should be ok. Will respin v2 with this
change.
Thanks,
-Mahesh.
^ permalink raw reply
* Re: [PATCH 1/2] macintosh: therm_windtunnel: drop using attach_adapter
From: Michael Ellerman @ 2018-08-23 3:31 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c
Cc: linuxppc-dev, Benjamin Herrenschmidt, linux-kernel, Wolfram Sang
In-Reply-To: <20180821150240.19964-2-wsa@the-dreams.de>
Wolfram Sang <wsa@the-dreams.de> writes:
> As we now have deferred probing, we can use a custom mechanism and
> finally get rid of the legacy interface from the i2c core.
>
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> ---
> drivers/macintosh/therm_windtunnel.c | 25 +++++++++++++++++++++++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
I don't have a G4 to test this on, so merge it and if it breaks we can
fix it up later.
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
cheers
> diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
> index 68dcbcb4fc5b..8c744578122a 100644
> --- a/drivers/macintosh/therm_windtunnel.c
> +++ b/drivers/macintosh/therm_windtunnel.c
> @@ -432,7 +432,6 @@ static struct i2c_driver g4fan_driver = {
> .driver = {
> .name = "therm_windtunnel",
> },
> - .attach_adapter = do_attach,
> .probe = do_probe,
> .remove = do_remove,
> .id_table = therm_windtunnel_id,
> @@ -445,7 +444,29 @@ static struct i2c_driver g4fan_driver = {
>
> static int therm_of_probe(struct platform_device *dev)
> {
> - return i2c_add_driver( &g4fan_driver );
> + struct i2c_adapter *adap;
> + int ret, i = 0;
> +
> + adap = i2c_get_adapter(0);
> + if (!adap)
> + return -EPROBE_DEFER;
> +
> + ret = i2c_add_driver(&g4fan_driver);
> + if (ret) {
> + i2c_put_adapter(adap);
> + return ret;
> + }
> +
> + /* We assume Macs have consecutive I2C bus numbers starting at 0 */
> + while (adap) {
> + do_attach(adap);
> + if (x.running)
> + return 0;
> + i2c_put_adapter(adap);
> + adap = i2c_get_adapter(++i);
> + }
> +
> + return -ENODEV;
> }
>
> static int
> --
> 2.11.0
^ permalink raw reply
* Re: [PATCH 0/2] i2c: remove deprecated attach_adapter callback
From: Michael Ellerman @ 2018-08-23 3:30 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c
Cc: linuxppc-dev, Benjamin Herrenschmidt, linux-kernel, Wolfram Sang
In-Reply-To: <20180821150240.19964-1-wsa@the-dreams.de>
Wolfram Sang <wsa@the-dreams.de> writes:
> So, I wanted to do this in the next cycle, but Linus seems to want it this
> cycle already [1], so here it is:
>
> Remove the attach_adapter callback from the 2.4 times by converting
> the last user to a custom probing mechanism based on deferred probing. We used
> this already in commit ac397c80de89 ("ALSA: ppc: keywest: drop using attach
> adapter") successfully on HW, so we agreed to use it on the windtunnel driver
> as well.
>
> With the last user gone, we can then remove the callback \o/ I think this
> allows for more cleanup in the core, but let's do this later and focus on the
> removal for now.
>
> Tested on a Renesas R-Car Salvator-XS board (M3N) by using and rebinding
> various I2C busses. Build bot and checkpatch are happy, too.
>
> I'd like to send a pull request to Linus this merge window, so looking forward
> to super fast comments, acks, etc...
Sure, I don't have a G4 hooked up to test this, so just merge it and if
it breaks we can fix it.
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/xive: Initialize symbol before usage
From: Michael Ellerman @ 2018-08-23 3:24 UTC (permalink / raw)
To: Breno Leitao, linuxppc-dev; +Cc: Breno Leitao
In-Reply-To: <1534877212-22678-1-git-send-email-leitao@debian.org>
Hi Breno,
Breno Leitao <leitao@debian.org> writes:
> Function xive_native_get_ipi() might uses chip_id without it being
> initialized. This gives the following error on 'smatch' tool:
>
> error: uninitialized symbol 'chip_id'
Which is correct, it can be used uninitialised. I'm surprised GCC
doesn't warn about it.
> This patch simply sets chip_id initial value to 0.
I'd prefer we fixed it differently, by explicitly initialising to zero
at the appropriate place in the code.
> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
> index 311185b9960a..fc56673a3c0f 100644
> --- a/arch/powerpc/sysdev/xive/native.c
> +++ b/arch/powerpc/sysdev/xive/native.c
> @@ -239,7 +239,7 @@ static bool xive_native_match(struct device_node *node)
> static int xive_native_get_ipi(unsigned int cpu, struct xive_cpu *xc)
> {
> struct device_node *np;
> - unsigned int chip_id;
> + unsigned int chip_id = 0;
> s64 irq;
>
> /* Find the chip ID */
The current code is:
/* Find the chip ID */
np = of_get_cpu_node(cpu, NULL);
if (np) {
if (of_property_read_u32(np, "ibm,chip-id", &chip_id) < 0)
chip_id = 0;
}
Where if np is NULL then we don't initialise chip_id.
Which could be:
np = of_get_cpu_node(cpu, NULL);
if (of_property_read_u32(np, "ibm,chip-id", &chip_id) < 0)
chip_id = 0;
Because of_property_read_u32() will just return an error if np is NULL.
It's also missing an of_node_put() of np, you should do a separate patch
to fix that. You can just do it unconditionally after the
of_property_read_u32().
cheers
^ permalink raw reply
* Re: Infinite looping observed in __offline_pages
From: Aneesh Kumar K.V @ 2018-08-23 3:01 UTC (permalink / raw)
To: Mike Kravetz, Michal Hocko, Haren Myneni
Cc: n-horiguchi, linuxppc-dev, linux-kernel, kamezawa.hiroyu, mgorman
In-Reply-To: <54c72a22-a921-fc64-460d-f66985d0df4e@oracle.com>
On 08/23/2018 12:28 AM, Mike Kravetz wrote:
> On 08/22/2018 02:30 AM, Aneesh Kumar K.V wrote:
>> commit 2e9d754ac211f2af3731f15df3cd8cd070b4cc54
>> Author: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> Date: Tue Aug 21 14:17:55 2018 +0530
>>
>> mm/hugetlb: filter out hugetlb pages if HUGEPAGE migration is not supported.
>>
>> When scanning for movable pages, filter out Hugetlb pages if hugepage migration
>> is not supported. Without this we hit infinte loop in __offline pages where we
>> do
>> pfn = scan_movable_pages(start_pfn, end_pfn);
>> if (pfn) { /* We have movable pages */
>> ret = do_migrate_range(pfn, end_pfn);
>> goto repeat;
>> }
>>
>> We do support hugetlb migration ony if the hugetlb pages are at pmd level. Here
>
> I thought migration at pgd level was added for POWER? commit 94310cbcaa3c
> (mm/madvise: enable (soft|hard) offline of HugeTLB pages at PGD level).
> Only remember, because I did not fully understand the use case. :)
>
yes. We hit the issue on older distro kernels.
>> we just check for Kernel config. The gigantic page size check is done in
>> page_huge_active.
>>
>> Reported-by: Haren Myneni <haren@linux.vnet.ibm.com>
>> CC: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 4eb6e824a80c..f9bdea685cf4 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1338,7 +1338,8 @@ static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
>> return pfn;
>> if (__PageMovable(page))
>> return pfn;
>> - if (PageHuge(page)) {
>> + if (IS_ENABLED(CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION) &&
>> + PageHuge(page)) {
>
> How about using hugepage_migration_supported instead? It would automatically
> catch those non-migratable huge page sizes. Something like:
>
Will do that.
> if (PageHuge(page) &&
> hugepage_migration_supported(page_hstate(page))) {
>
-aneesh
^ permalink raw reply
* Re: DT case sensitivity
From: Benjamin Herrenschmidt @ 2018-08-23 1:29 UTC (permalink / raw)
To: Rob Herring
Cc: Stephen Rothwell, Grant Likely, Michael Ellerman, Kumar Gala,
David Gibson, Frank Rowand, devicetree-spec, devicetree,
linuxppc-dev
In-Reply-To: <CAL_JsqKqkO31ekVMFKs+osLvKdZyDzRowF1thWXn5=tWhrJgvw@mail.gmail.com>
On Wed, 2018-08-22 at 20:26 -0500, Rob Herring wrote:
> On Wed, Aug 22, 2018 at 8:14 PM Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> >
> > On Wed, 2018-08-22 at 19:47 -0500, Rob Herring wrote:
> > > The default DT string handling in the kernel is node names and
> > > compatibles are case insensitive and property names are case sensitive
> > > (Sparc is the the only variation and is opposite). It seems only PPC
> > > (and perhaps only Power Macs?) needs to support case insensitive
> > > comparisons. It was probably a mistake to follow PPC for new arches
> > > and we should have made everything case sensitive from the start. So I
> > > have a few questions for the DT historians. :)
> >
> > Open Firmware itself is insensitive.
>
> Doesn't it depend on the implementation? Otherwise, how is Sparc different?
Not sure ... Forth itself is insensitive for words but maybe not for
string comparisons.
>
> > > What PPC systems are case insensitive? Can we limit that to certain systems?
> >
> > All PowerMacs at least, the problem is that I don't have DT images or
> > access to all the historical systems (and yes some people occasionally
> > still use them) to properly test a change in that area.
>
> I'm temped to break them so I can find folks to provide me with DT dumps. :)
I have a collection of DT dumps but I'm not sure about the legality of
publishing them...
Cheers,
Ben.
^ permalink raw reply
* Re: DT case sensitivity
From: Rob Herring @ 2018-08-23 1:26 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Stephen Rothwell, Grant Likely, Michael Ellerman, Kumar Gala,
David Gibson, Frank Rowand, devicetree-spec, devicetree,
linuxppc-dev
In-Reply-To: <3335cff2cc999c1dda58d75949ab3d12185fca79.camel@kernel.crashing.org>
On Wed, Aug 22, 2018 at 8:14 PM Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> On Wed, 2018-08-22 at 19:47 -0500, Rob Herring wrote:
> > The default DT string handling in the kernel is node names and
> > compatibles are case insensitive and property names are case sensitive
> > (Sparc is the the only variation and is opposite). It seems only PPC
> > (and perhaps only Power Macs?) needs to support case insensitive
> > comparisons. It was probably a mistake to follow PPC for new arches
> > and we should have made everything case sensitive from the start. So I
> > have a few questions for the DT historians. :)
>
> Open Firmware itself is insensitive.
Doesn't it depend on the implementation? Otherwise, how is Sparc different?
> > What PPC systems are case insensitive? Can we limit that to certain systems?
>
> All PowerMacs at least, the problem is that I don't have DT images or
> access to all the historical systems (and yes some people occasionally
> still use them) to properly test a change in that area.
I'm temped to break them so I can find folks to provide me with DT dumps. :)
Rob
^ permalink raw reply
* Re: Odd SIGSEGV issue introduced by commit 6b31d5955cb29 ("mm, oom: fix potential data corruption when oom_reaper races with writer")
From: Michael Ellerman @ 2018-08-23 1:25 UTC (permalink / raw)
To: Christophe LEROY, Michal Hocko, Ram Pai, Andrew Morton
Cc: linuxppc-dev@lists.ozlabs.org, linux-mm
In-Reply-To: <7767bdf4-a034-ecb9-1ac8-4fa87f335818@c-s.fr>
Christophe LEROY <christophe.leroy@c-s.fr> writes:
> Hello,
>
> I have an odd issue on my powerpc 8xx board.
>
> I am running latest 4.14 and get the following SIGSEGV which appears
> more or less randomly.
>
> [ 9.190354] touch[91]: unhandled signal 11 at 67807b58 nip 777cf114
> lr 777cf100 code 30001
> [ 24.634810] ifconfig[160]: unhandled signal 11 at 67ae7b58 nip
> 77aaf114 lr 77aaf100 code 30001
It would be interesting to see the code dump here and which registers
are being used.
Can you backport the show unhandled signal changes and see what that
shows us?
cheers
^ permalink raw reply
* Re: DT case sensitivity
From: Benjamin Herrenschmidt @ 2018-08-23 1:03 UTC (permalink / raw)
To: Rob Herring, Stephen Rothwell, Grant Likely, Michael Ellerman,
Kumar Gala, David Gibson, Frank Rowand, devicetree-spec,
devicetree, linuxppc-dev
In-Reply-To: <CAL_Jsq+XacT2O15CQ=F_6dmWf6OqNf8_fKAOrMJNR79a6mORaw@mail.gmail.com>
On Wed, 2018-08-22 at 19:47 -0500, Rob Herring wrote:
> The default DT string handling in the kernel is node names and
> compatibles are case insensitive and property names are case sensitive
> (Sparc is the the only variation and is opposite). It seems only PPC
> (and perhaps only Power Macs?) needs to support case insensitive
> comparisons. It was probably a mistake to follow PPC for new arches
> and we should have made everything case sensitive from the start. So I
> have a few questions for the DT historians. :)
Open Firmware itself is insensitive.
> What PPC systems are case insensitive? Can we limit that to certain systems?
All PowerMacs at least, the problem is that I don't have DT images or
access to all the historical systems (and yes some people occasionally
still use them) to properly test a change in that area.
> AFAICT, dtc at least (if not anything FDT based) has always been case
> sensitive at least for node and property names. I'm not sure about
> compatible strings?
>
> Anyone see potential issues with switching all platforms except PPC
> and Sparc to case sensitive comparisons?
Cheers,
Ben.
^ permalink raw reply
* DT case sensitivity
From: Rob Herring @ 2018-08-23 0:47 UTC (permalink / raw)
To: Stephen Rothwell, Grant Likely, Michael Ellerman,
Benjamin Herrenschmidt, Kumar Gala, David Gibson, Frank Rowand,
devicetree-spec, devicetree, linuxppc-dev
The default DT string handling in the kernel is node names and
compatibles are case insensitive and property names are case sensitive
(Sparc is the the only variation and is opposite). It seems only PPC
(and perhaps only Power Macs?) needs to support case insensitive
comparisons. It was probably a mistake to follow PPC for new arches
and we should have made everything case sensitive from the start. So I
have a few questions for the DT historians. :)
What PPC systems are case insensitive? Can we limit that to certain systems?
AFAICT, dtc at least (if not anything FDT based) has always been case
sensitive at least for node and property names. I'm not sure about
compatible strings?
Anyone see potential issues with switching all platforms except PPC
and Sparc to case sensitive comparisons?
Rob
^ permalink raw reply
* [PATCH] KVM: PPC: Book3S: Fix guest DMA when guest partially backed by THP pages
From: Paul Mackerras @ 2018-08-23 0:08 UTC (permalink / raw)
To: linuxppc-dev, kvm-ppc
Cc: Michael Ellerman, Alexey Kardashevskiy, David Gibson
Commit 76fa4975f3ed ("KVM: PPC: Check if IOMMU page is contained in the
pinned physical page", 2018-07-17) added some checks to ensure that guest
DMA mappings don't attempt to map more than the guest is entitled to
access. However, errors in the logic mean that legitimate guest requests
to map pages for DMA are being denied in some situations. Specifically,
if the first page of the range passed to mm_iommu_get() is mapped with
a normal page, and subsequent pages are mapped with transparent huge
pages, we end up with mem->pageshift == 0. That means that the page
size checks in mm_iommu_ua_to_hpa() and mm_iommu_up_to_hpa_rm() will
always fail for every page in that region, and thus the guest can never
map any memory in that region for DMA, typically leading to a flood of
error messages like this:
qemu-system-ppc64: VFIO_MAP_DMA: -22
qemu-system-ppc64: vfio_dma_map(0x10005f47780, 0x800000000000000, 0x10000, 0x7fff63ff0000) = -22 (Invalid argument)
The logic errors in mm_iommu_get() are:
(a) use of 'ua' not 'ua + (i << PAGE_SHIFT)' in the find_linux_pte() call
(meaning that find_linux_pte() returns the pte for the first address
in the range, not the address we are currently up to);
(b) use of 'pageshift' as the variable to receive the hugepage shift
returned by find_linux_pte() - for a normal page this gets set to
0, leading to us setting mem->pageshift to 0 when we conclude that
the pte returned by find_linux_pte didn't match the page we were
looking at;
(c) comparing 'compshift', which is a page order, i.e. log base 2 of the
number of pages, with 'pageshift', which is a log base 2 of the number
of bytes.
To fix these problems, this patch introduces 'cur_ua' to hold the current
user address and uses that in the find_linux_pte call; introduces
'pteshift' to hold the hugepage shift found by find_linux_pte(); and
compares 'pteshift' with 'compshift + PAGE_SHIFT' rather than 'compshift'.
The patch also moves the local_irq_restore to the point after the pte
pointer returned by find_linux_pte has been dereferenced because that
seems safer, and adds a check to avoid doing the find_linux_pte() call
once mem->pageshift has been reduced to PAGE_SHIFT, as an optimization.
Cc: stable@vger.kernel.org # v4.12+
Fixes: 76fa4975f3ed ("KVM: PPC: Check if IOMMU page is contained in the
pinned physical page")
Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
---
arch/powerpc/mm/mmu_context_iommu.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/mm/mmu_context_iommu.c b/arch/powerpc/mm/mmu_context_iommu.c
index a4ca57612558..c9ee9e23845f 100644
--- a/arch/powerpc/mm/mmu_context_iommu.c
+++ b/arch/powerpc/mm/mmu_context_iommu.c
@@ -129,6 +129,7 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries,
long i, j, ret = 0, locked_entries = 0;
unsigned int pageshift;
unsigned long flags;
+ unsigned long cur_ua;
struct page *page = NULL;
mutex_lock(&mem_list_mutex);
@@ -177,7 +178,8 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries,
}
for (i = 0; i < entries; ++i) {
- if (1 != get_user_pages_fast(ua + (i << PAGE_SHIFT),
+ cur_ua = ua + (i << PAGE_SHIFT);
+ if (1 != get_user_pages_fast(cur_ua,
1/* pages */, 1/* iswrite */, &page)) {
ret = -EFAULT;
for (j = 0; j < i; ++j)
@@ -196,7 +198,7 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries,
if (is_migrate_cma_page(page)) {
if (mm_iommu_move_page_from_cma(page))
goto populate;
- if (1 != get_user_pages_fast(ua + (i << PAGE_SHIFT),
+ if (1 != get_user_pages_fast(cur_ua,
1/* pages */, 1/* iswrite */,
&page)) {
ret = -EFAULT;
@@ -210,20 +212,21 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries,
}
populate:
pageshift = PAGE_SHIFT;
- if (PageCompound(page)) {
+ if (mem->pageshift > PAGE_SHIFT && PageCompound(page)) {
pte_t *pte;
struct page *head = compound_head(page);
unsigned int compshift = compound_order(head);
+ unsigned int pteshift;
local_irq_save(flags); /* disables as well */
- pte = find_linux_pte(mm->pgd, ua, NULL, &pageshift);
- local_irq_restore(flags);
+ pte = find_linux_pte(mm->pgd, cur_ua, NULL, &pteshift);
/* Double check it is still the same pinned page */
if (pte && pte_page(*pte) == head &&
- pageshift == compshift)
- pageshift = max_t(unsigned int, pageshift,
+ pteshift == compshift + PAGE_SHIFT)
+ pageshift = max_t(unsigned int, pteshift,
PAGE_SHIFT);
+ local_irq_restore(flags);
}
mem->pageshift = min(mem->pageshift, pageshift);
mem->hpas[i] = page_to_pfn(page) << PAGE_SHIFT;
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v2] crypto: vmx - Fix sleep-in-atomic bugs
From: Marcelo Henrique Cerri @ 2018-08-23 0:04 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: Herbert Xu, linux-crypto, Stephan Mueller, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman, Leonidas S . Barbosa,
Paulo Flabiano Smorigo, linuxppc-dev, stable
In-Reply-To: <20180822062631.5664-1-omosnace@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 6255 bytes --]
That looks good to me. Maybe Paulo can help testing it.
--
Regards,
Marcelo
On Wed, Aug 22, 2018 at 08:26:31AM +0200, Ondrej Mosnacek wrote:
> This patch fixes sleep-in-atomic bugs in AES-CBC and AES-XTS VMX
> implementations. The problem is that the blkcipher_* functions should
> not be called in atomic context.
>
> The bugs can be reproduced via the AF_ALG interface by trying to
> encrypt/decrypt sufficiently large buffers (at least 64 KiB) using the
> VMX implementations of 'cbc(aes)' or 'xts(aes)'. Such operations then
> trigger BUG in crypto_yield():
>
> [ 891.863680] BUG: sleeping function called from invalid context at include/crypto/algapi.h:424
> [ 891.864622] in_atomic(): 1, irqs_disabled(): 0, pid: 12347, name: kcapi-enc
> [ 891.864739] 1 lock held by kcapi-enc/12347:
> [ 891.864811] #0: 00000000f5d42c46 (sk_lock-AF_ALG){+.+.}, at: skcipher_recvmsg+0x50/0x530
> [ 891.865076] CPU: 5 PID: 12347 Comm: kcapi-enc Not tainted 4.19.0-0.rc0.git3.1.fc30.ppc64le #1
> [ 891.865251] Call Trace:
> [ 891.865340] [c0000003387578c0] [c000000000d67ea4] dump_stack+0xe8/0x164 (unreliable)
> [ 891.865511] [c000000338757910] [c000000000172a58] ___might_sleep+0x2f8/0x310
> [ 891.865679] [c000000338757990] [c0000000006bff74] blkcipher_walk_done+0x374/0x4a0
> [ 891.865825] [c0000003387579e0] [d000000007e73e70] p8_aes_cbc_encrypt+0x1c8/0x260 [vmx_crypto]
> [ 891.865993] [c000000338757ad0] [c0000000006c0ee0] skcipher_encrypt_blkcipher+0x60/0x80
> [ 891.866128] [c000000338757b10] [c0000000006ec504] skcipher_recvmsg+0x424/0x530
> [ 891.866283] [c000000338757bd0] [c000000000b00654] sock_recvmsg+0x74/0xa0
> [ 891.866403] [c000000338757c10] [c000000000b00f64] ___sys_recvmsg+0xf4/0x2f0
> [ 891.866515] [c000000338757d90] [c000000000b02bb8] __sys_recvmsg+0x68/0xe0
> [ 891.866631] [c000000338757e30] [c00000000000bbe4] system_call+0x5c/0x70
>
> Fixes: 8c755ace357c ("crypto: vmx - Adding CBC routines for VMX module")
> Fixes: c07f5d3da643 ("crypto: vmx - Adding support for XTS")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
> Still untested, please test and review if possible.
>
> Changes in v2:
> - fix leaving preemtption, etc. disabled when leaving the function
> (I switched to the more obvious and less efficient variant for the
> sake of clarity.)
>
> drivers/crypto/vmx/aes_cbc.c | 30 ++++++++++++++----------------
> drivers/crypto/vmx/aes_xts.c | 21 ++++++++++++++-------
> 2 files changed, 28 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/crypto/vmx/aes_cbc.c b/drivers/crypto/vmx/aes_cbc.c
> index 5285ece4f33a..b71895871be3 100644
> --- a/drivers/crypto/vmx/aes_cbc.c
> +++ b/drivers/crypto/vmx/aes_cbc.c
> @@ -107,24 +107,23 @@ static int p8_aes_cbc_encrypt(struct blkcipher_desc *desc,
> ret = crypto_skcipher_encrypt(req);
> skcipher_request_zero(req);
> } else {
> - preempt_disable();
> - pagefault_disable();
> - enable_kernel_vsx();
> -
> blkcipher_walk_init(&walk, dst, src, nbytes);
> ret = blkcipher_walk_virt(desc, &walk);
> while ((nbytes = walk.nbytes)) {
> + preempt_disable();
> + pagefault_disable();
> + enable_kernel_vsx();
> aes_p8_cbc_encrypt(walk.src.virt.addr,
> walk.dst.virt.addr,
> nbytes & AES_BLOCK_MASK,
> &ctx->enc_key, walk.iv, 1);
> + disable_kernel_vsx();
> + pagefault_enable();
> + preempt_enable();
> +
> nbytes &= AES_BLOCK_SIZE - 1;
> ret = blkcipher_walk_done(desc, &walk, nbytes);
> }
> -
> - disable_kernel_vsx();
> - pagefault_enable();
> - preempt_enable();
> }
>
> return ret;
> @@ -147,24 +146,23 @@ static int p8_aes_cbc_decrypt(struct blkcipher_desc *desc,
> ret = crypto_skcipher_decrypt(req);
> skcipher_request_zero(req);
> } else {
> - preempt_disable();
> - pagefault_disable();
> - enable_kernel_vsx();
> -
> blkcipher_walk_init(&walk, dst, src, nbytes);
> ret = blkcipher_walk_virt(desc, &walk);
> while ((nbytes = walk.nbytes)) {
> + preempt_disable();
> + pagefault_disable();
> + enable_kernel_vsx();
> aes_p8_cbc_encrypt(walk.src.virt.addr,
> walk.dst.virt.addr,
> nbytes & AES_BLOCK_MASK,
> &ctx->dec_key, walk.iv, 0);
> + disable_kernel_vsx();
> + pagefault_enable();
> + preempt_enable();
> +
> nbytes &= AES_BLOCK_SIZE - 1;
> ret = blkcipher_walk_done(desc, &walk, nbytes);
> }
> -
> - disable_kernel_vsx();
> - pagefault_enable();
> - preempt_enable();
> }
>
> return ret;
> diff --git a/drivers/crypto/vmx/aes_xts.c b/drivers/crypto/vmx/aes_xts.c
> index 8bd9aff0f55f..e9954a7d4694 100644
> --- a/drivers/crypto/vmx/aes_xts.c
> +++ b/drivers/crypto/vmx/aes_xts.c
> @@ -116,32 +116,39 @@ static int p8_aes_xts_crypt(struct blkcipher_desc *desc,
> ret = enc? crypto_skcipher_encrypt(req) : crypto_skcipher_decrypt(req);
> skcipher_request_zero(req);
> } else {
> + blkcipher_walk_init(&walk, dst, src, nbytes);
> +
> + ret = blkcipher_walk_virt(desc, &walk);
> +
> preempt_disable();
> pagefault_disable();
> enable_kernel_vsx();
>
> - blkcipher_walk_init(&walk, dst, src, nbytes);
> -
> - ret = blkcipher_walk_virt(desc, &walk);
> iv = walk.iv;
> memset(tweak, 0, AES_BLOCK_SIZE);
> aes_p8_encrypt(iv, tweak, &ctx->tweak_key);
>
> + disable_kernel_vsx();
> + pagefault_enable();
> + preempt_enable();
> +
> while ((nbytes = walk.nbytes)) {
> + preempt_disable();
> + pagefault_disable();
> + enable_kernel_vsx();
> if (enc)
> aes_p8_xts_encrypt(walk.src.virt.addr, walk.dst.virt.addr,
> nbytes & AES_BLOCK_MASK, &ctx->enc_key, NULL, tweak);
> else
> aes_p8_xts_decrypt(walk.src.virt.addr, walk.dst.virt.addr,
> nbytes & AES_BLOCK_MASK, &ctx->dec_key, NULL, tweak);
> + disable_kernel_vsx();
> + pagefault_enable();
> + preempt_enable();
>
> nbytes &= AES_BLOCK_SIZE - 1;
> ret = blkcipher_walk_done(desc, &walk, nbytes);
> }
> -
> - disable_kernel_vsx();
> - pagefault_enable();
> - preempt_enable();
> }
> return ret;
> }
> --
> 2.17.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 02/20] kernel/dma/direct: refine dma_direct_alloc zone selection
From: Benjamin Herrenschmidt @ 2018-08-23 0:01 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Paul Mackerras, Michael Ellerman, Tony Luck, Fenghua Yu,
Konrad Rzeszutek Wilk, Robin Murphy, linuxppc-dev, iommu,
linux-ia64
In-Reply-To: <20180822065856.GC19284@lst.de>
On Wed, 2018-08-22 at 08:58 +0200, Christoph Hellwig wrote:
> On Thu, Aug 09, 2018 at 09:54:33AM +1000, Benjamin Herrenschmidt wrote:
> > On Mon, 2018-07-30 at 18:38 +0200, Christoph Hellwig wrote:
> > > We need to take the DMA offset and encryption bit into account when selecting
> > > a zone. Add a helper that takes those into account and use it.
> >
> > That whole "encryption" stuff seems to be completely specific to the
> > way x86 does memory encryption, or am I mistaken ? It's not clear to me
> > what that does in practice and how it relates to DMA mappings.
>
> Not even all of x86, but AMD in particular, Intel does it yet another
> way. But it still is easier to take this into the core with a few
> overrides than duplicating all the code.
>
> > I'm also not sure about that whole business with ZONE_DMA and
> > ARCH_ZONE_DMA_BITS...
>
> ZONE_DMA usually (but not always) maps to 24-bits of address space,
> if it doesn't (I mostly through about s390 with it's odd 31-bits)
> the architecture can override it if it cares).
>
> > On ppc64, unless you enable swiotlb (which we only do currently on
> > some embedded platforms), you have all of memory in ZONE_DMA.
> >
> > [ 0.000000] Zone ranges:
> > [ 0.000000] DMA [mem 0x0000000000000000-0x0000001fffffffff]
> > [ 0.000000] DMA32 empty
> > [ 0.000000] Normal empty
> > [ 0.000000] Device empty
>
> This is really weird. Why would you wire up ZONE_DMA like this?
We always did :-) It predates my involvement and I think it predates
even Pauls. It's quite silly actually since the first powerpc machines
actually had ISA devices in them, but that's how it's been for ever. I
suppose we could change it but that would mean digging out some old
stuff to test.
> The general scheme that architectures should implement is:
>
> ZONE_DMA: Any memory below a magic threshold that is lower than
> 32-bit. Only enabled if actually required (usually
> either 24-bit for ISA, or some other weird architecture
> specific value like 32-bit for S/390)
It should have been ZONE_ISA_DMA :-)
> ZONE_DMA32: Memory <= 32-bit if the architecture supports more than
> 32-bits worth of physical address space. Should generally
> be enabled on all 64-bit architectures unless you have
> a very good reason not to.
Yeah so we sort-of enable the config option but only populate the zone
on platforms using swiotlb (freescale stuff). It's a bit messy at the
moment I must admit.
> ZONE_NORMAL: Everything above 32-bit not falling into HIGHMEM or
> MOVEABLE.
Cheers,
Ben.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox