[Qemu-devel] [PATCH] ARM MMU translation - fix small (4k) page access

[Qemu-devel] [PATCH] ARM MMU translation - fix small (4k) page access

[Scott Oom]

[-- Attachment #1: Type: text/plain, Size: 188 bytes --]

Hello,
Found a problem when using small pages and getting permission faults.
This patch corrects the decoding of access permissions for small pages
on ARM, was just off by 2 bits.

-Scott

[-- Attachment #2: qemu-arm-mmu-smallpages-access.patch --]
[-- Type: text/x-patch, Size: 731 bytes --]

Index: target-arm/helper.c
===================================================================
RCS file: /sources/qemu/qemu/target-arm/helper.c,v
retrieving revision 1.8
diff -u -3 -p -r1.8 helper.c
--- target-arm/helper.c	20 Jan 2007 17:12:09 -0000	1.8
+++ target-arm/helper.c	2 Feb 2007 05:12:03 -0000
@@ -338,7 +338,7 @@ static int get_phys_addr(CPUState *env, 
                 break;
             case 2: /* 4k page.  */
                 phys_addr = (desc & 0xfffff000) | (address & 0xfff);
-                ap = (desc >> (4 + ((address >> 13) & 6))) & 3;
+                ap = (desc >> (4 + ((address >> 11) & 6))) & 3; /* SRO */
                 break;
             case 3: /* 1k page.  */
                 if (type == 1) {

Re: [Qemu-devel] [PATCH] ARM MMU translation - fix small (4k) page access

[Justin Fletcher]

On Fri, 2 Feb 2007, Scott Oom wrote:

> Hello,
> Found a problem when using small pages and getting permission faults.
> This patch corrects the decoding of access permissions for small pages
> on ARM, was just off by 2 bits.

I may be confused on this, but it still doesn't seem right to me.

You have...

-                ap = (desc >> (4 + ((address >> 13) & 6))) & 3;
+                ap = (desc >> (4 + ((address >> 11) & 6))) & 3; /* SRO */

For 4K pages, the L2 table is ...
   b0-1 = 2
   b2   = B
   b3   = C
   b4-5 = AP0
   b6-7 = AP1
   b8-9 = AP2
   b10-11=AP3
   b12-31=physical address
(from ARMARM 'D', 3.3.7)

The use of AP0-AP3 is dependant on bits 10 and 11. So, the code should be 
more like...

                 ap = (desc >> (4 + ((address >> 10) & 3) )) & 3;

That is, (address>>10) & 3 => bits 10 and 11
          add on 4 as the offset to the AP fields in the descriptor
          shift down and & 3 to leave just those two bits.

The AP bits haven't been used all that often in my own use of qemu, and I 
imagine that most uses set all 3 to the same value.

-- 
Gerph <http://gerph.org/>
... It's only a lifetime.

Re: [Qemu-devel] [PATCH] ARM MMU translation - fix small (4k) page access

[Justin Fletcher]

On Fri, 2 Feb 2007, Justin Fletcher wrote:

> On Fri, 2 Feb 2007, Scott Oom wrote:
>
>> Hello,
>> Found a problem when using small pages and getting permission faults.
>> This patch corrects the decoding of access permissions for small pages
>> on ARM, was just off by 2 bits.
>
> I may be confused on this, but it still doesn't seem right to me.
>
> You have...
>
> -                ap = (desc >> (4 + ((address >> 13) & 6))) & 3;
> +                ap = (desc >> (4 + ((address >> 11) & 6))) & 3; /* SRO */

'sokay... I twigged whilst I was away from the machine that that 11 and 
the 6 is to ensure that the address is shifted up by 1 bit so that the 
desc shift is in the right place. Just me being confused, ignore me.

-- 
Gerph <http://gerph.org/>
... Caught up in circles, confusion is nothing new.

Re: [Qemu-devel] [PATCH] ARM MMU translation - fix small (4k) page access

[Scott Oom]

Justin Fletcher wrote:
> 
> I may be confused on this, but it still doesn't seem right to me.
> 
> You have...
> 
> -                ap = (desc >> (4 + ((address >> 13) & 6))) & 3;
> +                ap = (desc >> (4 + ((address >> 11) & 6))) & 3; /* SRO */
> 
> For 4K pages, the L2 table is ...
>   b0-1 = 2
>   b2   = B
>   b3   = C
>   b4-5 = AP0
>   b6-7 = AP1
>   b8-9 = AP2
>   b10-11=AP3
>   b12-31=physical address
> (from ARMARM 'D', 3.3.7)
> 
> The use of AP0-AP3 is dependant on bits 10 and 11. So, the code should
> be more like...
> 
>                 ap = (desc >> (4 + ((address >> 10) & 3) )) & 3;
> 
> That is, (address>>10) & 3 => bits 10 and 11
>          add on 4 as the offset to the AP fields in the descriptor
>          shift down and & 3 to leave just those two bits.
> 
Well, we need to take b10-11 and use them to index either 4-5, 6-7, 8-9
or 10-11.
(address >> 10) & 3 gives us 0, 1, 2 or 3, shift that left one to double
it (because each AP field is two bits).  Adding 4 gives 4, 6, 8, 10.  So
I believe the correct solution is:
                ap = (desc >> (4 + ((address >> 9) & 6))) & 3;

I thought if was just 2 bits different from the large page descriptor,
but the difference, the SBZ field, is 4 bits.  Comparing to the large
page descriptor:
                ap = (desc >> (4 + ((address >> 13) & 6))) & 3;


-Scott