Re: [Qemu-devel] [Qemu-ppc] Determining interest in PPC e500spin, yield, and openpic patches

[alarson@ddci.com]

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

David Gibson <david@gibson.dropbear.id.au> wrote on 06/14/2016 11:17:57 
PM:
Aaron Larson <alarson@ddci.com>

AL> 1. There is a defect in ppce500_spin.c:spin_kick() that creates an
AL>    incorrectly sized TLB entry.  This was reported as bug
AL>    https://bugs.launchpad.net/qemu/+bug/1587535  I can provide a
AL>    patch if desired.

DG> Absolutely.

OK, I'll start with this one.   Let me know if there is anything you'd 
like me to do
with that bug report.

When e500 PPC is booted multi-core, the non-boot cores are started via
the spin table.  ppce500_spin.c:spin_kick() calls
mmubooke_create_initial_mapping() to allocate a 64MB TLB entry, but
the created TLB entry is only 256KB.

The root cause is that the function computing the size of the TLB
entry, namely booke206_page_size_to_tlb assumes MAS1.TSIZE as defined
by latter PPC cores, specifically n to the power of FOUR * 1KB. The
result is then used by mmubooke_create_initial_mapping using
MAS1_TSIZE_SHIFT, but MAS1_TSIZE_SHIFT is defined assuming TLB entries
are n to the power of TWO * 1KB. I.e., a difference of shift=7 or
shift=8.

Simply changing MAS1_TSIZE_SHIFT from 7 to 8 is not appropriate since
the macro is used elsewhere.

The following patch has a fix for that, and also raises a separate
issue that I'd be happy to resolve after getting some guidance.



[-- Attachment #2: ppce500_spin-tlb.patch --]
[-- Type: application/octet-stream, Size: 1795 bytes --]

--- qemu-2.5.0.orig/hw/ppc/ppce500_spin.c	2015-12-16 16:04:49.000000000 -0600
+++ qemu-2.5.0/hw/ppc/ppce500_spin.c	2016-06-15 14:54:36.921768400 -0500
@@ -1,7 +1,7 @@
 /*
  * QEMU PowerPC e500v2 ePAPR spinning code
  *
- * Copyright (C) 2011 Freescale Semiconductor, Inc. All rights reserved.
+ * Copyright (C) 2011, 2016 Freescale Semiconductor, Inc. All rights reserved.
  *
  * Author: Alexander Graf, <agraf@suse.de>
  *
@@ -74,7 +74,11 @@
 /* Create -kernel TLB entries for BookE, linearly spanning 256MB.  */
 static inline hwaddr booke206_page_size_to_tlb(uint64_t size)
 {
-    return ctz32(size >> 10) >> 1;
+    /* The EREF indicates that TLB pages are (4 to the power of 2)KB, which
+     * corresponds to MAS1_TSIZE_SHIFT=8, but to support legacy processors that
+     * assume TLB pages are (2 to the power of 2)KB MAS1_TSIZE_SHIFT is
+     * currently 7. */
+    return ctz32(size >> 10) >> (MAS1_TSIZE_SHIFT-7);
 }
 
 static void mmubooke_create_initial_mapping(CPUPPCState *env,
@@ -104,6 +108,16 @@
 
     cpu_synchronize_state(cpu);
     stl_p(&curspin->pir, env->spr[SPR_PIR]);
+/* The stl_p() above seems wrong to me.  First of all, it seems more appropriate
+ * in a guest ROM/BOOT code than in qemu emulation.  However, SPR_PIR is never
+ * initialized, so the effect of the stl_p() is to overwrite the curspin->pir
+ * with 0. It makes more sense to load the SPR_PIR with the curspin->pir, which
+ * is what the following does.
+ *    env->spr[SPR_PIR]=ldl_p(&curspin->pir);
+ * Alternately SPR_PIR could be initialized from SPR_BOOKE_PIR which is properly
+ * initialized, so this could also work:
+ *    env->spr[SPR_PIR] = env->spr[SPR_BOOKE_PIR]
+*/
     env->nip = ldq_p(&curspin->addr) & (map_size - 1);
     env->gpr[3] = ldq_p(&curspin->r3);
     env->gpr[4] = 0;