Re: PPC440 tlbwe

[Nicholas Piggin]

On Tue Oct 10, 2023 at 10:41 PM AEST, BALATON Zoltan wrote:
> Hello,
>
> I have a question about tlbwe emulation in TCG for PPC440. This seems to 
> be implemented in mmu_helper.c::helper_440_tlbwe which calls tlb_flush a 
> lot flushing all TLB entries. This causes a big performance hit with 
> sam460ex running AmigaOS which seems to do tlbwe on every task or context 
> switch so it runs much slower than on other machines with a G4 CPU 
> (pegasos2 and amigaone we also emulate) that use hardware TLB which does 
> not have such problem in TCG. Even the embedded TLB emulation has 3 
> versions for 40x, 440 and BookE 2.06 (I think that's e500) and at least 
> the booke206 version uses flush_page instead of flushing everything. I 
> don't know much about this so is there somebody with better understanding 
> of this who can have a look at it and see if it can be optimised to 
> resolve this problem or give some advice?

It looks like 440 might be able to use the same page flushing as 4xx.

This patch is only compile tested and I don't know the book E TLB stuff
too well but it may give you a starting point.

I wonder if there are a couple of under-invalidation bugs in the 440
tlbwe:

- If we write a new size which is smaller than the entry's current
  size, then that should invalidate the addresses which are no longer
  covered by the entry. But we only invalidate when a larger size is
  written. Maybe the test is inverted.

- Changing protection and attributes (the 2 case) can downgrade prot
  (e.g., rw->ro) or possibly even change the translation with different
  attributes. This seems like it should invalidate.

I covered those with the patch.

I also don't understand why 4xx invalidates the address covered by the
new entry. AFAIKS if that was not covered by an existing HW TLB entry
then QEMU TLB should not contain entries for it. And if there is
another valid HW TLB entry then it should be a multi-hit programming
error. Maybe it is to make multi-hit error handling more deterministic
and in control of the target machine rather than depending on QEMU TLB?

Thanks,
Nick

---
 target/ppc/mmu_helper.c | 57 ++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
index f87d35379a..dc2f85edf7 100644
--- a/target/ppc/mmu_helper.c
+++ b/target/ppc/mmu_helper.c
@@ -749,12 +749,20 @@ target_ulong helper_4xx_tlbre_lo(CPUPPCState *env, 
target_ulong entry)
     return ret;
 }
 
+static void ppcemb_tlb_flush(CPUState *cs, ppcemb_tlb_t *tlb)
+{
+    target_ulong ea;
+
+    for (ea = tlb->EPN; ea < tlb->EPN + tlb->size; ea += TARGET_PAGE_SIZE) {
+        tlb_flush_page(cs, ea);
+    }
+}
+
 void helper_4xx_tlbwe_hi(CPUPPCState *env, target_ulong entry,
                          target_ulong val)
 {
     CPUState *cs = env_cpu(env);
     ppcemb_tlb_t *tlb;
-    target_ulong page, end;
 
     qemu_log_mask(CPU_LOG_MMU, "%s entry %d val " TARGET_FMT_lx "\n",
                   __func__, (int)entry,
@@ -763,13 +771,10 @@ void helper_4xx_tlbwe_hi(CPUPPCState *env, target_ulong 
entry,
     tlb = &env->tlb.tlbe[entry];
     /* Invalidate previous TLB (if it's valid) */
     if (tlb->prot & PAGE_VALID) {
-        end = tlb->EPN + tlb->size;
         qemu_log_mask(CPU_LOG_MMU, "%s: invalidate old TLB %d start "
                       TARGET_FMT_lx " end " TARGET_FMT_lx "\n", __func__,
-                      (int)entry, tlb->EPN, end);
-        for (page = tlb->EPN; page < end; page += TARGET_PAGE_SIZE) {
-            tlb_flush_page(cs, page);
-        }
+                      (int)entry, tlb->EPN, tlb->EPN + tlb->size);
+        ppcemb_tlb_flush(cs, tlb);
     }
     tlb->size = booke_tlb_to_page_size((val >> PPC4XX_TLBHI_SIZE_SHIFT)
                                        & PPC4XX_TLBHI_SIZE_MASK);
@@ -805,13 +810,10 @@ void helper_4xx_tlbwe_hi(CPUPPCState *env, target_ulong 
entry,
                   tlb->prot & PAGE_VALID ? 'v' : '-', (int)tlb->PID);
     /* Invalidate new TLB (if valid) */
     if (tlb->prot & PAGE_VALID) {
-        end = tlb->EPN + tlb->size;
         qemu_log_mask(CPU_LOG_MMU, "%s: invalidate TLB %d start "
                       TARGET_FMT_lx " end " TARGET_FMT_lx "\n", __func__,
-                      (int)entry, tlb->EPN, end);
-        for (page = tlb->EPN; page < end; page += TARGET_PAGE_SIZE) {
-            tlb_flush_page(cs, page);
-        }
+                      (int)entry, tlb->EPN, tlb->EPN + tlb->size);
+        ppcemb_tlb_flush(cs, tlb);
     }
 }
 
