[PATCH] powerpc/4xx/ocm: fix compiler error

[PATCH] powerpc/4xx/ocm: fix compiler error

[Christian Lamparter]

This patch fixes a recent regression in ocm:

ocm.c: In function ‘ocm_init_node’:
ocm.c:182:18: error: invalid operands to binary |
ocm.c:197:17: error: invalid operands to binary |

Fixes: 56f3c1413f5c ("powerpc/mm: properly set PAGE_KERNEL flags in ioremap()")
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
 arch/powerpc/platforms/4xx/ocm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/4xx/ocm.c b/arch/powerpc/platforms/4xx/ocm.c
index 561b09de69bf..04ff69ddac09 100644
--- a/arch/powerpc/platforms/4xx/ocm.c
+++ b/arch/powerpc/platforms/4xx/ocm.c
@@ -179,7 +179,8 @@ static void __init ocm_init_node(int count, struct device_node *node)
 	/* ioremap the non-cached region */
 	if (ocm->nc.memtotal) {
 		ocm->nc.virt = __ioremap(ocm->nc.phys, ocm->nc.memtotal,
-					 _PAGE_EXEC | PAGE_KERNEL_NCG);
+			_PAGE_EXEC | _PAGE_BASE_NC |
+			_PAGE_KERNEL_RW | _PAGE_NO_CACHE | _PAGE_GUARDED);
 
 		if (!ocm->nc.virt) {
 			printk(KERN_ERR
@@ -194,7 +195,8 @@ static void __init ocm_init_node(int count, struct device_node *node)
 
 	if (ocm->c.memtotal) {
 		ocm->c.virt = __ioremap(ocm->c.phys, ocm->c.memtotal,
-					_PAGE_EXEC | PAGE_KERNEL);
+					_PAGE_EXEC | _PAGE_BASE |
+					_PAGE_KERNEL_RW);
 
 		if (!ocm->c.virt) {
 			printk(KERN_ERR
-- 
2.20.1

Re: [PATCH] powerpc/4xx/ocm: fix compiler error

[christophe leroy]

Le 22/12/2018 à 11:09, Christian Lamparter a écrit :
> This patch fixes a recent regression in ocm:
> 
> ocm.c: In function ‘ocm_init_node’:
> ocm.c:182:18: error: invalid operands to binary |
> ocm.c:197:17: error: invalid operands to binary |
> 
> Fixes: 56f3c1413f5c ("powerpc/mm: properly set PAGE_KERNEL flags in ioremap()")
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>

What's the problem here ? Is PAGE_KERNEL_NCG undefined ? If that's the 
case, wouldn't it be better for fix that by including asm/pgtable.h ?

Christophe

> ---
>   arch/powerpc/platforms/4xx/ocm.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/4xx/ocm.c b/arch/powerpc/platforms/4xx/ocm.c
> index 561b09de69bf..04ff69ddac09 100644
> --- a/arch/powerpc/platforms/4xx/ocm.c
> +++ b/arch/powerpc/platforms/4xx/ocm.c
> @@ -179,7 +179,8 @@ static void __init ocm_init_node(int count, struct device_node *node)
>   	/* ioremap the non-cached region */
>   	if (ocm->nc.memtotal) {
>   		ocm->nc.virt = __ioremap(ocm->nc.phys, ocm->nc.memtotal,
> -					 _PAGE_EXEC | PAGE_KERNEL_NCG);
> +			_PAGE_EXEC | _PAGE_BASE_NC |
> +			_PAGE_KERNEL_RW | _PAGE_NO_CACHE | _PAGE_GUARDED);
>   
>   		if (!ocm->nc.virt) {
>   			printk(KERN_ERR
> @@ -194,7 +195,8 @@ static void __init ocm_init_node(int count, struct device_node *node)
>   
>   	if (ocm->c.memtotal) {
>   		ocm->c.virt = __ioremap(ocm->c.phys, ocm->c.memtotal,
> -					_PAGE_EXEC | PAGE_KERNEL);
> +					_PAGE_EXEC | _PAGE_BASE |
> +					_PAGE_KERNEL_RW);
>   
>   		if (!ocm->c.virt) {
>   			printk(KERN_ERR
> 

---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus

Re: [PATCH] powerpc/4xx/ocm: fix compiler error

[Christian Lamparter]

On Saturday, December 22, 2018 11:59:04 AM CET christophe leroy wrote:
> 
> Le 22/12/2018 à 11:09, Christian Lamparter a écrit :
> > This patch fixes a recent regression in ocm:
> > 
> > ocm.c: In function ‘ocm_init_node’:
> > ocm.c:182:18: error: invalid operands to binary |
> > ocm.c:197:17: error: invalid operands to binary |
> > 
> > Fixes: 56f3c1413f5c ("powerpc/mm: properly set PAGE_KERNEL flags in ioremap()")
> > Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> 
> What's the problem here ? Is PAGE_KERNEL_NCG undefined ? If that's the 
> case, wouldn't it be better for fix that by including asm/pgtable.h ?

PAGE_KERNEL[_NCG] type is a struct of pgprot_t, whereas _PAGE_EXEC is
considered an int.

arch/powerpc/platforms/4xx/ocm.c: In function ‘ocm_init_node’:
arch/powerpc/platforms/4xx/ocm.c:182:18: error: invalid operands to binary | (have ‘int’ and ‘pgprot_t’ {aka ‘struct <anonymous>’})
       _PAGE_EXEC | PAGE_KERNEL_NCG);
                  ^
arch/powerpc/platforms/4xx/ocm.c:197:17: error: invalid operands to binary | (have ‘int’ and ‘pgprot_t’ {aka ‘struct <anonymous>’})
       _PAGE_EXEC | PAGE_KERNEL);
                  ^

I think pgprot_val(PAGE_KERNEL[_NCG]) could be an option too.

Christian
> > ---
> >   arch/powerpc/platforms/4xx/ocm.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/powerpc/platforms/4xx/ocm.c b/arch/powerpc/platforms/4xx/ocm.c
> > index 561b09de69bf..04ff69ddac09 100644
> > --- a/arch/powerpc/platforms/4xx/ocm.c
> > +++ b/arch/powerpc/platforms/4xx/ocm.c
> > @@ -179,7 +179,8 @@ static void __init ocm_init_node(int count, struct device_node *node)
> >   	/* ioremap the non-cached region */
> >   	if (ocm->nc.memtotal) {
> >   		ocm->nc.virt = __ioremap(ocm->nc.phys, ocm->nc.memtotal,
> > -					 _PAGE_EXEC | PAGE_KERNEL_NCG);
> > +			_PAGE_EXEC | _PAGE_BASE_NC |
> > +			_PAGE_KERNEL_RW | _PAGE_NO_CACHE | _PAGE_GUARDED);
> >   
> >   		if (!ocm->nc.virt) {
> >   			printk(KERN_ERR
> > @@ -194,7 +195,8 @@ static void __init ocm_init_node(int count, struct device_node *node)
> >   
> >   	if (ocm->c.memtotal) {
> >   		ocm->c.virt = __ioremap(ocm->c.phys, ocm->c.memtotal,
> > -					_PAGE_EXEC | PAGE_KERNEL);
> > +					_PAGE_EXEC | _PAGE_BASE |
> > +					_PAGE_KERNEL_RW);
> >   
> >   		if (!ocm->c.virt) {
> >   			printk(KERN_ERR
> > 
> 

Re: [PATCH] powerpc/4xx/ocm: fix compiler error

[christophe leroy]

Le 22/12/2018 à 12:27, Christian Lamparter a écrit :
> On Saturday, December 22, 2018 11:59:04 AM CET christophe leroy wrote:
>>
>> Le 22/12/2018 à 11:09, Christian Lamparter a écrit :
>>> This patch fixes a recent regression in ocm:
>>>
>>> ocm.c: In function ‘ocm_init_node’:
>>> ocm.c:182:18: error: invalid operands to binary |
>>> ocm.c:197:17: error: invalid operands to binary |
>>>
>>> Fixes: 56f3c1413f5c ("powerpc/mm: properly set PAGE_KERNEL flags in ioremap()")
>>> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
>>
>> What's the problem here ? Is PAGE_KERNEL_NCG undefined ? If that's the
>> case, wouldn't it be better for fix that by including asm/pgtable.h ?
> 
> PAGE_KERNEL[_NCG] type is a struct of pgprot_t, whereas _PAGE_EXEC is
> considered an int.

Oops, I missed that.

Could you put the entire error message in the commit log ?

> 
> arch/powerpc/platforms/4xx/ocm.c: In function ‘ocm_init_node’:
> arch/powerpc/platforms/4xx/ocm.c:182:18: error: invalid operands to binary | (have ‘int’ and ‘pgprot_t’ {aka ‘struct <anonymous>’})
>         _PAGE_EXEC | PAGE_KERNEL_NCG);

Usually, Guarded implies no exec (at least on 6xx and 8xx). Does the 4xx 
accept guarded exec ?

>                    ^
> arch/powerpc/platforms/4xx/ocm.c:197:17: error: invalid operands to binary | (have ‘int’ and ‘pgprot_t’ {aka ‘struct <anonymous>’})
>         _PAGE_EXEC | PAGE_KERNEL);

That's PAGE_KERNEL_X

>                    ^
> 
> I think pgprot_val(PAGE_KERNEL[_NCG]) could be an option too.

Yes I may have a preference for that.

> 
> Christian
>>> ---
>>>    arch/powerpc/platforms/4xx/ocm.c | 6 ++++--
>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/4xx/ocm.c b/arch/powerpc/platforms/4xx/ocm.c
>>> index 561b09de69bf..04ff69ddac09 100644
>>> --- a/arch/powerpc/platforms/4xx/ocm.c
>>> +++ b/arch/powerpc/platforms/4xx/ocm.c
>>> @@ -179,7 +179,8 @@ static void __init ocm_init_node(int count, struct device_node *node)
>>>    	/* ioremap the non-cached region */
>>>    	if (ocm->nc.memtotal) {
>>>    		ocm->nc.virt = __ioremap(ocm->nc.phys, ocm->nc.memtotal,
>>> -					 _PAGE_EXEC | PAGE_KERNEL_NCG);
>>> +			_PAGE_EXEC | _PAGE_BASE_NC |
>>> +			_PAGE_KERNEL_RW | _PAGE_NO_CACHE | _PAGE_GUARDED);

Could be _PAGE_BASE_NC | _PAGE_KERNEL_RWX | _PAGE_NO_CACHE | _PAGE_GUARDED

Or pgprot_val(PAGE_KERNEL_NCG) | _PAGE_EXEC

Or pgprot_val(pgprot_noncached(PAGE_KERNEL_RWX)) (that's my preference)

>>>    
>>>    		if (!ocm->nc.virt) {
>>>    			printk(KERN_ERR
>>> @@ -194,7 +195,8 @@ static void __init ocm_init_node(int count, struct device_node *node)
>>>    
>>>    	if (ocm->c.memtotal) {
>>>    		ocm->c.virt = __ioremap(ocm->c.phys, ocm->c.memtotal,
>>> -					_PAGE_EXEC | PAGE_KERNEL);
>>> +					_PAGE_EXEC | _PAGE_BASE |
>>> +					_PAGE_KERNEL_RW);

What about _PAGE_BASE | _PAGE_KERNEL_RWX instead ?

Or pgprot_val(PAGE_KERNEL_RWX) (that's my preference)

Christophe

>>>    
>>>    		if (!ocm->c.virt) {
>>>    			printk(KERN_ERR
>>>
>>

---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus

Re: [PATCH] powerpc/4xx/ocm: fix compiler error

[Christian Lamparter]

On Saturday, December 22, 2018 2:08:02 PM CET christophe leroy wrote:
> 
> Le 22/12/2018 à 12:27, Christian Lamparter a écrit :
> > On Saturday, December 22, 2018 11:59:04 AM CET christophe leroy wrote:
> >>
> >> Le 22/12/2018 à 11:09, Christian Lamparter a écrit :
> >>> This patch fixes a recent regression in ocm:
> >>>
> >>> ocm.c: In function ‘ocm_init_node’:
> >>> ocm.c:182:18: error: invalid operands to binary |
> >>> ocm.c:197:17: error: invalid operands to binary |
> >>>
> >>> Fixes: 56f3c1413f5c ("powerpc/mm: properly set PAGE_KERNEL flags in ioremap()")
> >>> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> >>
> >> What's the problem here ? Is PAGE_KERNEL_NCG undefined ? If that's the
> >> case, wouldn't it be better for fix that by including asm/pgtable.h ?
> > 
> > PAGE_KERNEL[_NCG] type is a struct of pgprot_t, whereas _PAGE_EXEC is
> > considered an int.
> 
> Oops, I missed that.
> 
> Could you put the entire error message in the commit log ?
Will do in v2... which I'm going to sent out shortly. I mainly wanted to
make the most "trivial fix" since it probably needs to be backported to
4.20-stable at this late in the rc phase.

> > 
> > arch/powerpc/platforms/4xx/ocm.c: In function ‘ocm_init_node’:
> > arch/powerpc/platforms/4xx/ocm.c:182:18: error: invalid operands to binary | (have ‘int’ and ‘pgprot_t’ {aka ‘struct <anonymous>’})
> >         _PAGE_EXEC | PAGE_KERNEL_NCG);
> 
> Usually, Guarded implies no exec (at least on 6xx and 8xx). Does the 4xx 
> accept guarded exec ?

Oh, I simply copied over the individual _PAGE_* flags that the PAGE_KERNEL_NCG
macro set. The OCM is usually a small (32 KiB) DMA descriptor storage, I don't
think anyone would want to execute code in there.

> 
> >                    ^
> > arch/powerpc/platforms/4xx/ocm.c:197:17: error: invalid operands to binary | (have ‘int’ and ‘pgprot_t’ {aka ‘struct <anonymous>’})
> >         _PAGE_EXEC | PAGE_KERNEL);
> 
> That's PAGE_KERNEL_X
> 
> >                    ^
> > 
> > I think pgprot_val(PAGE_KERNEL[_NCG]) could be an option too.
> 
> Yes I may have a preference for that.

K, I liked having all _PAGE_*. But I'm fine either way. 
see you for v2. :-D 
> > 
> > Christian
> >>> ---
> >>>    arch/powerpc/platforms/4xx/ocm.c | 6 ++++--
> >>>    1 file changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/arch/powerpc/platforms/4xx/ocm.c b/arch/powerpc/platforms/4xx/ocm.c
> >>> index 561b09de69bf..04ff69ddac09 100644
> >>> --- a/arch/powerpc/platforms/4xx/ocm.c
> >>> +++ b/arch/powerpc/platforms/4xx/ocm.c
> >>> @@ -179,7 +179,8 @@ static void __init ocm_init_node(int count, struct device_node *node)
> >>>    	/* ioremap the non-cached region */
> >>>    	if (ocm->nc.memtotal) {
> >>>    		ocm->nc.virt = __ioremap(ocm->nc.phys, ocm->nc.memtotal,
> >>> -					 _PAGE_EXEC | PAGE_KERNEL_NCG);
> >>> +			_PAGE_EXEC | _PAGE_BASE_NC |
> >>> +			_PAGE_KERNEL_RW | _PAGE_NO_CACHE | _PAGE_GUARDED);
> 
> Could be _PAGE_BASE_NC | _PAGE_KERNEL_RWX | _PAGE_NO_CACHE | _PAGE_GUARDED
> 
> Or pgprot_val(PAGE_KERNEL_NCG) | _PAGE_EXEC
> 
> Or pgprot_val(pgprot_noncached(PAGE_KERNEL_RWX)) (that's my preference)
> 
> >>>    
> >>>    		if (!ocm->nc.virt) {
> >>>    			printk(KERN_ERR
> >>> @@ -194,7 +195,8 @@ static void __init ocm_init_node(int count, struct device_node *node)
> >>>    
> >>>    	if (ocm->c.memtotal) {
> >>>    		ocm->c.virt = __ioremap(ocm->c.phys, ocm->c.memtotal,
> >>> -					_PAGE_EXEC | PAGE_KERNEL);
> >>> +					_PAGE_EXEC | _PAGE_BASE |
> >>> +					_PAGE_KERNEL_RW);
> 
> What about _PAGE_BASE | _PAGE_KERNEL_RWX instead ?
> 
> Or pgprot_val(PAGE_KERNEL_RWX) (that's my preference)

I'll test if any of these make a difference and sent a separate patch.

Christian

Re: [PATCH] powerpc/4xx/ocm: fix compiler error

[Segher Boessenkool]

On Sat, Dec 22, 2018 at 02:08:02PM +0100, christophe leroy wrote:
> 
> Usually, Guarded implies no exec (at least on 6xx and 8xx).

Huh?  What do you mean here?


Segher

Re: [PATCH] powerpc/4xx/ocm: fix compiler error

[christophe leroy]

Le 22/12/2018 à 18:16, Segher Boessenkool a écrit :
> On Sat, Dec 22, 2018 at 02:08:02PM +0100, christophe leroy wrote:
>>
>> Usually, Guarded implies no exec (at least on 6xx and 8xx).
> 
> Huh?  What do you mean here?
> 

 From the 885 Reference Manual:

Address translation: the EA is translated by using the MMU’s TLB 
mechanism. Instructions are not fetched from no-execute or guarded 
memory and data accesses are not executed speculatively to or from the 
guarded memory.

6.1.3.4 Instruction TLB Error Exception (0x01300)
This type of exception occurs as a result of one of the following 
conditions if MSR[IR] = 1:
- The EA cannot be translated.
- The fetch access violates memory protection
- The fetch access is to guarded memory


 From e300core reference manual:

Translation Exception Conditions:
Exception condition: Instruction fetch from guarded memory
with MSR[IR] = 1 ==> ISI interrupt SRR1[3] = 1


Christophe

---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus

Re: [PATCH] powerpc/4xx/ocm: fix compiler error

[Segher Boessenkool]

On Sat, Dec 22, 2018 at 08:37:28PM +0100, christophe leroy wrote:
> Le 22/12/2018 à 18:16, Segher Boessenkool a écrit :
> >On Sat, Dec 22, 2018 at 02:08:02PM +0100, christophe leroy wrote:
> >>
> >>Usually, Guarded implies no exec (at least on 6xx and 8xx).
> >
> >Huh?  What do you mean here?
> 
> From the 885 Reference Manual:
> 
> Address translation: the EA is translated by using the MMU’s TLB 
> mechanism. Instructions are not fetched from no-execute or guarded 
> memory and data accesses are not executed speculatively to or from the 
> guarded memory.
> 
> 6.1.3.4 Instruction TLB Error Exception (0x01300)
> This type of exception occurs as a result of one of the following 
> conditions if MSR[IR] = 1:
> - The EA cannot be translated.
> - The fetch access violates memory protection
> - The fetch access is to guarded memory
> 
> 
> From e300core reference manual:
> 
> Translation Exception Conditions:
> Exception condition: Instruction fetch from guarded memory
> with MSR[IR] = 1 ==> ISI interrupt SRR1[3] = 1

Right, but you said 6xx as well, i.e. pure PowerPC.

If for example IR=0 you cannot have N=1, but you do have G=1.  There is
no case where G=1 implies N=1 afaik, or where fetch is prohibited some
other way (causes an ISI, say).


Segher

Re: [PATCH] powerpc/4xx/ocm: fix compiler error

[Gabriel Paubert]

On Sat, Dec 22, 2018 at 05:04:51PM -0600, Segher Boessenkool wrote:
> On Sat, Dec 22, 2018 at 08:37:28PM +0100, christophe leroy wrote:
> > Le 22/12/2018 à 18:16, Segher Boessenkool a écrit :
> > >On Sat, Dec 22, 2018 at 02:08:02PM +0100, christophe leroy wrote:
> > >>
> > >>Usually, Guarded implies no exec (at least on 6xx and 8xx).
> > >
> > >Huh?  What do you mean here?
> > 
> > From the 885 Reference Manual:
> > 
> > Address translation: the EA is translated by using the MMU’s TLB 
> > mechanism. Instructions are not fetched from no-execute or guarded 
> > memory and data accesses are not executed speculatively to or from the 
> > guarded memory.
> > 
> > 6.1.3.4 Instruction TLB Error Exception (0x01300)
> > This type of exception occurs as a result of one of the following 
> > conditions if MSR[IR] = 1:
> > - The EA cannot be translated.
> > - The fetch access violates memory protection
> > - The fetch access is to guarded memory
> > 
> > 
> > From e300core reference manual:
> > 
> > Translation Exception Conditions:
> > Exception condition: Instruction fetch from guarded memory
> > with MSR[IR] = 1 ==> ISI interrupt SRR1[3] = 1
> 
> Right, but you said 6xx as well, i.e. pure PowerPC.
> 
> If for example IR=0 you cannot have N=1, but you do have G=1.  There is
> no case where G=1 implies N=1 afaik, or where fetch is prohibited some
> other way (causes an ISI, say).

From the 750fx user's manual, similar wording can be found for other
processors (except 601), including the ones that use software TLB load
like 603/603e (the test for guarded memory is in the sample code for
the tlb miss handler):

"
An ISI exception occurs when no higher priority exception exists and an
attempt to fetch the next instruction fails. This exception is implemented 
as it is defined by the PowerPC architecture (OEA), and is taken for the
following conditions:
•   The effective address cannot be translated.
•   The fetch access is to a no-execute segment (SR[N] = 1).
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
•   The fetch access is to guarded storage and MSR[IR] = 1.
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
•   The fetch access is to a segment for which SR[T] is set.
•   The fetch access violates memory protection.
When an ISI exception is taken, instruction fetching resumes at offset
0x00400 from the physical base indicated by MSR[IP].
"

	Gabriel

Re: [PATCH] powerpc/4xx/ocm: fix compiler error

[Segher Boessenkool]

On Sun, Dec 23, 2018 at 09:29:44AM +0100, Gabriel Paubert wrote:
> •   The fetch access is to guarded storage and MSR[IR] = 1.

That is architected, yes.  You get an ISI if IR=1 and (N=1 or G=1).


Segher

Re: [PATCH] powerpc/4xx/ocm: fix compiler error

[christophe leroy]

Le 23/12/2018 à 10:31, Segher Boessenkool a écrit :
> On Sun, Dec 23, 2018 at 09:29:44AM +0100, Gabriel Paubert wrote:
>> •   The fetch access is to guarded storage and MSR[IR] = 1.
> 
> That is architected, yes.  You get an ISI if IR=1 and (N=1 or G=1).
> 

If IR=0, mappings don't apply. So in 4xx/ocm, this mapping with 
_PAGE_EXEC and _PAGE_GUARDED at the same time seems pointless.

A standard ioremap() should do it (ie without exec), or a non guarded 
exec mapping if execution is expected for this area.

Christophe

---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus