Re: [Qemu-devel] AArch64 QEMU System emulation: issue with TTBR0

[Ian Campbell]

On Sun, 2014-06-08 at 13:19 +0100, Peter Maydell wrote:
> On 8 June 2014 12:26, Ian Campbell <address@hidden> wrote:
> > On Tue, 2014-06-03 at 14:28 +0200, Claudio Fontana wrote:
> >> Thank you for the clarifications and advice, I think executable
> >> permissions might be involved, as removing the NX / PNX check in
> >> get_phys_addr_lpae() makes it proceed ahead
> >
> > I'm seeing something very similar running modprobe, I get a kernel fault
> > (see below) which I also tracked down to the NX/PNX checks in
> > get_phys_addr_lpae().
> >
> > At the moment I'm a bit suspicious of:
> >         /* Extract attributes from the descriptor and merge with table 
> > attrs */
> >         if (arm_feature(env, ARM_FEATURE_V8)) {
> >             attrs = extract64(descriptor, 2, 10)
> >                 | (extract64(descriptor, 53, 11) << 10);
> >         } else {
> >             attrs = extract64(descriptor, 2, 10)
> >                 | (extract64(descriptor, 52, 12) << 10);
> >         }
> >
> > I'm not sure what the reason for the v8 difference is, it seems like it
> > is skipping extracting the CONTIG bit but I've not dug into the v8 ARM
> > ARM to figure out why that might be desirable...
> 
> The CONTIG bit is purely a hint to the implementation, so it's
> valid to completely ignore it. (Given how QEMU's TLB works it
> doesn't really make sense for us.) However it's present in v7's
> long descriptor format as well, so I'm not sure why this change
> is guarded by ARM_FEATURE_V8.

Exactly what I was wondering.

>  Rob?
> 
> > Since in the v8 case extracts fewer bits from higher up but uses the
> > same << 10 shift, which seems like it ought to then confuse later checks
> > with use 1<<11 and 1<<12. Making that <<10 into <<11 doesn't help though
> > so I think I might be barking up the wrong tree...
> 
> I think this should be unconditionally
>             attrs = extract64(descriptor, 2, 10)
>                  | (extract64(descriptor, 52, 12) << 10);
> (as we currently have for v7 LPAE)
> as we're just assembling the upper and lower attribute bits
> into a contiguous set. We can then happily ignore anything
> like CONTIG that we don't care about.

I agree. With the following debug the existing code generates:
        get_phys_addr_lpae: looking up ffffffbffc000658
        get_phys_addr_lpae: l1 descr 4007f003 (!XNTable,!PXNTable,!XN,!PXN)
        get_phys_addr_lpae: l2 descr 46e13003 (!XNTable,!PXNTable,!XN,!PXN)
        get_phys_addr_lpae: l3 descr 2c0000046e11713 
(!XNTable,!PXNTable,XN,!PXN)
        attrs: lower 1c4 upper 16 (V8 style, 2..11, 53..63)
        attrs 59c4 | 0 => 59c4 (merging from tableattrs 0)
        get_phys_addr_lpae: fault with attrs 59c4, is_user 0, access_type 2, 
level 3
        get_phys_addr_lpae: tableattrs 0
        get_phys_addr_lpae: XN == 1000 yes, PXN == 800 yes

Notice the disconnect between PXN in the l3 descr and what we see when
we inject the fault (the final line). Making the change you suggest (by
uncommenting the "0 &&" in the debug) instead produces:
        get_phys_addr_lpae: looking up ffffffbffc000658
        get_phys_addr_lpae: l1 descr 4007f003 (!XNTable,!PXNTable,!XN,!PXN)
        get_phys_addr_lpae: l2 descr 46fee003 (!XNTable,!PXNTable,!XN,!PXN)
        get_phys_addr_lpae: l3 descr 2c0000046fec713 
(!XNTable,!PXNTable,XN,!PXN)
        attrs: lower 1c4 upper 2c (V7 style, 2..11, 52..63)
        attrs b1c4 | 0 => b1c4 (merging from tableattrs 0)
        get_phys_addr_lpae: fault with attrs b1c4, is_user 0, access_type 2, 
level 3
        get_phys_addr_lpae: tableattrs 0
        get_phys_addr_lpae: XN == 1000 yes, PXN == 800 no
        
Which I think is correct.

Ian.


diff --git a/target-arm/helper.c b/target-arm/helper.c
index ec031f5..2748245 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3793,6 +3793,8 @@ static int get_phys_addr_lpae(CPUARMState *env, 
target_ulong address,
     int32_t va_size = 32;
     int32_t tbi = 0;
 
+    qemu_log("--------------------------------------\n");
+    qemu_log("%s: looking up %lx\n", __func__, address);
     if (arm_el_is_aa64(env, 1)) {
         va_size = 64;
         if (extract64(address, 55, 1))
@@ -3905,6 +3907,11 @@ static int get_phys_addr_lpae(CPUARMState *env, 
target_ulong address,
         descaddr |= (address >> (granule_sz * (4 - level))) & descmask;
         descaddr &= ~7ULL;
         descriptor = ldq_phys(cs->as, descaddr);
+        qemu_log("%s: l%d descr %lx (%s,%s,%s,%s)\n", __func__, level, 
descriptor,
+                 descriptor & (1UL<<60) ? "XNTable" : "!XNTable",
+                 descriptor & (1UL<<59) ? "PXNTable" : "!PXNTable",
+                 descriptor & (1UL<<54) ? "XN" : "!XN",
+                 descriptor & (1UL<<53) ? "PXN" : "!PXN");
         if (!(descriptor & 1) ||
             (!(descriptor & 2) && (level == 3))) {
             /* Invalid, or the Reserved level 3 encoding */
@@ -3929,13 +3936,23 @@ static int get_phys_addr_lpae(CPUARMState *env, 
target_ulong address,
         page_size = (1 << ((granule_sz * (4 - level)) + 3));
         descaddr |= (address & (page_size - 1));
         /* Extract attributes from the descriptor and merge with table attrs */
-        if (arm_feature(env, ARM_FEATURE_V8)) {
+        if (/*0 &&*/ arm_feature(env, ARM_FEATURE_V8)) {
             attrs = extract64(descriptor, 2, 10)
                 | (extract64(descriptor, 53, 11) << 10);
+            qemu_log("attrs: lower %lx upper %lx (V8 style, 2..11, 53..63)\n",
+                     extract64(descriptor, 2, 10),
+                     extract64(descriptor, 53, 11));
         } else {
             attrs = extract64(descriptor, 2, 10)
                 | (extract64(descriptor, 52, 12) << 10);
+            qemu_log("attrs: lower %lx upper %lx (V7 style, 2..11, 52..63)\n",
+                     extract64(descriptor, 2, 10),
+                     extract64(descriptor, 52, 12));
         }
+        qemu_log("attrs %x | %x => %x (merging from tableattrs %x)\n",
+                 attrs, extract32(tableattrs, 0, 2) << 11,
+                 attrs | extract32(tableattrs, 0, 2) << 11,
+                 tableattrs);
         attrs |= extract32(tableattrs, 0, 2) << 11; /* XN, PXN */
         attrs |= extract32(tableattrs, 3, 1) << 5; /* APTable[1] => AP[2] */
         /* The sense of AP[1] vs APTable[0] is reversed, as APTable[0] == 1
@@ -3961,13 +3978,22 @@ static int get_phys_addr_lpae(CPUARMState *env, 
target_ulong address,
         goto do_fault;
     }
     *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
+#if 1
     if (attrs & (1 << 12) || (!is_user && (attrs & (1 << 11)))) {
         /* XN or PXN */
         if (access_type == 2) {
+            qemu_log("%s: fault with attrs %x, is_user %d, access_type %d, 
level %d\n", __func__,
+                attrs, is_user, access_type, level);
+            qemu_log("%s: tableattrs %x\n", __func__, tableattrs);
+            qemu_log("%s: XN == %x %s, PXN == %x %s\n", __func__,
+                     1<<12, (attrs & (1<<12)) ? "yes" : "no",
+                     1<<11, (attrs & (1<<11)) ? "yes" : "no");
+            abort();
             goto do_fault;
         }
         *prot &= ~PAGE_EXEC;
     }
+#endif
     if (attrs & (1 << 5)) {
         /* Write access forbidden */
         if (access_type == 1) {