@@ -854,52 +856,53 @@ target_ulong helper_4xx_tlbsx(CPUPPCState *env, 
target_ulong address)
 void helper_440_tlbwe(CPUPPCState *env, uint32_t word, target_ulong entry,
                       target_ulong value)
 {
+    CPUState *cs = env_cpu(env);
     ppcemb_tlb_t *tlb;
     target_ulong EPN, RPN, size;
-    int do_flush_tlbs;
 
     qemu_log_mask(CPU_LOG_MMU, "%s word %d entry %d value " TARGET_FMT_lx "\n",
                   __func__, word, (int)entry, value);
-    do_flush_tlbs = 0;
     entry &= 0x3F;
     tlb = &env->tlb.tlbe[entry];
     switch (word) {
     default:
         /* Just here to please gcc */
     case 0:
-        EPN = value & 0xFFFFFC00;
-        if ((tlb->prot & PAGE_VALID) && EPN != tlb->EPN) {
-            do_flush_tlbs = 1;
+        /* Invalidate previous TLB (if it's valid) */
+        if (tlb->prot & PAGE_VALID) {
+            ppcemb_tlb_flush(cs, tlb);
         }
-        tlb->EPN = EPN;
+
+        EPN = value & 0xFFFFFC00;
         size = booke_tlb_to_page_size((value >> 4) & 0xF);
-        if ((tlb->prot & PAGE_VALID) && tlb->size < size) {
-            do_flush_tlbs = 1;
-        }
+
+        tlb->EPN = EPN;
         tlb->size = size;
         tlb->attr &= ~0x1;
         tlb->attr |= (value >> 8) & 1;
         if (value & 0x200) {
             tlb->prot |= PAGE_VALID;
         } else {
-            if (tlb->prot & PAGE_VALID) {
-                tlb->prot &= ~PAGE_VALID;
-                do_flush_tlbs = 1;
-            }
+            tlb->prot &= ~PAGE_VALID;
         }
         tlb->PID = env->spr[SPR_440_MMUCR] & 0x000000FF;
-        if (do_flush_tlbs) {
-            tlb_flush(env_cpu(env));
+        /* Invalidate new TLB (if valid) XXX is this required? */
+        if (tlb->prot & PAGE_VALID) {
+            ppcemb_tlb_flush(cs, tlb);
         }
         break;
     case 1:
         RPN = value & 0xFFFFFC0F;
         if ((tlb->prot & PAGE_VALID) && tlb->RPN != RPN) {
-            tlb_flush(env_cpu(env));
+            ppcemb_tlb_flush(cs, tlb);
         }
         tlb->RPN = RPN;
         break;
     case 2:
+        /* Invalidate previous TLB (if it's valid) */
+        if (tlb->prot & PAGE_VALID) {
+            ppcemb_tlb_flush(cs, tlb);
+        }
         tlb->attr = (tlb->attr & 0x1) | (value & 0x0000FF00);
         tlb->prot = tlb->prot & PAGE_VALID;
         if (value & 0x1) {
-- 
2.42.0