Re: [Qemu-devel] [OpenRISC] [PATCH] Correction of the TLB handling of the OpenRISC target

[Sebastian Macke <sebastian@macke.de>]

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