* [RFC PATCH kernel] powerpc/ioda: Set "read" permission when "write" is set
@ 2016-01-12 4:40 Alexey Kardashevskiy
2016-01-12 23:07 ` Benjamin Herrenschmidt
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Alexey Kardashevskiy @ 2016-01-12 4:40 UTC (permalink / raw)
To: linuxppc-dev
Cc: Alexey Kardashevskiy, Benjamin Herrenschmidt, David Gibson,
Paul Mackerras
Quite often drivers set only "write" permission assuming that this
includes "read" permission as well and this works on plenty platforms.
However IODA2 is strict about this and produces an EEH when "read"
permission is not and reading happens.
This adds a workaround in IODA code to always add the "read" bit when
the "write" bit is set.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Ben, what was the driver which did not set "read" and caused EEH?
---
arch/powerpc/platforms/powernv/pci.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index f2dd772..c7dcae5 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -601,6 +601,9 @@ int pnv_tce_build(struct iommu_table *tbl, long index, long npages,
u64 rpn = __pa(uaddr) >> tbl->it_page_shift;
long i;
+ if (proto_tce & TCE_PCI_WRITE)
+ proto_tce |= TCE_PCI_READ;
+
for (i = 0; i < npages; i++) {
unsigned long newtce = proto_tce |
((rpn + i) << tbl->it_page_shift);
@@ -622,6 +625,9 @@ int pnv_tce_xchg(struct iommu_table *tbl, long index,
BUG_ON(*hpa & ~IOMMU_PAGE_MASK(tbl));
+ if (newtce & TCE_PCI_WRITE)
+ newtce |= TCE_PCI_READ;
+
oldtce = xchg(pnv_tce(tbl, idx), cpu_to_be64(newtce));
*hpa = be64_to_cpu(oldtce) & ~(TCE_PCI_READ | TCE_PCI_WRITE);
*direction = iommu_tce_direction(oldtce);
--
2.5.0.rc3
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [RFC PATCH kernel] powerpc/ioda: Set "read" permission when "write" is set
2016-01-12 4:40 [RFC PATCH kernel] powerpc/ioda: Set "read" permission when "write" is set Alexey Kardashevskiy
@ 2016-01-12 23:07 ` Benjamin Herrenschmidt
2016-01-13 2:24 ` Douglas Miller
2016-02-10 12:26 ` Douglas Miller
2016-02-16 3:18 ` [RFC, " Michael Ellerman
2 siblings, 1 reply; 12+ messages in thread
From: Benjamin Herrenschmidt @ 2016-01-12 23:07 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev; +Cc: David Gibson, Paul Mackerras
On Tue, 2016-01-12 at 15:40 +1100, Alexey Kardashevskiy wrote:
> Quite often drivers set only "write" permission assuming that this
> includes "read" permission as well and this works on plenty
> platforms.
> However IODA2 is strict about this and produces an EEH when "read"
> permission is not and reading happens.
>
> This adds a workaround in IODA code to always add the "read" bit when
> the "write" bit is set.
>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>
>
> Ben, what was the driver which did not set "read" and caused EEH?
aacraid
Cheers,
Ben.
>
> ---
> arch/powerpc/platforms/powernv/pci.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/powerpc/platforms/powernv/pci.c
> b/arch/powerpc/platforms/powernv/pci.c
> index f2dd772..c7dcae5 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -601,6 +601,9 @@ int pnv_tce_build(struct iommu_table *tbl, long
> index, long npages,
> u64 rpn = __pa(uaddr) >> tbl->it_page_shift;
> long i;
>
> + if (proto_tce & TCE_PCI_WRITE)
> + proto_tce |= TCE_PCI_READ;
> +
> for (i = 0; i < npages; i++) {
> unsigned long newtce = proto_tce |
> ((rpn + i) << tbl->it_page_shift);
> @@ -622,6 +625,9 @@ int pnv_tce_xchg(struct iommu_table *tbl, long
> index,
>
> BUG_ON(*hpa & ~IOMMU_PAGE_MASK(tbl));
>
> + if (newtce & TCE_PCI_WRITE)
> + newtce |= TCE_PCI_READ;
> +
> oldtce = xchg(pnv_tce(tbl, idx), cpu_to_be64(newtce));
> *hpa = be64_to_cpu(oldtce) & ~(TCE_PCI_READ |
> TCE_PCI_WRITE);
> *direction = iommu_tce_direction(oldtce);
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFC PATCH kernel] powerpc/ioda: Set "read" permission when "write" is set
2016-01-12 23:07 ` Benjamin Herrenschmidt
@ 2016-01-13 2:24 ` Douglas Miller
2016-01-19 3:52 ` Alexey Kardashevskiy
0 siblings, 1 reply; 12+ messages in thread
From: Douglas Miller @ 2016-01-13 2:24 UTC (permalink / raw)
To: linuxppc-dev
On 01/12/2016 05:07 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2016-01-12 at 15:40 +1100, Alexey Kardashevskiy wrote:
>> Quite often drivers set only "write" permission assuming that this
>> includes "read" permission as well and this works on plenty
>> platforms.
>> However IODA2 is strict about this and produces an EEH when "read"
>> permission is not and reading happens.
>>
>> This adds a workaround in IODA code to always add the "read" bit when
>> the "write" bit is set.
>>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>
>>
>> Ben, what was the driver which did not set "read" and caused EEH?
> aacraid
>
> Cheers,
> Ben.
Just to be precise, the driver wasn't responsible for setting READ. The
driver called scsi_dma_map() and the scsicmd was set (by scsi layer) as
DMA_FROM_DEVICE so the current code would set the permissions to
WRITE-ONLY. Previously, and in other architectures, this scsicmd would
have resulted in READ+WRITE permissions on the DMA map.
>
>> ---
>> arch/powerpc/platforms/powernv/pci.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/powernv/pci.c
>> b/arch/powerpc/platforms/powernv/pci.c
>> index f2dd772..c7dcae5 100644
>> --- a/arch/powerpc/platforms/powernv/pci.c
>> +++ b/arch/powerpc/platforms/powernv/pci.c
>> @@ -601,6 +601,9 @@ int pnv_tce_build(struct iommu_table *tbl, long
>> index, long npages,
>> u64 rpn = __pa(uaddr) >> tbl->it_page_shift;
>> long i;
>>
>> + if (proto_tce & TCE_PCI_WRITE)
>> + proto_tce |= TCE_PCI_READ;
>> +
>> for (i = 0; i < npages; i++) {
>> unsigned long newtce = proto_tce |
>> ((rpn + i) << tbl->it_page_shift);
>> @@ -622,6 +625,9 @@ int pnv_tce_xchg(struct iommu_table *tbl, long
>> index,
>>
>> BUG_ON(*hpa & ~IOMMU_PAGE_MASK(tbl));
>>
>> + if (newtce & TCE_PCI_WRITE)
>> + newtce |= TCE_PCI_READ;
>> +
>> oldtce = xchg(pnv_tce(tbl, idx), cpu_to_be64(newtce));
>> *hpa = be64_to_cpu(oldtce) & ~(TCE_PCI_READ |
>> TCE_PCI_WRITE);
>> *direction = iommu_tce_direction(oldtce);
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFC PATCH kernel] powerpc/ioda: Set "read" permission when "write" is set
2016-01-13 2:24 ` Douglas Miller
@ 2016-01-19 3:52 ` Alexey Kardashevskiy
2016-01-19 19:01 ` Douglas Miller
0 siblings, 1 reply; 12+ messages in thread
From: Alexey Kardashevskiy @ 2016-01-19 3:52 UTC (permalink / raw)
To: Douglas Miller, linuxppc-dev, Benjamin Herrenschmidt
On 01/13/2016 01:24 PM, Douglas Miller wrote:
>
>
> On 01/12/2016 05:07 PM, Benjamin Herrenschmidt wrote:
>> On Tue, 2016-01-12 at 15:40 +1100, Alexey Kardashevskiy wrote:
>>> Quite often drivers set only "write" permission assuming that this
>>> includes "read" permission as well and this works on plenty
>>> platforms.
>>> However IODA2 is strict about this and produces an EEH when "read"
>>> permission is not and reading happens.
>>>
>>> This adds a workaround in IODA code to always add the "read" bit when
>>> the "write" bit is set.
>>>
>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>
>>>
>>> Ben, what was the driver which did not set "read" and caused EEH?
>> aacraid
>>
>> Cheers,
>> Ben.
> Just to be precise, the driver wasn't responsible for setting READ. The
> driver called scsi_dma_map() and the scsicmd was set (by scsi layer) as
> DMA_FROM_DEVICE so the current code would set the permissions to
> WRITE-ONLY. Previously, and in other architectures, this scsicmd would have
> resulted in READ+WRITE permissions on the DMA map.
Does the patch fix the issue? Thanks.
>>
>>> ---
>>> arch/powerpc/platforms/powernv/pci.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/arch/powerpc/platforms/powernv/pci.c
>>> b/arch/powerpc/platforms/powernv/pci.c
>>> index f2dd772..c7dcae5 100644
>>> --- a/arch/powerpc/platforms/powernv/pci.c
>>> +++ b/arch/powerpc/platforms/powernv/pci.c
>>> @@ -601,6 +601,9 @@ int pnv_tce_build(struct iommu_table *tbl, long
>>> index, long npages,
>>> u64 rpn = __pa(uaddr) >> tbl->it_page_shift;
>>> long i;
>>> + if (proto_tce & TCE_PCI_WRITE)
>>> + proto_tce |= TCE_PCI_READ;
>>> +
>>> for (i = 0; i < npages; i++) {
>>> unsigned long newtce = proto_tce |
>>> ((rpn + i) << tbl->it_page_shift);
>>> @@ -622,6 +625,9 @@ int pnv_tce_xchg(struct iommu_table *tbl, long
>>> index,
>>> BUG_ON(*hpa & ~IOMMU_PAGE_MASK(tbl));
>>> + if (newtce & TCE_PCI_WRITE)
>>> + newtce |= TCE_PCI_READ;
>>> +
>>> oldtce = xchg(pnv_tce(tbl, idx), cpu_to_be64(newtce));
>>> *hpa = be64_to_cpu(oldtce) & ~(TCE_PCI_READ |
>>> TCE_PCI_WRITE);
>>> *direction = iommu_tce_direction(oldtce);
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
--
Alexey
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFC PATCH kernel] powerpc/ioda: Set "read" permission when "write" is set
2016-01-19 3:52 ` Alexey Kardashevskiy
@ 2016-01-19 19:01 ` Douglas Miller
2016-02-09 1:37 ` Alexey Kardashevskiy
0 siblings, 1 reply; 12+ messages in thread
From: Douglas Miller @ 2016-01-19 19:01 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev, Benjamin Herrenschmidt
On 01/18/2016 09:52 PM, Alexey Kardashevskiy wrote:
> On 01/13/2016 01:24 PM, Douglas Miller wrote:
>>
>>
>> On 01/12/2016 05:07 PM, Benjamin Herrenschmidt wrote:
>>> On Tue, 2016-01-12 at 15:40 +1100, Alexey Kardashevskiy wrote:
>>>> Quite often drivers set only "write" permission assuming that this
>>>> includes "read" permission as well and this works on plenty
>>>> platforms.
>>>> However IODA2 is strict about this and produces an EEH when "read"
>>>> permission is not and reading happens.
>>>>
>>>> This adds a workaround in IODA code to always add the "read" bit when
>>>> the "write" bit is set.
>>>>
>>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>>
>>>>
>>>> Ben, what was the driver which did not set "read" and caused EEH?
>>> aacraid
>>>
>>> Cheers,
>>> Ben.
>> Just to be precise, the driver wasn't responsible for setting READ. The
>> driver called scsi_dma_map() and the scsicmd was set (by scsi layer) as
>> DMA_FROM_DEVICE so the current code would set the permissions to
>> WRITE-ONLY. Previously, and in other architectures, this scsicmd
>> would have
>> resulted in READ+WRITE permissions on the DMA map.
>
>
> Does the patch fix the issue? Thanks.
>
>
>
>>>
>>>> ---
>>>> arch/powerpc/platforms/powernv/pci.c | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/arch/powerpc/platforms/powernv/pci.c
>>>> b/arch/powerpc/platforms/powernv/pci.c
>>>> index f2dd772..c7dcae5 100644
>>>> --- a/arch/powerpc/platforms/powernv/pci.c
>>>> +++ b/arch/powerpc/platforms/powernv/pci.c
>>>> @@ -601,6 +601,9 @@ int pnv_tce_build(struct iommu_table *tbl, long
>>>> index, long npages,
>>>> u64 rpn = __pa(uaddr) >> tbl->it_page_shift;
>>>> long i;
>>>> + if (proto_tce & TCE_PCI_WRITE)
>>>> + proto_tce |= TCE_PCI_READ;
>>>> +
>>>> for (i = 0; i < npages; i++) {
>>>> unsigned long newtce = proto_tce |
>>>> ((rpn + i) << tbl->it_page_shift);
>>>> @@ -622,6 +625,9 @@ int pnv_tce_xchg(struct iommu_table *tbl, long
>>>> index,
>>>> BUG_ON(*hpa & ~IOMMU_PAGE_MASK(tbl));
>>>> + if (newtce & TCE_PCI_WRITE)
>>>> + newtce |= TCE_PCI_READ;
>>>> +
>>>> oldtce = xchg(pnv_tce(tbl, idx), cpu_to_be64(newtce));
>>>> *hpa = be64_to_cpu(oldtce) & ~(TCE_PCI_READ |
>>>> TCE_PCI_WRITE);
>>>> *direction = iommu_tce_direction(oldtce);
>>> _______________________________________________
>>> Linuxppc-dev mailing list
>>> Linuxppc-dev@lists.ozlabs.org
>>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>>
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
>
I am still working on getting a machine to try this on. From code
inspection, it looks like it should work. The problem is shortage of
machines and machines tied-up by Test.
Thanks,
Doug
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFC PATCH kernel] powerpc/ioda: Set "read" permission when "write" is set
2016-01-19 19:01 ` Douglas Miller
@ 2016-02-09 1:37 ` Alexey Kardashevskiy
2016-02-09 14:28 ` Douglas Miller
0 siblings, 1 reply; 12+ messages in thread
From: Alexey Kardashevskiy @ 2016-02-09 1:37 UTC (permalink / raw)
To: Douglas Miller, linuxppc-dev, Benjamin Herrenschmidt
On 01/20/2016 06:01 AM, Douglas Miller wrote:
>
>
> On 01/18/2016 09:52 PM, Alexey Kardashevskiy wrote:
>> On 01/13/2016 01:24 PM, Douglas Miller wrote:
>>>
>>>
>>> On 01/12/2016 05:07 PM, Benjamin Herrenschmidt wrote:
>>>> On Tue, 2016-01-12 at 15:40 +1100, Alexey Kardashevskiy wrote:
>>>>> Quite often drivers set only "write" permission assuming that this
>>>>> includes "read" permission as well and this works on plenty
>>>>> platforms.
>>>>> However IODA2 is strict about this and produces an EEH when "read"
>>>>> permission is not and reading happens.
>>>>>
>>>>> This adds a workaround in IODA code to always add the "read" bit when
>>>>> the "write" bit is set.
>>>>>
>>>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>> ---
>>>>>
>>>>>
>>>>> Ben, what was the driver which did not set "read" and caused EEH?
>>>> aacraid
>>>>
>>>> Cheers,
>>>> Ben.
>>> Just to be precise, the driver wasn't responsible for setting READ. The
>>> driver called scsi_dma_map() and the scsicmd was set (by scsi layer) as
>>> DMA_FROM_DEVICE so the current code would set the permissions to
>>> WRITE-ONLY. Previously, and in other architectures, this scsicmd would have
>>> resulted in READ+WRITE permissions on the DMA map.
>>
>>
>> Does the patch fix the issue? Thanks.
>>
>>
>>
>>>>
>>>>> ---
>>>>> arch/powerpc/platforms/powernv/pci.c | 6 ++++++
>>>>> 1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/arch/powerpc/platforms/powernv/pci.c
>>>>> b/arch/powerpc/platforms/powernv/pci.c
>>>>> index f2dd772..c7dcae5 100644
>>>>> --- a/arch/powerpc/platforms/powernv/pci.c
>>>>> +++ b/arch/powerpc/platforms/powernv/pci.c
>>>>> @@ -601,6 +601,9 @@ int pnv_tce_build(struct iommu_table *tbl, long
>>>>> index, long npages,
>>>>> u64 rpn = __pa(uaddr) >> tbl->it_page_shift;
>>>>> long i;
>>>>> + if (proto_tce & TCE_PCI_WRITE)
>>>>> + proto_tce |= TCE_PCI_READ;
>>>>> +
>>>>> for (i = 0; i < npages; i++) {
>>>>> unsigned long newtce = proto_tce |
>>>>> ((rpn + i) << tbl->it_page_shift);
>>>>> @@ -622,6 +625,9 @@ int pnv_tce_xchg(struct iommu_table *tbl, long
>>>>> index,
>>>>> BUG_ON(*hpa & ~IOMMU_PAGE_MASK(tbl));
>>>>> + if (newtce & TCE_PCI_WRITE)
>>>>> + newtce |= TCE_PCI_READ;
>>>>> +
>>>>> oldtce = xchg(pnv_tce(tbl, idx), cpu_to_be64(newtce));
>>>>> *hpa = be64_to_cpu(oldtce) & ~(TCE_PCI_READ |
>>>>> TCE_PCI_WRITE);
>>>>> *direction = iommu_tce_direction(oldtce);
>>>> _______________________________________________
>>>> Linuxppc-dev mailing list
>>>> Linuxppc-dev@lists.ozlabs.org
>>>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>>>
>>> _______________________________________________
>>> Linuxppc-dev mailing list
>>> Linuxppc-dev@lists.ozlabs.org
>>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>>
>>
> I am still working on getting a machine to try this on. From code
> inspection, it looks like it should work. The problem is shortage of
> machines and machines tied-up by Test.
Any progress here? Thanks.
--
Alexey
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFC PATCH kernel] powerpc/ioda: Set "read" permission when "write" is set
2016-02-09 1:37 ` Alexey Kardashevskiy
@ 2016-02-09 14:28 ` Douglas Miller
2016-02-10 0:32 ` Alexey Kardashevskiy
0 siblings, 1 reply; 12+ messages in thread
From: Douglas Miller @ 2016-02-09 14:28 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev, Benjamin Herrenschmidt
We finally got the chance to test it end of last week. I forgot to
update everyone Monday. B all appearances, the patch fixes the problem.
We did not see any new issues with the patch (vs. same test scenarios
without).
I'll also update the bugzilla.
Thanks,
Doug
On 02/08/2016 07:37 PM, Alexey Kardashevskiy wrote:
> On 01/20/2016 06:01 AM, Douglas Miller wrote:
>>
>>
>> On 01/18/2016 09:52 PM, Alexey Kardashevskiy wrote:
>>> On 01/13/2016 01:24 PM, Douglas Miller wrote:
>>>>
>>>>
>>>> On 01/12/2016 05:07 PM, Benjamin Herrenschmidt wrote:
>>>>> On Tue, 2016-01-12 at 15:40 +1100, Alexey Kardashevskiy wrote:
>>>>>> Quite often drivers set only "write" permission assuming that this
>>>>>> includes "read" permission as well and this works on plenty
>>>>>> platforms.
>>>>>> However IODA2 is strict about this and produces an EEH when "read"
>>>>>> permission is not and reading happens.
>>>>>>
>>>>>> This adds a workaround in IODA code to always add the "read" bit
>>>>>> when
>>>>>> the "write" bit is set.
>>>>>>
>>>>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>> ---
>>>>>>
>>>>>>
>>>>>> Ben, what was the driver which did not set "read" and caused EEH?
>>>>> aacraid
>>>>>
>>>>> Cheers,
>>>>> Ben.
>>>> Just to be precise, the driver wasn't responsible for setting READ.
>>>> The
>>>> driver called scsi_dma_map() and the scsicmd was set (by scsi
>>>> layer) as
>>>> DMA_FROM_DEVICE so the current code would set the permissions to
>>>> WRITE-ONLY. Previously, and in other architectures, this scsicmd
>>>> would have
>>>> resulted in READ+WRITE permissions on the DMA map.
>>>
>>>
>>> Does the patch fix the issue? Thanks.
>>>
>>>
>>>
>>>>>
>>>>>> ---
>>>>>> arch/powerpc/platforms/powernv/pci.c | 6 ++++++
>>>>>> 1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/powerpc/platforms/powernv/pci.c
>>>>>> b/arch/powerpc/platforms/powernv/pci.c
>>>>>> index f2dd772..c7dcae5 100644
>>>>>> --- a/arch/powerpc/platforms/powernv/pci.c
>>>>>> +++ b/arch/powerpc/platforms/powernv/pci.c
>>>>>> @@ -601,6 +601,9 @@ int pnv_tce_build(struct iommu_table *tbl, long
>>>>>> index, long npages,
>>>>>> u64 rpn = __pa(uaddr) >> tbl->it_page_shift;
>>>>>> long i;
>>>>>> + if (proto_tce & TCE_PCI_WRITE)
>>>>>> + proto_tce |= TCE_PCI_READ;
>>>>>> +
>>>>>> for (i = 0; i < npages; i++) {
>>>>>> unsigned long newtce = proto_tce |
>>>>>> ((rpn + i) << tbl->it_page_shift);
>>>>>> @@ -622,6 +625,9 @@ int pnv_tce_xchg(struct iommu_table *tbl, long
>>>>>> index,
>>>>>> BUG_ON(*hpa & ~IOMMU_PAGE_MASK(tbl));
>>>>>> + if (newtce & TCE_PCI_WRITE)
>>>>>> + newtce |= TCE_PCI_READ;
>>>>>> +
>>>>>> oldtce = xchg(pnv_tce(tbl, idx), cpu_to_be64(newtce));
>>>>>> *hpa = be64_to_cpu(oldtce) & ~(TCE_PCI_READ |
>>>>>> TCE_PCI_WRITE);
>>>>>> *direction = iommu_tce_direction(oldtce);
>>>>> _______________________________________________
>>>>> Linuxppc-dev mailing list
>>>>> Linuxppc-dev@lists.ozlabs.org
>>>>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>>>>
>>>> _______________________________________________
>>>> Linuxppc-dev mailing list
>>>> Linuxppc-dev@lists.ozlabs.org
>>>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>>>
>>>
>> I am still working on getting a machine to try this on. From code
>> inspection, it looks like it should work. The problem is shortage of
>> machines and machines tied-up by Test.
>
> Any progress here? Thanks.
>
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFC PATCH kernel] powerpc/ioda: Set "read" permission when "write" is set
2016-02-09 14:28 ` Douglas Miller
@ 2016-02-10 0:32 ` Alexey Kardashevskiy
0 siblings, 0 replies; 12+ messages in thread
From: Alexey Kardashevskiy @ 2016-02-10 0:32 UTC (permalink / raw)
To: Douglas Miller, linuxppc-dev, Benjamin Herrenschmidt
On 02/10/2016 01:28 AM, Douglas Miller wrote:
> We finally got the chance to test it end of last week. I forgot to update
> everyone Monday. B all appearances, the patch fixes the problem. We did not
> see any new issues with the patch (vs. same test scenarios without).
>
> I'll also update the bugzilla.
Thanks. Care to add "Tested-by"?
>
> Thanks,
> Doug
>
> On 02/08/2016 07:37 PM, Alexey Kardashevskiy wrote:
>> On 01/20/2016 06:01 AM, Douglas Miller wrote:
>>>
>>>
>>> On 01/18/2016 09:52 PM, Alexey Kardashevskiy wrote:
>>>> On 01/13/2016 01:24 PM, Douglas Miller wrote:
>>>>>
>>>>>
>>>>> On 01/12/2016 05:07 PM, Benjamin Herrenschmidt wrote:
>>>>>> On Tue, 2016-01-12 at 15:40 +1100, Alexey Kardashevskiy wrote:
>>>>>>> Quite often drivers set only "write" permission assuming that this
>>>>>>> includes "read" permission as well and this works on plenty
>>>>>>> platforms.
>>>>>>> However IODA2 is strict about this and produces an EEH when "read"
>>>>>>> permission is not and reading happens.
>>>>>>>
>>>>>>> This adds a workaround in IODA code to always add the "read" bit when
>>>>>>> the "write" bit is set.
>>>>>>>
>>>>>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>>> ---
>>>>>>>
>>>>>>>
>>>>>>> Ben, what was the driver which did not set "read" and caused EEH?
>>>>>> aacraid
>>>>>>
>>>>>> Cheers,
>>>>>> Ben.
>>>>> Just to be precise, the driver wasn't responsible for setting READ. The
>>>>> driver called scsi_dma_map() and the scsicmd was set (by scsi layer) as
>>>>> DMA_FROM_DEVICE so the current code would set the permissions to
>>>>> WRITE-ONLY. Previously, and in other architectures, this scsicmd would
>>>>> have
>>>>> resulted in READ+WRITE permissions on the DMA map.
>>>>
>>>>
>>>> Does the patch fix the issue? Thanks.
>>>>
>>>>
>>>>
>>>>>>
>>>>>>> ---
>>>>>>> arch/powerpc/platforms/powernv/pci.c | 6 ++++++
>>>>>>> 1 file changed, 6 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/powerpc/platforms/powernv/pci.c
>>>>>>> b/arch/powerpc/platforms/powernv/pci.c
>>>>>>> index f2dd772..c7dcae5 100644
>>>>>>> --- a/arch/powerpc/platforms/powernv/pci.c
>>>>>>> +++ b/arch/powerpc/platforms/powernv/pci.c
>>>>>>> @@ -601,6 +601,9 @@ int pnv_tce_build(struct iommu_table *tbl, long
>>>>>>> index, long npages,
>>>>>>> u64 rpn = __pa(uaddr) >> tbl->it_page_shift;
>>>>>>> long i;
>>>>>>> + if (proto_tce & TCE_PCI_WRITE)
>>>>>>> + proto_tce |= TCE_PCI_READ;
>>>>>>> +
>>>>>>> for (i = 0; i < npages; i++) {
>>>>>>> unsigned long newtce = proto_tce |
>>>>>>> ((rpn + i) << tbl->it_page_shift);
>>>>>>> @@ -622,6 +625,9 @@ int pnv_tce_xchg(struct iommu_table *tbl, long
>>>>>>> index,
>>>>>>> BUG_ON(*hpa & ~IOMMU_PAGE_MASK(tbl));
>>>>>>> + if (newtce & TCE_PCI_WRITE)
>>>>>>> + newtce |= TCE_PCI_READ;
>>>>>>> +
>>>>>>> oldtce = xchg(pnv_tce(tbl, idx), cpu_to_be64(newtce));
>>>>>>> *hpa = be64_to_cpu(oldtce) & ~(TCE_PCI_READ |
>>>>>>> TCE_PCI_WRITE);
>>>>>>> *direction = iommu_tce_direction(oldtce);
>>>>>> _______________________________________________
>>>>>> Linuxppc-dev mailing list
>>>>>> Linuxppc-dev@lists.ozlabs.org
>>>>>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>>>>>
>>>>> _______________________________________________
>>>>> Linuxppc-dev mailing list
>>>>> Linuxppc-dev@lists.ozlabs.org
>>>>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>>>>
>>>>
>>> I am still working on getting a machine to try this on. From code
>>> inspection, it looks like it should work. The problem is shortage of
>>> machines and machines tied-up by Test.
>>
>> Any progress here? Thanks.
>>
>>
>>
>>
>
--
Alexey
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH kernel] powerpc/ioda: Set "read" permission when "write" is set
2016-01-12 4:40 [RFC PATCH kernel] powerpc/ioda: Set "read" permission when "write" is set Alexey Kardashevskiy
2016-01-12 23:07 ` Benjamin Herrenschmidt
@ 2016-02-10 12:26 ` Douglas Miller
2016-02-16 3:18 ` [RFC, " Michael Ellerman
2 siblings, 0 replies; 12+ messages in thread
From: Douglas Miller @ 2016-02-10 12:26 UTC (permalink / raw)
To: linuxppc-dev
Tested-by: Douglas Miller <dougmill@linux.vnet.ibm.com>
On 01/11/2016 10:40 PM, Alexey Kardashevskiy wrote:
> Quite often drivers set only "write" permission assuming that this
> includes "read" permission as well and this works on plenty platforms.
> However IODA2 is strict about this and produces an EEH when "read"
> permission is not and reading happens.
>
> This adds a workaround in IODA code to always add the "read" bit when
> the "write" bit is set.
>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>
>
> Ben, what was the driver which did not set "read" and caused EEH?
>
>
> ---
> arch/powerpc/platforms/powernv/pci.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
> index f2dd772..c7dcae5 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -601,6 +601,9 @@ int pnv_tce_build(struct iommu_table *tbl, long index, long npages,
> u64 rpn = __pa(uaddr) >> tbl->it_page_shift;
> long i;
>
> + if (proto_tce & TCE_PCI_WRITE)
> + proto_tce |= TCE_PCI_READ;
> +
> for (i = 0; i < npages; i++) {
> unsigned long newtce = proto_tce |
> ((rpn + i) << tbl->it_page_shift);
> @@ -622,6 +625,9 @@ int pnv_tce_xchg(struct iommu_table *tbl, long index,
>
> BUG_ON(*hpa & ~IOMMU_PAGE_MASK(tbl));
>
> + if (newtce & TCE_PCI_WRITE)
> + newtce |= TCE_PCI_READ;
> +
> oldtce = xchg(pnv_tce(tbl, idx), cpu_to_be64(newtce));
> *hpa = be64_to_cpu(oldtce) & ~(TCE_PCI_READ | TCE_PCI_WRITE);
> *direction = iommu_tce_direction(oldtce);
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFC, kernel] powerpc/ioda: Set "read" permission when "write" is set
2016-01-12 4:40 [RFC PATCH kernel] powerpc/ioda: Set "read" permission when "write" is set Alexey Kardashevskiy
2016-01-12 23:07 ` Benjamin Herrenschmidt
2016-02-10 12:26 ` Douglas Miller
@ 2016-02-16 3:18 ` Michael Ellerman
2016-02-16 13:20 ` Douglas Miller
2016-02-17 7:29 ` Alexey Kardashevskiy
2 siblings, 2 replies; 12+ messages in thread
From: Michael Ellerman @ 2016-02-16 3:18 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev
Cc: Alexey Kardashevskiy, Paul Mackerras, David Gibson
On Tue, 2016-12-01 at 04:40:20 UTC, Alexey Kardashevskiy wrote:
> Quite often drivers set only "write" permission assuming that this
> includes "read" permission as well and this works on plenty platforms.
> However IODA2 is strict about this and produces an EEH when "read"
> permission is not and reading happens.
>
> This adds a workaround in IODA code to always add the "read" bit when
> the "write" bit is set.
>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Tested-by: Douglas Miller <dougmill@linux.vnet.ibm.com>
Are you planning on sending a non-RFC version of this?
If so is it an urgent fix I should send upstream now? And if so should it
also be CC'ed to stable?
cheers
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC, kernel] powerpc/ioda: Set "read" permission when "write" is set
2016-02-16 3:18 ` [RFC, " Michael Ellerman
@ 2016-02-16 13:20 ` Douglas Miller
2016-02-17 7:29 ` Alexey Kardashevskiy
1 sibling, 0 replies; 12+ messages in thread
From: Douglas Miller @ 2016-02-16 13:20 UTC (permalink / raw)
To: linuxppc-dev
On 02/15/2016 09:18 PM, Michael Ellerman wrote:
> On Tue, 2016-12-01 at 04:40:20 UTC, Alexey Kardashevskiy wrote:
>> Quite often drivers set only "write" permission assuming that this
>> includes "read" permission as well and this works on plenty platforms.
>> However IODA2 is strict about this and produces an EEH when "read"
>> permission is not and reading happens.
>>
>> This adds a workaround in IODA code to always add the "read" bit when
>> the "write" bit is set.
>>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Tested-by: Douglas Miller <dougmill@linux.vnet.ibm.com>
> Are you planning on sending a non-RFC version of this?
>
> If so is it an urgent fix I should send upstream now? And if so should it
> also be CC'ed to stable?
>
> cheers
I vote for the more-urgent case. This is the result of a fairly recent
change in behavior and we may not have found all the adapters that will
break yet. If we get this upstream sooner, it means less exposure.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC, kernel] powerpc/ioda: Set "read" permission when "write" is set
2016-02-16 3:18 ` [RFC, " Michael Ellerman
2016-02-16 13:20 ` Douglas Miller
@ 2016-02-17 7:29 ` Alexey Kardashevskiy
1 sibling, 0 replies; 12+ messages in thread
From: Alexey Kardashevskiy @ 2016-02-17 7:29 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: Paul Mackerras, David Gibson
On 02/16/2016 02:18 PM, Michael Ellerman wrote:
> On Tue, 2016-12-01 at 04:40:20 UTC, Alexey Kardashevskiy wrote:
>> Quite often drivers set only "write" permission assuming that this
>> includes "read" permission as well and this works on plenty platforms.
>> However IODA2 is strict about this and produces an EEH when "read"
>> permission is not and reading happens.
>>
>> This adds a workaround in IODA code to always add the "read" bit when
>> the "write" bit is set.
>>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Tested-by: Douglas Miller <dougmill@linux.vnet.ibm.com>
>
> Are you planning on sending a non-RFC version of this?
Just posted.
> If so is it an urgent fix I should send upstream now?
Yes.
> And if so should it also be CC'ed to stable?
Ben suggested that yes, it should. Thanks. Sorry about breaking things.
--
Alexey
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-02-17 7:29 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-12 4:40 [RFC PATCH kernel] powerpc/ioda: Set "read" permission when "write" is set Alexey Kardashevskiy
2016-01-12 23:07 ` Benjamin Herrenschmidt
2016-01-13 2:24 ` Douglas Miller
2016-01-19 3:52 ` Alexey Kardashevskiy
2016-01-19 19:01 ` Douglas Miller
2016-02-09 1:37 ` Alexey Kardashevskiy
2016-02-09 14:28 ` Douglas Miller
2016-02-10 0:32 ` Alexey Kardashevskiy
2016-02-10 12:26 ` Douglas Miller
2016-02-16 3:18 ` [RFC, " Michael Ellerman
2016-02-16 13:20 ` Douglas Miller
2016-02-17 7:29 ` Alexey Kardashevskiy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).