Re: [PATCH 1/6] target/ppc: Fix 440 tlbwe TLB invalidation gaps

[Cédric Le Goater]

On 1/17/24 16:12, Nicholas Piggin wrote:
> The 440 software TLB write entry misses several cases that must flush
> the TCG TLB:
> - If the new size is smaller than the existing size, the EA no longer
>    covered should be flushed. This looks like an inverted inequality test.
> - If the TLB PID changes.
> - If the TLB attr bit 0 (translation address space) changes.
> - If low prot (access control) bits change.
>
> Fix this by removing tricks to avoid TLB flushes, and just invalidate
> the TLB if any valid entry is being changed, similarly to 4xx.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>


Acked-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

PS: A cover letter would have been nice :) I couldn't find it.


> ---
>   target/ppc/mmu_helper.c | 35 ++++++++++-------------------------
>   1 file changed, 10 insertions(+), 25 deletions(-)
>
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index f87d35379a..c140f3c96d 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -855,49 +855,34 @@ void helper_440_tlbwe(CPUPPCState *env, uint32_t word, 
> target_ulong entry,
>                         target_ulong value)
>   {
>       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];
> +
> +    /* Invalidate previous TLB (if it's valid) */
> +    if (tlb->prot & PAGE_VALID) {
> +        tlb_flush(env_cpu(env));
> +    }
> +
>       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;
> -        }
> -        tlb->EPN = EPN;
> -        size = booke_tlb_to_page_size((value >> 4) & 0xF);
> -        if ((tlb->prot & PAGE_VALID) && tlb->size < size) {
> -            do_flush_tlbs = 1;
> -        }
> -        tlb->size = size;
> +        tlb->EPN = value & 0xFFFFFC00;
> +        tlb->size = booke_tlb_to_page_size((value >> 4) & 0xF);
>           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));
> -        }
>           break;
>       case 1:
> -        RPN = value & 0xFFFFFC0F;
> -        if ((tlb->prot & PAGE_VALID) && tlb->RPN != RPN) {
> -            tlb_flush(env_cpu(env));
> -        }
> -        tlb->RPN = RPN;
> +        tlb->RPN = value & 0xFFFFFC0F;
>           break;
>       case 2:
>           tlb->attr = (tlb->attr & 0x1) | (value & 0x0000FF00);