Correction of the TLB handling of the OpenRISC target

Hi,

this patch corrects two problems for the OpenRISC Target in QEMU. The 
first one corrects one obvious bug
concerning the handling of page faults while reading from a page. The 
second part removes a non-conforming behavior for the first page of the 
memory.

I have tested this patch with the newest Linux kernel and compared the 
output with or1ksim.

Sebastian
From 4491bae7109e2b4de5a8de8a7e4b08d1f19ac70e Mon Sep 17 00:00:00 2001
From: Sebastian Macke <sebastian@macke.de>
Date: Tue, 1 Oct 2013 21:39:38 -0700
Subject: [PATCH] Correction of the TLB handling of the OpenRISC target

This patch correct two problems. The first one corrects one obvious bug
concerning the handling of page faults while reading from a page.
The second part removes a non-conforming behavior for the first page of
the memory.

I have tested this patch with the newest Linux kernel and compared the
output with or1ksim.

Signed-off-by: Sebastian Macke <sebastian@macke.de>
---
 target-openrisc/mmu.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

On Wed, Oct 2, 2013 at 8:33 AM, Jia Liu <proljc@gmail.com> wrote:

> Hi Sebastian,
>
> On Wed, Oct 2, 2013 at 1:12 PM, Sebastian Macke <sebastian@macke.de>
> wrote:
> > Hi,
> >
> > this patch corrects two problems for the OpenRISC Target in QEMU. The
> first
> > one corrects one obvious bug
> > concerning the handling of page faults while reading from a page.
>
> @@ -102,7 +102,7 @@ int cpu_openrisc_get_phys_data(OpenRISCCPU *cpu,
>          }
>      }
>
> -    if ((rw & 0) && ((right & PAGE_READ) == 0)) {
> +    if (!(rw & 1) && ((right & PAGE_READ) == 0)) {
>          return TLBRET_BADADDR;
>      }
>      if ((rw & 1) && ((right & PAGE_WRITE) == 0)) {
>
> They are just two type of one code...
>

No, (rw & 0) always evaluates to 0.


>
> > The second
> > part removes a non-conforming behavior for the first page of the memory.
>
> @@ -122,13 +122,6 @@ static int cpu_openrisc_get_phys_addr(OpenRISCCPU
> *cpu,
>  {
>      int ret = TLBRET_MATCH;
>
> -    /* [0x0000--0x2000]: unmapped */
> -    if (address < 0x2000 && (cpu->env.sr & SR_SM)) {
> -        *physical = address;
> -        *prot = PAGE_READ | PAGE_WRITE;
> -        return ret;
> -    }
> -
>
> May you please explain more about why the first page is non-conforming?
> The Arch manual told me 0x0000--0x2000 is unmapped.
>

It shows an example where *software* leaves 0x0000 - 0x2000 unmapped,
the hardware should still allow for this area to be mapped.

Stefan

Hi Jia,

On 10/1/2013 10:33 PM, Jia Liu wrote:
> Hi Sebastian,
>
> On Wed, Oct 2, 2013 at 1:12 PM, Sebastian Macke <sebastian@macke.de> wrote:
>> Hi,
>>
>> this patch corrects two problems for the OpenRISC Target in QEMU. The first
>> one corrects one obvious bug
>> concerning the handling of page faults while reading from a page.
> @@ -102,7 +102,7 @@ int cpu_openrisc_get_phys_data(OpenRISCCPU *cpu,
>           }
>       }
>
> -    if ((rw & 0) && ((right & PAGE_READ) == 0)) {
> +    if (!(rw & 1) && ((right & PAGE_READ) == 0)) {
>           return TLBRET_BADADDR;
>       }
>       if ((rw & 1) && ((right & PAGE_WRITE) == 0)) {
>
> They are just two type of one code...
No the result of (rw&0) is always zero and therefore a logic false. The 
whole comparison will therefore never be executed.
>> The second
>> part removes a non-conforming behavior for the first page of the memory.
> @@ -122,13 +122,6 @@ static int cpu_openrisc_get_phys_addr(OpenRISCCPU *cpu,
>   {
>       int ret = TLBRET_MATCH;
>
> -    /* [0x0000--0x2000]: unmapped */
> -    if (address < 0x2000 && (cpu->env.sr & SR_SM)) {
> -        *physical = address;
> -        *prot = PAGE_READ | PAGE_WRITE;
> -        return ret;
> -    }
> -
>
> May you please explain more about why the first page is non-conforming?
> The Arch manual told me 0x0000--0x2000 is unmapped.

There is a good reason why this page should be a mapped one.
Think of an accidental

int *a = NULL;
....
*a = 0;

In the current implementation this would work in the supervisor mode and 
could never be catched.
The Linux kernel handles this page as a mapped one without write access 
which is correct.

The specification is not clear about this mapping region because it is 
not consistent with the rest of the specification and of the 
implementation in or1ksim and the Linux kernel.

>> I have tested this patch with the newest Linux kernel and compared the
>> output with or1ksim.
> May you please upload a newest Linux kernel to somewhere?
Try http://www.simulationcorner.net/vmlinux
start it with
or32-system-qemu  -m 64 -kernel vmlinux
In / there are two example executables which accidently dereferences a 
null pointer in the Linux kernel. The correct behavior should be a 
kernel oops.

Sebastian
>> Sebastian
>>
>> _______________________________________________
>> OpenRISC mailing list
>> OpenRISC@lists.openrisc.net
>> http://lists.openrisc.net/listinfo/openrisc
>>
> Regards,
> Jia

On Wed, Oct 2, 2013 at 2:15 PM, Stefan Kristiansson
<stefan.kristiansson@saunalahti.fi> wrote:
> On Wed, Oct 2, 2013 at 8:33 AM, Jia Liu <proljc@gmail.com> wrote:
>>
>> Hi Sebastian,
>>
>> On Wed, Oct 2, 2013 at 1:12 PM, Sebastian Macke <sebastian@macke.de>
>> wrote:
>> > Hi,
>> >
>> > this patch corrects two problems for the OpenRISC Target in QEMU. The
>> > first
>> > one corrects one obvious bug
>> > concerning the handling of page faults while reading from a page.
>>
>> @@ -102,7 +102,7 @@ int cpu_openrisc_get_phys_data(OpenRISCCPU *cpu,
>>          }
>>      }
>>
>> -    if ((rw & 0) && ((right & PAGE_READ) == 0)) {
>> +    if (!(rw & 1) && ((right & PAGE_READ) == 0)) {
>>          return TLBRET_BADADDR;
>>      }
>>      if ((rw & 1) && ((right & PAGE_WRITE) == 0)) {
>>
>> They are just two type of one code...
>
>
> No, (rw & 0) always evaluates to 0.
>
>>
>>
>> > The second
>> > part removes a non-conforming behavior for the first page of the memory.
>>
>> @@ -122,13 +122,6 @@ static int cpu_openrisc_get_phys_addr(OpenRISCCPU
>> *cpu,
>>  {
>>      int ret = TLBRET_MATCH;
>>
>> -    /* [0x0000--0x2000]: unmapped */
>> -    if (address < 0x2000 && (cpu->env.sr & SR_SM)) {
>> -        *physical = address;
>> -        *prot = PAGE_READ | PAGE_WRITE;
>> -        return ret;
>> -    }
>> -
>>
>> May you please explain more about why the first page is non-conforming?
>> The Arch manual told me 0x0000--0x2000 is unmapped.
>
>
> It shows an example where *software* leaves 0x0000 - 0x2000 unmapped,
> the hardware should still allow for this area to be mapped.

OK, thank you. I will send a PULL Request soon.

Reviewed-by: Jia Liu <proljc@gmail.com>

>
> Stefan

Regards,
Jia