* Re: use generic DMA mapping code in powerpc V4
From: Christian Zigotzky @ 2019-01-29 15:03 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, Darren Stevens, linux-kernel, Julian Margetson,
linux-mm, iommu, Paul Mackerras, Olof Johansson, linuxppc-dev
In-Reply-To: <D64B1ED5-46F9-43CF-9B21-FABB2807289B@xenosoft.de>
Hi Christoph,
I compiled kernels for the X5000 and X1000 from your new branch
'powerpc-dma.6-debug.2' today. The kernels boot and the P.A. Semi
Ethernet works!
Cheers,
Christian
On 28 January 2019 at 5:52PM, Christian Zigotzky wrote:
> Thanks a lot! I will test it tomorrow.
>
> — Christian
>
> Sent from my iPhone
>
>> On 28. Jan 2019, at 17:22, Christoph Hellwig <hch@lst.de> wrote:
>>
>>> On Mon, Jan 28, 2019 at 08:04:22AM +0100, Christoph Hellwig wrote:
>>>> On Sun, Jan 27, 2019 at 02:13:09PM +0100, Christian Zigotzky wrote:
>>>> Christoph,
>>>>
>>>> What shall I do next?
>>> I'll need to figure out what went wrong with the new zone selection
>>> on powerpc and give you another branch to test.
>> Can you try the new powerpc-dma.6-debug.2 branch:
>>
>> git://git.infradead.org/users/hch/misc.git powerpc-dma.6-debug.2
>>
>> Gitweb:
>>
>> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/powerpc-dma.6-debug.2
>
^ permalink raw reply
* Re: [PATCH 00/19] KVM: PPC: Book3S HV: add XIVE native exploitation mode
From: Cédric Le Goater @ 2019-01-29 13:51 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm, kvm-ppc, linuxppc-dev, David Gibson
In-Reply-To: <20190128055108.GC3237@blackberry>
>>> Another general comment is that you seem to have written all this
>>> code assuming we are using HV KVM in a host running bare-metal.
>>
>> Yes. I didn't look at the other configurations. I thought that we could
>> use the kernel_irqchip=off option to begin with. A couple of checks
>> are indeed missing.
>
> Using kernel_irqchip=off would mean that we would not be able to use
> the in-kernel XICS emulation, which would have a performance impact.
yes. But it is not supported today. Correct ?
> We need an explicit capability for XIVE exploitation that can be
> enabled or disabled on the qemu command line, so that we can enforce a
> uniform set of capabilities across all the hosts in a migration
> domain. And it's no good to say we have the capability when all
> attempts to use it will fail. Therefore the kernel needs to say that
> it doesn't have the capability in a PR KVM guest or in a nested HV
> guest.
OK. I will work on adding a KVM_CAP_PPC_NESTED_IRQ_HV capability
for future use.
>>> However, we could be using PR KVM (either in a bare-metal host or in a
>>> guest), or we could be doing nested HV KVM where we are using the
>>> kvm_hv module inside a KVM guest and using special hypercalls for
>>> controlling our guests.
>>
>> Yes.
>>
>> It would be good to talk a little about the nested support (offline
>> may be) to make sure that we are not missing some major interface that
>> would require a lot of change. If we need to prepare ground, I think
>> the timing is good.
>>
>> The size of the IRQ number space might be a problem. It seems we
>> would need to increase it considerably to support multiple nested
>> guests. That said I haven't look much how nested is designed.
>
> The current design of nested HV is that the entire non-volatile state
> of all the nested guests is encapsulated within the state and
> resources of the L1 hypervisor. That means that if the L1 hypervisor
> gets migrated, all of its guests go across inside it and there is no
> extra state that L0 needs to be aware of. That would imply that the
> VP number space for the nested guests would need to come from within
> the VP number space for L1; but the amount of VP space we allocate to
> each guest doesn't seem to be large enough for that to be practical.
If the KVM XIVE device had some information on the max number of CPUs
provisioned for the guest, we could optimize the VP allocation.
That might be a larger KVM topic though. There are some static limits
on the number of CPUs in QEMU and in KVM, which have no relation AFAICT.
Thanks,
C.
^ permalink raw reply
* Re: [PATCH v2] perf mem/c2c: Fix perf_mem_events to support powerpc
From: Arnaldo Carvalho de Melo @ 2019-01-29 14:17 UTC (permalink / raw)
To: Jiri Olsa
Cc: dzickus, Ravi Bangoria, maddy, linux-kernel, fowles, jmario,
namhyung, linuxppc-dev
In-Reply-To: <20190129134236.GB30190@krava>
Em Tue, Jan 29, 2019 at 02:42:36PM +0100, Jiri Olsa escreveu:
> On Tue, Jan 29, 2019 at 06:54:12PM +0530, Ravi Bangoria wrote:
> > Powerpc hw does not have inbuilt latency filter (--ldlat) for mem-load
> > event and, perf_mem_events by default includes ldlat=30 which is
> > causing failure on powerpc. Refactor code to support perf mem/c2c on
> > powerpc.
> >
> > This patch depends on kernel side changes done my Madhavan:
> > https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-December/182596.html
> >
> > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > ---
>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
Applied to perf/urgent, as soon as the kernel bits are there tooling
will be ready.
- Arnaldo
> thanks,
> jirka
>
> > tools/perf/Documentation/perf-c2c.txt | 16 ++++++++++++----
> > tools/perf/Documentation/perf-mem.txt | 2 +-
> > tools/perf/arch/powerpc/util/Build | 1 +
> > tools/perf/arch/powerpc/util/mem-events.c | 11 +++++++++++
> > tools/perf/util/mem-events.c | 2 +-
> > 5 files changed, 26 insertions(+), 6 deletions(-)
> > create mode 100644 tools/perf/arch/powerpc/util/mem-events.c
> >
> > diff --git a/tools/perf/Documentation/perf-c2c.txt b/tools/perf/Documentation/perf-c2c.txt
> > index 095aebd..e6150f2 100644
> > --- a/tools/perf/Documentation/perf-c2c.txt
> > +++ b/tools/perf/Documentation/perf-c2c.txt
> > @@ -19,8 +19,11 @@ C2C stands for Cache To Cache.
> > The perf c2c tool provides means for Shared Data C2C/HITM analysis. It allows
> > you to track down the cacheline contentions.
> >
> > -The tool is based on x86's load latency and precise store facility events
> > -provided by Intel CPUs. These events provide:
> > +On x86, the tool is based on load latency and precise store facility events
> > +provided by Intel CPUs. On PowerPC, the tool uses random instruction sampling
> > +with thresholding feature.
> > +
> > +These events provide:
> > - memory address of the access
> > - type of the access (load and store details)
> > - latency (in cycles) of the load access
> > @@ -46,7 +49,7 @@ RECORD OPTIONS
> >
> > -l::
> > --ldlat::
> > - Configure mem-loads latency.
> > + Configure mem-loads latency. (x86 only)
> >
> > -k::
> > --all-kernel::
> > @@ -119,11 +122,16 @@ Following perf record options are configured by default:
> > -W,-d,--phys-data,--sample-cpu
> >
> > Unless specified otherwise with '-e' option, following events are monitored by
> > -default:
> > +default on x86:
> >
> > cpu/mem-loads,ldlat=30/P
> > cpu/mem-stores/P
> >
> > +and following on PowerPC:
> > +
> > + cpu/mem-loads/
> > + cpu/mem-stores/
> > +
> > User can pass any 'perf record' option behind '--' mark, like (to enable
> > callchains and system wide monitoring):
> >
> > diff --git a/tools/perf/Documentation/perf-mem.txt b/tools/perf/Documentation/perf-mem.txt
> > index f8d2167..199ea0f 100644
> > --- a/tools/perf/Documentation/perf-mem.txt
> > +++ b/tools/perf/Documentation/perf-mem.txt
> > @@ -82,7 +82,7 @@ RECORD OPTIONS
> > Be more verbose (show counter open errors, etc)
> >
> > --ldlat <n>::
> > - Specify desired latency for loads event.
> > + Specify desired latency for loads event. (x86 only)
> >
> > In addition, for report all perf report options are valid, and for record
> > all perf record options.
> > diff --git a/tools/perf/arch/powerpc/util/Build b/tools/perf/arch/powerpc/util/Build
> > index 2e659531..ba98bd0 100644
> > --- a/tools/perf/arch/powerpc/util/Build
> > +++ b/tools/perf/arch/powerpc/util/Build
> > @@ -2,6 +2,7 @@ libperf-y += header.o
> > libperf-y += sym-handling.o
> > libperf-y += kvm-stat.o
> > libperf-y += perf_regs.o
> > +libperf-y += mem-events.o
> >
> > libperf-$(CONFIG_DWARF) += dwarf-regs.o
> > libperf-$(CONFIG_DWARF) += skip-callchain-idx.o
> > diff --git a/tools/perf/arch/powerpc/util/mem-events.c b/tools/perf/arch/powerpc/util/mem-events.c
> > new file mode 100644
> > index 0000000..f1194fc
> > --- /dev/null
> > +++ b/tools/perf/arch/powerpc/util/mem-events.c
> > @@ -0,0 +1,11 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "mem-events.h"
> > +
> > +/* PowerPC does not support 'ldlat' parameter. */
> > +char *perf_mem_events__name(int i)
> > +{
> > + if (i == PERF_MEM_EVENTS__LOAD)
> > + return (char *) "cpu/mem-loads/";
> > +
> > + return (char *) "cpu/mem-stores/";
> > +}
> > diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
> > index 93f74d8..42c3e5a 100644
> > --- a/tools/perf/util/mem-events.c
> > +++ b/tools/perf/util/mem-events.c
> > @@ -28,7 +28,7 @@ struct perf_mem_event perf_mem_events[PERF_MEM_EVENTS__MAX] = {
> > static char mem_loads_name[100];
> > static bool mem_loads_name__init;
> >
> > -char *perf_mem_events__name(int i)
> > +char * __weak perf_mem_events__name(int i)
> > {
> > if (i == PERF_MEM_EVENTS__LOAD) {
> > if (!mem_loads_name__init) {
> > --
> > 1.8.3.1
> >
--
- Arnaldo
^ permalink raw reply
* Re: [PATCH 18/19] KVM: PPC: Book3S HV: add passthrough support
From: Cédric Le Goater @ 2019-01-29 13:47 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm, kvm-ppc, linuxppc-dev, David Gibson
In-Reply-To: <20190129024503.GA11368@blackberry>
On 1/29/19 3:45 AM, Paul Mackerras wrote:
> On Mon, Jan 28, 2019 at 07:26:00PM +0100, Cédric Le Goater wrote:
>> On 1/28/19 7:13 AM, Paul Mackerras wrote:
>>> Would we end up with too many VMAs if we just used mmap() to
>>> change the mappings from the software-generated pages to the
>>> hardware-generated interrupt pages?
>> The sPAPR IRQ number space is 0x8000 wide now. The first 4K are
>> dedicated to CPU IPIs and the remaining 4K are for devices. We can
>
> Confused. You say the number space has 32768 entries but then imply
> there are only 8K entries. Do you mean that the API allows for 15-bit
> IRQ numbers but we are only making using of 8192 of them?
Ouch. My bad. Let's do it again.
The sPAPR IRQ number space is 0x2000 wide :
https://git.qemu.org/?p=qemu.git;a=blob;f=hw/ppc/spapr_irq.c;h=1da7a32348fced0bd638717022fc37a83fc5e279;hb=HEAD#l396
The first 4K are dedicated to the CPU IPIs and the remaining 4K are for
devices (which can be extended if needed).
So that's 8192 x 2 ESB pages.
>> extend the last range if needed as these are for MSIs. Dynamic
>> extensions under KVM should work also.
>>
>> This to say that we have with 8K x 2 (trigger+EOI) pages. This is a
>> lot of mmap(), too much. Also the KVM model needs to be compatible
>
> I wasn't suggesting an mmap per IRQ, I meant that the bulk of the
> space would be covered by a single mmap, overlaid by subsequent mmaps
> where we need to map real device interrupts.
ok. The same fault handler could be used to populate the VMA with the
ESB pages.
But it would mean extra work on the QEMU side, which is not needed
with this patch.
>> with the QEMU emulated one and it was simpler to have one overall
>> memory region for the IPI ESBs, one for the END ESBs (if we support
>> that one day) and one for the TIMA.
>>
>>> Are the necessary pages for a PCI
>>> passthrough device contiguous in both host real space
>>
>> They should as they are the PHB4 ESBs.
>>
>>> and guest real space ?
>>
>> also. That's how we organized the mapping.
>
> "How we organized the mapping" is a significant design decision that I
> haven't seen documented anywhere, and is really needed for
> understanding what's going on.
OK. I will add comments on that. See below for some description.
There is nothing fancy, it's simply indexed with the interrupt number,
like for HW, or for the QEMU XIVE emulated model.
>>> If so we'd only need one mmap() for all the device's interrupt
>>> pages.
>>
>> Ah. So we would want to make a special case for the passthrough
>> device and have a mmap() and a memory region for its ESBs. Hmm.
>>
>> Wouldn't that require to hot plug a memory region under the guest ?
>
> No; the way that a memory region works is that userspace can do
> whatever disparate mappings it likes within the region on the user
> process side, and the corresponding region of guest real address space
> follows that automatically.
yes. I suppose this should work also for 'ram device' memory mappings.
So when the passthrough device is added to the guest, we would add a
new 'ram device' memory region for the device interrupt ESB pages
that would overlap with the initial guest ESB pages.
This is really a different approach.
>> which means that we need to provision an address space/container
>> region for theses regions. What are the benefits ?
>>
>> Is clearing the PTEs and repopulating the VMA unsafe ?
>
> Explicitly unmapping parts of the VMA seems like the wrong way to do
> it. If you have a device mmap where the device wants to change the
> physical page underlying parts of the mapping, there should be a way
> for it to do that explicitly (but I don't know off the top of my head
> what the interface to do that is).
>
> However, I still haven't seen a clear and concise explanation of what
> is being changed, when, and why we need to do that.
Yes. I agree on that. The problem is not very different from what we
have today with the XICS-over-XIVE glue in KVM. Let me try to explain.
The KVM XICS-over-XIVE device and the proposed KVM XIVE native device
implement an IRQ space for the guest using the generic IPI interrupts
of the XIVE IC controller. These interrupts are allocated at the OPAL
level and "mapped" into the guest IRQ number space in the range 0-0x1FFF.
Interrupt management is performed in the XIVE way: using loads and
stores on the addresses of the XIVE IPI interrupt ESB pages.
Both KVM devices share the same internal structure caching information
on the interrupts, among which the xive_irq_data struct containing the
addresses of the IPI ESB pages and an extra one in case of passthrough.
The later contains the addresses of the ESB pages of the underlying HW
controller interrupts, PHB4 in all cases for now.
A guest when running in the XICS legacy interrupt mode lets the KVM
XICS-over-XIVE device "handle" interrupt management, that is to perform
the loads and stores on the addresses of the ESB pages of the guest
interrupts.
However, when running in XIVE native exploitation mode, the KVM XIVE
native device exposes the interrupt ESB pages to the guest and lets
the guest perform directly the loads and stores.
The VMA exposing the ESB pages make use of a custom VM fault handler
which role is to populate the VMA with appropriate pages. When a fault
occurs, the guest IRQ number is deduced from the offset, and the ESB
pages of associated XIVE IPI interrupt are inserted in the VMA (using
the internal structure caching information on the interrupts).
Supporting device passthrough in the guest running in XIVE native
exploitation mode adds some extra refinements because the ESB pages
of a different HW controller (PHB4) need to be exposed to the guest
along with the initial IPI ESB pages of the XIVE IC controller. But
the overall mechanic is the same.
When the device HW irqs are mapped into or unmapped from the guest
IRQ number space, the passthru_irq helpers, kvmppc_xive_set_mapped()
and kvmppc_xive_clr_mapped(), are called to record or clear the
passthrough interrupt information and to perform the switch.
The approach taken by this patch is to clear the ESB pages of the
guest IRQ number being mapped and let the VM fault handler repopulate.
The handler will insert the ESB page corresponding to the HW interrupt
of the device being passed-through or the initial IPI ESB page if the
device is being removed.
Thanks,
C.
^ permalink raw reply
* Re: [PATCH 18/19] KVM: PPC: Book3S HV: add passthrough support
From: Cédric Le Goater @ 2019-01-29 13:46 UTC (permalink / raw)
To: Paul Mackerras, Benjamin Herrenschmidt
Cc: linuxppc-dev, kvm, kvm-ppc, David Gibson
In-Reply-To: <20190128044354.GB3237@blackberry>
On 1/28/19 5:43 AM, Paul Mackerras wrote:
> On Thu, Jan 24, 2019 at 08:25:15AM +1100, Benjamin Herrenschmidt wrote:
>> On Wed, 2019-01-23 at 21:30 +1100, Paul Mackerras wrote:
>>>> Afaik bcs we change the mapping to point to the real HW irq ESB page
>>>> instead of the "IPI" that was there at VM init time.
>>>
>>> So that makes it sound like there is a whole lot going on that hasn't
>>> even been hinted at in the patch descriptions... It sounds like we
>>> need a good description of how all this works and fits together
>>> somewhere under Documentation/.
>>>
>>> In any case we need much more informative patch descriptions. I
>>> realize that it's all currently in Cedric's head, but I bet that in
>>> two or three years' time when we come to try to debug something, it
>>> won't be in anyone's head...
>>
>> The main problem is understanding XIVE itself. It's not realistic to
>> ask Cedric to write a proper documentation for XIVE as part of the
>> patch series, but sadly IBM doesn't have a good one to provide either.
>
> There are: (a) the XIVE hardware, (b) the definition of the XIVE
> hypercalls that guests use, and (c) the design decisions around how to
> implement that hypercall interface. We need to get (b) published
> somehow, but it is mostly (c) that I would expect the patch
> descriptions to explain.
>
> It sounds like there will be a mapping to userspace where the pages
> can sometimes point to an IPI page and sometimes point to a real HW
> irq ESB page.
Just to be clear. In both cases, these pages are real HW ESB pages.
They are just attached to a different controller : the XIVE IC for
the IPIs and the PHB4 for the others.
> That is, the same guest "hardware" irq number sometimes
> refers to a software-generated interrupt (what you called an "IPI"
> above) and sometimes to a hardware-generated interrupt. That fact,> the reason why it is so and the consequences all need to be explained
> somewhere. They are really not obvious and I don't believe they are
> part of either the XIVE hardware spec or the XIVE hypercall spec.
I tried to put the reasons behind the current approach in another
thread, not saying this is the correct one.
Thanks,
C.
^ permalink raw reply
* Re: [PATCH v2] perf mem/c2c: Fix perf_mem_events to support powerpc
From: Jiri Olsa @ 2019-01-29 13:42 UTC (permalink / raw)
To: Ravi Bangoria
Cc: dzickus, maddy, linux-kernel, acme, fowles, jmario, namhyung,
linuxppc-dev
In-Reply-To: <20190129132412.771-1-ravi.bangoria@linux.ibm.com>
On Tue, Jan 29, 2019 at 06:54:12PM +0530, Ravi Bangoria wrote:
> Powerpc hw does not have inbuilt latency filter (--ldlat) for mem-load
> event and, perf_mem_events by default includes ldlat=30 which is
> causing failure on powerpc. Refactor code to support perf mem/c2c on
> powerpc.
>
> This patch depends on kernel side changes done my Madhavan:
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-December/182596.html
>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
Acked-by: Jiri Olsa <jolsa@kernel.org>
thanks,
jirka
> tools/perf/Documentation/perf-c2c.txt | 16 ++++++++++++----
> tools/perf/Documentation/perf-mem.txt | 2 +-
> tools/perf/arch/powerpc/util/Build | 1 +
> tools/perf/arch/powerpc/util/mem-events.c | 11 +++++++++++
> tools/perf/util/mem-events.c | 2 +-
> 5 files changed, 26 insertions(+), 6 deletions(-)
> create mode 100644 tools/perf/arch/powerpc/util/mem-events.c
>
> diff --git a/tools/perf/Documentation/perf-c2c.txt b/tools/perf/Documentation/perf-c2c.txt
> index 095aebd..e6150f2 100644
> --- a/tools/perf/Documentation/perf-c2c.txt
> +++ b/tools/perf/Documentation/perf-c2c.txt
> @@ -19,8 +19,11 @@ C2C stands for Cache To Cache.
> The perf c2c tool provides means for Shared Data C2C/HITM analysis. It allows
> you to track down the cacheline contentions.
>
> -The tool is based on x86's load latency and precise store facility events
> -provided by Intel CPUs. These events provide:
> +On x86, the tool is based on load latency and precise store facility events
> +provided by Intel CPUs. On PowerPC, the tool uses random instruction sampling
> +with thresholding feature.
> +
> +These events provide:
> - memory address of the access
> - type of the access (load and store details)
> - latency (in cycles) of the load access
> @@ -46,7 +49,7 @@ RECORD OPTIONS
>
> -l::
> --ldlat::
> - Configure mem-loads latency.
> + Configure mem-loads latency. (x86 only)
>
> -k::
> --all-kernel::
> @@ -119,11 +122,16 @@ Following perf record options are configured by default:
> -W,-d,--phys-data,--sample-cpu
>
> Unless specified otherwise with '-e' option, following events are monitored by
> -default:
> +default on x86:
>
> cpu/mem-loads,ldlat=30/P
> cpu/mem-stores/P
>
> +and following on PowerPC:
> +
> + cpu/mem-loads/
> + cpu/mem-stores/
> +
> User can pass any 'perf record' option behind '--' mark, like (to enable
> callchains and system wide monitoring):
>
> diff --git a/tools/perf/Documentation/perf-mem.txt b/tools/perf/Documentation/perf-mem.txt
> index f8d2167..199ea0f 100644
> --- a/tools/perf/Documentation/perf-mem.txt
> +++ b/tools/perf/Documentation/perf-mem.txt
> @@ -82,7 +82,7 @@ RECORD OPTIONS
> Be more verbose (show counter open errors, etc)
>
> --ldlat <n>::
> - Specify desired latency for loads event.
> + Specify desired latency for loads event. (x86 only)
>
> In addition, for report all perf report options are valid, and for record
> all perf record options.
> diff --git a/tools/perf/arch/powerpc/util/Build b/tools/perf/arch/powerpc/util/Build
> index 2e659531..ba98bd0 100644
> --- a/tools/perf/arch/powerpc/util/Build
> +++ b/tools/perf/arch/powerpc/util/Build
> @@ -2,6 +2,7 @@ libperf-y += header.o
> libperf-y += sym-handling.o
> libperf-y += kvm-stat.o
> libperf-y += perf_regs.o
> +libperf-y += mem-events.o
>
> libperf-$(CONFIG_DWARF) += dwarf-regs.o
> libperf-$(CONFIG_DWARF) += skip-callchain-idx.o
> diff --git a/tools/perf/arch/powerpc/util/mem-events.c b/tools/perf/arch/powerpc/util/mem-events.c
> new file mode 100644
> index 0000000..f1194fc
> --- /dev/null
> +++ b/tools/perf/arch/powerpc/util/mem-events.c
> @@ -0,0 +1,11 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "mem-events.h"
> +
> +/* PowerPC does not support 'ldlat' parameter. */
> +char *perf_mem_events__name(int i)
> +{
> + if (i == PERF_MEM_EVENTS__LOAD)
> + return (char *) "cpu/mem-loads/";
> +
> + return (char *) "cpu/mem-stores/";
> +}
> diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
> index 93f74d8..42c3e5a 100644
> --- a/tools/perf/util/mem-events.c
> +++ b/tools/perf/util/mem-events.c
> @@ -28,7 +28,7 @@ struct perf_mem_event perf_mem_events[PERF_MEM_EVENTS__MAX] = {
> static char mem_loads_name[100];
> static bool mem_loads_name__init;
>
> -char *perf_mem_events__name(int i)
> +char * __weak perf_mem_events__name(int i)
> {
> if (i == PERF_MEM_EVENTS__LOAD) {
> if (!mem_loads_name__init) {
> --
> 1.8.3.1
>
^ permalink raw reply
* [PATCH v2] perf mem/c2c: Fix perf_mem_events to support powerpc
From: Ravi Bangoria @ 2019-01-29 13:24 UTC (permalink / raw)
To: acme, jolsa
Cc: dzickus, Ravi Bangoria, maddy, fowles, linux-kernel, jmario,
namhyung, linuxppc-dev
In-Reply-To: <dd6d04f3-c8aa-1d57-c466-c6b3c333db51@linux.ibm.com>
Powerpc hw does not have inbuilt latency filter (--ldlat) for mem-load
event and, perf_mem_events by default includes ldlat=30 which is
causing failure on powerpc. Refactor code to support perf mem/c2c on
powerpc.
This patch depends on kernel side changes done my Madhavan:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-December/182596.html
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
tools/perf/Documentation/perf-c2c.txt | 16 ++++++++++++----
tools/perf/Documentation/perf-mem.txt | 2 +-
tools/perf/arch/powerpc/util/Build | 1 +
tools/perf/arch/powerpc/util/mem-events.c | 11 +++++++++++
tools/perf/util/mem-events.c | 2 +-
5 files changed, 26 insertions(+), 6 deletions(-)
create mode 100644 tools/perf/arch/powerpc/util/mem-events.c
diff --git a/tools/perf/Documentation/perf-c2c.txt b/tools/perf/Documentation/perf-c2c.txt
index 095aebd..e6150f2 100644
--- a/tools/perf/Documentation/perf-c2c.txt
+++ b/tools/perf/Documentation/perf-c2c.txt
@@ -19,8 +19,11 @@ C2C stands for Cache To Cache.
The perf c2c tool provides means for Shared Data C2C/HITM analysis. It allows
you to track down the cacheline contentions.
-The tool is based on x86's load latency and precise store facility events
-provided by Intel CPUs. These events provide:
+On x86, the tool is based on load latency and precise store facility events
+provided by Intel CPUs. On PowerPC, the tool uses random instruction sampling
+with thresholding feature.
+
+These events provide:
- memory address of the access
- type of the access (load and store details)
- latency (in cycles) of the load access
@@ -46,7 +49,7 @@ RECORD OPTIONS
-l::
--ldlat::
- Configure mem-loads latency.
+ Configure mem-loads latency. (x86 only)
-k::
--all-kernel::
@@ -119,11 +122,16 @@ Following perf record options are configured by default:
-W,-d,--phys-data,--sample-cpu
Unless specified otherwise with '-e' option, following events are monitored by
-default:
+default on x86:
cpu/mem-loads,ldlat=30/P
cpu/mem-stores/P
+and following on PowerPC:
+
+ cpu/mem-loads/
+ cpu/mem-stores/
+
User can pass any 'perf record' option behind '--' mark, like (to enable
callchains and system wide monitoring):
diff --git a/tools/perf/Documentation/perf-mem.txt b/tools/perf/Documentation/perf-mem.txt
index f8d2167..199ea0f 100644
--- a/tools/perf/Documentation/perf-mem.txt
+++ b/tools/perf/Documentation/perf-mem.txt
@@ -82,7 +82,7 @@ RECORD OPTIONS
Be more verbose (show counter open errors, etc)
--ldlat <n>::
- Specify desired latency for loads event.
+ Specify desired latency for loads event. (x86 only)
In addition, for report all perf report options are valid, and for record
all perf record options.
diff --git a/tools/perf/arch/powerpc/util/Build b/tools/perf/arch/powerpc/util/Build
index 2e659531..ba98bd0 100644
--- a/tools/perf/arch/powerpc/util/Build
+++ b/tools/perf/arch/powerpc/util/Build
@@ -2,6 +2,7 @@ libperf-y += header.o
libperf-y += sym-handling.o
libperf-y += kvm-stat.o
libperf-y += perf_regs.o
+libperf-y += mem-events.o
libperf-$(CONFIG_DWARF) += dwarf-regs.o
libperf-$(CONFIG_DWARF) += skip-callchain-idx.o
diff --git a/tools/perf/arch/powerpc/util/mem-events.c b/tools/perf/arch/powerpc/util/mem-events.c
new file mode 100644
index 0000000..f1194fc
--- /dev/null
+++ b/tools/perf/arch/powerpc/util/mem-events.c
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "mem-events.h"
+
+/* PowerPC does not support 'ldlat' parameter. */
+char *perf_mem_events__name(int i)
+{
+ if (i == PERF_MEM_EVENTS__LOAD)
+ return (char *) "cpu/mem-loads/";
+
+ return (char *) "cpu/mem-stores/";
+}
diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
index 93f74d8..42c3e5a 100644
--- a/tools/perf/util/mem-events.c
+++ b/tools/perf/util/mem-events.c
@@ -28,7 +28,7 @@ struct perf_mem_event perf_mem_events[PERF_MEM_EVENTS__MAX] = {
static char mem_loads_name[100];
static bool mem_loads_name__init;
-char *perf_mem_events__name(int i)
+char * __weak perf_mem_events__name(int i)
{
if (i == PERF_MEM_EVENTS__LOAD) {
if (!mem_loads_name__init) {
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCHv5 1/4] dt-bindings: add DT binding for the layerscape PCIe controller with EP mode
From: Lorenzo Pieralisi @ 2019-01-29 12:06 UTC (permalink / raw)
To: Xiaowei Bao, robh+dt
Cc: mark.rutland, roy.zang, linuxppc-dev, arnd, devicetree, gregkh,
kstewart, niklas.cassel, linux-pci, linux-kernel, kishon,
minghuan.Lian, cyrille.pitchen, linux-arm-kernel, pombredanne,
bhelgaas, leoyang.li, shawnguo, shawn.lin, mingkai.hu
In-Reply-To: <20190121094500.10657-1-xiaowei.bao@nxp.com>
Rob,
Is it OK for you if I pull this series into the pci tree ?
Please let me know, thanks.
Lorenzo
On Mon, Jan 21, 2019 at 05:44:57PM +0800, Xiaowei Bao wrote:
> Add the documentation for the Device Tree binding for the layerscape PCIe
> controller with EP mode.
>
> Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com>
> Reviewed-by: Minghuan Lian <minghuan.lian@nxp.com>
> Reviewed-by: Zhiqiang Hou <zhiqiang.hou@nxp.com>
> ---
> v2:
> - Add the SoC specific compatibles.
> v3:
> - modify the commit message.
> v4:
> - no change.
> v5:
> - no change.
>
> .../devicetree/bindings/pci/layerscape-pci.txt | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/layerscape-pci.txt b/Documentation/devicetree/bindings/pci/layerscape-pci.txt
> index 9b2b8d6..e20ceaa 100644
> --- a/Documentation/devicetree/bindings/pci/layerscape-pci.txt
> +++ b/Documentation/devicetree/bindings/pci/layerscape-pci.txt
> @@ -13,6 +13,7 @@ information.
>
> Required properties:
> - compatible: should contain the platform identifier such as:
> + RC mode:
> "fsl,ls1021a-pcie"
> "fsl,ls2080a-pcie", "fsl,ls2085a-pcie"
> "fsl,ls2088a-pcie"
> @@ -20,6 +21,8 @@ Required properties:
> "fsl,ls1046a-pcie"
> "fsl,ls1043a-pcie"
> "fsl,ls1012a-pcie"
> + EP mode:
> + "fsl,ls1046a-pcie-ep", "fsl,ls-pcie-ep"
> - reg: base addresses and lengths of the PCIe controller register blocks.
> - interrupts: A list of interrupt outputs of the controller. Must contain an
> entry for each entry in the interrupt-names property.
> --
> 1.7.1
>
^ permalink raw reply
* Re: [PATCH v3] cxl: Wrap iterations over afu slices inside 'afu_list_lock'
From: Vaibhav Jain @ 2019-01-29 11:08 UTC (permalink / raw)
To: Andrew Donnellan, linuxppc-dev, Frederic Barrat
Cc: Philippe Bergheaud, Alastair D'Silva, Christophe Lombard
In-Reply-To: <70f4edca-8a05-df9c-b45e-a8576626d4fd@au1.ibm.com>
Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:
> On 29/1/19 5:15 pm, Vaibhav Jain wrote:
>> Within cxl module, iteration over array 'adapter->slices' may be racy
>
> adapter->slices isn't an array, adapter->afu is the array.
Thanks for catching this. Have fixed the patch description in the resent patch.
>
> Does this need to go to stable? (I'm guessing we've been hitting actual
> Oopses?)
Re-send patch marked to stable.
> Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
Thanks !!
--
Vaibhav Jain <vaibhav@linux.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.
^ permalink raw reply
* [RESEND PATCH v3] cxl: Wrap iterations over afu slices inside 'afu_list_lock'
From: Vaibhav Jain @ 2019-01-29 11:06 UTC (permalink / raw)
To: linuxppc-dev, Frederic Barrat
Cc: Philippe Bergheaud, Christophe Lombard, stable,
Alastair D'Silva, Andrew Donnellan, Vaibhav Jain,
Christophe Lombard
Within cxl module, iteration over array 'adapter->afu' may be racy
at few points as it might be simultaneously read during an EEH and its
contents being set to NULL while driver is being unloaded or unbound
from the adapter. This might result in a NULL pointer to 'struct afu'
being de-referenced during an EEH thereby causing a kernel oops.
This patch fixes this by making sure that all access to the array
'adapter->afu' is wrapped within the context of spin-lock
'adapter->afu_list_lock'.
Cc: stable@vger.kernel.org
Fixes: 9e8df8a2196("cxl: EEH support")
Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
Acked-by: Frederic Barrat <fbarrat@linux.ibm.com>
Acked-by: Christophe Lombard <clombard@linux.vnet.ibm.com>
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Changelog:
Resend:
* Fixed the reference to 'adapter->afu' in patch description. [Andrew]
* Added the 'Fixes' tag and marked the patch to stable
v3:
* Updated a slice loop in cxl_pci_error_detectected() to ignore NULL
slices [Fred]
* Added a NULL AFU check in cxl_pci_slot_reset() [Fred]
v2:
* Fixed a wrong comparison of non-null pointer [Fred]
* Moved a call to cxl_vphb_error_detected() within a branch that
checks for not null AFU pointer in 'adapter->slices' [Fred]
* Removed a misleading comment in code.
---
drivers/misc/cxl/guest.c | 2 ++
drivers/misc/cxl/pci.c | 39 ++++++++++++++++++++++++++++++---------
2 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
index 5d28d9e454f5..08f4a512afad 100644
--- a/drivers/misc/cxl/guest.c
+++ b/drivers/misc/cxl/guest.c
@@ -267,6 +267,7 @@ static int guest_reset(struct cxl *adapter)
int i, rc;
pr_devel("Adapter reset request\n");
+ spin_lock(&adapter->afu_list_lock);
for (i = 0; i < adapter->slices; i++) {
if ((afu = adapter->afu[i])) {
pci_error_handlers(afu, CXL_ERROR_DETECTED_EVENT,
@@ -283,6 +284,7 @@ static int guest_reset(struct cxl *adapter)
pci_error_handlers(afu, CXL_RESUME_EVENT, 0);
}
}
+ spin_unlock(&adapter->afu_list_lock);
return rc;
}
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index c79ba1c699ad..300531d6136f 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -1805,7 +1805,7 @@ static pci_ers_result_t cxl_vphb_error_detected(struct cxl_afu *afu,
/* There should only be one entry, but go through the list
* anyway
*/
- if (afu->phb == NULL)
+ if (afu == NULL || afu->phb == NULL)
return result;
list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) {
@@ -1832,7 +1832,8 @@ static pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
{
struct cxl *adapter = pci_get_drvdata(pdev);
struct cxl_afu *afu;
- pci_ers_result_t result = PCI_ERS_RESULT_NEED_RESET, afu_result;
+ pci_ers_result_t result = PCI_ERS_RESULT_NEED_RESET;
+ pci_ers_result_t afu_result = PCI_ERS_RESULT_NEED_RESET;
int i;
/* At this point, we could still have an interrupt pending.
@@ -1843,6 +1844,7 @@ static pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
/* If we're permanently dead, give up. */
if (state == pci_channel_io_perm_failure) {
+ spin_lock(&adapter->afu_list_lock);
for (i = 0; i < adapter->slices; i++) {
afu = adapter->afu[i];
/*
@@ -1851,6 +1853,7 @@ static pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
*/
cxl_vphb_error_detected(afu, state);
}
+ spin_unlock(&adapter->afu_list_lock);
return PCI_ERS_RESULT_DISCONNECT;
}
@@ -1932,11 +1935,17 @@ static pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
* * In slot_reset, free the old resources and allocate new ones.
* * In resume, clear the flag to allow things to start.
*/
+
+ /* Make sure no one else changes the afu list */
+ spin_lock(&adapter->afu_list_lock);
+
for (i = 0; i < adapter->slices; i++) {
afu = adapter->afu[i];
- afu_result = cxl_vphb_error_detected(afu, state);
+ if (afu == NULL)
+ continue;
+ afu_result = cxl_vphb_error_detected(afu, state);
cxl_context_detach_all(afu);
cxl_ops->afu_deactivate_mode(afu, afu->current_mode);
pci_deconfigure_afu(afu);
@@ -1948,6 +1957,7 @@ static pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
(result == PCI_ERS_RESULT_NEED_RESET))
result = PCI_ERS_RESULT_NONE;
}
+ spin_unlock(&adapter->afu_list_lock);
/* should take the context lock here */
if (cxl_adapter_context_lock(adapter) != 0)
@@ -1980,14 +1990,18 @@ static pci_ers_result_t cxl_pci_slot_reset(struct pci_dev *pdev)
*/
cxl_adapter_context_unlock(adapter);
+ spin_lock(&adapter->afu_list_lock);
for (i = 0; i < adapter->slices; i++) {
afu = adapter->afu[i];
+ if (afu == NULL)
+ continue;
+
if (pci_configure_afu(afu, adapter, pdev))
- goto err;
+ goto err_unlock;
if (cxl_afu_select_best_mode(afu))
- goto err;
+ goto err_unlock;
if (afu->phb == NULL)
continue;
@@ -1999,16 +2013,16 @@ static pci_ers_result_t cxl_pci_slot_reset(struct pci_dev *pdev)
ctx = cxl_get_context(afu_dev);
if (ctx && cxl_release_context(ctx))
- goto err;
+ goto err_unlock;
ctx = cxl_dev_context_init(afu_dev);
if (IS_ERR(ctx))
- goto err;
+ goto err_unlock;
afu_dev->dev.archdata.cxl_ctx = ctx;
if (cxl_ops->afu_check_and_enable(afu))
- goto err;
+ goto err_unlock;
afu_dev->error_state = pci_channel_io_normal;
@@ -2029,8 +2043,13 @@ static pci_ers_result_t cxl_pci_slot_reset(struct pci_dev *pdev)
result = PCI_ERS_RESULT_DISCONNECT;
}
}
+
+ spin_unlock(&adapter->afu_list_lock);
return result;
+err_unlock:
+ spin_unlock(&adapter->afu_list_lock);
+
err:
/* All the bits that happen in both error_detected and cxl_remove
* should be idempotent, so we don't need to worry about leaving a mix
@@ -2051,10 +2070,11 @@ static void cxl_pci_resume(struct pci_dev *pdev)
* This is not the place to be checking if everything came back up
* properly, because there's no return value: do that in slot_reset.
*/
+ spin_lock(&adapter->afu_list_lock);
for (i = 0; i < adapter->slices; i++) {
afu = adapter->afu[i];
- if (afu->phb == NULL)
+ if (afu == NULL || afu->phb == NULL)
continue;
list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) {
@@ -2063,6 +2083,7 @@ static void cxl_pci_resume(struct pci_dev *pdev)
afu_dev->driver->err_handler->resume(afu_dev);
}
}
+ spin_unlock(&adapter->afu_list_lock);
}
static const struct pci_error_handlers cxl_err_handler = {
--
2.20.1
^ permalink raw reply related
* Re: [PATCH V5 0/5] NestMMU pte upgrade workaround for mprotect
From: Aneesh Kumar K.V @ 2019-01-29 10:43 UTC (permalink / raw)
To: npiggin, benh, paulus, mpe, akpm, x86; +Cc: linux-mm, linuxppc-dev
In-Reply-To: <20190116085035.29729-1-aneesh.kumar@linux.ibm.com>
Andrew,
How do you want to merge this? Michael Ellerman suggests this should go
via -mm tree.
-aneesh
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> We can upgrade pte access (R -> RW transition) via mprotect. We need
> to make sure we follow the recommended pte update sequence as outlined in
> commit bd5050e38aec ("powerpc/mm/radix: Change pte relax sequence to handle nest MMU hang")
> for such updates. This patch series do that.
>
> Changes from V4:
> * Drop EXPORT_SYMBOL
>
> Changes from V3:
> * Build fix for x86
>
> Changes from V2:
> * Update commit message for patch 4
> * use radix tlb flush routines directly.
>
> Changes from V1:
> * Restrict ths only for R->RW upgrade. We don't need to do this for Autonuma
> * Restrict this only for radix translation mode.
>
>
> Aneesh Kumar K.V (5):
> mm: Update ptep_modify_prot_start/commit to take vm_area_struct as arg
> mm: update ptep_modify_prot_commit to take old pte value as arg
> arch/powerpc/mm: Nest MMU workaround for mprotect RW upgrade.
> mm/hugetlb: Add prot_modify_start/commit sequence for hugetlb update
> arch/powerpc/mm/hugetlb: NestMMU workaround for hugetlb mprotect RW
> upgrade
>
> arch/powerpc/include/asm/book3s/64/hugetlb.h | 12 ++++++++++
> arch/powerpc/include/asm/book3s/64/pgtable.h | 18 ++++++++++++++
> arch/powerpc/include/asm/book3s/64/radix.h | 4 ++++
> arch/powerpc/mm/hugetlbpage-hash64.c | 25 ++++++++++++++++++++
> arch/powerpc/mm/hugetlbpage-radix.c | 17 +++++++++++++
> arch/powerpc/mm/pgtable-book3s64.c | 25 ++++++++++++++++++++
> arch/powerpc/mm/pgtable-radix.c | 18 ++++++++++++++
> arch/s390/include/asm/pgtable.h | 5 ++--
> arch/s390/mm/pgtable.c | 8 ++++---
> arch/x86/include/asm/paravirt.h | 13 +++++-----
> arch/x86/include/asm/paravirt_types.h | 5 ++--
> arch/x86/xen/mmu.h | 4 ++--
> arch/x86/xen/mmu_pv.c | 8 +++----
> fs/proc/task_mmu.c | 8 ++++---
> include/asm-generic/pgtable.h | 18 +++++++-------
> include/linux/hugetlb.h | 20 ++++++++++++++++
> mm/hugetlb.c | 8 ++++---
> mm/memory.c | 8 +++----
> mm/mprotect.c | 6 ++---
> 19 files changed, 189 insertions(+), 41 deletions(-)
>
> --
> 2.20.1
^ permalink raw reply
* Re: [PATCH] perf mem/c2c: Fix perf_mem_events to support powerpc
From: Ravi Bangoria @ 2019-01-29 10:40 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Michael Ellerman
Cc: dzickus, Ravi Bangoria, maddy, linuxppc-dev, fowles, linux-kernel,
jmario, namhyung, jolsa
In-Reply-To: <20190129095315.GH4344@kernel.org>
On 1/29/19 3:23 PM, Arnaldo Carvalho de Melo wrote:
> I think its just a tooling side, I haven't processed it because I'm
> waiting for Ravi to address Jiri's comment, after that I'm happy to put
> it in my perf/urgent branch that I'm brewing to push to Ingo today or
> tomorrow.
Ah.. Will try to send v2 today.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 09/21] memblock: drop memblock_alloc_base()
From: Michael Ellerman @ 2019-01-29 10:29 UTC (permalink / raw)
To: Mike Rapoport, linux-mm
Cc: Rich Felker, linux-ia64, devicetree, Catalin Marinas,
Heiko Carstens, x86, linux-mips, Max Filippov, Guo Ren,
sparclinux, Christoph Hellwig, linux-s390, linux-c6x-dev,
Yoshinori Sato, Richard Weinberger, linux-sh, Russell King,
kasan-dev, Mike Rapoport, Geert Uytterhoeven, Mark Salter,
Dennis Zhou, Matt Turner, linux-snps-arc, uclinux-h8-devel,
Petr Mladek, linux-xtensa, linux-alpha, linux-um, linux-m68k,
Rob Herring, Greentime Hu, xen-devel, Stafford Horne, Guan Xuetao,
linux-arm-kernel, Michal Simek, Tony Luck, Greg Kroah-Hartman,
linux-usb, linux-kernel, Paul Burton, openrisc, Vineet Gupta,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <1548057848-15136-10-git-send-email-rppt@linux.ibm.com>
Mike Rapoport <rppt@linux.ibm.com> writes:
> The memblock_alloc_base() function tries to allocate a memory up to the
> limit specified by its max_addr parameter and panics if the allocation
> fails. Replace its usage with memblock_phys_alloc_range() and make the
> callers check the return value and panic in case of error.
>
> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> ---
> arch/powerpc/kernel/rtas.c | 6 +++++-
> arch/powerpc/mm/hash_utils_64.c | 8 ++++++--
> arch/s390/kernel/smp.c | 6 +++++-
> drivers/macintosh/smu.c | 2 +-
> include/linux/memblock.h | 2 --
> mm/memblock.c | 14 --------------
> 6 files changed, 17 insertions(+), 21 deletions(-)
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
cheers
^ permalink raw reply
* Re: [PATCH v3] cxl: Wrap iterations over afu slices inside 'afu_list_lock'
From: christophe lombard @ 2019-01-29 10:08 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev, Frederic Barrat
Cc: Philippe Bergheaud, Alastair D'Silva, Christophe Lombard,
Andrew Donnellan
In-Reply-To: <20190129061558.2122-1-vaibhav@linux.ibm.com>
On 29/01/2019 07:15, Vaibhav Jain wrote:
> Within cxl module, iteration over array 'adapter->slices' may be racy
> at few points as it might be simultaneously read during an EEH and its
> contents being set to NULL while driver is being unloaded or unbound
> from the adapter. This might result in a NULL pointer to 'struct afu'
> being de-referenced during an EEH thereby causing a kernel oops.
>
> This patch fixes this by making sure that all access to the array
> 'adapter->slices' is wrapped within the context of spin-lock
> 'adapter->afu_list_lock'.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog:
>
> v3:
> * Updated a slice loop in cxl_pci_error_detectected() to ignore NULL
> slices [Fred]
> * Added a NULL AFU check in cxl_pci_slot_reset() [Fred]
>
> v2:
> * Fixed a wrong comparison of non-null pointer [Fred]
> * Moved a call to cxl_vphb_error_detected() within a branch that
> checks for not null AFU pointer in 'adapter->slices' [Fred]
> * Removed a misleading comment in code.
> ---
Thanks
Acked-by: Christophe Lombard<clombard@linux.vnet.ibm.com>
^ permalink raw reply
* Re: [PATCH v2 06/21] memblock: memblock_phys_alloc_try_nid(): don't panic
From: Michael Ellerman @ 2019-01-29 9:58 UTC (permalink / raw)
To: Mike Rapoport, linux-mm
Cc: Rich Felker, linux-ia64, devicetree, Catalin Marinas,
Heiko Carstens, x86, linux-mips, Max Filippov, Guo Ren,
sparclinux, Christoph Hellwig, linux-s390, linux-c6x-dev,
Yoshinori Sato, Richard Weinberger, linux-sh, Russell King,
kasan-dev, Mike Rapoport, Geert Uytterhoeven, Mark Salter,
Dennis Zhou, Matt Turner, linux-snps-arc, uclinux-h8-devel,
Petr Mladek, linux-xtensa, linux-alpha, linux-um, linux-m68k,
Rob Herring, Greentime Hu, xen-devel, Stafford Horne, Guan Xuetao,
linux-arm-kernel, Michal Simek, Tony Luck, Greg Kroah-Hartman,
linux-usb, linux-kernel, Paul Burton, openrisc, Vineet Gupta,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <87y373rdll.fsf@concordia.ellerman.id.au>
Michael Ellerman <mpe@ellerman.id.au> writes:
> Mike Rapoport <rppt@linux.ibm.com> writes:
>
>> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
>> index ae34e3a..2c61ea4 100644
>> --- a/arch/arm64/mm/numa.c
>> +++ b/arch/arm64/mm/numa.c
>> @@ -237,6 +237,10 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)
>> pr_info("Initmem setup node %d [<memory-less node>]\n", nid);
>>
>> nd_pa = memblock_phys_alloc_try_nid(nd_size, SMP_CACHE_BYTES, nid);
>> + if (!nd_pa)
>> + panic("Cannot allocate %zu bytes for node %d data\n",
>> + nd_size, nid);
>> +
>> nd = __va(nd_pa);
Wrong hunk, O_o
> Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
You know what I mean though :)
cheers
^ permalink raw reply
* Re: [PATCH v2 06/21] memblock: memblock_phys_alloc_try_nid(): don't panic
From: Michael Ellerman @ 2019-01-29 9:56 UTC (permalink / raw)
To: Mike Rapoport, linux-mm
Cc: Rich Felker, linux-ia64, devicetree, Catalin Marinas,
Heiko Carstens, x86, linux-mips, Max Filippov, Guo Ren,
sparclinux, Christoph Hellwig, linux-s390, linux-c6x-dev,
Yoshinori Sato, Richard Weinberger, linux-sh, Russell King,
kasan-dev, Mike Rapoport, Geert Uytterhoeven, Mark Salter,
Dennis Zhou, Matt Turner, linux-snps-arc, uclinux-h8-devel,
Petr Mladek, linux-xtensa, linux-alpha, linux-um, linux-m68k,
Rob Herring, Greentime Hu, xen-devel, Stafford Horne, Guan Xuetao,
linux-arm-kernel, Michal Simek, Tony Luck, Greg Kroah-Hartman,
linux-usb, linux-kernel, Paul Burton, openrisc, Vineet Gupta,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <1548057848-15136-7-git-send-email-rppt@linux.ibm.com>
Mike Rapoport <rppt@linux.ibm.com> writes:
> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
> index ae34e3a..2c61ea4 100644
> --- a/arch/arm64/mm/numa.c
> +++ b/arch/arm64/mm/numa.c
> @@ -237,6 +237,10 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)
> pr_info("Initmem setup node %d [<memory-less node>]\n", nid);
>
> nd_pa = memblock_phys_alloc_try_nid(nd_size, SMP_CACHE_BYTES, nid);
> + if (!nd_pa)
> + panic("Cannot allocate %zu bytes for node %d data\n",
> + nd_size, nid);
> +
> nd = __va(nd_pa);
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
cheers
^ permalink raw reply
* Re: [PATCH] perf mem/c2c: Fix perf_mem_events to support powerpc
From: Arnaldo Carvalho de Melo @ 2019-01-29 9:53 UTC (permalink / raw)
To: Michael Ellerman
Cc: dzickus, Ravi Bangoria, maddy, linuxppc-dev, fowles, linux-kernel,
jmario, namhyung, jolsa
In-Reply-To: <874l9rsson.fsf@concordia.ellerman.id.au>
Em Tue, Jan 29, 2019 at 08:45:44PM +1100, Michael Ellerman escreveu:
> Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes:
>
> > On 1/14/19 9:44 AM, Ravi Bangoria wrote:
> >> Powerpc hw does not have inbuilt latency filter (--ldlat) for mem-load
> >> event and, perf_mem_events by default includes ldlat=30 which is
> >> causing failure on powerpc. Refactor code to support perf mem/c2c on
> >> powerpc.
> >>
> >> This patch depends on kernel side changes done my Madhavan:
> >> https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-December/182596.html
> >>
> >> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> >
> >
> > Arnaldo / Michael, Any thoughts?
>
> I haven't merged the kernel patch, I think because Maddy told me not to
> because it would break the userspace tooling :)
>
> What is the actual dependency between them? ie. should we merge the
> kernel fix first or second or what?
I think its just a tooling side, I haven't processed it because I'm
waiting for Ravi to address Jiri's comment, after that I'm happy to put
it in my perf/urgent branch that I'm brewing to push to Ingo today or
tomorrow.
- Arnaldo
^ permalink raw reply
* Re: [PATCH v2 02/21] powerpc: use memblock functions returning virtual address
From: Michael Ellerman @ 2019-01-29 9:52 UTC (permalink / raw)
To: Mike Rapoport, linux-mm
Cc: Rich Felker, linux-ia64, devicetree, Catalin Marinas,
Heiko Carstens, x86, linux-mips, Max Filippov, Guo Ren,
sparclinux, Christoph Hellwig, linux-s390, linux-c6x-dev,
Yoshinori Sato, Richard Weinberger, linux-sh, Russell King,
kasan-dev, Mike Rapoport, Geert Uytterhoeven, Mark Salter,
Dennis Zhou, Matt Turner, linux-snps-arc, uclinux-h8-devel,
Petr Mladek, linux-xtensa, linux-alpha, linux-um, linux-m68k,
Rob Herring, Greentime Hu, xen-devel, Stafford Horne, Guan Xuetao,
linux-arm-kernel, Michal Simek, Tony Luck, Greg Kroah-Hartman,
linux-usb, linux-kernel, Paul Burton, openrisc, Vineet Gupta,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <1548057848-15136-3-git-send-email-rppt@linux.ibm.com>
Mike Rapoport <rppt@linux.ibm.com> writes:
> From: Christophe Leroy <christophe.leroy@c-s.fr>
>
> Since only the virtual address of allocated blocks is used,
> lets use functions returning directly virtual address.
>
> Those functions have the advantage of also zeroing the block.
>
> [ MR:
> - updated error message in alloc_stack() to be more verbose
> - convereted several additional call sites ]
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> ---
> arch/powerpc/kernel/dt_cpu_ftrs.c | 3 +--
> arch/powerpc/kernel/irq.c | 5 -----
> arch/powerpc/kernel/paca.c | 6 +++++-
> arch/powerpc/kernel/prom.c | 5 ++++-
> arch/powerpc/kernel/setup_32.c | 26 ++++++++++++++++----------
> 5 files changed, 26 insertions(+), 19 deletions(-)
LGTM.
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
cheers
^ permalink raw reply
* Re: [PATCH] perf mem/c2c: Fix perf_mem_events to support powerpc
From: Michael Ellerman @ 2019-01-29 9:45 UTC (permalink / raw)
To: Ravi Bangoria, acme
Cc: dzickus, maddy, linuxppc-dev, fowles, linux-kernel, jmario,
namhyung, jolsa
In-Reply-To: <f308ef3e-f34b-6000-8826-27bdf8c3704e@linux.ibm.com>
Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes:
> On 1/14/19 9:44 AM, Ravi Bangoria wrote:
>> Powerpc hw does not have inbuilt latency filter (--ldlat) for mem-load
>> event and, perf_mem_events by default includes ldlat=30 which is
>> causing failure on powerpc. Refactor code to support perf mem/c2c on
>> powerpc.
>>
>> This patch depends on kernel side changes done my Madhavan:
>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-December/182596.html
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>
>
> Arnaldo / Michael, Any thoughts?
I haven't merged the kernel patch, I think because Maddy told me not to
because it would break the userspace tooling :)
What is the actual dependency between them? ie. should we merge the
kernel fix first or second or what?
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: Perform full re-add of CPU for topology update post-migration
From: Michael Ellerman @ 2019-01-29 9:37 UTC (permalink / raw)
To: Michael Bringmann, Nathan Fontenot, linuxppc-dev; +Cc: ldufour
In-Reply-To: <810d1cee-98ce-6e8f-f87b-f9acb8ba35f2@linux.vnet.ibm.com>
Michael Bringmann <mwb@linux.vnet.ibm.com> writes:
> On 10/29/18 1:43 PM, Nathan Fontenot wrote:
>> On pseries systems, performing a partition migration can result in
>> altering the nodes a CPU is assigned to on the destination system. For
>> exampl, pre-migration on the source system CPUs are in node 1 and 3,
>> post-migration on the destination system CPUs are in nodes 2 and 3.
>>
>> Handling the node change for a CPU can cause corruption in the slab
>> cache if we hit a timing where a CPUs node is changed while cache_reap()
>> is invoked. The corruption occurs because the slab cache code appears
>> to rely on the CPU and slab cache pages being on the same node.
>>
>> The current dynamic updating of a CPUs node done in arch/powerpc/mm/numa.c
>> does not prevent us from hitting this scenario.
>>
>> Changing the device tree property update notification handler that
>> recognizes an affinity change for a CPU to do a full DLPAR remove and
>> add of the CPU instead of dynamically changing its node resolves this
>> issue.
>>
>> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com
> Signed-off-by: Michael W. Bringmann <mwb@linux.vnet.ibm.com>
Are you sure that's what you meant? ie. you wrote some of the patch?
What I'd like is to get a Tested-by from you.
cheers
^ permalink raw reply
* Re: [RFC 1/6] powerpc:/drc Define interface to acquire arch-specific drc info
From: Michael Ellerman @ 2019-01-29 9:31 UTC (permalink / raw)
To: Tyrel Datwyler, Michael Bringmann, linuxppc-dev, linux-kernel
Cc: Juliet Kim, Thomas Falcon, Tyrel Datwyler
In-Reply-To: <e7030879-59e4-88fd-3785-fdd98b7f0a3b@gmail.com>
Tyrel Datwyler <turtle.in.the.kernel@gmail.com> writes:
> On 12/14/2018 12:50 PM, Michael Bringmann wrote:
>> Define interface to acquire arch-specific drc info to match against
>> hotpluggable devices. The current implementation exposes several
>> pseries-specific dynamic memory properties in generic kernel code.
>> This patch set provides an interface to pull that code out of the
>> generic kernel.
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> ---
>> include/linux/topology.h | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>> index cb0775e..df97f5f 100644
>> --- a/include/linux/topology.h
>> +++ b/include/linux/topology.h
>> @@ -44,6 +44,15 @@
>
> As far as I know pseries is the only platform that uses DR connectors, and I
> highly doubt that any other powerpc platform or arch ever will. So, I'm not sure
> that this is really generic enough to belong in topology.h.
Right. This does not belong in include/linux.
> If anything I would
> suggest putting this in an include in arch/powerpc/include/ named something like
> drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp
> that want/need to use this functionality.
Yeah that would make more sense.
Using "arch" in the name is wrong, it's pseries specific so
pseries_find_drc_match() would be more appropriate.
>> +int arch_find_drc_match(struct device_node *dn,
>> + bool (*usercb)(struct device_node *dn,
>> + u32 drc_index, char *drc_name,
>> + char *drc_type, u32 drc_power_domain,
>> + void *data),
>> + char *opt_drc_type, char *opt_drc_name,
>> + bool match_drc_index, bool ck_php_type,
>> + void *data);
This function signature is kind of insane.
You end with calls like:
+ return arch_find_drc_match(dn, rpaphp_add_slot_cb,
+ NULL, NULL, false, true, NULL);
Which is impossible to parse.
I feel like maybe this isn't the right level of abstraction.
cheers
^ permalink raw reply
* Re: [RFC 1/6] powerpc:/drc Define interface to acquire arch-specific drc info
From: Michael Ellerman @ 2019-01-29 9:25 UTC (permalink / raw)
To: Michael Bringmann, Tyrel Datwyler, linuxppc-dev, linux-kernel
Cc: Juliet Kim, Thomas Falcon, Tyrel Datwyler, nathanl
In-Reply-To: <d881d17c-1157-a6a0-0784-33510a4ee674@linux.vnet.ibm.com>
Michael Bringmann <mwb@linux.vnet.ibm.com> writes:
> On 1/25/19 10:09 AM, Michael Bringmann wrote:
>> Adding Nathan Lynch
>>
>> On 1/24/19 6:04 PM, Tyrel Datwyler wrote:
>>> On 12/14/2018 12:50 PM, Michael Bringmann wrote:
>>>> Define interface to acquire arch-specific drc info to match against
>>>> hotpluggable devices. The current implementation exposes several
>>>> pseries-specific dynamic memory properties in generic kernel code.
>>>> This patch set provides an interface to pull that code out of the
>>>> generic kernel.
>>>>
>>>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>>>> ---
>>>> include/linux/topology.h | 9 +++++++++
>>>> 1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>>>> index cb0775e..df97f5f 100644
>>>> --- a/include/linux/topology.h
>>>> +++ b/include/linux/topology.h
>>>> @@ -44,6 +44,15 @@
>>>
>>> As far as I know pseries is the only platform that uses DR connectors, and I
>>> highly doubt that any other powerpc platform or arch ever will. So, I'm not sure
>>> that this is really generic enough to belong in topology.h. If anything I would
>>> suggest putting this in an include in arch/powerpc/include/ named something like
>>> drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp
>>> that want/need to use this functionality.
>
> It looks like the 'rpaphp' and 'rpadlpar_io' modules are also dependent upon the
> powerpc platform.
Yes that's right.
> Shouldn't the relevant source files be moved completely to the
> powerpc-specific directories out of drivers/pci/hotplug as well?
I don't think so. They are PCI hotplug drivers, so they should sit with
the other PCI hotplug drivers.
It's true that PCI hotplug drivers are more platform specific than other
types of drivers, but still they have some things in common with other
PCI hotplug drivers.
cheers
^ permalink raw reply
* Re: [PATCH v3] cxl: Wrap iterations over afu slices inside 'afu_list_lock'
From: Frederic Barrat @ 2019-01-29 8:29 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev
Cc: Philippe Bergheaud, Alastair D'Silva, Christophe Lombard,
Andrew Donnellan
In-Reply-To: <20190129061558.2122-1-vaibhav@linux.ibm.com>
Le 29/01/2019 à 07:15, Vaibhav Jain a écrit :
> Within cxl module, iteration over array 'adapter->slices' may be racy
> at few points as it might be simultaneously read during an EEH and its
> contents being set to NULL while driver is being unloaded or unbound
> from the adapter. This might result in a NULL pointer to 'struct afu'
> being de-referenced during an EEH thereby causing a kernel oops.
>
> This patch fixes this by making sure that all access to the array
> 'adapter->slices' is wrapped within the context of spin-lock
> 'adapter->afu_list_lock'.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
Thanks for fixing this!
Acked-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Changelog:
>
> v3:
> * Updated a slice loop in cxl_pci_error_detectected() to ignore NULL
> slices [Fred]
> * Added a NULL AFU check in cxl_pci_slot_reset() [Fred]
>
> v2:
> * Fixed a wrong comparison of non-null pointer [Fred]
> * Moved a call to cxl_vphb_error_detected() within a branch that
> checks for not null AFU pointer in 'adapter->slices' [Fred]
> * Removed a misleading comment in code.
> ---
> drivers/misc/cxl/guest.c | 2 ++
> drivers/misc/cxl/pci.c | 39 ++++++++++++++++++++++++++++++---------
> 2 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
> index 5d28d9e454f5..08f4a512afad 100644
> --- a/drivers/misc/cxl/guest.c
> +++ b/drivers/misc/cxl/guest.c
> @@ -267,6 +267,7 @@ static int guest_reset(struct cxl *adapter)
> int i, rc;
>
> pr_devel("Adapter reset request\n");
> + spin_lock(&adapter->afu_list_lock);
> for (i = 0; i < adapter->slices; i++) {
> if ((afu = adapter->afu[i])) {
> pci_error_handlers(afu, CXL_ERROR_DETECTED_EVENT,
> @@ -283,6 +284,7 @@ static int guest_reset(struct cxl *adapter)
> pci_error_handlers(afu, CXL_RESUME_EVENT, 0);
> }
> }
> + spin_unlock(&adapter->afu_list_lock);
> return rc;
> }
>
> diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
> index c79ba1c699ad..300531d6136f 100644
> --- a/drivers/misc/cxl/pci.c
> +++ b/drivers/misc/cxl/pci.c
> @@ -1805,7 +1805,7 @@ static pci_ers_result_t cxl_vphb_error_detected(struct cxl_afu *afu,
> /* There should only be one entry, but go through the list
> * anyway
> */
> - if (afu->phb == NULL)
> + if (afu == NULL || afu->phb == NULL)
> return result;
>
> list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) {
> @@ -1832,7 +1832,8 @@ static pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
> {
> struct cxl *adapter = pci_get_drvdata(pdev);
> struct cxl_afu *afu;
> - pci_ers_result_t result = PCI_ERS_RESULT_NEED_RESET, afu_result;
> + pci_ers_result_t result = PCI_ERS_RESULT_NEED_RESET;
> + pci_ers_result_t afu_result = PCI_ERS_RESULT_NEED_RESET;
> int i;
>
> /* At this point, we could still have an interrupt pending.
> @@ -1843,6 +1844,7 @@ static pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
>
> /* If we're permanently dead, give up. */
> if (state == pci_channel_io_perm_failure) {
> + spin_lock(&adapter->afu_list_lock);
> for (i = 0; i < adapter->slices; i++) {
> afu = adapter->afu[i];
> /*
> @@ -1851,6 +1853,7 @@ static pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
> */
> cxl_vphb_error_detected(afu, state);
> }
> + spin_unlock(&adapter->afu_list_lock);
> return PCI_ERS_RESULT_DISCONNECT;
> }
>
> @@ -1932,11 +1935,17 @@ static pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
> * * In slot_reset, free the old resources and allocate new ones.
> * * In resume, clear the flag to allow things to start.
> */
> +
> + /* Make sure no one else changes the afu list */
> + spin_lock(&adapter->afu_list_lock);
> +
> for (i = 0; i < adapter->slices; i++) {
> afu = adapter->afu[i];
>
> - afu_result = cxl_vphb_error_detected(afu, state);
> + if (afu == NULL)
> + continue;
>
> + afu_result = cxl_vphb_error_detected(afu, state);
> cxl_context_detach_all(afu);
> cxl_ops->afu_deactivate_mode(afu, afu->current_mode);
> pci_deconfigure_afu(afu);
> @@ -1948,6 +1957,7 @@ static pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
> (result == PCI_ERS_RESULT_NEED_RESET))
> result = PCI_ERS_RESULT_NONE;
> }
> + spin_unlock(&adapter->afu_list_lock);
>
> /* should take the context lock here */
> if (cxl_adapter_context_lock(adapter) != 0)
> @@ -1980,14 +1990,18 @@ static pci_ers_result_t cxl_pci_slot_reset(struct pci_dev *pdev)
> */
> cxl_adapter_context_unlock(adapter);
>
> + spin_lock(&adapter->afu_list_lock);
> for (i = 0; i < adapter->slices; i++) {
> afu = adapter->afu[i];
>
> + if (afu == NULL)
> + continue;
> +
> if (pci_configure_afu(afu, adapter, pdev))
> - goto err;
> + goto err_unlock;
>
> if (cxl_afu_select_best_mode(afu))
> - goto err;
> + goto err_unlock;
>
> if (afu->phb == NULL)
> continue;
> @@ -1999,16 +2013,16 @@ static pci_ers_result_t cxl_pci_slot_reset(struct pci_dev *pdev)
> ctx = cxl_get_context(afu_dev);
>
> if (ctx && cxl_release_context(ctx))
> - goto err;
> + goto err_unlock;
>
> ctx = cxl_dev_context_init(afu_dev);
> if (IS_ERR(ctx))
> - goto err;
> + goto err_unlock;
>
> afu_dev->dev.archdata.cxl_ctx = ctx;
>
> if (cxl_ops->afu_check_and_enable(afu))
> - goto err;
> + goto err_unlock;
>
> afu_dev->error_state = pci_channel_io_normal;
>
> @@ -2029,8 +2043,13 @@ static pci_ers_result_t cxl_pci_slot_reset(struct pci_dev *pdev)
> result = PCI_ERS_RESULT_DISCONNECT;
> }
> }
> +
> + spin_unlock(&adapter->afu_list_lock);
> return result;
>
> +err_unlock:
> + spin_unlock(&adapter->afu_list_lock);
> +
> err:
> /* All the bits that happen in both error_detected and cxl_remove
> * should be idempotent, so we don't need to worry about leaving a mix
> @@ -2051,10 +2070,11 @@ static void cxl_pci_resume(struct pci_dev *pdev)
> * This is not the place to be checking if everything came back up
> * properly, because there's no return value: do that in slot_reset.
> */
> + spin_lock(&adapter->afu_list_lock);
> for (i = 0; i < adapter->slices; i++) {
> afu = adapter->afu[i];
>
> - if (afu->phb == NULL)
> + if (afu == NULL || afu->phb == NULL)
> continue;
>
> list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) {
> @@ -2063,6 +2083,7 @@ static void cxl_pci_resume(struct pci_dev *pdev)
> afu_dev->driver->err_handler->resume(afu_dev);
> }
> }
> + spin_unlock(&adapter->afu_list_lock);
> }
>
> static const struct pci_error_handlers cxl_err_handler = {
>
^ permalink raw reply
* RE: [PATCH] ucc_geth: Reset BQL queue when stopping device
From: Mathias Thore @ 2019-01-29 8:07 UTC (permalink / raw)
To: Li Yang
Cc: netdev@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
David Gounaris
In-Reply-To: <CADRPPNS3M=j4DDMtq_Mb+Xo1M71_yM+_N7n48u-z5pSSzdBoXg@mail.gmail.com>
Is there a scenario where we are clearing the TX ring but don't want to reset the BQL TX queue?
I think it makes sense to keep it in ucc_geth_free_tx since the reason it is needed isn't the timeout per se, but rather the clearing of the TX ring. This way, it will be performed no matter why the driver ends up calling this function.
-----Original Message-----
From: Li Yang [mailto:leoyang.li@nxp.com]
Sent: Monday, 28 January 2019 22:37
To: Mathias Thore <Mathias.Thore@infinera.com>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>; netdev@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; David Gounaris <David.Gounaris@infinera.com>; Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Subject: Re: [PATCH] ucc_geth: Reset BQL queue when stopping device
On Mon, Jan 28, 2019 at 8:37 AM Mathias Thore <Mathias.Thore@infinera.com> wrote:
>
> Hi,
>
>
> This is what we observed: there was a storm on the medium so that our controller could not do its TX, resulting in timeout. When timeout occurs, the driver clears all descriptors from the TX queue. The function called in this patch is used to reflect this clearing also in the BQL layer. Without it, the controller would get stuck, unable to perform TX, even several minutes after the storm had ended. Bringing the device down and then up again would solve the problem, but this patch also solves it automatically.
The explanation makes sense. So this should only be required in the timeout scenario instead of other clean up scenarios like device shutdown? If so, it probably it will be better to be done in ucc_geth_timeout_work()?
>
>
> Some other drivers do the same, for example e1000e driver calls netdev_reset_queue in its e1000_clean_tx_ring function. It is possible that other drivers should do the same; I have no way of verifying this.
>
>
> Regards,
>
> Mathias
>
> --
>
>
> From: Christophe Leroy <christophe.leroy@c-s.fr>
> Sent: Monday, January 28, 2019 10:48 AM
> To: Mathias Thore; leoyang.li@nxp.com; netdev@vger.kernel.org;
> linuxppc-dev@lists.ozlabs.org; David Gounaris; Joakim Tjernlund
> Subject: Re: [PATCH] ucc_geth: Reset BQL queue when stopping device
>
>
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> Hi,
>
> Le 28/01/2019 à 10:07, Mathias Thore a écrit :
> > After a timeout event caused by for example a broadcast storm, when
> > the MAC and PHY are reset, the BQL TX queue needs to be reset as
> > well. Otherwise, the device will exhibit severe performance issues
> > even after the storm has ended.
>
> What are the symptomns ?
>
> Is this reset needed on any network driver in that case, or is it
> something particular for the ucc_geth ?
> For instance, the freescale fs_enet doesn't have that reset. Should it
> have it too ?
>
> Christophe
>
> >
> > Co-authored-by: David Gounaris <david.gounaris@infinera.com>
> > Signed-off-by: Mathias Thore <mathias.thore@infinera.com>
> > ---
> > drivers/net/ethernet/freescale/ucc_geth.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/freescale/ucc_geth.c
> > b/drivers/net/ethernet/freescale/ucc_geth.c
> > index c3d539e209ed..eb3e65e8868f 100644
> > --- a/drivers/net/ethernet/freescale/ucc_geth.c
> > +++ b/drivers/net/ethernet/freescale/ucc_geth.c
> > @@ -1879,6 +1879,8 @@ static void ucc_geth_free_tx(struct ucc_geth_private *ugeth)
> > u16 i, j;
> > u8 __iomem *bd;
> >
> > + netdev_reset_queue(ugeth->ndev);
> > +
> > ug_info = ugeth->ug_info;
> > uf_info = &ug_info->uf_info;
> >
> >
>
^ permalink raw reply
* Re: [RFC PATCH 1/2] powerpc/powernv: Add support for CXL mode switch that need PHB reset
From: Vaibhav Jain @ 2019-01-29 7:00 UTC (permalink / raw)
To: Frederic Barrat, linuxppc-dev
Cc: Philippe Bergheaud, Alastair D'Silva, Christophe Lombard,
Andrew Donnellan
In-Reply-To: <823c21d9-b408-6663-868a-fd87609647fe@linux.ibm.com>
Frederic Barrat <fbarrat@linux.ibm.com> writes:
>> + opal_poll_events(NULL);
>
> Why is a call to opal_poll_events() needed?
Trying to make sure that opal pollers are run on the current CPU and any
opal timer are executed.
--
Vaibhav Jain <vaibhav@linux.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.
^ 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