qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: PPC440 tlbwe


From: BALATON Zoltan
Subject: Re: PPC440 tlbwe
Date: Wed, 11 Oct 2023 21:29:00 +0200 (CEST)

On Wed, 11 Oct 2023, Nicholas Piggin wrote:
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.

Thank you very much. I've tried that sam460ex still appears to work with this patch and can boot AmigaOS. Maybe Rene in cc can do some more testing to find out if this broke anything or how much it improves performance.

I don't know BookE MMU at all so I hoped somebody here with more knowledge could make more sense of this code than me.

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've seen random crashes with a DSI exception in AmigaOS before so it could be there are bugs with TLBs but these aren't consistently reproducible so could not find out what caused it.

Regards,
BALATON Zoltan

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) {




reply via email to

[Prev in Thread] Current Thread [Next in Thread]