* Re: [PATCH script] hwmon: Use octal not symbolic permissions
From: Joe Perches @ 2018-03-27 11:48 UTC (permalink / raw)
To: Guenter Roeck, Jean Delvare
Cc: linux-hwmon, linux-kernel, linuxppc-dev, linux-arm-kernel,
patches
In-Reply-To: <5ec8703b-2979-7a1d-fbc3-bd528d7a5e29@roeck-us.net>
On Tue, 2018-03-27 at 03:28 -0700, Guenter Roeck wrote:
> On 03/27/2018 12:35 AM, Joe Perches wrote:
> > On Mon, 2018-03-26 at 23:33 -0700, Guenter Roeck wrote:
> > > Since the hwmon subsystem has been labeled as both "obsolete" and "obscure",
> >
> > fyi: It's marked Maintained in MAINTAINERS
> >
>
> I did not say that it was not maintained, and I am aware of the information
> in the MAINTAINERS file, thanks.
As am I.
I do agree that hwmon is obscure. Almost everything about
anything is obscure to those that don't fully understand it.
I am unaware though of any use of "obsolete" in drivers/hwmon/
or in Documentation/ about hwmon or the wiki/
In what sense do you mean hwmon is obsolete?
And the cocci script?
Anything later than:
https://systeme.lip6.fr/pipermail/cocci/2016-December/003887.html
https://github.com/groeck/coccinelle-patches/tree/master/hwmon
?
btw: I think the perl scripts I posted for DEV_ATTR_<RW|RO|WO>
transforms are simpler and less prone to tool versioning
issues.
https://lkml.org/lkml/2017/12/22/844
It'd be fairly simple to add SENSOR_ support.
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-27 11:25 UTC (permalink / raw)
To: Will Deacon, Arnd Bergmann
Cc: Jason Gunthorpe, Sinan Kaya, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney, Peter Zijlstra,
Ingo Molnar, Jonathan Corbet
In-Reply-To: <20180327110258.GF2464@arm.com>
On Tue, 2018-03-27 at 12:02 +0100, Will Deacon wrote:
> can see it now has ownership. Note that, when using writel(), a prior
> > wmb() is not needed to guarantee that the cache coherent memory writes
> > have completed before writing to the cache incoherent MMIO region.
> > The cheaper writel_relaxed() does not guarantee the DMA to be visible
> > to the device and must not be used here.
>
> Fair enough. I'd rather people used _relaxed by default, but I have to admit
> that it will probably just result in them getting things wrong. Just a tiny
> bit of wordsmithing brings this to:
I prefer people using writel() by default for the simple reason that
99% of writels out there are configuration stuff for which the
performance difference doesn't matter, and people will just get it
wrong.
Let's focus on the rare fast path for optimisation.
>
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index a863009849a3..3247547d1c36 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -1909,9 +1909,6 @@ There are some more advanced barrier functions:
> /* assign ownership */
> desc->status = DEVICE_OWN;
>
> - /* force memory to sync before notifying device via MMIO */
> - wmb();
> -
> /* notify device of new descriptors */
> writel(DESC_NOTIFY, doorbell);
> }
> @@ -1919,11 +1916,15 @@ There are some more advanced barrier functions:
> The dma_rmb() allows us guarantee the device has released ownership
> before we read the data from the descriptor, and the dma_wmb() allows
> us to guarantee the data is written to the descriptor before the device
> - can see it now has ownership. The wmb() is needed to guarantee that the
> - cache coherent memory writes have completed before attempting a write to
> - the cache incoherent MMIO region.
> -
> - See Documentation/DMA-API.txt for more information on consistent memory.
> + can see it now has ownership. Note that, when using writel(), a prior
> + wmb() is not needed to guarantee that the cache coherent memory writes
> + have completed before writing to the MMIO region. The cheaper
> + writel_relaxed() does not provide this guarantee and must not be used
> + here.
> +
> + See the subsection "Kernel I/O barrier effects" for more information on
> + relaxed I/O accessors and the Documentation/DMA-API.txt file for more
> + information on consistent memory.
>
>
> MMIO WRITE BARRIER
>
>
> If you're happy with that, I'll send it as a proper patch.
>
> Cheers,
>
> Will
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Will Deacon @ 2018-03-27 11:24 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Arnd Bergmann, Jason Gunthorpe, Sinan Kaya, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney
In-Reply-To: <1522149602.7364.44.camel@kernel.crashing.org>
On Tue, Mar 27, 2018 at 10:20:02PM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2018-03-27 at 10:42 +0100, Will Deacon wrote:
> > >
> > > This example adds a wmb() between two writes to a coherent DMA
> > > area, it is definitely required there. I'm pretty sure I've never seen
> > > any bug reports pointing to a missing wmb() between memory
> > > and MMIO write accesses, but if you remember seeing them in the
> > > list, maybe you can look again for some evidence of something going
> > > wrong on x86 without it?
> >
> > If this is just about ordering accesses to coherent DMA, then using
> > dma_wmb() instead will be much better performance on arm/arm64.
>
> Ah, something we should look into for powerpc as well, as we could use
> an lwsync for that which is also cheaper than a full sync wmb does.
>
> dma_wmb() is basically the same as smp_wmb() without the CONFIG_SMP
> conditional right ?
Almost -- the slight change we have on arm64 is to say that it's
"outer-shareable", which means it also orders non-cacheable accesses
in the case that dma_alloc_coherent is used to allocate a consistent
buffer for a non-coherent device.
Will
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-27 11:23 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jason Gunthorpe, Sinan Kaya, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Will Deacon,
Paul E. McKenney
In-Reply-To: <CAK8P3a0gCYjw78snni_1Jkk5uA5a-j0x-XZDrN=RaZfXctOSoA@mail.gmail.com>
On Tue, 2018-03-27 at 11:44 +0200, Arnd Bergmann wrote:
> > The interesting thing is that we do seem to have a whole LOT of these
> > spurrious wmb before writel all over the tree, I suspect because of
> > that incorrect recommendation in memory-barriers.txt.
> >
> > We should fix that.
>
> Maybe the problem is just that it's so counter-intuitive that we don't
> need that barrier in Linux, when the hardware does need one on some
> architectures.
>
> How about we define a barrier type instruction specifically for this
> purpose, something like wmb_before_mmio() and have all architectures
> define that to an empty macro?
This is exactly what wmb() is about and exactly what Linux rejected
back in the day (and in hindsight I agree with him).
> That way, having correct code using wmb_before_mmio() will not
> trigger an incorrect review comment that leads to extra wmb(). ;-)
Ah, you mean have an empty macro that will always be empty on all
architectures just to fool people ? :-)
Not sure that will fly ... I think we just need to be documenting that
stuff better and not have incorrect examples. Also a sweep to remove
some useless ones like the one in e1000e would help.
Cheers,
Ben.
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-27 11:20 UTC (permalink / raw)
To: Will Deacon, Arnd Bergmann
Cc: Jason Gunthorpe, Sinan Kaya, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney
In-Reply-To: <20180327094159.GA29373@arm.com>
On Tue, 2018-03-27 at 10:42 +0100, Will Deacon wrote:
> >
> > This example adds a wmb() between two writes to a coherent DMA
> > area, it is definitely required there. I'm pretty sure I've never seen
> > any bug reports pointing to a missing wmb() between memory
> > and MMIO write accesses, but if you remember seeing them in the
> > list, maybe you can look again for some evidence of something going
> > wrong on x86 without it?
>
> If this is just about ordering accesses to coherent DMA, then using
> dma_wmb() instead will be much better performance on arm/arm64.
Ah, something we should look into for powerpc as well, as we could use
an lwsync for that which is also cheaper than a full sync wmb does.
dma_wmb() is basically the same as smp_wmb() without the CONFIG_SMP
conditional right ?
Cheers,
Ben.
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Arnd Bergmann @ 2018-03-27 11:05 UTC (permalink / raw)
To: Will Deacon
Cc: Benjamin Herrenschmidt, Jason Gunthorpe, Sinan Kaya, David Laight,
Oliver, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney, Peter Zijlstra,
Ingo Molnar, Jonathan Corbet
In-Reply-To: <20180327110258.GF2464@arm.com>
On Tue, Mar 27, 2018 at 1:02 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Mar 27, 2018 at 12:53:49PM +0200, Arnd Bergmann wrote:
>> On Tue, Mar 27, 2018 at 12:09 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > On Tue, Mar 27, 2018 at 12:05:06PM +0200, Arnd Bergmann wrote:
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index a863009849a3..3247547d1c36 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -1909,9 +1909,6 @@ There are some more advanced barrier functions:
> /* assign ownership */
> desc->status = DEVICE_OWN;
>
> - /* force memory to sync before notifying device via MMIO */
> - wmb();
> -
> /* notify device of new descriptors */
> writel(DESC_NOTIFY, doorbell);
> }
> @@ -1919,11 +1916,15 @@ There are some more advanced barrier functions:
> The dma_rmb() allows us guarantee the device has released ownership
> before we read the data from the descriptor, and the dma_wmb() allows
> us to guarantee the data is written to the descriptor before the device
> - can see it now has ownership. The wmb() is needed to guarantee that the
> - cache coherent memory writes have completed before attempting a write to
> - the cache incoherent MMIO region.
> -
> - See Documentation/DMA-API.txt for more information on consistent memory.
> + can see it now has ownership. Note that, when using writel(), a prior
> + wmb() is not needed to guarantee that the cache coherent memory writes
> + have completed before writing to the MMIO region. The cheaper
> + writel_relaxed() does not provide this guarantee and must not be used
> + here.
> +
> + See the subsection "Kernel I/O barrier effects" for more information on
> + relaxed I/O accessors and the Documentation/DMA-API.txt file for more
> + information on consistent memory.
>
>
> MMIO WRITE BARRIER
>
>
> If you're happy with that, I'll send it as a proper patch.
Looks good to me, thanks!
Arnd
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Will Deacon @ 2018-03-27 11:02 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Benjamin Herrenschmidt, Jason Gunthorpe, Sinan Kaya, David Laight,
Oliver, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney, Peter Zijlstra,
Ingo Molnar, Jonathan Corbet
In-Reply-To: <CAK8P3a0Xep0-EpGS9jnjUf1iJhRxH1WRztGQrvkdrLVaxBPT1w@mail.gmail.com>
On Tue, Mar 27, 2018 at 12:53:49PM +0200, Arnd Bergmann wrote:
> On Tue, Mar 27, 2018 at 12:09 PM, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Mar 27, 2018 at 12:05:06PM +0200, Arnd Bergmann wrote:
> >> > -
> >> > - See Documentation/DMA-API.txt for more information on consistent memory.
> >> > + can see it now has ownership. Note that, when using writel(), a prior
> >> > + wmb() is not needed to guarantee that the cache coherent memory writes
> >> > + have completed before writing to the cache incoherent MMIO region.
> >> > + If this ordering between incoherent MMIO and coherent memory regions
>
> One more thing: I think the term "incoherent MMIO" is a bit confusing, I'd
> prefer just "MMIO" here. At least I don't have the faintest clue what the
> difference between "coherent MMIO" and "incoherent MMIO" would be ;-)
Yes, you're right. I was just following the terminology that's already used
here, but actually that seems not be used anywhere else in the document!
I'll kill it.
> >> > + is not required, writel_relaxed() can be used instead and is significantly
> >> > + cheaper on some weakly-ordered architectures.
> >>
> >> I think that's a great improvement, but I'm a bit worried about recommending
> >> writel_relaxed() too much: I've seen a lot of drivers that just always use
> >> writel_relaxed() over write(), and some of them get that wrong when they
> >> don't understand the difference but end up using DMA without explicit
> >> barriers anyway.
> >>
> >> Also, having an architecture-independent driver use wmb()+writel_relaxed()
> >> ends up being more expensive than just using write(). Not sure how to
> >> best phrase it though.
> >
> > Perhaps I add reword that with a simple example to say:
> >
> > If this ordering between incoherent MMIO and coherent memory regions
> > is not required (e.g. in a sequence of accesses all to the MMIO region)
> > [...]
> >
> > since that seems to be the usual case where the _relaxed accessors help.
>
> That still doesn't quite capture what I'd like driver writes to do: in essence
> I would recommend them to use writel() all the time, except in performance
> critical code that has been shown to be correct and has a comment to explain
> why _relaxed() is ok in that particular function.
>
> Maybe it can just be rephrased to warn against the use of writel_relaxed()
> here, and explain the difference that way:
>
> can see it now has ownership. Note that, when using writel(), a prior
> wmb() is not needed to guarantee that the cache coherent memory writes
> have completed before writing to the cache incoherent MMIO region.
> The cheaper writel_relaxed() does not guarantee the DMA to be visible
> to the device and must not be used here.
Fair enough. I'd rather people used _relaxed by default, but I have to admit
that it will probably just result in them getting things wrong. Just a tiny
bit of wordsmithing brings this to:
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index a863009849a3..3247547d1c36 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1909,9 +1909,6 @@ There are some more advanced barrier functions:
/* assign ownership */
desc->status = DEVICE_OWN;
- /* force memory to sync before notifying device via MMIO */
- wmb();
-
/* notify device of new descriptors */
writel(DESC_NOTIFY, doorbell);
}
@@ -1919,11 +1916,15 @@ There are some more advanced barrier functions:
The dma_rmb() allows us guarantee the device has released ownership
before we read the data from the descriptor, and the dma_wmb() allows
us to guarantee the data is written to the descriptor before the device
- can see it now has ownership. The wmb() is needed to guarantee that the
- cache coherent memory writes have completed before attempting a write to
- the cache incoherent MMIO region.
-
- See Documentation/DMA-API.txt for more information on consistent memory.
+ can see it now has ownership. Note that, when using writel(), a prior
+ wmb() is not needed to guarantee that the cache coherent memory writes
+ have completed before writing to the MMIO region. The cheaper
+ writel_relaxed() does not provide this guarantee and must not be used
+ here.
+
+ See the subsection "Kernel I/O barrier effects" for more information on
+ relaxed I/O accessors and the Documentation/DMA-API.txt file for more
+ information on consistent memory.
MMIO WRITE BARRIER
If you're happy with that, I'll send it as a proper patch.
Cheers,
Will
^ permalink raw reply related
* Re: RFC on writel and writel_relaxed
From: Arnd Bergmann @ 2018-03-27 10:53 UTC (permalink / raw)
To: Will Deacon
Cc: Benjamin Herrenschmidt, Jason Gunthorpe, Sinan Kaya, David Laight,
Oliver, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney, Peter Zijlstra,
Ingo Molnar, Jonathan Corbet
In-Reply-To: <20180327100944.GD29373@arm.com>
On Tue, Mar 27, 2018 at 12:09 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Mar 27, 2018 at 12:05:06PM +0200, Arnd Bergmann wrote:
>> > -
>> > - See Documentation/DMA-API.txt for more information on consistent memory.
>> > + can see it now has ownership. Note that, when using writel(), a prior
>> > + wmb() is not needed to guarantee that the cache coherent memory writes
>> > + have completed before writing to the cache incoherent MMIO region.
>> > + If this ordering between incoherent MMIO and coherent memory regions
One more thing: I think the term "incoherent MMIO" is a bit confusing, I'd
prefer just "MMIO" here. At least I don't have the faintest clue what the
difference between "coherent MMIO" and "incoherent MMIO" would be ;-)
>> > + is not required, writel_relaxed() can be used instead and is significantly
>> > + cheaper on some weakly-ordered architectures.
>>
>> I think that's a great improvement, but I'm a bit worried about recommending
>> writel_relaxed() too much: I've seen a lot of drivers that just always use
>> writel_relaxed() over write(), and some of them get that wrong when they
>> don't understand the difference but end up using DMA without explicit
>> barriers anyway.
>>
>> Also, having an architecture-independent driver use wmb()+writel_relaxed()
>> ends up being more expensive than just using write(). Not sure how to
>> best phrase it though.
>
> Perhaps I add reword that with a simple example to say:
>
> If this ordering between incoherent MMIO and coherent memory regions
> is not required (e.g. in a sequence of accesses all to the MMIO region)
> [...]
>
> since that seems to be the usual case where the _relaxed accessors help.
That still doesn't quite capture what I'd like driver writes to do: in essence
I would recommend them to use writel() all the time, except in performance
critical code that has been shown to be correct and has a comment to explain
why _relaxed() is ok in that particular function.
Maybe it can just be rephrased to warn against the use of writel_relaxed()
here, and explain the difference that way:
can see it now has ownership. Note that, when using writel(), a prior
wmb() is not needed to guarantee that the cache coherent memory writes
have completed before writing to the cache incoherent MMIO region.
The cheaper writel_relaxed() does not guarantee the DMA to be visible
to the device and must not be used here.
Arnd
^ permalink raw reply
* Re: [PATCH script] hwmon: Use octal not symbolic permissions
From: Guenter Roeck @ 2018-03-27 10:28 UTC (permalink / raw)
To: Joe Perches, Jean Delvare
Cc: linux-hwmon, linux-kernel, linuxppc-dev, linux-arm-kernel,
patches
In-Reply-To: <1522136107.12357.58.camel@perches.com>
On 03/27/2018 12:35 AM, Joe Perches wrote:
> On Mon, 2018-03-26 at 23:33 -0700, Guenter Roeck wrote:
>> Since the hwmon subsystem has been labeled as both "obsolete" and "obscure",
>
> fyi: It's marked Maintained in MAINTAINERS
>
I did not say that it was not maintained, and I am aware of the information
in the MAINTAINERS file, thanks.
Guenter
^ permalink raw reply
* Re: [PATCH 2/2] smp: introduce kick_active_cpus_sync()
From: Will Deacon @ 2018-03-27 10:21 UTC (permalink / raw)
To: Yury Norov
Cc: Paul E. McKenney, Chris Metcalf, Christopher Lameter,
Russell King - ARM Linux, Mark Rutland, Steven Rostedt,
Mathieu Desnoyers, Catalin Marinas, Pekka Enberg, David Rientjes,
Joonsoo Kim, Andrew Morton, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman, linux-arm-kernel, linuxppc-dev,
kvm-ppc, linux-mm, linux-kernel
In-Reply-To: <20180325175004.28162-3-ynorov@caviumnetworks.com>
On Sun, Mar 25, 2018 at 08:50:04PM +0300, Yury Norov wrote:
> kick_all_cpus_sync() forces all CPUs to sync caches by sending broadcast IPI.
> If CPU is in extended quiescent state (idle task or nohz_full userspace), this
> work may be done at the exit of this state. Delaying synchronization helps to
> save power if CPU is in idle state and decrease latency for real-time tasks.
>
> This patch introduces kick_active_cpus_sync() and uses it in mm/slab and arm64
> code to delay syncronization.
>
> For task isolation (https://lkml.org/lkml/2017/11/3/589), IPI to the CPU running
> isolated task would be fatal, as it breaks isolation. The approach with delaying
> of synchronization work helps to maintain isolated state.
>
> I've tested it with test from task isolation series on ThunderX2 for more than
> 10 hours (10k giga-ticks) without breaking isolation.
>
> Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
> ---
> arch/arm64/kernel/insn.c | 2 +-
> include/linux/smp.h | 2 ++
> kernel/smp.c | 24 ++++++++++++++++++++++++
> mm/slab.c | 2 +-
> 4 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> index 2718a77da165..9d7c492e920e 100644
> --- a/arch/arm64/kernel/insn.c
> +++ b/arch/arm64/kernel/insn.c
> @@ -291,7 +291,7 @@ int __kprobes aarch64_insn_patch_text(void *addrs[], u32 insns[], int cnt)
> * synchronization.
> */
> ret = aarch64_insn_patch_text_nosync(addrs[0], insns[0]);
> - kick_all_cpus_sync();
> + kick_active_cpus_sync();
> return ret;
> }
> }
I think this means that runtime modifications to the kernel text might not
be picked up by CPUs coming out of idle. Shouldn't we add an ISB on that
path to avoid executing stale instructions?
Will
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Will Deacon @ 2018-03-27 10:09 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Benjamin Herrenschmidt, Jason Gunthorpe, Sinan Kaya, David Laight,
Oliver, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney, Peter Zijlstra,
Ingo Molnar, Jonathan Corbet
In-Reply-To: <CAK8P3a0s+kP5=qEqTqAHGwB7j9ydaXD+oQJdLrFz=8swuWaDBg@mail.gmail.com>
On Tue, Mar 27, 2018 at 12:05:06PM +0200, Arnd Bergmann wrote:
> On Tue, Mar 27, 2018 at 11:57 AM, Will Deacon <will.deacon@arm.com> wrote:
>
> >
> > From db0daeaf94f0f6232f8206fc07a74211324b11d9 Mon Sep 17 00:00:00 2001
> > From: Will Deacon <will.deacon@arm.com>
> > Date: Tue, 27 Mar 2018 10:49:58 +0100
> > Subject: [PATCH] docs/memory-barriers.txt: Fix broken DMA vs MMIO ordering
> > example
> >
> > The section of memory-barriers.txt that describes the dma_Xmb() barriers
> > has an incorrect example claiming that a wmb() is required after writing
> > to coherent memory in order for those writes to be visible to a device
> > before a subsequent MMIO access using writel() can reach the device.
> >
> > In fact, this ordering guarantee is provided (at significant cost on some
> > architectures such as arm and power) by writel, so the wmb() is not
> > necessary. writel_relaxed exists for cases where this ordering is not
> > required.
> >
> > Fix the example and update the text to make this clearer.
> >
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Jonathan Corbet <corbet@lwn.net>
> > Reported-by: Sinan Kaya <okaya@codeaurora.org>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> > Documentation/memory-barriers.txt | 18 ++++++++++--------
> > 1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> > index a863009849a3..2556b4b0e6f9 100644
> > --- a/Documentation/memory-barriers.txt
> > +++ b/Documentation/memory-barriers.txt
> > @@ -1909,9 +1909,6 @@ There are some more advanced barrier functions:
> > /* assign ownership */
> > desc->status = DEVICE_OWN;
> >
> > - /* force memory to sync before notifying device via MMIO */
> > - wmb();
> > -
> > /* notify device of new descriptors */
> > writel(DESC_NOTIFY, doorbell);
> > }
> > @@ -1919,11 +1916,16 @@ There are some more advanced barrier functions:
> > The dma_rmb() allows us guarantee the device has released ownership
> > before we read the data from the descriptor, and the dma_wmb() allows
> > us to guarantee the data is written to the descriptor before the device
> > - can see it now has ownership. The wmb() is needed to guarantee that the
> > - cache coherent memory writes have completed before attempting a write to
> > - the cache incoherent MMIO region.
> > -
> > - See Documentation/DMA-API.txt for more information on consistent memory.
> > + can see it now has ownership. Note that, when using writel(), a prior
> > + wmb() is not needed to guarantee that the cache coherent memory writes
> > + have completed before writing to the cache incoherent MMIO region.
> > + If this ordering between incoherent MMIO and coherent memory regions
> > + is not required, writel_relaxed() can be used instead and is significantly
> > + cheaper on some weakly-ordered architectures.
>
> I think that's a great improvement, but I'm a bit worried about recommending
> writel_relaxed() too much: I've seen a lot of drivers that just always use
> writel_relaxed() over write(), and some of them get that wrong when they
> don't understand the difference but end up using DMA without explicit
> barriers anyway.
>
> Also, having an architecture-independent driver use wmb()+writel_relaxed()
> ends up being more expensive than just using write(). Not sure how to
> best phrase it though.
Perhaps I add reword that with a simple example to say:
If this ordering between incoherent MMIO and coherent memory regions
is not required (e.g. in a sequence of accesses all to the MMIO region)
[...]
since that seems to be the usual case where the _relaxed accessors help.
Will
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Arnd Bergmann @ 2018-03-27 10:05 UTC (permalink / raw)
To: Will Deacon
Cc: Benjamin Herrenschmidt, Jason Gunthorpe, Sinan Kaya, David Laight,
Oliver, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney, Peter Zijlstra,
Ingo Molnar, Jonathan Corbet
In-Reply-To: <20180327095745.GB29373@arm.com>
On Tue, Mar 27, 2018 at 11:57 AM, Will Deacon <will.deacon@arm.com> wrote:
>
> From db0daeaf94f0f6232f8206fc07a74211324b11d9 Mon Sep 17 00:00:00 2001
> From: Will Deacon <will.deacon@arm.com>
> Date: Tue, 27 Mar 2018 10:49:58 +0100
> Subject: [PATCH] docs/memory-barriers.txt: Fix broken DMA vs MMIO ordering
> example
>
> The section of memory-barriers.txt that describes the dma_Xmb() barriers
> has an incorrect example claiming that a wmb() is required after writing
> to coherent memory in order for those writes to be visible to a device
> before a subsequent MMIO access using writel() can reach the device.
>
> In fact, this ordering guarantee is provided (at significant cost on some
> architectures such as arm and power) by writel, so the wmb() is not
> necessary. writel_relaxed exists for cases where this ordering is not
> required.
>
> Fix the example and update the text to make this clearer.
>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Reported-by: Sinan Kaya <okaya@codeaurora.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> Documentation/memory-barriers.txt | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index a863009849a3..2556b4b0e6f9 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -1909,9 +1909,6 @@ There are some more advanced barrier functions:
> /* assign ownership */
> desc->status = DEVICE_OWN;
>
> - /* force memory to sync before notifying device via MMIO */
> - wmb();
> -
> /* notify device of new descriptors */
> writel(DESC_NOTIFY, doorbell);
> }
> @@ -1919,11 +1916,16 @@ There are some more advanced barrier functions:
> The dma_rmb() allows us guarantee the device has released ownership
> before we read the data from the descriptor, and the dma_wmb() allows
> us to guarantee the data is written to the descriptor before the device
> - can see it now has ownership. The wmb() is needed to guarantee that the
> - cache coherent memory writes have completed before attempting a write to
> - the cache incoherent MMIO region.
> -
> - See Documentation/DMA-API.txt for more information on consistent memory.
> + can see it now has ownership. Note that, when using writel(), a prior
> + wmb() is not needed to guarantee that the cache coherent memory writes
> + have completed before writing to the cache incoherent MMIO region.
> + If this ordering between incoherent MMIO and coherent memory regions
> + is not required, writel_relaxed() can be used instead and is significantly
> + cheaper on some weakly-ordered architectures.
I think that's a great improvement, but I'm a bit worried about recommending
writel_relaxed() too much: I've seen a lot of drivers that just always use
writel_relaxed() over write(), and some of them get that wrong when they
don't understand the difference but end up using DMA without explicit
barriers anyway.
Also, having an architecture-independent driver use wmb()+writel_relaxed()
ends up being more expensive than just using write(). Not sure how to
best phrase it though.
Arnd
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Will Deacon @ 2018-03-27 10:00 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Benjamin Herrenschmidt, Jason Gunthorpe, Sinan Kaya, David Laight,
Oliver, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney
In-Reply-To: <CAK8P3a0gCYjw78snni_1Jkk5uA5a-j0x-XZDrN=RaZfXctOSoA@mail.gmail.com>
On Tue, Mar 27, 2018 at 11:44:22AM +0200, Arnd Bergmann wrote:
> On Tue, Mar 27, 2018 at 10:56 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Tue, 2018-03-27 at 09:56 +0200, Arnd Bergmann wrote:
> >> On Tue, Mar 27, 2018 at 12:27 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >>
> >> I'm pretty sure I've never seen
> >> any bug reports pointing to a missing wmb() between memory
> >> and MMIO write accesses, but if you remember seeing them in the
> >> list, maybe you can look again for some evidence of something going
> >> wrong on x86 without it?
> >
> > The interesting thing is that we do seem to have a whole LOT of these
> > spurrious wmb before writel all over the tree, I suspect because of
> > that incorrect recommendation in memory-barriers.txt.
> >
> > We should fix that.
>
> Maybe the problem is just that it's so counter-intuitive that we don't
> need that barrier in Linux, when the hardware does need one on some
> architectures.
>
> How about we define a barrier type instruction specifically for this
> purpose, something like wmb_before_mmio() and have all architectures
> define that to an empty macro?
>
> That way, having correct code using wmb_before_mmio() will not
> trigger an incorrect review comment that leads to extra wmb(). ;-)
Please don't add more barriers :)! I think that will make it even more
difficult to understand how to use the ones we already have -- the problem
here seems to be that the documentation that was added for the dma_*
barriers got this wrong, but it was at least in contradiction with the
section elsewhere in memory-barriers.txt that describes the relaxed I/O
accessors.
I guess somebody could hack checkpatch to look for back-to-back wmb/writel
sequences? I suspect we could do something with coccinelle too.
Will
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Will Deacon @ 2018-03-27 9:57 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Arnd Bergmann, Jason Gunthorpe, Sinan Kaya, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney,
peterz, mingo, corbet
In-Reply-To: <1522141019.7364.43.camel@kernel.crashing.org>
[+ locking/ordering/docs people]
On Tue, Mar 27, 2018 at 07:56:59PM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2018-03-27 at 09:56 +0200, Arnd Bergmann wrote:
> > On Tue, Mar 27, 2018 at 12:27 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > > On Tue, Mar 27, 2018 at 09:01:57AM +1100, Benjamin Herrenschmidt wrote:
> > > > On Mon, 2018-03-26 at 17:46 -0400, Sinan Kaya wrote:
> > >
> > > I even see patches adding wmb() based on actual observed memory
> > > corruption during testing on Intel:
> > >
> > > https://patchwork.kernel.org/patch/10177207/
> > >
> > > So you think all of this is unnecessary and writel is totally strongly
> > > ordered, even on multi-socket Intel?
> >
> > This example adds a wmb() between two writes to a coherent DMA
> > area, it is definitely required there.
>
> Ah you are right, I incorrectly assumed that the "prod_db" function was
> an MMIO. So we do NOT have a counter example where wmb is needed on
> x86, pfiew ! :-)
>
> > I'm pretty sure I've never seen
> > any bug reports pointing to a missing wmb() between memory
> > and MMIO write accesses, but if you remember seeing them in the
> > list, maybe you can look again for some evidence of something going
> > wrong on x86 without it?
>
> The interesting thing is that we do seem to have a whole LOT of these
> spurrious wmb before writel all over the tree, I suspect because of
> that incorrect recommendation in memory-barriers.txt.
>
> We should fix that.
Patch below. Thoughts?
Will
--->8
>From db0daeaf94f0f6232f8206fc07a74211324b11d9 Mon Sep 17 00:00:00 2001
From: Will Deacon <will.deacon@arm.com>
Date: Tue, 27 Mar 2018 10:49:58 +0100
Subject: [PATCH] docs/memory-barriers.txt: Fix broken DMA vs MMIO ordering
example
The section of memory-barriers.txt that describes the dma_Xmb() barriers
has an incorrect example claiming that a wmb() is required after writing
to coherent memory in order for those writes to be visible to a device
before a subsequent MMIO access using writel() can reach the device.
In fact, this ordering guarantee is provided (at significant cost on some
architectures such as arm and power) by writel, so the wmb() is not
necessary. writel_relaxed exists for cases where this ordering is not
required.
Fix the example and update the text to make this clearer.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Reported-by: Sinan Kaya <okaya@codeaurora.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
Documentation/memory-barriers.txt | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index a863009849a3..2556b4b0e6f9 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1909,9 +1909,6 @@ There are some more advanced barrier functions:
/* assign ownership */
desc->status = DEVICE_OWN;
- /* force memory to sync before notifying device via MMIO */
- wmb();
-
/* notify device of new descriptors */
writel(DESC_NOTIFY, doorbell);
}
@@ -1919,11 +1916,16 @@ There are some more advanced barrier functions:
The dma_rmb() allows us guarantee the device has released ownership
before we read the data from the descriptor, and the dma_wmb() allows
us to guarantee the data is written to the descriptor before the device
- can see it now has ownership. The wmb() is needed to guarantee that the
- cache coherent memory writes have completed before attempting a write to
- the cache incoherent MMIO region.
-
- See Documentation/DMA-API.txt for more information on consistent memory.
+ can see it now has ownership. Note that, when using writel(), a prior
+ wmb() is not needed to guarantee that the cache coherent memory writes
+ have completed before writing to the cache incoherent MMIO region.
+ If this ordering between incoherent MMIO and coherent memory regions
+ is not required, writel_relaxed() can be used instead and is significantly
+ cheaper on some weakly-ordered architectures.
+
+ See the subsection "Kernel I/O barrier effects" for more information on
+ relaxed I/O accessors and the Documentation/DMA-API.txt file for more
+ information on consistent memory.
MMIO WRITE BARRIER
--
2.1.4
^ permalink raw reply related
* Re: RFC on writel and writel_relaxed
From: Arnd Bergmann @ 2018-03-27 9:44 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Jason Gunthorpe, Sinan Kaya, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Will Deacon,
Paul E. McKenney
In-Reply-To: <1522141019.7364.43.camel@kernel.crashing.org>
On Tue, Mar 27, 2018 at 10:56 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Tue, 2018-03-27 at 09:56 +0200, Arnd Bergmann wrote:
>> On Tue, Mar 27, 2018 at 12:27 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>>
>> I'm pretty sure I've never seen
>> any bug reports pointing to a missing wmb() between memory
>> and MMIO write accesses, but if you remember seeing them in the
>> list, maybe you can look again for some evidence of something going
>> wrong on x86 without it?
>
> The interesting thing is that we do seem to have a whole LOT of these
> spurrious wmb before writel all over the tree, I suspect because of
> that incorrect recommendation in memory-barriers.txt.
>
> We should fix that.
Maybe the problem is just that it's so counter-intuitive that we don't
need that barrier in Linux, when the hardware does need one on some
architectures.
How about we define a barrier type instruction specifically for this
purpose, something like wmb_before_mmio() and have all architectures
define that to an empty macro?
That way, having correct code using wmb_before_mmio() will not
trigger an incorrect review comment that leads to extra wmb(). ;-)
Arnd
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Will Deacon @ 2018-03-27 9:42 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jason Gunthorpe, Benjamin Herrenschmidt, Sinan Kaya, David Laight,
Oliver, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney
In-Reply-To: <CAK8P3a3a+zUNLjqw9CL5rYkXhJ-GMVTVnBU2XUu2f3njZfrqKw@mail.gmail.com>
On Tue, Mar 27, 2018 at 09:56:47AM +0200, Arnd Bergmann wrote:
> On Tue, Mar 27, 2018 at 12:27 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > On Tue, Mar 27, 2018 at 09:01:57AM +1100, Benjamin Herrenschmidt wrote:
> >> On Mon, 2018-03-26 at 17:46 -0400, Sinan Kaya wrote:
> >
> > I even see patches adding wmb() based on actual observed memory
> > corruption during testing on Intel:
> >
> > https://patchwork.kernel.org/patch/10177207/
> >
> > So you think all of this is unnecessary and writel is totally strongly
> > ordered, even on multi-socket Intel?
>
> This example adds a wmb() between two writes to a coherent DMA
> area, it is definitely required there. I'm pretty sure I've never seen
> any bug reports pointing to a missing wmb() between memory
> and MMIO write accesses, but if you remember seeing them in the
> list, maybe you can look again for some evidence of something going
> wrong on x86 without it?
If this is just about ordering accesses to coherent DMA, then using
dma_wmb() instead will be much better performance on arm/arm64.
Will
^ permalink raw reply
* [PATCH v13 3/3] mm, x86, powerpc: display pkey in smaps only if arch supports pkeys
From: Ram Pai @ 2018-03-27 9:09 UTC (permalink / raw)
To: mpe, mingo, akpm
Cc: linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kselftest, linux-kernel, dave.hansen, benh, paulus,
khandual, aneesh.kumar, bsingharora, hbabu, mhocko, bauerman,
ebiederm, linuxram, corbet, arnd
In-Reply-To: <1522141768-25485-1-git-send-email-linuxram@us.ibm.com>
Currently the architecture specific code is expected to
display the protection keys in smap for a given vma.
This can lead to redundant code and possibly to divergent
formats in which the key gets displayed.
This patch changes the implementation. It displays the
pkey only if the architecture support pkeys, i.e
arch_pkeys_enabled() returns true. This patch
provides x86 implementation for arch_pkeys_enabled().
x86 arch_show_smap() function is not needed anymore.
Deleting it.
cc: Michael Ellermen <mpe@ellerman.id.au>
cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
cc: Andrew Morton <akpm@linux-foundation.org>
Reviewed-by: Dave Hansen <dave.hansen@intel.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
(fixed compilation errors for x86 configs)
Acked-by: Michal Hocko <mhocko@suse.com>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
arch/powerpc/include/asm/mmu_context.h | 5 -----
arch/x86/include/asm/mmu_context.h | 5 -----
arch/x86/include/asm/pkeys.h | 1 +
arch/x86/kernel/fpu/xstate.c | 5 +++++
arch/x86/kernel/setup.c | 8 --------
fs/proc/task_mmu.c | 10 +++++-----
include/linux/pkeys.h | 7 ++++++-
7 files changed, 17 insertions(+), 24 deletions(-)
diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
index 051b3d6..566b3c2 100644
--- a/arch/powerpc/include/asm/mmu_context.h
+++ b/arch/powerpc/include/asm/mmu_context.h
@@ -203,11 +203,6 @@ static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,
#define thread_pkey_regs_restore(new_thread, old_thread)
#define thread_pkey_regs_init(thread)
-static inline int vma_pkey(struct vm_area_struct *vma)
-{
- return 0;
-}
-
static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
{
return 0x0UL;
diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
index 1de72ce..e597d09 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -295,11 +295,6 @@ static inline int vma_pkey(struct vm_area_struct *vma)
return (vma->vm_flags & vma_pkey_mask) >> VM_PKEY_SHIFT;
}
-#else
-static inline int vma_pkey(struct vm_area_struct *vma)
-{
- return 0;
-}
#endif
/*
diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index a0ba1ff..f6c287b 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h
@@ -6,6 +6,7 @@
extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
unsigned long init_val);
+extern bool arch_pkeys_enabled(void);
/*
* Try to dedicate one of the protection keys to be used as an
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 87a57b7..4f566e9 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -945,6 +945,11 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
return 0;
}
+
+bool arch_pkeys_enabled(void)
+{
+ return boot_cpu_has(X86_FEATURE_OSPKE);
+}
#endif /* ! CONFIG_ARCH_HAS_PKEYS */
/*
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 4c616be..117ed01 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1307,11 +1307,3 @@ static int __init register_kernel_offset_dumper(void)
return 0;
}
__initcall(register_kernel_offset_dumper);
-
-void arch_show_smap(struct seq_file *m, struct vm_area_struct *vma)
-{
- if (!boot_cpu_has(X86_FEATURE_OSPKE))
- return;
-
- seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma));
-}
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 6d83bb7..70aa912 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -18,10 +18,12 @@
#include <linux/page_idle.h>
#include <linux/shmem_fs.h>
#include <linux/uaccess.h>
+#include <linux/pkeys.h>
#include <asm/elf.h>
#include <asm/tlb.h>
#include <asm/tlbflush.h>
+#include <asm/mmu_context.h>
#include "internal.h"
void task_mem(struct seq_file *m, struct mm_struct *mm)
@@ -733,10 +735,6 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask,
}
#endif /* HUGETLB_PAGE */
-void __weak arch_show_smap(struct seq_file *m, struct vm_area_struct *vma)
-{
-}
-
static int show_smap(struct seq_file *m, void *v, int is_pid)
{
struct proc_maps_private *priv = m->private;
@@ -856,9 +854,11 @@ static int show_smap(struct seq_file *m, void *v, int is_pid)
(unsigned long)(mss->pss >> (10 + PSS_SHIFT)));
if (!rollup_mode) {
- arch_show_smap(m, vma);
+ if (arch_pkeys_enabled())
+ seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma));
show_smap_vma_flags(m, vma);
}
+
m_cache_vma(m, vma);
return ret;
}
diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
index 0794ca7..49dff15 100644
--- a/include/linux/pkeys.h
+++ b/include/linux/pkeys.h
@@ -3,7 +3,6 @@
#define _LINUX_PKEYS_H
#include <linux/mm_types.h>
-#include <asm/mmu_context.h>
#ifdef CONFIG_ARCH_HAS_PKEYS
#include <asm/pkeys.h>
@@ -13,6 +12,7 @@
#define arch_override_mprotect_pkey(vma, prot, pkey) (0)
#define PKEY_DEDICATED_EXECUTE_ONLY 0
#define ARCH_VM_PKEY_FLAGS 0
+#define vma_pkey(vma) 0
static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
{
@@ -35,6 +35,11 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
return 0;
}
+static inline bool arch_pkeys_enabled(void)
+{
+ return false;
+}
+
static inline void copy_init_pkru_to_fpregs(void)
{
}
--
1.7.1
^ permalink raw reply related
* [PATCH v13 2/3] mm, powerpc, x86: introduce an additional vma bit for powerpc pkey
From: Ram Pai @ 2018-03-27 9:09 UTC (permalink / raw)
To: mpe, mingo, akpm
Cc: linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kselftest, linux-kernel, dave.hansen, benh, paulus,
khandual, aneesh.kumar, bsingharora, hbabu, mhocko, bauerman,
ebiederm, linuxram, corbet, arnd
In-Reply-To: <1522141768-25485-1-git-send-email-linuxram@us.ibm.com>
Currently only 4bits are allocated in the vma flags to hold 16
keys. This is sufficient for x86. PowerPC supports 32 keys,
which needs 5bits. This patch allocates an additional bit.
cc: Dave Hansen <dave.hansen@intel.com>
cc: Michael Ellermen <mpe@ellerman.id.au>
cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
cc: Andrew Morton <akpm@linux-foundation.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Balbir Singh <bsingharora@gmail.com>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
fs/proc/task_mmu.c | 1 +
include/linux/mm.h | 3 ++-
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 6b996d0..6d83bb7 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -685,6 +685,7 @@ static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
[ilog2(VM_PKEY_BIT1)] = "",
[ilog2(VM_PKEY_BIT2)] = "",
[ilog2(VM_PKEY_BIT3)] = "",
+ [ilog2(VM_PKEY_BIT4)] = "",
#endif /* CONFIG_ARCH_HAS_PKEYS */
};
size_t i;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index ad207ad..d534f46 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -231,9 +231,10 @@ extern int overcommit_kbytes_handler(struct ctl_table *, int, void __user *,
#ifdef CONFIG_ARCH_HAS_PKEYS
# define VM_PKEY_SHIFT VM_HIGH_ARCH_BIT_0
# define VM_PKEY_BIT0 VM_HIGH_ARCH_0 /* A protection key is a 4-bit value */
-# define VM_PKEY_BIT1 VM_HIGH_ARCH_1
+# define VM_PKEY_BIT1 VM_HIGH_ARCH_1 /* on x86 and 5-bit value on ppc64 */
# define VM_PKEY_BIT2 VM_HIGH_ARCH_2
# define VM_PKEY_BIT3 VM_HIGH_ARCH_3
+# define VM_PKEY_BIT4 VM_HIGH_ARCH_4
#endif /* CONFIG_ARCH_HAS_PKEYS */
#if defined(CONFIG_X86)
--
1.7.1
^ permalink raw reply related
* [PATCH v13 1/3] mm, powerpc, x86: define VM_PKEY_BITx bits if CONFIG_ARCH_HAS_PKEYS is enabled
From: Ram Pai @ 2018-03-27 9:09 UTC (permalink / raw)
To: mpe, mingo, akpm
Cc: linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kselftest, linux-kernel, dave.hansen, benh, paulus,
khandual, aneesh.kumar, bsingharora, hbabu, mhocko, bauerman,
ebiederm, linuxram, corbet, arnd
In-Reply-To: <1522141768-25485-1-git-send-email-linuxram@us.ibm.com>
VM_PKEY_BITx are defined only if CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
is enabled. Powerpc also needs these bits. Hence lets define the
VM_PKEY_BITx bits for any architecture that enables
CONFIG_ARCH_HAS_PKEYS.
cc: Michael Ellermen <mpe@ellerman.id.au>
cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
cc: Andrew Morton <akpm@linux-foundation.org>
Reviewed-by: Dave Hansen <dave.hansen@intel.com>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/pkeys.h | 2 ++
fs/proc/task_mmu.c | 4 ++--
include/linux/mm.h | 9 +++++----
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 0d3c630..99344d7 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -26,6 +26,8 @@
# define VM_PKEY_BIT2 VM_HIGH_ARCH_2
# define VM_PKEY_BIT3 VM_HIGH_ARCH_3
# define VM_PKEY_BIT4 VM_HIGH_ARCH_4
+#elif !defined(VM_PKEY_BIT4)
+# define VM_PKEY_BIT4 VM_HIGH_ARCH_4
#endif
#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index ec6d298..6b996d0 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -679,13 +679,13 @@ static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
[ilog2(VM_MERGEABLE)] = "mg",
[ilog2(VM_UFFD_MISSING)]= "um",
[ilog2(VM_UFFD_WP)] = "uw",
-#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
+#ifdef CONFIG_ARCH_HAS_PKEYS
/* These come out via ProtectionKey: */
[ilog2(VM_PKEY_BIT0)] = "",
[ilog2(VM_PKEY_BIT1)] = "",
[ilog2(VM_PKEY_BIT2)] = "",
[ilog2(VM_PKEY_BIT3)] = "",
-#endif
+#endif /* CONFIG_ARCH_HAS_PKEYS */
};
size_t i;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index ad06d42..ad207ad 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -228,15 +228,16 @@ extern int overcommit_kbytes_handler(struct ctl_table *, int, void __user *,
#define VM_HIGH_ARCH_4 BIT(VM_HIGH_ARCH_BIT_4)
#endif /* CONFIG_ARCH_USES_HIGH_VMA_FLAGS */
-#if defined(CONFIG_X86)
-# define VM_PAT VM_ARCH_1 /* PAT reserves whole VMA at once (x86) */
-#if defined (CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS)
+#ifdef CONFIG_ARCH_HAS_PKEYS
# define VM_PKEY_SHIFT VM_HIGH_ARCH_BIT_0
# define VM_PKEY_BIT0 VM_HIGH_ARCH_0 /* A protection key is a 4-bit value */
# define VM_PKEY_BIT1 VM_HIGH_ARCH_1
# define VM_PKEY_BIT2 VM_HIGH_ARCH_2
# define VM_PKEY_BIT3 VM_HIGH_ARCH_3
-#endif
+#endif /* CONFIG_ARCH_HAS_PKEYS */
+
+#if defined(CONFIG_X86)
+# define VM_PAT VM_ARCH_1 /* PAT reserves whole VMA at once (x86) */
#elif defined(CONFIG_PPC)
# define VM_SAO VM_ARCH_1 /* Strong Access Ordering (powerpc) */
#elif defined(CONFIG_PARISC)
--
1.7.1
^ permalink raw reply related
* [PATCH v13 0/3] mm, x86, powerpc: Enhancements to Memory Protection Keys.
From: Ram Pai @ 2018-03-27 9:09 UTC (permalink / raw)
To: mpe, mingo, akpm
Cc: linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kselftest, linux-kernel, dave.hansen, benh, paulus,
khandual, aneesh.kumar, bsingharora, hbabu, mhocko, bauerman,
ebiederm, linuxram, corbet, arnd
This patch series provides arch-neutral enhancements to
enable memory-keys on new architecutes, and the corresponding
changes in x86 and powerpc specific code to support that.
a) Provides ability to support upto 32 keys. PowerPC
can handle 32 keys and hence needs this.
b) Arch-neutral code; and not the arch-specific code,
determines the format of the string, that displays the key
for each vma in smaps.
History:
-------
version v13:
(1) fixed a git bisect error. :(
version v12:
(1) fixed compilation errors seen with various x86
configs.
version v11:
(1) code that displays key in smaps is not any more
defined under CONFIG_ARCH_HAS_PKEYS.
- Comment by Eric W. Biederman and Michal Hocko
(2) merged two patches that implemented (1).
- comment by Michal Hocko
version prior to v11:
(1) used one additional bit from VM_HIGH_ARCH_*
to support 32 keys.
- Suggestion by Dave Hansen.
(2) powerpc specific changes to support memory keys.
Ram Pai (3):
mm, powerpc, x86: define VM_PKEY_BITx bits if CONFIG_ARCH_HAS_PKEYS
is enabled
mm, powerpc, x86: introduce an additional vma bit for powerpc pkey
mm, x86, powerpc: display pkey in smaps only if arch supports pkeys
arch/powerpc/include/asm/mmu_context.h | 5 -----
arch/powerpc/include/asm/pkeys.h | 2 ++
arch/x86/include/asm/mmu_context.h | 5 -----
arch/x86/include/asm/pkeys.h | 1 +
arch/x86/kernel/fpu/xstate.c | 5 +++++
arch/x86/kernel/setup.c | 8 --------
fs/proc/task_mmu.c | 15 ++++++++-------
include/linux/mm.h | 12 +++++++-----
include/linux/pkeys.h | 7 ++++++-
9 files changed, 29 insertions(+), 31 deletions(-)
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-27 8:56 UTC (permalink / raw)
To: Arnd Bergmann, Jason Gunthorpe
Cc: Sinan Kaya, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Will Deacon,
Paul E. McKenney
In-Reply-To: <CAK8P3a3a+zUNLjqw9CL5rYkXhJ-GMVTVnBU2XUu2f3njZfrqKw@mail.gmail.com>
On Tue, 2018-03-27 at 09:56 +0200, Arnd Bergmann wrote:
> On Tue, Mar 27, 2018 at 12:27 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > On Tue, Mar 27, 2018 at 09:01:57AM +1100, Benjamin Herrenschmidt wrote:
> > > On Mon, 2018-03-26 at 17:46 -0400, Sinan Kaya wrote:
> >
> > I even see patches adding wmb() based on actual observed memory
> > corruption during testing on Intel:
> >
> > https://patchwork.kernel.org/patch/10177207/
> >
> > So you think all of this is unnecessary and writel is totally strongly
> > ordered, even on multi-socket Intel?
>
> This example adds a wmb() between two writes to a coherent DMA
> area, it is definitely required there.
Ah you are right, I incorrectly assumed that the "prod_db" function was
an MMIO. So we do NOT have a counter example where wmb is needed on
x86, pfiew ! :-)
> I'm pretty sure I've never seen
> any bug reports pointing to a missing wmb() between memory
> and MMIO write accesses, but if you remember seeing them in the
> list, maybe you can look again for some evidence of something going
> wrong on x86 without it?
The interesting thing is that we do seem to have a whole LOT of these
spurrious wmb before writel all over the tree, I suspect because of
that incorrect recommendation in memory-barriers.txt.
We should fix that.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 17/21] powerpc: Add missing prototype for sys_debug_setcontext
From: Michael Ellerman @ 2018-03-27 8:50 UTC (permalink / raw)
To: Mathieu Malaterre
Cc: Benjamin Herrenschmidt, Paul Mackerras, Jiri Slaby, linuxppc-dev,
LKML, Al Viro
In-Reply-To: <CA+7wUszeX3vdctpddcEvx+j-evwSBbX=Q7Mh1-mx+GNFbFh0=A@mail.gmail.com>
Hi Mathieu,
Mathieu Malaterre <malat@debian.org> writes:
> On Thu, Mar 8, 2018 at 11:36 AM, Michael Ellerman <mpe@ellerman.id.au> wr=
ote:
>> Mathieu Malaterre <malat@debian.org> writes:
>>> On Sun, Mar 4, 2018 at 11:54 AM, Michael Ellerman <mpe@ellerman.id.au> =
wrote:
>>>> Mathieu Malaterre <malat@debian.org> writes:
>>>>> In commit 81e7009ea46c ("powerpc: merge ppc signal.c and ppc64 signal=
32.c")
>>>>> the function sys_debug_setcontext was added without a prototype.
>>>>>
>>>>> Fix compilation warning (treated as error in W=3D1):
>>>>>
>>>>> CC arch/powerpc/kernel/signal_32.o
>>>>> arch/powerpc/kernel/signal_32.c:1227:5: error: no previous prototype =
for =E2=80=98sys_debug_setcontext=E2=80=99 [-Werror=3Dmissing-prototypes]
>>>>> int sys_debug_setcontext(struct ucontext __user *ctx,
>>>>> ^~~~~~~~~~~~~~~~~~~~
>>>>> cc1: all warnings being treated as errors
>>>>
>>>> This one should actually be using the SYSCALL_DEFINE syntax, so that it
>>>> can be used with CONFIG_FTRACE_SYSCALLS.
>>>>
>>>> See eg. our mmap:
>>>>
>>>> SYSCALL_DEFINE6(mmap, unsigned long, addr, size_t, len,
>>>> unsigned long, prot, unsigned long, flags,
>>>> unsigned long, fd, off_t, offset)
>>>> {
>>>> return do_mmap2(addr, len, prot, flags, fd, offset, PAGE_SHIFT=
);
>>>> }
>>>>
>>>>
>>>> We probably still need this patch, but I'm not entirely sure because t=
he
>>>> SYSCALL_DEFINE macro does all sorts of shenanigans.
>>>
>>> I see. Could you please drop this patch then. The patch does not look
>>> that trivial anymore. I'll need to dig a bit more on how to do the
>>> syscall stuff with a 7 params function.
>>
>> Ergh, yuck, seems we're the first suckers to need do that.
>>
>> I think I'll take this patch for now, it's still good for now at least,
>> and then the SYSCALL_DEFINE stuff can be an addition.
>
> Just to close the loop (for later reference). Here is what Al Viro
> told me about this function
>
> [begin quote]
> int sys_debug_setcontext(struct ucontext __user *ctx,
> int ndbg, struct sig_dbg_op __user *dbg,
> int r6, int r7, int r8,
> struct pt_regs *regs)
>
> can't be converted to SYSCALL_DEFINE... not because it's a 7-argument
> syscall (it isn't); the problem is that the last argument here does
> *not* come from userland. What it really is trying to be is
> 3-argument syscall:
> SYSCALL_DEFINE3(debug_setcontext, struct ucontext __user *, ctx,
> int, ndbg,
> struct sig_dbg_op __user *, dbg)
> with no dummy r6/r7/r8. The thing is, ppc dispatcher combines the
> "pass 6 arguments" and "pass pt_regs *" by passing both.
>
> debug_setcontext(), swapcontext(), sigreturn() and rt_sigreturn() are,
> AFAICS, the only ppc syscalls that make use of that argument. All of
> those are relying upon regs =3D=3D current_pt_regs() =3D=3D current->thre=
ad.regs,
> [...]
> But in any case, these are not 7-argument syscalls - debug_setcontext(2)
> and swapcontext(2) are 3-argument and sigreturn()/rt_sigreturn() have
> no arguments at all.
> [end quote]
Awesome follow-up thanks.
I should have realised that's what the code was doing, but I didn't look
closely enough. Apologies to Al for having to stare at our horrible
signal code again :)
We should really clean all that up, I can't see any reason those
syscalls aren't using current_pt_regs().
That would then mean we could use SYSCALL_DEFINE.
I wrote that down so we don't (might not) forget:
https://github.com/linuxppc/linux/issues/131
> Sorry about the wrong analysis in the number of arguments count. I
> believe commit 0d60619e1c0ca (powerpc/next) should be just fine for
> now.
No worries.
cheers
^ permalink raw reply
* Re: [PATCH 01/10] powerpc: Add security feature flags for Spectre/Meltdown
From: kbuild test robot @ 2018-03-27 8:46 UTC (permalink / raw)
To: Michael Ellerman; +Cc: kbuild-all, linuxppc-dev
In-Reply-To: <20180326091215.7784-1-mpe@ellerman.id.au>
[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]
Hi Michael,
I love your patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.16-rc7 next-20180326]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Michael-Ellerman/powerpc-Add-security-feature-flags-for-Spectre-Meltdown/20180327-041008
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-g5_defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc
Note: the linux-review/Michael-Ellerman/powerpc-Add-security-feature-flags-for-Spectre-Meltdown/20180327-041008 HEAD 95772a19ac473c1aa27b1e5e038791da104b6a06 builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
In file included from arch/powerpc/kernel/security.c:7:0:
>> arch/powerpc/include/asm/security_features.h:24:15: error: unknown type name 'bool'
static inline bool security_ftr_enabled(unsigned long feature)
^~~~
>> arch/powerpc/kernel/security.c:10:41: error: expected '=', ',', ';', 'asm' or '__attribute__' before '__read_mostly'
unsigned long powerpc_security_features __read_mostly = \
^~~~~~~~~~~~~
vim +/bool +24 arch/powerpc/include/asm/security_features.h
23
> 24 static inline bool security_ftr_enabled(unsigned long feature)
25 {
26 return !!(powerpc_security_features & feature);
27 }
28
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 20798 bytes --]
^ permalink raw reply
* Re: [PATCH 14/19] powerpc/altivec: Add missing prototypes for altivec
From: Mathieu Malaterre @ 2018-03-27 8:39 UTC (permalink / raw)
To: LEROY Christophe
Cc: linuxppc-dev, Paul Mackerras, kvm-ppc, LKML, Michael Ellerman
In-Reply-To: <20180324211020.Horde.kEO4b3mHPV88FKM0OiRT0Q1@messagerie.si.c-s.fr>
Christophe,
On Sat, Mar 24, 2018 at 9:10 PM, LEROY Christophe
<christophe.leroy@c-s.fr> wrote:
> Mathieu Malaterre <malat@debian.org> a =C3=A9crit :
>
>
>> On Fri, Mar 23, 2018 at 1:19 PM, christophe leroy
>> <christophe.leroy@c-s.fr> wrote:
>>>
>>>
>>>
>>> Le 22/03/2018 =C3=A0 21:20, Mathieu Malaterre a =C3=A9crit :
>>>>
>>>>
>>>> Some functions prototypes were missing for the non-altivec code. Add t=
he
>>>> missing prototypes directly in xor_vmx, fix warnings treated as errors
>>>> with
>>>> W=3D1:
>>>>
>>>> arch/powerpc/lib/xor_vmx_glue.c:18:6: error: no previous prototype
>>>> for
>>>> =E2=80=98xor_altivec_2=E2=80=99 [-Werror=3Dmissing-prototypes]
>>>> arch/powerpc/lib/xor_vmx_glue.c:29:6: error: no previous prototype
>>>> for
>>>> =E2=80=98xor_altivec_3=E2=80=99 [-Werror=3Dmissing-prototypes]
>>>> arch/powerpc/lib/xor_vmx_glue.c:40:6: error: no previous prototype
>>>> for
>>>> =E2=80=98xor_altivec_4=E2=80=99 [-Werror=3Dmissing-prototypes]
>>>> arch/powerpc/lib/xor_vmx_glue.c:52:6: error: no previous prototype
>>>> for
>>>> =E2=80=98xor_altivec_5=E2=80=99 [-Werror=3Dmissing-prototypes]
>>>>
>>>> Signed-off-by: Mathieu Malaterre <malat@debian.org>
>>>> ---
>>>> arch/powerpc/lib/xor_vmx.h | 14 ++++++++++++++
>>>> 1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/arch/powerpc/lib/xor_vmx.h b/arch/powerpc/lib/xor_vmx.h
>>>> index 5c2b0839b179..2173e3c84151 100644
>>>> --- a/arch/powerpc/lib/xor_vmx.h
>>>> +++ b/arch/powerpc/lib/xor_vmx.h
>>>> @@ -19,3 +19,17 @@ void __xor_altivec_4(unsigned long bytes, unsigned
>>>> long
>>>> *v1_in,
>>>> void __xor_altivec_5(unsigned long bytes, unsigned long *v1_in,
>>>> unsigned long *v2_in, unsigned long *v3_i=
n,
>>>> unsigned long *v4_in, unsigned long
>>>> *v5_in);
>>>> +
>>>> +void xor_altivec_2(unsigned long bytes, unsigned long *v1_in,
>>>> + unsigned long *v2_in);
>>>> +
>>>
>>>
>>>
>>> Only used in one place, should be static instead of adding it in a .h
>>>
>>> Same for the other ones.
>>
>>
>> $ git grep xor_altivec_2
>> [...]
>> arch/powerpc/lib/xor_vmx_glue.c:EXPORT_SYMBOL(xor_altivec_2);
>>
>> Are you sure I can change this function to static ?
>
>
> Yes you are right. But in fact those fonctions are already defined in
> asm/xor. h
> So you just need to add the missing #include
I originally tried it, but this leads to:
CC arch/powerpc/lib/xor_vmx_glue.o
In file included from arch/powerpc/lib/xor_vmx_glue.c:16:0:
./arch/powerpc/include/asm/xor.h:39:15: error: variable
=E2=80=98xor_block_altivec=E2=80=99 has initializer but incomplete type
static struct xor_block_template xor_block_altivec =3D {
^~~~~~~~~~~~~~~~~~
./arch/powerpc/include/asm/xor.h:40:2: error: unknown field =E2=80=98name=
=E2=80=99
specified in initializer
.name =3D "altivec",
^
[...]
The file <asm/xor.h> (powerpc) is pretty much expected to be included
after <include/linux/raid/xor.h>.
I did not want to tweak <asm/xor.h> to test for #ifdef _XOR_H just before
#ifdef _XOR_H
static struct xor_block_template xor_block_altivec =3D {
[...]
since this seems like a hack to me.
Is this ok to test for #ifdef _XOR_H in <arch/powerpc/include/asm/xor.h> ?
> Christophe
>
>
>>
>>> Christophe
>>>
>>>
>>>> +void xor_altivec_3(unsigned long bytes, unsigned long *v1_in,
>>>> + unsigned long *v2_in, unsigned long
>>>> *v3_in);
>>>> +
>>>> +void xor_altivec_4(unsigned long bytes, unsigned long *v1_in,
>>>> + unsigned long *v2_in, unsigned long *v3_i=
n,
>>>> + unsigned long *v4_in);
>>>> +
>>>> +void xor_altivec_5(unsigned long bytes, unsigned long *v1_in,
>>>> + unsigned long *v2_in, unsigned long *v3_i=
n,
>>>> + unsigned long *v4_in, unsigned long
>>>> *v5_in);
>>>>
>>>
>>> ---
>>> L'absence de virus dans ce courrier =C3=A9lectronique a =C3=A9t=C3=A9 v=
=C3=A9rifi=C3=A9e par le
>>> logiciel antivirus Avast.
>>> https://www.avast.com/antivirus
>>>
>
>
^ permalink raw reply
* [bug report] powerpc/4xx: Adding PCIe MSI support
From: Dan Carpenter @ 2018-03-27 8:37 UTC (permalink / raw)
To: rsarmah; +Cc: linuxppc-dev
[ This is really really ancient code. - dan ]
Hello Rupjyoti Sarmah,
The patch 3fb7933850fa: "powerpc/4xx: Adding PCIe MSI support" from
Mar 29, 2011, leads to the following static checker warning:
arch/powerpc/platforms/4xx/msi.c:100 ppc4xx_setup_msi_irqs()
warn: 'int_no >= 0' 'false' implies 'int_no < 0' is 'true'
arch/powerpc/platforms/4xx/msi.c
79 static int ppc4xx_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
80 {
81 int int_no = -ENOMEM;
82 unsigned int virq;
83 struct msi_msg msg;
84 struct msi_desc *entry;
85 struct ppc4xx_msi *msi_data = &ppc4xx_msi;
86
87 dev_dbg(&dev->dev, "PCIE-MSI:%s called. vec %x type %d\n",
88 __func__, nvec, type);
89 if (type == PCI_CAP_ID_MSIX)
90 pr_debug("ppc4xx msi: MSI-X untested, trying anyway.\n");
91
92 msi_data->msi_virqs = kmalloc((msi_irqs) * sizeof(int), GFP_KERNEL);
93 if (!msi_data->msi_virqs)
94 return -ENOMEM;
95
96 for_each_pci_msi_entry(entry, dev) {
97 int_no = msi_bitmap_alloc_hwirqs(&msi_data->bitmap, 1);
98 if (int_no >= 0)
^^^^^^^^^^^
99 break;
100 if (int_no < 0) {
^^^^^^^^^^
Smatch is saying that this check could be removed, which is true.
101 pr_debug("%s: fail allocating msi interrupt\n",
102 __func__);
103 }
104 virq = irq_of_parse_and_map(msi_data->msi_dev, int_no);
^^^^^^
It doesn't seem right to pass negative indexes to irq_of_parse_and_map().
It could result in a read before the start of the array in
of_irq_parse_oldworld(), I think.
105 if (!virq) {
106 dev_err(&dev->dev, "%s: fail mapping irq\n", __func__);
107 msi_bitmap_free_hwirqs(&msi_data->bitmap, int_no, 1);
108 return -ENOSPC;
109 }
110 dev_dbg(&dev->dev, "%s: virq = %d\n", __func__, virq);
111
112 /* Setup msi address space */
113 msg.address_hi = msi_data->msi_addr_hi;
114 msg.address_lo = msi_data->msi_addr_lo;
115
116 irq_set_msi_desc(virq, entry);
117 msg.data = int_no;
118 pci_write_msi_msg(virq, &msg);
119 }
120 return 0;
121 }
regards,
dan carpenter
^ 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