* Re: KVM on POWER8 host lock up since 10d91611f426 ("powerpc/64s: Reimplement book3s idle code in C")
From: Michael Ellerman @ 2020-08-31 12:58 UTC (permalink / raw)
To: Ruediger Oertel, Nicholas Piggin, linuxppc-dev,
Michal Suchánek
In-Reply-To: <50693d7e-6c8b-4908-d003-25b99a867850@suse.de>
Ruediger Oertel <ro@suse.de> writes:
> Am 31.08.20 um 03:14 schrieb Nicholas Piggin:
>> Excerpts from Michal Suchánek's message of August 31, 2020 6:11 am:
>>> Hello,
>>>
>>> on POWER8 KVM hosts lock up since commit 10d91611f426 ("powerpc/64s:
>>> Reimplement book3s idle code in C").
>>>
>>> The symptom is host locking up completely after some hours of KVM
>>> workload with messages like
>>>
>>> 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
>>> 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 71
>>> 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
>>> 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 71
>>> 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
>>>
>>> printed before the host locks up.
>>>
>>> The machines run sandboxed builds which is a mixed workload resulting in
>>> IO/single core/mutiple core load over time and there are periods of no
>>> activity and no VMS runnig as well. The VMs are shortlived so VM
>>> setup/terdown is somewhat excercised as well.
>>>
>>> POWER9 with the new guest entry fast path does not seem to be affected.
>>>
>>> Reverted the patch and the followup idle fixes on top of 5.2.14 and
>>> re-applied commit a3f3072db6ca ("powerpc/powernv/idle: Restore IAMR
>>> after idle") which gives same idle code as 5.1.16 and the kernel seems
>>> stable.
>>>
>>> Config is attached.
>>>
>>> I cannot easily revert this commit, especially if I want to use the same
>>> kernel on POWER8 and POWER9 - many of the POWER9 fixes are applicable
>>> only to the new idle code.
>>>
>>> Any idea what can be the problem?
>>
>> So hwthread_state is never getting back to to HWTHREAD_IN_IDLE on
>> those threads. I wonder what they are doing. POWER8 doesn't have a good
>> NMI IPI and I don't know if it supports pdbg dumping registers from the
>> BMC unfortunately. Do the messages always come in pairs of CPUs?
>>
>> I'm not sure where to start with reproducing, I'll have to try. How many
>> vCPUs in the guests? Do you have several guests running at once?
>
> Hello all,
>
> some details on the setup...
> these machines are buildservice workers, (build.opensuse.org) and all they
> do is spawn new VMs, run a package building job inside (rpmbuild, debbuild,...)
>
> The machines are running in OPAL/PowerNV mode, with "ppc64_cpu --smt=off".
> The number of VMs varies across the machines:
> obs-power8-01: 18 instances, "-smp 16,threads=8"
> obs-power8-02: 20 instances, "-smp 8,threads=8"
> obs-power8-03: 30 instances, "-smp 8,threads=8"
> obs-power8-04: 20 instances, "-smp 8,threads=8"
Can you send us the output of:
# grep . /sys/module/kvm_hv/parameters/*
cheers
^ permalink raw reply
* Re: fsl_espi errors on v5.7.15
From: Heiner Kallweit @ 2020-08-31 12:33 UTC (permalink / raw)
To: Chris Packham, Nicholas Piggin, benh@kernel.crashing.org,
broonie@kernel.org, mpe@ellerman.id.au, paulus@samba.org
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
linux-spi@vger.kernel.org
In-Reply-To: <6a9eb498-2982-05de-52f9-da5f6a626e49@alliedtelesis.co.nz>
On 30.08.2020 23:59, Chris Packham wrote:
>
> On 31/08/20 9:41 am, Heiner Kallweit wrote:
>> On 30.08.2020 23:00, Chris Packham wrote:
>>> On 31/08/20 12:30 am, Nicholas Piggin wrote:
>>>> Excerpts from Chris Packham's message of August 28, 2020 8:07 am:
>>> <snip>
>>>
>>>>>>>>> I've also now seen the RX FIFO not empty error on the T2080RDB
>>>>>>>>>
>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>>>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>>
>>>>>>>>> With my current workaround of emptying the RX FIFO. It seems
>>>>>>>>> survivable. Interestingly it only ever seems to be 1 extra byte in the
>>>>>>>>> RX FIFO and it seems to be after either a READ_SR or a READ_FSR.
>>>>>>>>>
>>>>>>>>> fsl_espi ffe110000.spi: tx 70
>>>>>>>>> fsl_espi ffe110000.spi: rx 03
>>>>>>>>> fsl_espi ffe110000.spi: Extra RX 00
>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>>>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>> fsl_espi ffe110000.spi: tx 05
>>>>>>>>> fsl_espi ffe110000.spi: rx 00
>>>>>>>>> fsl_espi ffe110000.spi: Extra RX 03
>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>>>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>> fsl_espi ffe110000.spi: tx 05
>>>>>>>>> fsl_espi ffe110000.spi: rx 00
>>>>>>>>> fsl_espi ffe110000.spi: Extra RX 03
>>>>>>>>>
>>>>>>>>> From all the Micron SPI-NOR datasheets I've got access to it is
>>>>>>>>> possible to continually read the SR/FSR. But I've no idea why it
>>>>>>>>> happens some times and not others.
>>>>>>>> So I think I've got a reproduction and I think I've bisected the problem
>>>>>>>> to commit 3282a3da25bd ("powerpc/64: Implement soft interrupt replay in
>>>>>>>> C"). My day is just finishing now so I haven't applied too much scrutiny
>>>>>>>> to this result. Given the various rabbit holes I've been down on this
>>>>>>>> issue already I'd take this information with a good degree of skepticism.
>>>>>>>>
>>>>>>> OK, so an easy test should be to re-test with a 5.4 kernel.
>>>>>>> It doesn't have yet the change you're referring to, and the fsl-espi driver
>>>>>>> is basically the same as in 5.7 (just two small changes in 5.7).
>>>>>> There's 6cc0c16d82f88 and maybe also other interrupt related patches
>>>>>> around this time that could affect book E, so it's good if that exact
>>>>>> patch is confirmed.
>>>>> My confirmation is basically that I can induce the issue in a 5.4 kernel
>>>>> by cherry-picking 3282a3da25bd. I'm also able to "fix" the issue in
>>>>> 5.9-rc2 by reverting that one commit.
>>>>>
>>>>> I both cases it's not exactly a clean cherry-pick/revert so I also
>>>>> confirmed the bisection result by building at 3282a3da25bd (which sees
>>>>> the issue) and the commit just before (which does not).
>>>> Thanks for testing, that confirms it well.
>>>>
>>>> [snip patch]
>>>>
>>>>> I still saw the issue with this change applied. PPC_IRQ_SOFT_MASK_DEBUG
>>>>> didn't report anything (either with or without the change above).
>>>> Okay, it was a bit of a shot in the dark. I still can't see what
>>>> else has changed.
>>>>
>>>> What would cause this, a lost interrupt? A spurious interrupt? Or
>>>> higher interrupt latency?
>>>>
>>>> I don't think the patch should cause significantly worse latency,
>>>> (it's supposed to be a bit better if anything because it doesn't set
>>>> up the full interrupt frame). But it's possible.
>>> My working theory is that the SPI_DON indication is all about the TX
>>> direction an now that the interrupts are faster we're hitting an error
>>> because there is still RX activity going on. Heiner disagrees with my
>>> interpretation of the SPI_DON indication and the fact that it doesn't
>>> happen every time does throw doubt on it.
>>>
>> It's right that the eSPI spec can be interpreted that SPI_DON refers to
>> TX only. However this wouldn't really make sense, because also for RX
>> we program the frame length, and therefore want to be notified once the
>> full frame was received. Also practical experience shows that SPI_DON
>> is set also after RX-only transfers.
>> Typical SPI NOR use case is that you write read command + start address,
>> followed by a longer read. If the TX-only interpretation would be right,
>> we'd always end up with SPI_DON not being set.
>>
>>> I can't really explain the extra RX byte in the fifo. We know how many
>>> bytes to expect and we pull that many from the fifo so it's not as if
>>> we're missing an interrupt causing us to skip the last byte. I've been
>>> looking for some kind of off-by-one calculation but again if it were
>>> something like that it'd happen all the time.
>>>
>> Maybe it helps to know what value this extra byte in the FIFO has. Is it:
>> - a duplicate of the last read byte
>> - or the next byte (at <end address> + 1)
>> - or a fixed value, e.g. always 0x00 or 0xff
>
> The values were up thread a bit but I'll repeat them here
>
> fsl_espi ffe110000.spi: tx 70
> fsl_espi ffe110000.spi: rx 03
> fsl_espi ffe110000.spi: Extra RX 00
> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
> fsl_espi ffe110000.spi: tx 05
> fsl_espi ffe110000.spi: rx 00
> fsl_espi ffe110000.spi: Extra RX 03
> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
> fsl_espi ffe110000.spi: tx 05
> fsl_espi ffe110000.spi: rx 00
> fsl_espi ffe110000.spi: Extra RX 03
>
>
> The rx 00 Extra RX 03 is a bit concerning. I've only ever seen them with
> either a READ_SR or a READ_FSR. Never a data read.
>
Just remembered something about SPIE_DON:
Transfers are always full duplex, therefore in case of a read the chip
sends dummy zero's. Having said that in case of a read SPIE_DON means
that the last dummy zero was shifted out.
READ_SR and READ_FSR are the shortest transfers, 1 byte out and 1 byte in.
So the issue may have a dependency on the length of the transfer.
However I see no good explanation so far. You can try adding a delay of
a few miroseconds between the following to commands in fsl_espi_bufs().
fsl_espi_write_reg(espi, ESPI_SPIM, mask);
/* Prevent filling the fifo from getting interrupted */
spin_lock_irq(&espi->lock);
Maybe enabling interrupts and seeing the SPIE_DON interrupt are too close.
^ permalink raw reply
* Re: KVM on POWER8 host lock up since 10d91611f426 ("powerpc/64s: Reimplement book3s idle code in C")
From: Michael Ellerman @ 2020-08-31 10:50 UTC (permalink / raw)
To: Michal Suchánek, Nicholas Piggin; +Cc: linuxppc-dev, ro, Hari Bathini
In-Reply-To: <20200831091523.GC29521@kitsune.suse.cz>
Michal Suchánek <msuchanek@suse.de> writes:
> On Mon, Aug 31, 2020 at 11:14:18AM +1000, Nicholas Piggin wrote:
>> Excerpts from Michal Suchánek's message of August 31, 2020 6:11 am:
>> > Hello,
>> >
>> > on POWER8 KVM hosts lock up since commit 10d91611f426 ("powerpc/64s:
>> > Reimplement book3s idle code in C").
>> >
>> > The symptom is host locking up completely after some hours of KVM
>> > workload with messages like
>> >
>> > 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
>> > 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 71
>> > 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
>> > 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 71
>> > 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
>> >
>> > printed before the host locks up.
>> >
>> > The machines run sandboxed builds which is a mixed workload resulting in
>> > IO/single core/mutiple core load over time and there are periods of no
>> > activity and no VMS runnig as well. The VMs are shortlived so VM
>> > setup/terdown is somewhat excercised as well.
>> >
>> > POWER9 with the new guest entry fast path does not seem to be affected.
>> >
>> > Reverted the patch and the followup idle fixes on top of 5.2.14 and
>> > re-applied commit a3f3072db6ca ("powerpc/powernv/idle: Restore IAMR
>> > after idle") which gives same idle code as 5.1.16 and the kernel seems
>> > stable.
>> >
>> > Config is attached.
>> >
>> > I cannot easily revert this commit, especially if I want to use the same
>> > kernel on POWER8 and POWER9 - many of the POWER9 fixes are applicable
>> > only to the new idle code.
>> >
>> > Any idea what can be the problem?
>>
>> So hwthread_state is never getting back to to HWTHREAD_IN_IDLE on
>> those threads. I wonder what they are doing. POWER8 doesn't have a good
>> NMI IPI and I don't know if it supports pdbg dumping registers from the
>> BMC unfortunately.
>
> It may be possible to set up fadump with a later kernel version that
> supports it on powernv and dump the whole kernel.
Your firmware won't support it AFAIK.
You could try kdump, but if we have CPUs stuck in KVM then there's a
good chance it won't work :/
cheers
^ permalink raw reply
* Re: KVM on POWER8 host lock up since 10d91611f426 ("powerpc/64s: Reimplement book3s idle code in C")
From: Ruediger Oertel @ 2020-08-31 9:03 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev, Michal Suchánek
In-Reply-To: <1598835313.5688ngko4f.astroid@bobo.none>
[-- Attachment #1.1: Type: text/plain, Size: 3794 bytes --]
Am 31.08.20 um 03:14 schrieb Nicholas Piggin:
> So hwthread_state is never getting back to to HWTHREAD_IN_IDLE on
> those threads. I wonder what they are doing. POWER8 doesn't have a good
> NMI IPI and I don't know if it supports pdbg dumping registers from the
> BMC unfortunately.
> Do the messages always come in pairs of CPUs?
well,
- one problem is that at some point the machine just locks up completely,
so I can not tell if there were lines not printed any more and in some
cases all I get is a single line
- looking at the stats in generally it's either one cpu printed several
times or a pair ("not strictly") alternatingly
2020-07-30T03:16:16+00:00 obs-power8-03 kernel: [51284.029821] KVM: couldn't grab cpu 92
2020-07-30T03:16:16+00:00 obs-power8-03 kernel: [51284.058630] KVM: couldn't grab cpu 92
2020-07-30T03:16:16+00:00 obs-power8-03 kernel: [51284.108268] KVM: couldn't grab cpu 92
2020-07-30T03:16:16+00:00 obs-power8-03 kernel: [51284.210206] KVM: couldn't grab cpu 92
2020-07-30T03:16:16+00:00 obs-power8-03 kernel: [51284.323465] KVM: couldn't grab cpu 92
2020-07-30T03:16:16+00:00 obs-power8-03 kernel: [51284.334420] KVM: couldn't grab cpu 92
2020-07-30T03:16:16+00:00 obs-power8-03 kernel: [51284.345470] KVM: couldn't grab cpu 92
2020-07-30T03:16:16+00:00 obs-power8-03 kernel: [51284.395185] KVM: couldn't grab cpu 92
2020-07-30T03:16:16+00:00 obs-power8-03 kernel: [51284.517182] KVM: couldn't grab cpu 92
2020-07-30T03:16:17+00:00 obs-power8-03 kernel: [51284.600716] KVM: couldn't grab cpu 92
2020-07-30T03:16:18+00:00 obs-power8-03 kernel: [51286.201589] KVM: couldn't grab cpu 92
2020-07-30T03:16:19+00:00 obs-power8-03 kernel: [51286.627273] KVM: couldn't grab cpu 92
2020-07-30T16:44:16+00:00 obs-power8-04 kernel: [30099.726288] KVM: couldn't grab cpu 61
2020-07-30T16:44:16+00:00 obs-power8-04 kernel: [30099.736843] KVM: couldn't grab cpu 125
2020-07-30T16:44:17+00:00 obs-power8-04 kernel: [30099.747429] KVM: couldn't grab cpu 125
2020-07-30T16:44:17+00:00 obs-power8-04 kernel: [30099.877138] KVM: couldn't grab cpu 61
2020-07-30T16:44:17+00:00 obs-power8-04 kernel: [30099.916422] KVM: couldn't grab cpu 125
2020-07-30T16:44:17+00:00 obs-power8-04 kernel: [30099.931755] KVM: couldn't grab cpu 61
2020-07-30T16:44:17+00:00 obs-power8-04 kernel: [30100.029003] KVM: couldn't grab cpu 61
2020-07-30T16:44:17+00:00 obs-power8-04 kernel: [30100.334895] KVM: couldn't grab cpu 125
2020-07-30T16:44:17+00:00 obs-power8-04 kernel: [30100.392713] KVM: couldn't grab cpu 61
2020-07-30T16:44:17+00:00 obs-power8-04 kernel: [30100.569011] KVM: couldn't grab cpu 125
2020-07-30T16:44:17+00:00 obs-power8-04 kernel: [30100.617048] KVM: couldn't grab cpu 125
2020-07-30T16:44:17+00:00 obs-power8-04 kernel: [30100.628107] KVM: couldn't grab cpu 125
2020-07-30T16:44:18+00:00 obs-power8-04 kernel: [30100.809046] KVM: couldn't grab cpu 125
2020-07-30T16:44:18+00:00 obs-power8-04 kernel: [30101.001097] KVM: couldn't grab cpu 61
2020-07-30T16:44:19+00:00 obs-power8-04 kernel: [30102.109007] KVM: couldn't grab cpu 125
2020-07-30T16:44:19+00:00 obs-power8-04 kernel: [30102.254470] KVM: couldn't grab cpu 61
>
> I'm not sure where to start with reproducing, I'll have to try. How many
> vCPUs in the guests? Do you have several guests running at once?
>
> Thanks,
> Nick
>
--
with kind regards (mit freundlichem Grinsen),
Ruediger Oertel (ro@suse.com,ro@suse.de,bugfinder@t-online.de)
--------Do-Not-Accept-Binary-Blobs.----Ever.----From-Anyone.------------
Key fingerprint = 17DC 6553 86A7 384B 53C5 CA5C 3CE4 F2E7 23F2 B417
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg,
Germany, (HRB 36809, AG Nürnberg), Geschäftsführer: Felix Imendörffer
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: KVM on POWER8 host lock up since 10d91611f426 ("powerpc/64s: Reimplement book3s idle code in C")
From: Ruediger Oertel @ 2020-08-31 8:48 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev, Michal Suchánek
In-Reply-To: <1598835313.5688ngko4f.astroid@bobo.none>
[-- Attachment #1.1: Type: text/plain, Size: 3377 bytes --]
Am 31.08.20 um 03:14 schrieb Nicholas Piggin:
> Excerpts from Michal Suchánek's message of August 31, 2020 6:11 am:
>> Hello,
>>
>> on POWER8 KVM hosts lock up since commit 10d91611f426 ("powerpc/64s:
>> Reimplement book3s idle code in C").
>>
>> The symptom is host locking up completely after some hours of KVM
>> workload with messages like
>>
>> 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
>> 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 71
>> 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
>> 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 71
>> 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
>>
>> printed before the host locks up.
>>
>> The machines run sandboxed builds which is a mixed workload resulting in
>> IO/single core/mutiple core load over time and there are periods of no
>> activity and no VMS runnig as well. The VMs are shortlived so VM
>> setup/terdown is somewhat excercised as well.
>>
>> POWER9 with the new guest entry fast path does not seem to be affected.
>>
>> Reverted the patch and the followup idle fixes on top of 5.2.14 and
>> re-applied commit a3f3072db6ca ("powerpc/powernv/idle: Restore IAMR
>> after idle") which gives same idle code as 5.1.16 and the kernel seems
>> stable.
>>
>> Config is attached.
>>
>> I cannot easily revert this commit, especially if I want to use the same
>> kernel on POWER8 and POWER9 - many of the POWER9 fixes are applicable
>> only to the new idle code.
>>
>> Any idea what can be the problem?
>
> So hwthread_state is never getting back to to HWTHREAD_IN_IDLE on
> those threads. I wonder what they are doing. POWER8 doesn't have a good
> NMI IPI and I don't know if it supports pdbg dumping registers from the
> BMC unfortunately. Do the messages always come in pairs of CPUs?
>
> I'm not sure where to start with reproducing, I'll have to try. How many
> vCPUs in the guests? Do you have several guests running at once?
Hello all,
some details on the setup...
these machines are buildservice workers, (build.opensuse.org) and all they
do is spawn new VMs, run a package building job inside (rpmbuild, debbuild,...)
The machines are running in OPAL/PowerNV mode, with "ppc64_cpu --smt=off".
The number of VMs varies across the machines:
obs-power8-01: 18 instances, "-smp 16,threads=8"
obs-power8-02: 20 instances, "-smp 8,threads=8"
obs-power8-03: 30 instances, "-smp 8,threads=8"
obs-power8-04: 20 instances, "-smp 8,threads=8"
obs-power8-05: 36 instances, "-smp 4,threads=2" (this one with "ppc64_cpu --subcores-per-core=4"
but anyway the stalls can be seen on all of them, sometimes after 4 hours
sometimes just after about a day. The 01 with more cpu overcommit seems
a little faster reproducing the problem, but that's more gut feeling than
anything backed by real numbers.
--
with kind regards (mit freundlichem Grinsen),
Ruediger Oertel (ro@suse.com,ro@suse.de,bugfinder@t-online.de)
--------Do-Not-Accept-Binary-Blobs.----Ever.----From-Anyone.------------
Key fingerprint = 17DC 6553 86A7 384B 53C5 CA5C 3CE4 F2E7 23F2 B417
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg,
Germany, (HRB 36809, AG Nürnberg), Geschäftsführer: Felix Imendörffer
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH 5/5] powerpc: use the generic dma_ops_bypass mode
From: Cédric Le Goater @ 2020-08-31 7:19 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linuxppc-dev, Daniel Borkmann, Alexey Kardashevskiy,
Björn Töpel, Joerg Roedel, Jesper Dangaard Brouer,
linux-kernel, iommu, Oliver O'Halloran, Greg Kroah-Hartman,
aacraid, Robin Murphy, Lu Baolu
In-Reply-To: <20200831064038.GB27617@lst.de>
On 8/31/20 8:40 AM, Christoph Hellwig wrote:
> On Sun, Aug 30, 2020 at 11:04:21AM +0200, Cédric Le Goater wrote:
>> Hello,
>>
>> On 7/8/20 5:24 PM, Christoph Hellwig wrote:
>>> Use the DMA API bypass mechanism for direct window mappings. This uses
>>> common code and speed up the direct mapping case by avoiding indirect
>>> calls just when not using dma ops at all. It also fixes a problem where
>>> the sync_* methods were using the bypass check for DMA allocations, but
>>> those are part of the streaming ops.
>>>
>>> Note that this patch loses the DMA_ATTR_WEAK_ORDERING override, which
>>> has never been well defined, as is only used by a few drivers, which
>>> IIRC never showed up in the typical Cell blade setups that are affected
>>> by the ordering workaround.
>>>
>>> Fixes: efd176a04bef ("powerpc/pseries/dma: Allow SWIOTLB")
>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>> ---
>>> arch/powerpc/Kconfig | 1 +
>>> arch/powerpc/include/asm/device.h | 5 --
>>> arch/powerpc/kernel/dma-iommu.c | 90 ++++---------------------------
>>> 3 files changed, 10 insertions(+), 86 deletions(-)
>>
>> I am seeing corruptions on a couple of POWER9 systems (boston) when
>> stressed with IO. stress-ng gives some results but I have first seen
>> it when compiling the kernel in a guest and this is still the best way
>> to raise the issue.
>>
>> These systems have of a SAS Adaptec controller :
>>
>> 0003:01:00.0 Serial Attached SCSI controller: Adaptec Series 8 12G SAS/PCIe 3 (rev 01)
>>
>> When the failure occurs, the POWERPC EEH interrupt fires and dumps
>> lowlevel PHB4 registers among which :
>>
>> [ 2179.251069490,3] PHB#0003[0:3]: phbErrorStatus = 0000028000000000
>> [ 2179.251117476,3] PHB#0003[0:3]: phbFirstErrorStatus = 0000020000000000
>>
>> The bits raised identify a PPC 'TCE' error, which means it is related
>> to DMAs. See below for more details.
>>
>>
>> Reverting this patch "fixes" the issue but it is probably else where,
>> in some other layers or in the aacraid driver. How should I proceed
>> to get more information ?
>
> The aacraid DMA masks look like a mess. Can you try the hack
> below and see it it helps?
No effect. The system crashes the same. But Alexey spotted some issue
with swiotlb.
C.
^ permalink raw reply
* Re: [PATCH 5/5] powerpc: use the generic dma_ops_bypass mode
From: Cédric Le Goater @ 2020-08-30 9:04 UTC (permalink / raw)
To: Christoph Hellwig, iommu, Alexey Kardashevskiy
Cc: linuxppc-dev, Daniel Borkmann, Björn Töpel,
Joerg Roedel, Jesper Dangaard Brouer, linux-kernel,
Oliver O'Halloran, Greg Kroah-Hartman, aacraid, Robin Murphy,
Lu Baolu
In-Reply-To: <20200708152449.316476-6-hch@lst.de>
Hello,
On 7/8/20 5:24 PM, Christoph Hellwig wrote:
> Use the DMA API bypass mechanism for direct window mappings. This uses
> common code and speed up the direct mapping case by avoiding indirect
> calls just when not using dma ops at all. It also fixes a problem where
> the sync_* methods were using the bypass check for DMA allocations, but
> those are part of the streaming ops.
>
> Note that this patch loses the DMA_ATTR_WEAK_ORDERING override, which
> has never been well defined, as is only used by a few drivers, which
> IIRC never showed up in the typical Cell blade setups that are affected
> by the ordering workaround.
>
> Fixes: efd176a04bef ("powerpc/pseries/dma: Allow SWIOTLB")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/include/asm/device.h | 5 --
> arch/powerpc/kernel/dma-iommu.c | 90 ++++---------------------------
> 3 files changed, 10 insertions(+), 86 deletions(-)
I am seeing corruptions on a couple of POWER9 systems (boston) when
stressed with IO. stress-ng gives some results but I have first seen
it when compiling the kernel in a guest and this is still the best way
to raise the issue.
These systems have of a SAS Adaptec controller :
0003:01:00.0 Serial Attached SCSI controller: Adaptec Series 8 12G SAS/PCIe 3 (rev 01)
When the failure occurs, the POWERPC EEH interrupt fires and dumps
lowlevel PHB4 registers among which :
[ 2179.251069490,3] PHB#0003[0:3]: phbErrorStatus = 0000028000000000
[ 2179.251117476,3] PHB#0003[0:3]: phbFirstErrorStatus = 0000020000000000
The bits raised identify a PPC 'TCE' error, which means it is related
to DMAs. See below for more details.
Reverting this patch "fixes" the issue but it is probably else where,
in some other layers or in the aacraid driver. How should I proceed
to get more information ?
Thanks,
C.
[ 2054.970339] EEH: Frozen PE#1fd on PHB#3 detected
[ 2054.970375] EEH: PE location: UOPWR.BOS0019-Node0-Onboard SAS, PHB location: N/A
[ 2179.249415973,3] PHB#0003[0:3]: brdgCtl = 00000002
[ 2179.249515795,3] PHB#0003[0:3]: deviceStatus = 00060040
[ 2179.249596452,3] PHB#0003[0:3]: slotStatus = 00402000
[ 2179.249633728,3] PHB#0003[0:3]: linkStatus = a0830008
[ 2179.249674807,3] PHB#0003[0:3]: devCmdStatus = 00100107
[ 2179.249725974,3] PHB#0003[0:3]: devSecStatus = 00100107
[ 2179.249773550,3] PHB#0003[0:3]: rootErrorStatus = 00000000
[ 2179.249809823,3] PHB#0003[0:3]: corrErrorStatus = 00000000
[ 2179.249850439,3] PHB#0003[0:3]: uncorrErrorStatus = 00000000
[ 2179.249887411,3] PHB#0003[0:3]: devctl = 00000040
[ 2179.249928677,3] PHB#0003[0:3]: devStat = 00000006
[ 2179.249967150,3] PHB#0003[0:3]: tlpHdr1 = 00000000
[ 2179.250054987,3] PHB#0003[0:3]: tlpHdr2 = 00000000
[ 2179.250146600,3] PHB#0003[0:3]: tlpHdr3 = 00000000
[ 2179.250262780,3] PHB#0003[0:3]: tlpHdr4 = 00000000
[ 2179.250343101,3] PHB#0003[0:3]: sourceId = 00000000
[ 2179.250514264,3] PHB#0003[0:3]: nFir = 0000000000000000
[ 2179.250717971,3] PHB#0003[0:3]: nFirMask = 0030001c00000000
[ 2179.250791474,3] PHB#0003[0:3]: nFirWOF = 0000000000000000
[ 2179.250842054,3] PHB#0003[0:3]: phbPlssr = 0000001c00000000
[ 2179.250886003,3] PHB#0003[0:3]: phbCsr = 0000001c00000000
[ 2179.250929859,3] PHB#0003[0:3]: lemFir = 0000000100000080
[ 2179.250973720,3] PHB#0003[0:3]: lemErrorMask = 0000000000000000
[ 2179.251018622,3] PHB#0003[0:3]: lemWOF = 0000000000000080
[ 2179.251069490,3] PHB#0003[0:3]: phbErrorStatus = 0000028000000000
[ 2179.251117476,3] PHB#0003[0:3]: phbFirstErrorStatus = 0000020000000000
[ 2179.251162218,3] PHB#0003[0:3]: phbErrorLog0 = 2148000098000240
[ 2179.251206251,3] PHB#0003[0:3]: phbErrorLog1 = a008400000000000
[ 2179.251253096,3] PHB#0003[0:3]: phbTxeErrorStatus = 0000000000000000
[ 2179.265087656,3] PHB#0003[0:3]: phbTxeFirstErrorStatus = 0000000000000000
[ 2179.265142247,3] PHB#0003[0:3]: phbTxeErrorLog0 = 0000000000000000
[ 2179.265189734,3] PHB#0003[0:3]: phbTxeErrorLog1 = 0000000000000000
[ 2179.266335296,3] PHB#0003[0:3]: phbRxeArbErrorStatus = 0000000000000000
[ 2179.266380366,3] PHB#0003[0:3]: phbRxeArbFrstErrorStatus = 0000000000000000
[ 2179.266426523,3] PHB#0003[0:3]: phbRxeArbErrorLog0 = 0000000000000000
[ 2179.267537283,3] PHB#0003[0:3]: phbRxeArbErrorLog1 = 0000000000000000
[ 2179.267581708,3] PHB#0003[0:3]: phbRxeMrgErrorStatus = 0000000000000000
[ 2179.267628324,3] PHB#0003[0:3]: phbRxeMrgFrstErrorStatus = 0000000000000000
[ 2179.268748771,3] PHB#0003[0:3]: phbRxeMrgErrorLog0 = 0000000000000000
[ 2179.268794753,3] PHB#0003[0:3]: phbRxeMrgErrorLog1 = 0000000000000000
[ 2179.268841144,3] PHB#0003[0:3]: phbRxeTceErrorStatus = 6000000000000000
[ 2179.269945557,3] PHB#0003[0:3]: phbRxeTceFrstErrorStatus = 2000000000000000
[ 2179.269997896,3] PHB#0003[0:3]: phbRxeTceErrorLog0 = c0000000000001fd
[ 2179.270094740,3] PHB#0003[0:3]: phbRxeTceErrorLog1 = 0000000000000000
[ 2179.270144030,3] PHB#0003[0:3]: phbPblErrorStatus = 0000000000020000
[ 2179.281523166,3] PHB#0003[0:3]: phbPblFirstErrorStatus = 0000000000020000
[ 2179.281575378,3] PHB#0003[0:3]: phbPblErrorLog0 = 0000000000000000
[ 2179.281627897,3] PHB#0003[0:3]: phbPblErrorLog1 = 0000000000000000
[ 2179.282710177,3] PHB#0003[0:3]: phbPcieDlpErrorLog1 = 0000000000000000
[ 2179.282761495,3] PHB#0003[0:3]: phbPcieDlpErrorLog2 = 0000000000000000
[ 2179.282813999,3] PHB#0003[0:3]: phbPcieDlpErrorStatus = 0000000000000000
[ 2179.283926192,3] PHB#0003[0:3]: phbRegbErrorStatus = 0000004000000000
[ 2179.283978457,3] PHB#0003[0:3]: phbRegbFirstErrorStatus = 0000004000000000
[ 2179.284033525,3] PHB#0003[0:3]: phbRegbErrorLog0 = 8800005800000000
[ 2179.285123005,3] PHB#0003[0:3]: phbRegbErrorLog1 = 0000000007011000
[ 2179.285178505,3] PHB#0003[0:3]: PEST[1fd] = 8300b03800000000 8000000000000000
[ 2055.043233] EEH: Recovering PHB#3-PE#1fd
[ 2055.043274] EEH: PE location: UOPWR.BOS0019-Node0-Onboard SAS, PHB location: N/A
[ 2055.043423] aacraid 0003:01:00.0: aacraid: PCI error detected 2
[ 2055.099306] blk_update_request: I/O error, dev sda, sector 510949048 op 0x1:(WRITE) flags 0xc800 phys_seg 4 prio class 0
^ permalink raw reply
* Re: KVM on POWER8 host lock up since 10d91611f426 ("powerpc/64s: Reimplement book3s idle code in C")
From: Michal Suchánek @ 2020-08-31 9:15 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: ro, linuxppc-dev
In-Reply-To: <1598835313.5688ngko4f.astroid@bobo.none>
On Mon, Aug 31, 2020 at 11:14:18AM +1000, Nicholas Piggin wrote:
> Excerpts from Michal Suchánek's message of August 31, 2020 6:11 am:
> > Hello,
> >
> > on POWER8 KVM hosts lock up since commit 10d91611f426 ("powerpc/64s:
> > Reimplement book3s idle code in C").
> >
> > The symptom is host locking up completely after some hours of KVM
> > workload with messages like
> >
> > 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
> > 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 71
> > 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
> > 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 71
> > 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
> >
> > printed before the host locks up.
> >
> > The machines run sandboxed builds which is a mixed workload resulting in
> > IO/single core/mutiple core load over time and there are periods of no
> > activity and no VMS runnig as well. The VMs are shortlived so VM
> > setup/terdown is somewhat excercised as well.
> >
> > POWER9 with the new guest entry fast path does not seem to be affected.
> >
> > Reverted the patch and the followup idle fixes on top of 5.2.14 and
> > re-applied commit a3f3072db6ca ("powerpc/powernv/idle: Restore IAMR
> > after idle") which gives same idle code as 5.1.16 and the kernel seems
> > stable.
> >
> > Config is attached.
> >
> > I cannot easily revert this commit, especially if I want to use the same
> > kernel on POWER8 and POWER9 - many of the POWER9 fixes are applicable
> > only to the new idle code.
> >
> > Any idea what can be the problem?
>
> So hwthread_state is never getting back to to HWTHREAD_IN_IDLE on
> those threads. I wonder what they are doing. POWER8 doesn't have a good
> NMI IPI and I don't know if it supports pdbg dumping registers from the
> BMC unfortunately.
It may be possible to set up fadump with a later kernel version that
supports it on powernv and dump the whole kernel.
Thanks
Michal
^ permalink raw reply
* Re: KVM on POWER8 host lock up since 10d91611f426 ("powerpc/64s: Reimplement book3s idle code in C")
From: Michal Suchánek @ 2020-08-31 9:07 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: ro, linuxppc-dev
In-Reply-To: <1598835313.5688ngko4f.astroid@bobo.none>
On Mon, Aug 31, 2020 at 11:14:18AM +1000, Nicholas Piggin wrote:
> Excerpts from Michal Suchánek's message of August 31, 2020 6:11 am:
> > Hello,
> >
> > on POWER8 KVM hosts lock up since commit 10d91611f426 ("powerpc/64s:
> > Reimplement book3s idle code in C").
> >
> > The symptom is host locking up completely after some hours of KVM
> > workload with messages like
> >
> > 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
> > 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 71
> > 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
> > 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 71
> > 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
> >
> > printed before the host locks up.
> >
> > The machines run sandboxed builds which is a mixed workload resulting in
> > IO/single core/mutiple core load over time and there are periods of no
> > activity and no VMS runnig as well. The VMs are shortlived so VM
> > setup/terdown is somewhat excercised as well.
> >
> > POWER9 with the new guest entry fast path does not seem to be affected.
> >
> > Reverted the patch and the followup idle fixes on top of 5.2.14 and
> > re-applied commit a3f3072db6ca ("powerpc/powernv/idle: Restore IAMR
> > after idle") which gives same idle code as 5.1.16 and the kernel seems
> > stable.
> >
> > Config is attached.
> >
> > I cannot easily revert this commit, especially if I want to use the same
> > kernel on POWER8 and POWER9 - many of the POWER9 fixes are applicable
> > only to the new idle code.
> >
> > Any idea what can be the problem?
>
> So hwthread_state is never getting back to to HWTHREAD_IN_IDLE on
> those threads. I wonder what they are doing. POWER8 doesn't have a good
> NMI IPI and I don't know if it supports pdbg dumping registers from the
> BMC unfortunately. Do the messages always come in pairs of CPUs?
>
> I'm not sure where to start with reproducing, I'll have to try. How many
> vCPUs in the guests? Do you have several guests running at once?
The guests are spawned on demand - there are like 20-30 'slots'
configured where a VM may be running or it may be idle with no VM
spawned when there are no jobs available.
Thanks
Michal
^ permalink raw reply
* [PATCH 1/2] powerpc/8xx: Refactor calculation of number of entries per PTE in page tables
From: Christophe Leroy @ 2020-08-31 8:30 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
On 8xx, the number of entries occupied by a PTE in the page tables
depends on the size of the page. At the time being, this calculation
is done in two places: in pte_update() and in set_huge_pte_at()
Refactor this calculation into a helper called
number_of_cells_per_pte(). For the time being, the val param is
unused. It will be used by following patch.
Instead of opencoding is_hugepd(), use hugepd_ok() with a forward
declaration.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/nohash/32/pgtable.h | 18 ++++++++++++------
arch/powerpc/mm/pgtable.c | 6 ++++--
2 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/include/asm/nohash/32/pgtable.h b/arch/powerpc/include/asm/nohash/32/pgtable.h
index b9e134d0f03a..80bbc21b87f0 100644
--- a/arch/powerpc/include/asm/nohash/32/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/32/pgtable.h
@@ -227,6 +227,17 @@ static inline void pmd_clear(pmd_t *pmdp)
*/
#ifdef CONFIG_PPC_8xx
static pmd_t *pmd_off(struct mm_struct *mm, unsigned long addr);
+static int hugepd_ok(hugepd_t hpd);
+
+static int number_of_cells_per_pte(pmd_t *pmd, pte_basic_t val, int huge)
+{
+ if (!huge)
+ return PAGE_SIZE / SZ_4K;
+ else if (hugepd_ok(*((hugepd_t *)pmd)))
+ return 1;
+ else
+ return SZ_512K / SZ_4K;
+}
static inline pte_basic_t pte_update(struct mm_struct *mm, unsigned long addr, pte_t *p,
unsigned long clr, unsigned long set, int huge)
@@ -237,12 +248,7 @@ static inline pte_basic_t pte_update(struct mm_struct *mm, unsigned long addr, p
int num, i;
pmd_t *pmd = pmd_off(mm, addr);
- if (!huge)
- num = PAGE_SIZE / SZ_4K;
- else if ((pmd_val(*pmd) & _PMD_PAGE_MASK) != _PMD_PAGE_8M)
- num = SZ_512K / SZ_4K;
- else
- num = 1;
+ num = number_of_cells_per_pte(pmd, new, huge);
for (i = 0; i < num; i++, entry++, new += SZ_4K)
*entry = new;
diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
index 9c0547d77af3..2dcad640b869 100644
--- a/arch/powerpc/mm/pgtable.c
+++ b/arch/powerpc/mm/pgtable.c
@@ -266,8 +266,7 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep, pte_
pmd_t *pmd = pmd_off(mm, addr);
pte_basic_t val;
pte_basic_t *entry = &ptep->pte;
- int num = is_hugepd(*((hugepd_t *)pmd)) ? 1 : SZ_512K / SZ_4K;
- int i;
+ int num, i;
/*
* Make sure hardware valid bit is not set. We don't do
@@ -280,6 +279,9 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep, pte_
pte = set_pte_filter(pte);
val = pte_val(pte);
+
+ num = number_of_cells_per_pte(pmd, val, 1);
+
for (i = 0; i < num; i++, entry++, val += SZ_4K)
*entry = val;
}
--
2.25.0
^ permalink raw reply related
* [PATCH 2/2] powerpc/8xx: Support 16k hugepages with 4k pages
From: Christophe Leroy @ 2020-08-31 8:30 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <f6ea2483c2c389567b007945948f704d18cfaeea.1598862623.git.christophe.leroy@csgroup.eu>
The 8xx has 4 page sizes: 4k, 16k, 512k and 8M
4k and 16k can be selected at build time as standard page sizes,
and 512k and 8M are hugepages.
When 4k standard pages are selected, 16k pages are not available.
Allow 16k pages as hugepages when 4k pages are used.
To allow that, implement arch_make_huge_pte() which receives
the necessary arguments to allow setting the PTE in accordance
with the page size:
- 512 k pages must have _PAGE_HUGE and _PAGE_SPS. They are set
by pte_mkhuge(). arch_make_huge_pte() does nothing.
- 16 k pages must have only _PAGE_SPS. arch_make_huge_pte() clears
_PAGE_HUGE.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/nohash/32/hugetlb-8xx.h | 14 ++++++++++++++
arch/powerpc/include/asm/nohash/32/pgtable.h | 2 ++
arch/powerpc/mm/hugetlbpage.c | 2 +-
arch/powerpc/mm/nohash/tlb.c | 4 ----
arch/powerpc/mm/ptdump/8xx.c | 5 +++++
include/uapi/asm-generic/hugetlb_encode.h | 1 +
include/uapi/linux/mman.h | 1 +
7 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/include/asm/nohash/32/hugetlb-8xx.h b/arch/powerpc/include/asm/nohash/32/hugetlb-8xx.h
index e752a5807a59..39be9aea86db 100644
--- a/arch/powerpc/include/asm/nohash/32/hugetlb-8xx.h
+++ b/arch/powerpc/include/asm/nohash/32/hugetlb-8xx.h
@@ -65,4 +65,18 @@ static inline void huge_ptep_set_wrprotect(struct mm_struct *mm,
pte_update(mm, addr, ptep, clr, set, 1);
}
+#ifdef CONFIG_PPC_4K_PAGES
+static inline pte_t arch_make_huge_pte(pte_t entry, struct vm_area_struct *vma,
+ struct page *page, int writable)
+{
+ size_t size = huge_page_size(hstate_vma(vma));
+
+ if (size == SZ_16K)
+ return __pte(pte_val(entry) & ~_PAGE_HUGE);
+ else
+ return entry;
+}
+#define arch_make_huge_pte arch_make_huge_pte
+#endif
+
#endif /* _ASM_POWERPC_NOHASH_32_HUGETLB_8XX_H */
diff --git a/arch/powerpc/include/asm/nohash/32/pgtable.h b/arch/powerpc/include/asm/nohash/32/pgtable.h
index 80bbc21b87f0..ee2243ba96cf 100644
--- a/arch/powerpc/include/asm/nohash/32/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/32/pgtable.h
@@ -235,6 +235,8 @@ static int number_of_cells_per_pte(pmd_t *pmd, pte_basic_t val, int huge)
return PAGE_SIZE / SZ_4K;
else if (hugepd_ok(*((hugepd_t *)pmd)))
return 1;
+ else if (IS_ENABLED(CONFIG_PPC_4K_PAGES) && !(val & _PAGE_HUGE))
+ return SZ_16K / SZ_4K;
else
return SZ_512K / SZ_4K;
}
diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index e7ae2a2c4545..36c3800769fb 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c
@@ -180,7 +180,7 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, unsigned long addr, unsigned long sz
if (!hpdp)
return NULL;
- if (IS_ENABLED(CONFIG_PPC_8xx) && sz == SZ_512K)
+ if (IS_ENABLED(CONFIG_PPC_8xx) && pshift < PMD_SHIFT)
return pte_alloc_map(mm, (pmd_t *)hpdp, addr);
BUG_ON(!hugepd_none(*hpdp) && !hugepd_ok(*hpdp));
diff --git a/arch/powerpc/mm/nohash/tlb.c b/arch/powerpc/mm/nohash/tlb.c
index 14514585db98..5872f69141d5 100644
--- a/arch/powerpc/mm/nohash/tlb.c
+++ b/arch/powerpc/mm/nohash/tlb.c
@@ -83,16 +83,12 @@ struct mmu_psize_def mmu_psize_defs[MMU_PAGE_COUNT] = {
};
#elif defined(CONFIG_PPC_8xx)
struct mmu_psize_def mmu_psize_defs[MMU_PAGE_COUNT] = {
- /* we only manage 4k and 16k pages as normal pages */
-#ifdef CONFIG_PPC_4K_PAGES
[MMU_PAGE_4K] = {
.shift = 12,
},
-#else
[MMU_PAGE_16K] = {
.shift = 14,
},
-#endif
[MMU_PAGE_512K] = {
.shift = 19,
},
diff --git a/arch/powerpc/mm/ptdump/8xx.c b/arch/powerpc/mm/ptdump/8xx.c
index 8a797dcbf475..86da2a669680 100644
--- a/arch/powerpc/mm/ptdump/8xx.c
+++ b/arch/powerpc/mm/ptdump/8xx.c
@@ -11,8 +11,13 @@
static const struct flag_info flag_array[] = {
{
+#ifdef CONFIG_PPC_16K_PAGES
.mask = _PAGE_HUGE,
.val = _PAGE_HUGE,
+#else
+ .mask = _PAGE_SPS,
+ .val = _PAGE_SPS,
+#endif
.set = "huge",
.clear = " ",
}, {
diff --git a/include/uapi/asm-generic/hugetlb_encode.h b/include/uapi/asm-generic/hugetlb_encode.h
index b0f8e87235bd..4f3d5aaa11f5 100644
--- a/include/uapi/asm-generic/hugetlb_encode.h
+++ b/include/uapi/asm-generic/hugetlb_encode.h
@@ -20,6 +20,7 @@
#define HUGETLB_FLAG_ENCODE_SHIFT 26
#define HUGETLB_FLAG_ENCODE_MASK 0x3f
+#define HUGETLB_FLAG_ENCODE_16KB (14 << HUGETLB_FLAG_ENCODE_SHIFT)
#define HUGETLB_FLAG_ENCODE_64KB (16 << HUGETLB_FLAG_ENCODE_SHIFT)
#define HUGETLB_FLAG_ENCODE_512KB (19 << HUGETLB_FLAG_ENCODE_SHIFT)
#define HUGETLB_FLAG_ENCODE_1MB (20 << HUGETLB_FLAG_ENCODE_SHIFT)
diff --git a/include/uapi/linux/mman.h b/include/uapi/linux/mman.h
index 923cc162609c..f55bc680b5b0 100644
--- a/include/uapi/linux/mman.h
+++ b/include/uapi/linux/mman.h
@@ -27,6 +27,7 @@
#define MAP_HUGE_SHIFT HUGETLB_FLAG_ENCODE_SHIFT
#define MAP_HUGE_MASK HUGETLB_FLAG_ENCODE_MASK
+#define MAP_HUGE_16KB HUGETLB_FLAG_ENCODE_16KB
#define MAP_HUGE_64KB HUGETLB_FLAG_ENCODE_64KB
#define MAP_HUGE_512KB HUGETLB_FLAG_ENCODE_512KB
#define MAP_HUGE_1MB HUGETLB_FLAG_ENCODE_1MB
--
2.25.0
^ permalink raw reply related
* [PATCH] selftests/vm: Fix display of page size in map_hugetlb
From: Christophe Leroy @ 2020-08-31 8:23 UTC (permalink / raw)
To: Shuah Khan, Andrew Morton; +Cc: linuxppc-dev, linux-kernel, linux-kselftest
The displayed size is in bytes while the text says it is in kB.
Shift it by 10 to really display kBytes.
Fixes: fa7b9a805c79 ("tools/selftest/vm: allow choosing mem size and page size in map_hugetlb")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
tools/testing/selftests/vm/map_hugetlb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/vm/map_hugetlb.c b/tools/testing/selftests/vm/map_hugetlb.c
index 6af951900aa3..312889edb84a 100644
--- a/tools/testing/selftests/vm/map_hugetlb.c
+++ b/tools/testing/selftests/vm/map_hugetlb.c
@@ -83,7 +83,7 @@ int main(int argc, char **argv)
}
if (shift)
- printf("%u kB hugepages\n", 1 << shift);
+ printf("%u kB hugepages\n", 1 << (shift - 10));
else
printf("Default size hugepages\n");
printf("Mapping %lu Mbytes\n", (unsigned long)length >> 20);
--
2.25.0
^ permalink raw reply related
* [PATCH] powerpc: Fix random segfault when freeing hugetlb range
From: Christophe Leroy @ 2020-08-31 7:58 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
The following random segfault is observed from time to time with
map_hugetlb selftest:
root@localhost:~# ./map_hugetlb 1 19
524288 kB hugepages
Mapping 1 Mbytes
Segmentation fault
[ 31.219972] map_hugetlb[365]: segfault (11) at 117 nip 77974f8c lr 779a6834 code 1 in ld-2.23.so[77966000+21000]
[ 31.220192] map_hugetlb[365]: code: 9421ffc0 480318d1 93410028 90010044 9361002c 93810030 93a10034 93c10038
[ 31.220307] map_hugetlb[365]: code: 93e1003c 93210024 8123007c 81430038 <80e90004> 814a0004 7f443a14 813a0004
[ 31.221911] BUG: Bad rss-counter state mm:(ptrval) type:MM_FILEPAGES val:33
[ 31.229362] BUG: Bad rss-counter state mm:(ptrval) type:MM_ANONPAGES val:5
This fault is due to hugetlb_free_pgd_range() freeing page tables
that are also used by regular pages.
As explain in the comment at the beginning of
hugetlb_free_pgd_range(), the verification done in free_pgd_range()
on floor and ceiling is not done here, which means
hugetlb_free_pte_range() can free outside the expected range.
As the verification cannot be done in hugetlb_free_pgd_range(), it
must be done in hugetlb_free_pte_range().
Fixes: b250c8c08c79 ("powerpc/8xx: Manage 512k huge pages as standard pages.")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/mm/hugetlbpage.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index 26292544630f..e7ae2a2c4545 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c
@@ -330,10 +330,24 @@ static void free_hugepd_range(struct mmu_gather *tlb, hugepd_t *hpdp, int pdshif
get_hugepd_cache_index(pdshift - shift));
}
-static void hugetlb_free_pte_range(struct mmu_gather *tlb, pmd_t *pmd, unsigned long addr)
+static void hugetlb_free_pte_range(struct mmu_gather *tlb, pmd_t *pmd,
+ unsigned long addr, unsigned long end,
+ unsigned long floor, unsigned long ceiling)
{
+ unsigned long start = addr;
pgtable_t token = pmd_pgtable(*pmd);
+ start &= PMD_MASK;
+ if (start < floor)
+ return;
+ if (ceiling) {
+ ceiling &= PMD_MASK;
+ if (!ceiling)
+ return;
+ }
+ if (end - 1 > ceiling - 1)
+ return;
+
pmd_clear(pmd);
pte_free_tlb(tlb, token, addr);
mm_dec_nr_ptes(tlb->mm);
@@ -363,7 +377,7 @@ static void hugetlb_free_pmd_range(struct mmu_gather *tlb, pud_t *pud,
*/
WARN_ON(!IS_ENABLED(CONFIG_PPC_8xx));
- hugetlb_free_pte_range(tlb, pmd, addr);
+ hugetlb_free_pte_range(tlb, pmd, addr, end, floor, ceiling);
continue;
}
--
2.25.0
^ permalink raw reply related
* Re: [PATCH 5/5] powerpc: use the generic dma_ops_bypass mode
From: Christoph Hellwig @ 2020-08-31 6:40 UTC (permalink / raw)
To: Cédric Le Goater
Cc: linuxppc-dev, Daniel Borkmann, Alexey Kardashevskiy,
Björn Töpel, Joerg Roedel, Jesper Dangaard Brouer,
linux-kernel, iommu, Oliver O'Halloran, Greg Kroah-Hartman,
aacraid, Robin Murphy, Christoph Hellwig, Lu Baolu
In-Reply-To: <505bcc1d-01a7-9655-88e1-ebddd0b94d56@kaod.org>
On Sun, Aug 30, 2020 at 11:04:21AM +0200, Cédric Le Goater wrote:
> Hello,
>
> On 7/8/20 5:24 PM, Christoph Hellwig wrote:
> > Use the DMA API bypass mechanism for direct window mappings. This uses
> > common code and speed up the direct mapping case by avoiding indirect
> > calls just when not using dma ops at all. It also fixes a problem where
> > the sync_* methods were using the bypass check for DMA allocations, but
> > those are part of the streaming ops.
> >
> > Note that this patch loses the DMA_ATTR_WEAK_ORDERING override, which
> > has never been well defined, as is only used by a few drivers, which
> > IIRC never showed up in the typical Cell blade setups that are affected
> > by the ordering workaround.
> >
> > Fixes: efd176a04bef ("powerpc/pseries/dma: Allow SWIOTLB")
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> > arch/powerpc/Kconfig | 1 +
> > arch/powerpc/include/asm/device.h | 5 --
> > arch/powerpc/kernel/dma-iommu.c | 90 ++++---------------------------
> > 3 files changed, 10 insertions(+), 86 deletions(-)
>
> I am seeing corruptions on a couple of POWER9 systems (boston) when
> stressed with IO. stress-ng gives some results but I have first seen
> it when compiling the kernel in a guest and this is still the best way
> to raise the issue.
>
> These systems have of a SAS Adaptec controller :
>
> 0003:01:00.0 Serial Attached SCSI controller: Adaptec Series 8 12G SAS/PCIe 3 (rev 01)
>
> When the failure occurs, the POWERPC EEH interrupt fires and dumps
> lowlevel PHB4 registers among which :
>
> [ 2179.251069490,3] PHB#0003[0:3]: phbErrorStatus = 0000028000000000
> [ 2179.251117476,3] PHB#0003[0:3]: phbFirstErrorStatus = 0000020000000000
>
> The bits raised identify a PPC 'TCE' error, which means it is related
> to DMAs. See below for more details.
>
>
> Reverting this patch "fixes" the issue but it is probably else where,
> in some other layers or in the aacraid driver. How should I proceed
> to get more information ?
The aacraid DMA masks look like a mess. Can you try the hack
below and see it it helps?
diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 769af4ca9ca97e..79c6b744dbb66c 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -2228,18 +2228,6 @@ int aac_get_adapter_info(struct aac_dev* dev)
expose_physicals = 0;
}
- if (dev->dac_support) {
- if (!pci_set_dma_mask(dev->pdev, DMA_BIT_MASK(64))) {
- if (!dev->in_reset)
- dev_info(&dev->pdev->dev, "64 Bit DAC enabled\n");
- } else if (!pci_set_dma_mask(dev->pdev, DMA_BIT_MASK(32))) {
- dev_info(&dev->pdev->dev, "DMA mask set failed, 64 Bit DAC disabled\n");
- dev->dac_support = 0;
- } else {
- dev_info(&dev->pdev->dev, "No suitable DMA available\n");
- rcode = -ENOMEM;
- }
- }
/*
* Deal with configuring for the individualized limits of each packet
* interface.
diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index adbdc3b7c7a706..dbb23b351a4e7d 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -1479,7 +1479,6 @@ static int _aac_reset_adapter(struct aac_dev *aac, int forced, u8 reset_type)
struct Scsi_Host *host = aac->scsi_host_ptr;
int jafo = 0;
int bled;
- u64 dmamask;
int num_of_fibs = 0;
/*
@@ -1558,22 +1557,7 @@ static int _aac_reset_adapter(struct aac_dev *aac, int forced, u8 reset_type)
kfree(aac->fsa_dev);
aac->fsa_dev = NULL;
- dmamask = DMA_BIT_MASK(32);
quirks = aac_get_driver_ident(index)->quirks;
- if (quirks & AAC_QUIRK_31BIT)
- retval = pci_set_dma_mask(aac->pdev, dmamask);
- else if (!(quirks & AAC_QUIRK_SRC))
- retval = pci_set_dma_mask(aac->pdev, dmamask);
- else
- retval = pci_set_consistent_dma_mask(aac->pdev, dmamask);
-
- if (quirks & AAC_QUIRK_31BIT && !retval) {
- dmamask = DMA_BIT_MASK(31);
- retval = pci_set_consistent_dma_mask(aac->pdev, dmamask);
- }
-
- if (retval)
- goto out;
if ((retval = (*(aac_get_driver_ident(index)->init))(aac)))
goto out;
diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index 8588da0a065551..d897a9d59e24a1 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -1634,8 +1634,6 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
struct list_head *insert = &aac_devices;
int error;
int unique_id = 0;
- u64 dmamask;
- int mask_bits = 0;
extern int aac_sync_mode;
/*
@@ -1658,33 +1656,6 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
if (error)
goto out;
- if (!(aac_drivers[index].quirks & AAC_QUIRK_SRC)) {
- error = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
- if (error) {
- dev_err(&pdev->dev, "PCI 32 BIT dma mask set failed");
- goto out_disable_pdev;
- }
- }
-
- /*
- * If the quirk31 bit is set, the adapter needs adapter
- * to driver communication memory to be allocated below 2gig
- */
- if (aac_drivers[index].quirks & AAC_QUIRK_31BIT) {
- dmamask = DMA_BIT_MASK(31);
- mask_bits = 31;
- } else {
- dmamask = DMA_BIT_MASK(32);
- mask_bits = 32;
- }
-
- error = pci_set_consistent_dma_mask(pdev, dmamask);
- if (error) {
- dev_err(&pdev->dev, "PCI %d B consistent dma mask set failed\n"
- , mask_bits);
- goto out_disable_pdev;
- }
-
pci_set_master(pdev);
shost = scsi_host_alloc(&aac_driver_template, sizeof(struct aac_dev));
^ permalink raw reply related
* Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()
From: Vinod Koul @ 2020-08-31 6:39 UTC (permalink / raw)
To: Linus Torvalds
Cc: Herbert Xu, Joerg Roedel, Linux Kernel Mailing List, Li Yang,
Zhang Wei, Guenter Roeck, dma, Andrew Morton, linuxppc-dev,
Dan Williams, Luc Van Oostenryck
In-Reply-To: <CAHk-=wjDEiWF_DsCVFPFqNa+JCS5SkOygbqeq8_=ZNOrFt7-rg@mail.gmail.com>
Hi Linus,
On 29-08-20, 14:20, Linus Torvalds wrote:
> On Sat, Aug 29, 2020 at 1:40 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > Except for
> >
> > CHECK: spaces preferred around that '+' (ctx:VxV)
> > #29: FILE: drivers/dma/fsldma.h:223:
> > + u32 val_lo = in_be32((u32 __iomem *)addr+1);
>
> Added spaces.
>
> > I don't see anything wrong with it either, so
> >
> > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> >
> > Since I didn't see the real problem with the original code,
> > I'd take that with a grain of salt, though.
>
> Well, honestly, the old code was so confused that just making it build
> is clearly already an improvement even if everything else were to be
> wrong.
>
> So I committed my "fix". If it turns out there's more wrong in there
> and somebody tests it, we can fix it again. But now it hopefully
> compiles, at least.
>
> My bet is that if that driver ever worked on ppc32, it will continue
> to work whatever we do to that function.
>
> I _think_ the old code happened to - completely by mistake - get the
> value right for the case of "little endian access, with dma_addr_t
> being 32-bit". Because then it would still read the upper bits wrong,
> but the cast to dma_addr_t would then throw those bits away. And the
> lower bits would be right.
>
> But for big-endian accesses or for ARCH_DMA_ADDR_T_64BIT it really
> looks like it always returned a completely incorrect value.
>
> And again - the driver may have worked even with that completely
> incorrect value, since the use of it seems to be very incidental.
Thank you for the fix.
Acked-By: Vinod Koul <vkoul@kernel.org>
>
> In either case ("it didn't work before" or "it worked because the
> value doesn't really matter"), I don't think I could possibly have
> made things worse.
>
> Famous last words.
I guess no one tested this on 32bits seems to have caused this.
--
~Vinod
^ permalink raw reply
* [PATCH] powerpc/powernv/pci: Drop pnv_phb->initialized
From: Oliver O'Halloran @ 2020-08-31 6:15 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Oliver O'Halloran
The pnv_phb->initialized flag is an odd beast. It was added back in 2012 in
commit db1266c85261 ("powerpc/powernv: Skip check on PE if necessary") to
allow devices to be enabled even if their PE assignments hadn't been
completed yet. I can't think of any situation where we would (or should)
have PCI devices being enabled before their PEs are assigned, so I can only
assume it was a workaround for a bug or some other undesirable behaviour
from the PCI core.
Since commit dc3d8f85bb57 ("powerpc/powernv/pci: Re-work bus PE
configuration") the PE setup occurs before the PCI core allows driver to
attach to the device so the problem should no longer exist. Even it does
allowing the device to be enabled before we have assigned the device to a
PE is almost certainly broken and will cause spurious EEH events so we
should probably just remove it.
It's also worth pointing out that ->initialized flag is set in
pnv_pci_ioda_create_dbgfs() which has the entire function body wrapped in
flag. That has the fun side effect of bypassing any other checks in
pnv_pci_enable_device_hook() which is probably not what anybody wants.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/platforms/powernv/pci-ioda.c | 17 -----------------
arch/powerpc/platforms/powernv/pci.h | 1 -
2 files changed, 18 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 023a4f987bb2..6ac3c637b313 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -2410,9 +2410,6 @@ static void pnv_pci_ioda_create_dbgfs(void)
list_for_each_entry_safe(hose, tmp, &hose_list, list_node) {
phb = hose->private_data;
- /* Notify initialization of PHB done */
- phb->initialized = 1;
-
sprintf(name, "PCI%04x", hose->global_number);
phb->dbgfs = debugfs_create_dir(name, powerpc_debugfs_root);
@@ -2609,17 +2606,8 @@ static resource_size_t pnv_pci_default_alignment(void)
*/
static bool pnv_pci_enable_device_hook(struct pci_dev *dev)
{
- struct pnv_phb *phb = pci_bus_to_pnvhb(dev->bus);
struct pci_dn *pdn;
- /* The function is probably called while the PEs have
- * not be created yet. For example, resource reassignment
- * during PCI probe period. We just skip the check if
- * PEs isn't ready.
- */
- if (!phb->initialized)
- return true;
-
pdn = pci_get_pdn(dev);
if (!pdn || pdn->pe_number == IODA_INVALID_PE)
return false;
@@ -2629,14 +2617,9 @@ static bool pnv_pci_enable_device_hook(struct pci_dev *dev)
static bool pnv_ocapi_enable_device_hook(struct pci_dev *dev)
{
- struct pci_controller *hose = pci_bus_to_host(dev->bus);
- struct pnv_phb *phb = hose->private_data;
struct pci_dn *pdn;
struct pnv_ioda_pe *pe;
- if (!phb->initialized)
- return true;
-
pdn = pci_get_pdn(dev);
if (!pdn)
return false;
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index 739a0b3b72e1..36d22920f5a3 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -119,7 +119,6 @@ struct pnv_phb {
int flags;
void __iomem *regs;
u64 regs_phys;
- int initialized;
spinlock_t lock;
#ifdef CONFIG_DEBUG_FS
--
2.26.2
^ permalink raw reply related
* Re: [PATCH v1 09/10] powerpc/pseries/iommu: Make use of DDW even if it does not map the partition
From: Alexey Kardashevskiy @ 2020-08-31 4:35 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <f3ccef770985b8dfd8d5cb2b9f7b55f2e734d402.camel@gmail.com>
On 29/08/2020 04:36, Leonardo Bras wrote:
> On Mon, 2020-08-24 at 15:17 +1000, Alexey Kardashevskiy wrote:
>>
>> On 18/08/2020 09:40, Leonardo Bras wrote:
>>> As of today, if the biggest DDW that can be created can't map the whole
>>> partition, it's creation is skipped and the default DMA window
>>> "ibm,dma-window" is used instead.
>>>
>>> DDW is 16x bigger than the default DMA window,
>>
>> 16x only under very specific circumstances which are
>> 1. phyp
>> 2. sriov
>> 3. device class in hmc (or what that priority number is in the lpar config).
>
> Yeah, missing details.
>
>>> having the same amount of
>>> pages, but increasing the page size to 64k.
>>> Besides larger DMA window,
>>
>> "Besides being larger"?
>
> You are right there.
>
>>
>>> it performs better for allocations over 4k,
>>
>> Better how?
>
> I was thinking for allocations larger than (512 * 4k), since >2
> hypercalls are needed here, and for 64k pages would still be just 1
> hypercall up to (512 * 64k).
> But yeah, not the usual case anyway.
Yup.
>
>>
>>> so it would be nice to use it instead.
>>
>> I'd rather say something like:
>> ===
>> So far we assumed we can map the guest RAM 1:1 to the bus which worked
>> with a small number of devices. SRIOV changes it as the user can
>> configure hundreds VFs and since phyp preallocates TCEs and does not
>> allow IOMMU pages bigger than 64K, it has to limit the number of TCEs
>> per a PE to limit waste of physical pages.
>> ===
>
> I mixed this in my commit message, it looks like this:
>
> ===
> powerpc/pseries/iommu: Make use of DDW for indirect mapping
>
> So far it's assumed possible to map the guest RAM 1:1 to the bus, which
> works with a small number of devices. SRIOV changes it as the user can
> configure hundreds VFs and since phyp preallocates TCEs and does not
> allow IOMMU pages bigger than 64K, it has to limit the number of TCEs
> per a PE to limit waste of physical pages.
>
> As of today, if the assumed direct mapping is not possible, DDW
> creation is skipped and the default DMA window "ibm,dma-window" is used
> instead.
>
> The default DMA window uses 4k pages instead of 64k pages, and since
> the amount of pages is the same,
Is the amount really the same? I thought you can prioritize some VFs
over others (== allocate different number of TCEs). Does it really
matter if it is the same?
> making use of DDW instead of the
> default DMA window for indirect mapping will expand in 16x the amount
> of memory that can be mapped on DMA.
Stop saying "16x", it is not guaranteed by anything :)
>
> The DDW created will be used for direct mapping by default. [...]
> ===
>
> What do you think?
>
>>> The DDW created will be used for direct mapping by default.
>>> If it's not available, indirect mapping will be used instead.
>>>
>>> For indirect mapping, it's necessary to update the iommu_table so
>>> iommu_alloc() can use the DDW created. For this,
>>> iommu_table_update_window() is called when everything else succeeds
>>> at enable_ddw().
>>>
>>> Removing the default DMA window for using DDW with indirect mapping
>>> is only allowed if there is no current IOMMU memory allocated in
>>> the iommu_table. enable_ddw() is aborted otherwise.
>>>
>>> As there will never have both direct and indirect mappings at the same
>>> time, the same property name can be used for the created DDW.
>>>
>>> So renaming
>>> define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
>>> to
>>> define DMA64_PROPNAME "linux,dma64-ddr-window-info"
>>> looks the right thing to do.
>>
>> I know I suggested this but this does not look so good anymore as I
>> suspect it breaks kexec (from older kernel to this one) so you either
>> need to check for both DT names or just keep the old one. Changing the
>> macro name is fine.
>>
>
> Yeah, having 'direct' in the name don't really makes sense if it's used
> for indirect mapping. I will just add this new define instead of
> replacing the old one, and check for both.
> Is that ok?
No, having two of these does not seem right or useful. It is pseries
which does not use petitboot (relies on grub instead so until the target
kernel is started, there will be no ddw) so realistically we need this
property for kexec/kdump which uses the same kernel but different
initramdisk so for that purpose we need the same property name.
But I can see myself annoyed when I try petitboot in the hacked pseries
qemu and things may crash :) On this basis I'd suggest keeping the name
and adding a comment next to it that it is not always "direct" anymore.
>
>>
>>> To make sure the property differentiates both cases, a new u32 for flags
>>> was added at the end of the property, where BIT(0) set means direct
>>> mapping.
>>>
>>> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
>>> ---
>>> arch/powerpc/platforms/pseries/iommu.c | 108 +++++++++++++++++++------
>>> 1 file changed, 84 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
>>> index 3a1ef02ad9d5..9544e3c91ced 100644
>>> --- a/arch/powerpc/platforms/pseries/iommu.c
>>> +++ b/arch/powerpc/platforms/pseries/iommu.c
>>> @@ -350,8 +350,11 @@ struct dynamic_dma_window_prop {
>>> __be64 dma_base; /* address hi,lo */
>>> __be32 tce_shift; /* ilog2(tce_page_size) */
>>> __be32 window_shift; /* ilog2(tce_window_size) */
>>> + __be32 flags; /* DDW properties, see bellow */
>>> };
>>>
>>> +#define DDW_FLAGS_DIRECT 0x01
>>
>> This is set if ((1<<window_shift) >= ddw_memory_hotplug_max()), you
>> could simply check window_shift and drop the flags.
>>
>
> Yeah, it's better this way, I will revert this.
>
>>
>>> +
>>> struct direct_window {
>>> struct device_node *device;
>>> const struct dynamic_dma_window_prop *prop;
>>> @@ -377,7 +380,7 @@ static LIST_HEAD(direct_window_list);
>>> static DEFINE_SPINLOCK(direct_window_list_lock);
>>> /* protects initializing window twice for same device */
>>> static DEFINE_MUTEX(direct_window_init_mutex);
>>> -#define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
>>> +#define DMA64_PROPNAME "linux,dma64-ddr-window-info"
>>>
>>> static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
>>> unsigned long num_pfn, const void *arg)
>>> @@ -836,7 +839,7 @@ static void remove_ddw(struct device_node *np, bool remove_prop)
>>> if (ret)
>>> return;
>>>
>>> - win = of_find_property(np, DIRECT64_PROPNAME, NULL);
>>> + win = of_find_property(np, DMA64_PROPNAME, NULL);
>>> if (!win)
>>> return;
>>>
>>> @@ -852,7 +855,7 @@ static void remove_ddw(struct device_node *np, bool remove_prop)
>>> np, ret);
>>> }
>>>
>>> -static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr)
>>> +static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr, bool *direct_mapping)
>>> {
>>> struct direct_window *window;
>>> const struct dynamic_dma_window_prop *direct64;
>>> @@ -864,6 +867,7 @@ static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr)
>>> if (window->device == pdn) {
>>> direct64 = window->prop;
>>> *dma_addr = be64_to_cpu(direct64->dma_base);
>>> + *direct_mapping = be32_to_cpu(direct64->flags) & DDW_FLAGS_DIRECT;
>>> found = true;
>>> break;
>>> }
>>> @@ -901,8 +905,8 @@ static int find_existing_ddw_windows(void)
>>> if (!firmware_has_feature(FW_FEATURE_LPAR))
>>> return 0;
>>>
>>> - for_each_node_with_property(pdn, DIRECT64_PROPNAME) {
>>> - direct64 = of_get_property(pdn, DIRECT64_PROPNAME, &len);
>>> + for_each_node_with_property(pdn, DMA64_PROPNAME) {
>>> + direct64 = of_get_property(pdn, DMA64_PROPNAME, &len);
>>> if (!direct64)
>>> continue;
>>>
>>> @@ -1124,7 +1128,8 @@ static void reset_dma_window(struct pci_dev *dev, struct device_node *par_dn)
>>> }
>>>
>>> static int ddw_property_create(struct property **ddw_win, const char *propname,
>>> - u32 liobn, u64 dma_addr, u32 page_shift, u32 window_shift)
>>> + u32 liobn, u64 dma_addr, u32 page_shift,
>>> + u32 window_shift, bool direct_mapping)
>>> {
>>> struct dynamic_dma_window_prop *ddwprop;
>>> struct property *win64;
>>> @@ -1144,6 +1149,36 @@ static int ddw_property_create(struct property **ddw_win, const char *propname,
>>> ddwprop->dma_base = cpu_to_be64(dma_addr);
>>> ddwprop->tce_shift = cpu_to_be32(page_shift);
>>> ddwprop->window_shift = cpu_to_be32(window_shift);
>>> + if (direct_mapping)
>>> + ddwprop->flags = cpu_to_be32(DDW_FLAGS_DIRECT);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int iommu_table_update_window(struct iommu_table **tbl, int nid, unsigned long liobn,
>>> + unsigned long win_addr, unsigned long page_shift,
>>> + unsigned long window_size)
>>
>> Rather strange helper imho. I'd extract the most of
>> iommu_table_setparms_lpar() into iommu_table_setparms() (except
>> of_parse_dma_window) and call new helper from where you call
>> iommu_table_update_window; and do
>> iommu_pseries_alloc_table/iommu_tce_table_put there.
>>
>
> I don't see how to extract iommu_table_setparms_lpar() into
> iommu_table_setparms(), they look to be used for different machine
> types.
>
> Do mean you extracting most of iommu_table_setparms_lpar() (and maybe
> iommu_table_setparms() ) into a new helper, which is called in both
> functions and use it instead of iommu_table_update_window() ?
Yes, this.
>
>>
>>> +{
>>> + struct iommu_table *new_tbl, *old_tbl;
>>> +
>>> + new_tbl = iommu_pseries_alloc_table(nid);
>>> + if (!new_tbl)
>>> + return -ENOMEM;
>>> +
>>> + old_tbl = *tbl;
>>> + new_tbl->it_index = liobn;
>>> + new_tbl->it_offset = win_addr >> page_shift;
>>> + new_tbl->it_page_shift = page_shift;
>>> + new_tbl->it_size = window_size >> page_shift;
>>> + new_tbl->it_base = old_tbl->it_base;
>>
>> Should not be used in pseries.
>>
>
> The point here is to migrate the values from the older tbl to the
The actual window/table is new (on the hypervisor side), you are not
migrating a single TCE, you deleted one whole window and created another
whole window, calling it "migration" is confusing, especially when PAPR
actually defines TCE migration.
> newer. I Would like to understand why this is bad, if it will still be
> 'unused' as the older tbl.
Having explicit values is more readable imho.
>>
>>> + new_tbl->it_busno = old_tbl->it_busno;
>>> + new_tbl->it_blocksize = old_tbl->it_blocksize;
>>
>> 16 for pseries and does not change (may be even make it a macro).
>>
>>> + new_tbl->it_type = old_tbl->it_type;
>>
>> TCE_PCI.
>>
>
> Same as above.
>
>>
>>> + new_tbl->it_ops = old_tbl->it_ops;
>>> +
>>> + iommu_init_table(new_tbl, nid, old_tbl->it_reserved_start, old_tbl->it_reserved_end);
>>> + iommu_tce_table_put(old_tbl);
>>> + *tbl = new_tbl;
>>>
>>> return 0;
>>> }
>>> @@ -1171,12 +1206,16 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> struct direct_window *window;
>>> struct property *win64 = NULL;
>>> struct failed_ddw_pdn *fpdn;
>>> - bool default_win_removed = false;
>>> + bool default_win_removed = false, maps_whole_partition = false;
>>
>> s/maps_whole_partition/direct_mapping/
>>
>
> Sure, I will get it replaced.
>
>>
>>> + struct pci_dn *pci = PCI_DN(pdn);
>>> + struct iommu_table *tbl = pci->table_group->tables[0];
>>>
>>> mutex_lock(&direct_window_init_mutex);
>>>
>>> - if (find_existing_ddw(pdn, &dev->dev.archdata.dma_offset))
>>> - goto out_unlock;
>>> + if (find_existing_ddw(pdn, &dev->dev.archdata.dma_offset, &maps_whole_partition)) {
>>> + mutex_unlock(&direct_window_init_mutex);
>>> + return maps_whole_partition;
>>> + }
>>>
>>> /*
>>> * If we already went through this for a previous function of
>>> @@ -1258,16 +1297,24 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> query.page_size);
>>> goto out_failed;
>>> }
>>> +
>>> /* verify the window * number of ptes will map the partition */
>>> - /* check largest block * page size > max memory hotplug addr */
>>> max_addr = ddw_memory_hotplug_max();
>>> if (query.largest_available_block < (max_addr >> page_shift)) {
>>> - dev_dbg(&dev->dev, "can't map partition max 0x%llx with %llu "
>>> - "%llu-sized pages\n", max_addr, query.largest_available_block,
>>> - 1ULL << page_shift);
>>> - goto out_failed;
>>> + dev_dbg(&dev->dev, "can't map partition max 0x%llx with %llu %llu-sized pages\n",
>>> + max_addr, query.largest_available_block,
>>> + 1ULL << page_shift);
>>> +
>>> + len = order_base_2(query.largest_available_block << page_shift);
>>> + } else {
>>> + maps_whole_partition = true;
>>> + len = order_base_2(max_addr);
>>> }
>>> - len = order_base_2(max_addr);
>>> +
>>> + /* DDW + IOMMU on single window may fail if there is any allocation */
>>> + if (default_win_removed && !maps_whole_partition &&
>>> + iommu_table_in_use(tbl))
>>> + goto out_failed;
>>>
>>> ret = create_ddw(dev, ddw_avail, &create, page_shift, len);
>>> if (ret != 0)
>>> @@ -1277,8 +1324,8 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> create.liobn, dn);
>>>
>>> win_addr = ((u64)create.addr_hi << 32) | create.addr_lo;
>>> - ret = ddw_property_create(&win64, DIRECT64_PROPNAME, create.liobn, win_addr,
>>> - page_shift, len);
>>> + ret = ddw_property_create(&win64, DMA64_PROPNAME, create.liobn, win_addr,
>>> + page_shift, len, maps_whole_partition);
>>> if (ret) {
>>> dev_info(&dev->dev,
>>> "couldn't allocate property, property name, or value\n");
>>> @@ -1297,12 +1344,25 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> if (!window)
>>> goto out_prop_del;
>>>
>>> - ret = walk_system_ram_range(0, memblock_end_of_DRAM() >> PAGE_SHIFT,
>>> - win64->value, tce_setrange_multi_pSeriesLP_walk);
>>> - if (ret) {
>>> - dev_info(&dev->dev, "failed to map direct window for %pOF: %d\n",
>>> - dn, ret);
>>> - goto out_free_window;
>>> + if (maps_whole_partition) {
>>> + /* DDW maps the whole partition, so enable direct DMA mapping */
>>> + ret = walk_system_ram_range(0, memblock_end_of_DRAM() >> PAGE_SHIFT,
>>> + win64->value, tce_setrange_multi_pSeriesLP_walk);
>>> + if (ret) {
>>> + dev_info(&dev->dev, "failed to map direct window for %pOF: %d\n",
>>> + dn, ret);
>>> + goto out_free_window;
>>> + }
>>> + } else {
>>> + /* New table for using DDW instead of the default DMA window */
>>> + if (iommu_table_update_window(&tbl, pci->phb->node, create.liobn,
>>> + win_addr, page_shift, 1UL << len))
>>> + goto out_free_window;
>>> +
>>> + set_iommu_table_base(&dev->dev, tbl);
>>> + WARN_ON(dev->dev.archdata.dma_offset >= SZ_4G);
>>
>> What is this check for exactly? Why 4G, not >= 0, for example?
>
> I am not really sure, you suggested adding it here:
> http://patchwork.ozlabs.org/project/linuxppc-dev/patch/20200716071658.467820-6-leobras.c@gmail.com/#2488874
Ah right I did suggest this :) My bad. I think I suggested it before
suggesting to keep the reserved area boundaries checked/adjusted to the
window boundaries, may as well drop this. Thanks,
>
> I can remove it if it's ok.
>
>>
>>> + goto out_unlock;
>>> +
>>> }
>>>
>>> dev->dev.archdata.dma_offset = win_addr;
>>> @@ -1340,7 +1400,7 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>>
>>> out_unlock:
>>> mutex_unlock(&direct_window_init_mutex);
>>> - return win64;
>>> + return win64 && maps_whole_partition;
>>> }
>>>
>>> static void pci_dma_dev_setup_pSeriesLP(struct pci_dev *dev)
>>>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v1 08/10] powerpc/pseries/iommu: Add ddw_property_create() and refactor enable_ddw()
From: Alexey Kardashevskiy @ 2020-08-31 4:34 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <06f732abbc3e6d4745428c4fc8cc98baf960a2e0.camel@gmail.com>
On 29/08/2020 01:25, Leonardo Bras wrote:
> On Mon, 2020-08-24 at 15:07 +1000, Alexey Kardashevskiy wrote:
>>
>> On 18/08/2020 09:40, Leonardo Bras wrote:
>>> Code used to create a ddw property that was previously scattered in
>>> enable_ddw() is now gathered in ddw_property_create(), which deals with
>>> allocation and filling the property, letting it ready for
>>> of_property_add(), which now occurs in sequence.
>>>
>>> This created an opportunity to reorganize the second part of enable_ddw():
>>>
>>> Without this patch enable_ddw() does, in order:
>>> kzalloc() property & members, create_ddw(), fill ddwprop inside property,
>>> ddw_list_add(), do tce_setrange_multi_pSeriesLP_walk in all memory,
>>> of_add_property().
>>>
>>> With this patch enable_ddw() does, in order:
>>> create_ddw(), ddw_property_create(), of_add_property(), ddw_list_add(),
>>> do tce_setrange_multi_pSeriesLP_walk in all memory.
>>>
>>> This change requires of_remove_property() in case anything fails after
>>> of_add_property(), but we get to do tce_setrange_multi_pSeriesLP_walk
>>> in all memory, which looks the most expensive operation, only if
>>> everything else succeeds.
>>>
>>> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
>>> ---
>>> arch/powerpc/platforms/pseries/iommu.c | 97 +++++++++++++++-----------
>>> 1 file changed, 57 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
>>> index 4031127c9537..3a1ef02ad9d5 100644
>>> --- a/arch/powerpc/platforms/pseries/iommu.c
>>> +++ b/arch/powerpc/platforms/pseries/iommu.c
>>> @@ -1123,6 +1123,31 @@ static void reset_dma_window(struct pci_dev *dev, struct device_node *par_dn)
>>> ret);
>>> }
>>>
>>> +static int ddw_property_create(struct property **ddw_win, const char *propname,
>>
>> @propname is always the same, do you really want to pass it every time?
>
> I think it reads better, like "create a ddw property with this name".
This reads as "there are at least two ddw properties".
> Also, it makes possible to create ddw properties with other names, in
> case we decide to create properties with different names depending on
> the window created.
It is one window at any given moment, why call it different names... I
get the part that it is not always "direct" anymore but still...
> Also, it's probably optimized / inlined at this point.
> Is it ok doing it like this?
>
>>
>>> + u32 liobn, u64 dma_addr, u32 page_shift, u32 window_shift)
>>> +{
>>> + struct dynamic_dma_window_prop *ddwprop;
>>> + struct property *win64;
>>> +
>>> + *ddw_win = win64 = kzalloc(sizeof(*win64), GFP_KERNEL);
>>> + if (!win64)
>>> + return -ENOMEM;
>>> +
>>> + win64->name = kstrdup(propname, GFP_KERNEL);
>>
>> Not clear why "win64->name = DIRECT64_PROPNAME" would not work here, the
>> generic OF code does not try kfree() it but it is probably out of scope
>> here.
>
> Yeah, I had that question too.
> Previous code was like that, and I as trying not to mess too much on
> how it's done.
>
>>> + ddwprop = kzalloc(sizeof(*ddwprop), GFP_KERNEL);
>>> + win64->value = ddwprop;
>>> + win64->length = sizeof(*ddwprop);
>>> + if (!win64->name || !win64->value)
>>> + return -ENOMEM;
>>
>> Up to 2 memory leaks here. I see the cleanup at "out_free_prop:" but
>> still looks fragile. Instead you could simply return win64 as the only
>> error possible here is -ENOMEM and returning NULL is equally good.
>
> I agree. It's better if this function have it's own cleaning routine.
> It will be fixed for next version.
>
>>
>>
>>> +
>>> + ddwprop->liobn = cpu_to_be32(liobn);
>>> + ddwprop->dma_base = cpu_to_be64(dma_addr);
>>> + ddwprop->tce_shift = cpu_to_be32(page_shift);
>>> + ddwprop->window_shift = cpu_to_be32(window_shift);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> /*
>>> * If the PE supports dynamic dma windows, and there is space for a table
>>> * that can map all pages in a linear offset, then setup such a table,
>>> @@ -1140,12 +1165,11 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> struct ddw_query_response query;
>>> struct ddw_create_response create;
>>> int page_shift;
>>> - u64 max_addr;
>>> + u64 max_addr, win_addr;
>>> struct device_node *dn;
>>> u32 ddw_avail[DDW_APPLICABLE_SIZE];
>>> struct direct_window *window;
>>> - struct property *win64;
>>> - struct dynamic_dma_window_prop *ddwprop;
>>> + struct property *win64 = NULL;
>>> struct failed_ddw_pdn *fpdn;
>>> bool default_win_removed = false;
>>>
>>> @@ -1244,38 +1268,34 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> goto out_failed;
>>> }
>>> len = order_base_2(max_addr);
>>> - win64 = kzalloc(sizeof(struct property), GFP_KERNEL);
>>> - if (!win64) {
>>> - dev_info(&dev->dev,
>>> - "couldn't allocate property for 64bit dma window\n");
>>> +
>>> + ret = create_ddw(dev, ddw_avail, &create, page_shift, len);
>>> + if (ret != 0)
>>
>> It is usually just "if (ret)"
>
> It was previously like that, and all query_ddw() checks return value
> this way.
ah I see.
> Should I update them all or just this one?
Pick one variant and make sure all new lines use just that. In this
patch you add both variants. Thanks,
>
> Thanks!
>
>>
>>
>>> goto out_failed;
>>> - }
>>> - win64->name = kstrdup(DIRECT64_PROPNAME, GFP_KERNEL);
>>> - win64->value = ddwprop = kmalloc(sizeof(*ddwprop), GFP_KERNEL);
>>> - win64->length = sizeof(*ddwprop);
>>> - if (!win64->name || !win64->value) {
>>> +
>>> + dev_dbg(&dev->dev, "created tce table LIOBN 0x%x for %pOF\n",
>>> + create.liobn, dn);
>>> +
>>> + win_addr = ((u64)create.addr_hi << 32) | create.addr_lo;
>>> + ret = ddw_property_create(&win64, DIRECT64_PROPNAME, create.liobn, win_addr,
>>> + page_shift, len);
>>> + if (ret) {
>>> dev_info(&dev->dev,
>>> - "couldn't allocate property name and value\n");
>>> + "couldn't allocate property, property name, or value\n");
>>> goto out_free_prop;
>>> }
>>>
>>> - ret = create_ddw(dev, ddw_avail, &create, page_shift, len);
>>> - if (ret != 0)
>>> + ret = of_add_property(pdn, win64);
>>> + if (ret) {
>>> + dev_err(&dev->dev, "unable to add dma window property for %pOF: %d",
>>> + pdn, ret);
>>> goto out_free_prop;
>>> -
>>> - ddwprop->liobn = cpu_to_be32(create.liobn);
>>> - ddwprop->dma_base = cpu_to_be64(((u64)create.addr_hi << 32) |
>>> - create.addr_lo);
>>> - ddwprop->tce_shift = cpu_to_be32(page_shift);
>>> - ddwprop->window_shift = cpu_to_be32(len);
>>> -
>>> - dev_dbg(&dev->dev, "created tce table LIOBN 0x%x for %pOF\n",
>>> - create.liobn, dn);
>>> + }
>>>
>>> /* Add new window to existing DDW list */
>>> - window = ddw_list_add(pdn, ddwprop);
>>> + window = ddw_list_add(pdn, win64->value);
>>> if (!window)
>>> - goto out_clear_window;
>>> + goto out_prop_del;
>>>
>>> ret = walk_system_ram_range(0, memblock_end_of_DRAM() >> PAGE_SHIFT,
>>> win64->value, tce_setrange_multi_pSeriesLP_walk);
>>> @@ -1285,14 +1305,7 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> goto out_free_window;
>>> }
>>>
>>> - ret = of_add_property(pdn, win64);
>>> - if (ret) {
>>> - dev_err(&dev->dev, "unable to add dma window property for %pOF: %d",
>>> - pdn, ret);
>>> - goto out_free_window;
>>> - }
>>> -
>>> - dev->dev.archdata.dma_offset = be64_to_cpu(ddwprop->dma_base);
>>> + dev->dev.archdata.dma_offset = win_addr;
>>> goto out_unlock;
>>>
>>> out_free_window:
>>> @@ -1302,14 +1315,18 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>>
>>> kfree(window);
>>>
>>> -out_clear_window:
>>> - remove_ddw(pdn, true);
>>> +out_prop_del:
>>> + of_remove_property(pdn, win64);
>>>
>>> out_free_prop:
>>> - kfree(win64->name);
>>> - kfree(win64->value);
>>> - kfree(win64);
>>> - win64 = NULL;
>>> + if (win64) {
>>> + kfree(win64->name);
>>> + kfree(win64->value);
>>> + kfree(win64);
>>> + win64 = NULL;
>>> + }
>>> +
>>> + remove_ddw(pdn, true);
>>>
>>> out_failed:
>>> if (default_win_removed)
>>>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v1 01/10] powerpc/pseries/iommu: Replace hard-coded page shift
From: Alexey Kardashevskiy @ 2020-08-31 3:48 UTC (permalink / raw)
To: Oliver O'Halloran
Cc: Christophe Leroy, Leonardo Bras, David Dai, Ram Pai,
Linux Kernel Mailing List, Murilo Fossa Vicentini, Paul Mackerras,
Joel Stanley, Brian King, linuxppc-dev, Thiago Jung Bauermann
In-Reply-To: <CAOSf1CG49ztvNoG43hcSHyLB9UY6Nc8maY_q6nvQmiyFQOAp3A@mail.gmail.com>
On 31/08/2020 11:41, Oliver O'Halloran wrote:
> On Mon, Aug 31, 2020 at 10:08 AM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>> On 29/08/2020 05:55, Leonardo Bras wrote:
>>> On Fri, 2020-08-28 at 12:27 +1000, Alexey Kardashevskiy wrote:
>>>>
>>>> On 28/08/2020 01:32, Leonardo Bras wrote:
>>>>> Hello Alexey, thank you for this feedback!
>>>>>
>>>>> On Sat, 2020-08-22 at 19:33 +1000, Alexey Kardashevskiy wrote:
>>>>>>> +#define TCE_RPN_BITS 52 /* Bits 0-51 represent RPN on TCE */
>>>>>>
>>>>>> Ditch this one and use MAX_PHYSMEM_BITS instead? I am pretty sure this
>>>>>> is the actual limit.
>>>>>
>>>>> I understand this MAX_PHYSMEM_BITS(51) comes from the maximum physical memory addressable in the machine. IIUC, it means we can access physical address up to (1ul << MAX_PHYSMEM_BITS).
>>>>>
>>>>> This 52 comes from PAPR "Table 9. TCE Definition" which defines bits
>>>>> 0-51 as the RPN. By looking at code, I understand that it means we may input any address < (1ul << 52) to TCE.
>>>>>
>>>>> In practice, MAX_PHYSMEM_BITS should be enough as of today, because I suppose we can't ever pass a physical page address over
>>>>> (1ul << 51), and TCE accepts up to (1ul << 52).
>>>>> But if we ever increase MAX_PHYSMEM_BITS, it doesn't necessarily means that TCE_RPN_BITS will also be increased, so I think they are independent values.
>>>>>
>>>>> Does it make sense? Please let me know if I am missing something.
>>>>
>>>> The underlying hardware is PHB3/4 about which the IODA2 Version 2.4
>>>> 6Apr2012.pdf spec says:
>>>>
>>>> "The number of most significant RPN bits implemented in the TCE is
>>>> dependent on the max size of System Memory to be supported by the platform".
>>>>
>>>> IODA3 is the same on this matter.
>>>>
>>>> This is MAX_PHYSMEM_BITS and PHB itself does not have any other limits
>>>> on top of that. So the only real limit comes from MAX_PHYSMEM_BITS and
>>>> where TCE_RPN_BITS comes from exactly - I have no idea.
>>>
>>> Well, I created this TCE_RPN_BITS = 52 because the previous mask was a
>>> hardcoded 40-bit mask (0xfffffffffful), for hard-coded 12-bit (4k)
>>> pagesize, and on PAPR+/LoPAR also defines TCE as having bits 0-51
>>> described as RPN, as described before.
>>>
>>> IODA3 Revision 3.0_prd1 (OpenPowerFoundation), Figure 3.4 and 3.5.
>>> shows system memory mapping into a TCE, and the TCE also has bits 0-51
>>> for the RPN (52 bits). "Table 3.6. TCE Definition" also shows it.
>>>> In fact, by the looks of those figures, the RPN_MASK should always be a
>>> 52-bit mask, and RPN = (page >> tceshift) & RPN_MASK.
>>
>> I suspect the mask is there in the first place for extra protection
>> against too big addresses going to the TCE table (or/and for virtial vs
>> physical addresses). Using 52bit mask makes no sense for anything, you
>> could just drop the mask and let c compiler deal with 64bit "uint" as it
>> is basically a 4K page address anywhere in the 64bit space. Thanks,
>
> Assuming 4K pages you need 52 RPN bits to cover the whole 64bit
> physical address space. The IODA3 spec does explicitly say the upper
> bits are optional and the implementation only needs to support enough
> to cover up to the physical address limit, which is 56bits of P9 /
> PHB4. If you want to validate that the address will fit inside of
> MAX_PHYSMEM_BITS then fine, but I think that should be done as a
> WARN_ON or similar rather than just silently masking off the bits.
We can do this and probably should anyway but I am also pretty sure we
can just ditch the mask and have the hypervisor return an error which
will show up in dmesg.
--
Alexey
^ permalink raw reply
* Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()
From: Michael Ellerman @ 2020-08-31 1:54 UTC (permalink / raw)
To: Linus Torvalds, Guenter Roeck
Cc: Herbert Xu, Joerg Roedel, Linux Kernel Mailing List, Li Yang,
Zhang Wei, Vinod Koul, dma, Andrew Morton, linuxppc-dev,
Dan Williams, Luc Van Oostenryck
In-Reply-To: <CAHk-=wjDEiWF_DsCVFPFqNa+JCS5SkOygbqeq8_=ZNOrFt7-rg@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Sat, Aug 29, 2020 at 1:40 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> Except for
>>
>> CHECK: spaces preferred around that '+' (ctx:VxV)
>> #29: FILE: drivers/dma/fsldma.h:223:
>> + u32 val_lo = in_be32((u32 __iomem *)addr+1);
>
> Added spaces.
>
>> I don't see anything wrong with it either, so
>>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>
>> Since I didn't see the real problem with the original code,
>> I'd take that with a grain of salt, though.
>
> Well, honestly, the old code was so confused that just making it build
> is clearly already an improvement even if everything else were to be
> wrong.
The old code is not that old, only ~18 months:
a1ff82a9c165 ("dmaengine: fsldma: Adding macro FSL_DMA_IN/OUT implement for ARM platform") (Jan 2019)
So I think it's possible it's never been tested on 32-bit ppc at all.
I did have a 32-bit FSL machine but it lost its network card in a power
outage and now it won't boot (and I can't get to it physically).
> So I committed my "fix". If it turns out there's more wrong in there
> and somebody tests it, we can fix it again. But now it hopefully
> compiles, at least.
>
> My bet is that if that driver ever worked on ppc32, it will continue
> to work whatever we do to that function.
>
> I _think_ the old code happened to - completely by mistake - get the
> value right for the case of "little endian access, with dma_addr_t
> being 32-bit". Because then it would still read the upper bits wrong,
> but the cast to dma_addr_t would then throw those bits away. And the
> lower bits would be right.
>
> But for big-endian accesses or for ARCH_DMA_ADDR_T_64BIT it really
> looks like it always returned a completely incorrect value.
>
> And again - the driver may have worked even with that completely
> incorrect value, since the use of it seems to be very incidental.
>
> In either case ("it didn't work before" or "it worked because the
> value doesn't really matter"), I don't think I could possibly have
> made things worse.
Agreed.
Hopefully someone from NXP can test it.
cheers
^ permalink raw reply
* Re: [PATCH v1 01/10] powerpc/pseries/iommu: Replace hard-coded page shift
From: Oliver O'Halloran @ 2020-08-31 1:41 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Christophe Leroy, Leonardo Bras, David Dai, Ram Pai,
Linux Kernel Mailing List, Murilo Fossa Vicentini, Paul Mackerras,
Joel Stanley, Brian King, linuxppc-dev, Thiago Jung Bauermann
In-Reply-To: <aaaf993a-d233-f5be-b809-5911a6a9872d@ozlabs.ru>
On Mon, Aug 31, 2020 at 10:08 AM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
> On 29/08/2020 05:55, Leonardo Bras wrote:
> > On Fri, 2020-08-28 at 12:27 +1000, Alexey Kardashevskiy wrote:
> >>
> >> On 28/08/2020 01:32, Leonardo Bras wrote:
> >>> Hello Alexey, thank you for this feedback!
> >>>
> >>> On Sat, 2020-08-22 at 19:33 +1000, Alexey Kardashevskiy wrote:
> >>>>> +#define TCE_RPN_BITS 52 /* Bits 0-51 represent RPN on TCE */
> >>>>
> >>>> Ditch this one and use MAX_PHYSMEM_BITS instead? I am pretty sure this
> >>>> is the actual limit.
> >>>
> >>> I understand this MAX_PHYSMEM_BITS(51) comes from the maximum physical memory addressable in the machine. IIUC, it means we can access physical address up to (1ul << MAX_PHYSMEM_BITS).
> >>>
> >>> This 52 comes from PAPR "Table 9. TCE Definition" which defines bits
> >>> 0-51 as the RPN. By looking at code, I understand that it means we may input any address < (1ul << 52) to TCE.
> >>>
> >>> In practice, MAX_PHYSMEM_BITS should be enough as of today, because I suppose we can't ever pass a physical page address over
> >>> (1ul << 51), and TCE accepts up to (1ul << 52).
> >>> But if we ever increase MAX_PHYSMEM_BITS, it doesn't necessarily means that TCE_RPN_BITS will also be increased, so I think they are independent values.
> >>>
> >>> Does it make sense? Please let me know if I am missing something.
> >>
> >> The underlying hardware is PHB3/4 about which the IODA2 Version 2.4
> >> 6Apr2012.pdf spec says:
> >>
> >> "The number of most significant RPN bits implemented in the TCE is
> >> dependent on the max size of System Memory to be supported by the platform".
> >>
> >> IODA3 is the same on this matter.
> >>
> >> This is MAX_PHYSMEM_BITS and PHB itself does not have any other limits
> >> on top of that. So the only real limit comes from MAX_PHYSMEM_BITS and
> >> where TCE_RPN_BITS comes from exactly - I have no idea.
> >
> > Well, I created this TCE_RPN_BITS = 52 because the previous mask was a
> > hardcoded 40-bit mask (0xfffffffffful), for hard-coded 12-bit (4k)
> > pagesize, and on PAPR+/LoPAR also defines TCE as having bits 0-51
> > described as RPN, as described before.
> >
> > IODA3 Revision 3.0_prd1 (OpenPowerFoundation), Figure 3.4 and 3.5.
> > shows system memory mapping into a TCE, and the TCE also has bits 0-51
> > for the RPN (52 bits). "Table 3.6. TCE Definition" also shows it.
> >> In fact, by the looks of those figures, the RPN_MASK should always be a
> > 52-bit mask, and RPN = (page >> tceshift) & RPN_MASK.
>
> I suspect the mask is there in the first place for extra protection
> against too big addresses going to the TCE table (or/and for virtial vs
> physical addresses). Using 52bit mask makes no sense for anything, you
> could just drop the mask and let c compiler deal with 64bit "uint" as it
> is basically a 4K page address anywhere in the 64bit space. Thanks,
Assuming 4K pages you need 52 RPN bits to cover the whole 64bit
physical address space. The IODA3 spec does explicitly say the upper
bits are optional and the implementation only needs to support enough
to cover up to the physical address limit, which is 56bits of P9 /
PHB4. If you want to validate that the address will fit inside of
MAX_PHYSMEM_BITS then fine, but I think that should be done as a
WARN_ON or similar rather than just silently masking off the bits.
^ permalink raw reply
* Re: [PATCH 1/4] mm: fix exec activate_mm vs TLB shootdown and lazy tlb switching race
From: Nicholas Piggin @ 2020-08-31 1:25 UTC (permalink / raw)
To: peterz
Cc: Jens Axboe, linux-arch, Aneesh Kumar K.V, linux-kernel, linux-mm,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20200828111525.GX1362448@hirez.programming.kicks-ass.net>
Excerpts from peterz@infradead.org's message of August 28, 2020 9:15 pm:
> On Fri, Aug 28, 2020 at 08:00:19PM +1000, Nicholas Piggin wrote:
>
>> Closing this race only requires interrupts to be disabled while ->mm
>> and ->active_mm are being switched, but the TLB problem requires also
>> holding interrupts off over activate_mm. Unfortunately not all archs
>> can do that yet, e.g., arm defers the switch if irqs are disabled and
>> expects finish_arch_post_lock_switch() to be called to complete the
>> flush; um takes a blocking lock in activate_mm().
>
> ARM at least has activate_mm() := switch_mm(), so it could be made to
> work.
>
Yeah, so long as that post_lock_switch switch did the right thing with
respect to its TLB flushing. It should do because arm doesn't seem to
check ->mm or ->active_mm (and if it was broken, the scheduler context
switch would be suspect too). I don't think the fix would be hard, just
that I don't have a good way to test it and qemu isn't great for testing
this kind of thing.
um too I think could probably defer that lock until after interrupts are
enabled again. I might throw a bunch of arch conversion patches over the
wall if this gets merged and try to move things along.
Thanks,
Nick
^ permalink raw reply
* Re: KVM on POWER8 host lock up since 10d91611f426 ("powerpc/64s: Reimplement book3s idle code in C")
From: Nicholas Piggin @ 2020-08-31 1:14 UTC (permalink / raw)
To: linuxppc-dev, Michal Suchánek; +Cc: ro
In-Reply-To: <20200830201145.GA29521@kitsune.suse.cz>
Excerpts from Michal Suchánek's message of August 31, 2020 6:11 am:
> Hello,
>
> on POWER8 KVM hosts lock up since commit 10d91611f426 ("powerpc/64s:
> Reimplement book3s idle code in C").
>
> The symptom is host locking up completely after some hours of KVM
> workload with messages like
>
> 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
> 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 71
> 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
> 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 71
> 2020-08-30T10:51:31+00:00 obs-power8-01 kernel: KVM: couldn't grab cpu 47
>
> printed before the host locks up.
>
> The machines run sandboxed builds which is a mixed workload resulting in
> IO/single core/mutiple core load over time and there are periods of no
> activity and no VMS runnig as well. The VMs are shortlived so VM
> setup/terdown is somewhat excercised as well.
>
> POWER9 with the new guest entry fast path does not seem to be affected.
>
> Reverted the patch and the followup idle fixes on top of 5.2.14 and
> re-applied commit a3f3072db6ca ("powerpc/powernv/idle: Restore IAMR
> after idle") which gives same idle code as 5.1.16 and the kernel seems
> stable.
>
> Config is attached.
>
> I cannot easily revert this commit, especially if I want to use the same
> kernel on POWER8 and POWER9 - many of the POWER9 fixes are applicable
> only to the new idle code.
>
> Any idea what can be the problem?
So hwthread_state is never getting back to to HWTHREAD_IN_IDLE on
those threads. I wonder what they are doing. POWER8 doesn't have a good
NMI IPI and I don't know if it supports pdbg dumping registers from the
BMC unfortunately. Do the messages always come in pairs of CPUs?
I'm not sure where to start with reproducing, I'll have to try. How many
vCPUs in the guests? Do you have several guests running at once?
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v1 07/10] powerpc/pseries/iommu: Allow DDW windows starting at 0x00
From: Alexey Kardashevskiy @ 2020-08-31 0:50 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <2d2b1a048faf75c8e68f95b3bf2d9514721786c6.camel@gmail.com>
On 29/08/2020 00:04, Leonardo Bras wrote:
> On Mon, 2020-08-24 at 13:44 +1000, Alexey Kardashevskiy wrote:
>>
>>> On 18/08/2020 09:40, Leonardo Bras wrote:
>>> enable_ddw() currently returns the address of the DMA window, which is
>>> considered invalid if has the value 0x00.
>>>
>>> Also, it only considers valid an address returned from find_existing_ddw
>>> if it's not 0x00.
>>>
>>> Changing this behavior makes sense, given the users of enable_ddw() only
>>> need to know if direct mapping is possible. It can also allow a DMA window
>>> starting at 0x00 to be used.
>>>
>>> This will be helpful for using a DDW with indirect mapping, as the window
>>> address will be different than 0x00, but it will not map the whole
>>> partition.
>>>
>>> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
>>> ---
>>> arch/powerpc/platforms/pseries/iommu.c | 30 ++++++++++++--------------
>>> 1 file changed, 14 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
>>> index fcdefcc0f365..4031127c9537 100644
>>> --- a/arch/powerpc/platforms/pseries/iommu.c
>>> +++ b/arch/powerpc/platforms/pseries/iommu.c
>>> @@ -852,24 +852,25 @@ static void remove_ddw(struct device_node *np, bool remove_prop)
>>> np, ret);
>>> }
>>>>
>>> -static u64 find_existing_ddw(struct device_node *pdn)
>>> +static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr)
>>> {
>>> struct direct_window *window;
>>> const struct dynamic_dma_window_prop *direct64;
>>> - u64 dma_addr = 0;
>>> + bool found = false;
>>>
>>> spin_lock(&direct_window_list_lock);
>>> /* check if we already created a window and dupe that config if so */
>>> list_for_each_entry(window, &direct_window_list, list) {
>>> if (window->device == pdn) {
>>> direct64 = window->prop;
>>> - dma_addr = be64_to_cpu(direct64->dma_base);
>>> + *dma_addr = be64_to_cpu(direct64->dma_base);
>>> + found = true;
>>> break;
>>> }
>>> }
>>> spin_unlock(&direct_window_list_lock);
>>>
>>> - return dma_addr;
>>> + return found;
>>> }
>>>
>>> static struct direct_window *ddw_list_add(struct device_node *pdn,
>>> @@ -1131,15 +1132,15 @@ static void reset_dma_window(struct pci_dev *dev, struct device_node *par_dn)
>>> * pdn: the parent pe node with the ibm,dma_window property
>>> * Future: also check if we can remap the base window for our base page size
>>> *
>>> - * returns the dma offset for use by the direct mapped DMA code.
>>> + * returns true if can map all pages (direct mapping), false otherwise..
>>> */
>>> -static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> +static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> {
>>> int len, ret;
>>> struct ddw_query_response query;
>>> struct ddw_create_response create;
>>> int page_shift;
>>> - u64 dma_addr, max_addr;
>>> + u64 max_addr;
>>> struct device_node *dn;
>>> u32 ddw_avail[DDW_APPLICABLE_SIZE];
>>> struct direct_window *window;
>>> @@ -1150,8 +1151,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>>
>>> mutex_lock(&direct_window_init_mutex);
>>>
>>> - dma_addr = find_existing_ddw(pdn);
>>> - if (dma_addr != 0)
>>> + if (find_existing_ddw(pdn, &dev->dev.archdata.dma_offset))
>>> goto out_unlock;
>>>
>>> /*
>>> @@ -1292,7 +1292,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> goto out_free_window;
>>> }
>>>
>>> - dma_addr = be64_to_cpu(ddwprop->dma_base);
>>> + dev->dev.archdata.dma_offset = be64_to_cpu(ddwprop->dma_base);
>>
>> Do not you need the same chunk in the find_existing_ddw() case above as
>> well? Thanks,
>
> The new signature of find_existing_ddw() is
> static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr)
>
> And on enable_ddw(), we call
> find_existing_ddw(pdn, &dev->dev.archdata.dma_offset)
>
> And inside the function we do:
> *dma_addr = be64_to_cpu(direct64->dma_base);
>
> I think it's the same as the chunk before.
> Am I missing something?
ah no, sorry, you are not missing anything.
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
--
Alexey
^ permalink raw reply
* Re: [PATCH v1 02/10] powerpc/kernel/iommu: Align size for IOMMU_PAGE_SIZE on iommu_*_coherent()
From: Alexey Kardashevskiy @ 2020-08-31 0:47 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <2aacd45f047489642da1731c92d3555ad101e3c7.camel@gmail.com>
On 29/08/2020 06:41, Leonardo Bras wrote:
> On Fri, 2020-08-28 at 11:40 +1000, Alexey Kardashevskiy wrote:
>>> I think it would be better to keep the code as much generic as possible
>>> regarding page sizes.
>>
>> Then you need to test it. Does 4K guest even boot (it should but I would
>> not bet much on it)?
>
> Maybe testing with host 64k pagesize and IOMMU 16MB pagesize in qemu
> should be enough, is there any chance to get indirect mapping in qemu
> like this? (DDW but with smaller DMA window available)
You will have to hack the guest kernel to always do indirect mapping or
hack QEMU's rtas_ibm_query_pe_dma_window() to return a small number of
available TCEs. But you will be testing QEMU/KVM which behave quite
differently to pHyp in this particular case.
>>>> Because if we want the former (==support), then we'll have to align the
>>>> size up to the bigger page size when allocating/zeroing system pages,
>>>> etc.
>>>
>>> This part I don't understand. Why do we need to align everything to the
>>> bigger pagesize?
>>>
>>> I mean, is not that enough that the range [ret, ret + size[ is both
>>> allocated by mm and mapped on a iommu range?
>>>
>>> Suppose a iommu_alloc_coherent() of 16kB on PAGESIZE = 4k and
>>> IOMMU_PAGE_SIZE() == 64k.
>>> Why 4 * cpu_pages mapped by a 64k IOMMU page is not enough?
>>> All the space the user asked for is allocated and mapped for DMA.
>>
>> The user asked to map 16K, the rest - 48K - is used for something else
>> (may be even mapped to another device) but you are making all 64K
>> accessible by the device which only should be able to access 16K.
>>
>> In practice, if this happens, H_PUT_TCE will simply fail.
>
> I have noticed mlx5 driver getting a few bytes in a buffer, and using
> iommu_map_page(). It does map a whole page for as few bytes as the user
Whole 4K system page or whole 64K iommu page?
> wants mapped, and the other bytes get used for something else, or just
> mapped on another DMA page.
> It seems to work fine.
With 4K system page and 64K IOMMU page? In practice it would take an
effort or/and bad luck to see it crashing. Thanks,
>
>>
>>
>>>> Bigger pages are not the case here as I understand it.
>>>
>>> I did not get this part, what do you mean?
>>
>> Possible IOMMU page sizes are 4K, 64K, 2M, 16M, 256M, 1GB, and the
>> supported set of sizes is different for P8/P9 and type of IO (PHB,
>> NVLink/CAPI).
>>
>>
>>>>> Update those functions to guarantee alignment with requested size
>>>>> using IOMMU_PAGE_ALIGN() before doing iommu_alloc() / iommu_free().
>>>>>
>>>>> Also, on iommu_range_alloc(), replace ALIGN(n, 1 << tbl->it_page_shift)
>>>>> with IOMMU_PAGE_ALIGN(n, tbl), which seems easier to read.
>>>>>
>>>>> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
>>>>> ---
>>>>> arch/powerpc/kernel/iommu.c | 17 +++++++++--------
>>>>> 1 file changed, 9 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>>>>> index 9704f3f76e63..d7086087830f 100644
>>>>> --- a/arch/powerpc/kernel/iommu.c
>>>>> +++ b/arch/powerpc/kernel/iommu.c
>>>>> @@ -237,10 +237,9 @@ static unsigned long iommu_range_alloc(struct device *dev,
>>>>> }
>>>>>
>>>>> if (dev)
>>>>> - boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
>>>>> - 1 << tbl->it_page_shift);
>>>>> + boundary_size = IOMMU_PAGE_ALIGN(dma_get_seg_boundary(dev) + 1, tbl);
>>>>
>>>> Run checkpatch.pl, should complain about a long line.
>>>
>>> It's 86 columns long, which is less than the new limit of 100 columns
>>> Linus announced a few weeks ago. checkpatch.pl was updated too:
>>> https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kernel-Deprecates-80-Col
>>
>> Yay finally :) Thanks,
>
> :)
>
>>
>>
>>>>
>>>>> else
>>>>> - boundary_size = ALIGN(1UL << 32, 1 << tbl->it_page_shift);
>>>>> + boundary_size = IOMMU_PAGE_ALIGN(1UL << 32, tbl);
>>>>> /* 4GB boundary for iseries_hv_alloc and iseries_hv_map */
>>>>>
>>>>> n = iommu_area_alloc(tbl->it_map, limit, start, npages, tbl->it_offset,
>>>>> @@ -858,6 +857,7 @@ void *iommu_alloc_coherent(struct device *dev, struct iommu_table *tbl,
>>>>> unsigned int order;
>>>>> unsigned int nio_pages, io_order;
>>>>> struct page *page;
>>>>> + size_t size_io = size;
>>>>>
>>>>> size = PAGE_ALIGN(size);
>>>>> order = get_order(size);
>>>>> @@ -884,8 +884,9 @@ void *iommu_alloc_coherent(struct device *dev, struct iommu_table *tbl,
>>>>> memset(ret, 0, size);
>>>>>
>>>>> /* Set up tces to cover the allocated range */
>>>>> - nio_pages = size >> tbl->it_page_shift;
>>>>> - io_order = get_iommu_order(size, tbl);
>>>>> + size_io = IOMMU_PAGE_ALIGN(size_io, tbl);
>>>>> + nio_pages = size_io >> tbl->it_page_shift;
>>>>> + io_order = get_iommu_order(size_io, tbl);
>>>>> mapping = iommu_alloc(dev, tbl, ret, nio_pages, DMA_BIDIRECTIONAL,
>>>>> mask >> tbl->it_page_shift, io_order, 0);
>>>>> if (mapping == DMA_MAPPING_ERROR) {
>>>>> @@ -900,11 +901,11 @@ void iommu_free_coherent(struct iommu_table *tbl, size_t size,
>>>>> void *vaddr, dma_addr_t dma_handle)
>>>>> {
>>>>> if (tbl) {
>>>>> - unsigned int nio_pages;
>>>>> + size_t size_io = IOMMU_PAGE_ALIGN(size, tbl);
>>>>> + unsigned int nio_pages = size_io >> tbl->it_page_shift;
>>>>>
>>>>> - size = PAGE_ALIGN(size);
>>>>> - nio_pages = size >> tbl->it_page_shift;
>>>>> iommu_free(tbl, dma_handle, nio_pages);
>>>>> +
>>>>
>>>> Unrelated new line.
>>>
>>> Will be removed. Thanks!
>>>
>>>>
>>>>> size = PAGE_ALIGN(size);
>>>>> free_pages((unsigned long)vaddr, get_order(size));
>>>>> }
>>>>>
>
--
Alexey
^ 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