Re: [Qemu-devel] [PATCH 3/3] ppc: Fix radix RC updates

[Benjamin Herrenschmidt]

On Fri, 2018-12-14 at 10:58 +1100, Benjamin Herrenschmidt wrote:
> They should be atomic for MTTCG. Note: a real POWER9 core doesn't actually
> implement atomic PTE updates, it always fault for SW to handle it. Only
> the nest MMU (used by some accelerator devices and GPUs) implements
> those HW updates.
> 
> However, the architecture does allow the core to do it, and doing so
> in TCG is faster than letting the guest do it.

Note: ppc hash MMU needs fixes too but of a different nature. I have
some queued up as well, but as-is, they are entangled with other ppc
target fixes, so I'll send them separately via David.

Cheers,
Ben.

> Signed-off-by: Benjamin Herrenschmidt <address@hidden>
> ---
>  target/ppc/cpu.h         |  1 +
>  target/ppc/mmu-radix64.c | 70 +++++++++++++++++++++++++++++++++-------
>  2 files changed, 59 insertions(+), 12 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index ab68abe8a2..afdef2af2f 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -493,6 +493,7 @@ struct ppc_slb_t {
>  #define DSISR_AMR                0x00200000
>  /* Unsupported Radix Tree Configuration */
>  #define DSISR_R_BADCONFIG        0x00080000
> +#define DSISR_ATOMIC_RC          0x00040000
>  
>  /* SRR1 error code fields */
>  
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index ab76cbc835..dba95aabdc 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -28,6 +28,15 @@
>  #include "mmu-radix64.h"
>  #include "mmu-book3s-v3.h"
>  
> +static inline bool ppc_radix64_hw_rc_updates(CPUPPCState *env)
> +{
> +#ifdef CONFIG_ATOMIC64
> +    return true;
> +#else
> +    return !qemu_tcg_mttcg_enabled();
> +#endif
> +}
> +
>  static bool ppc_radix64_get_fully_qualified_addr(CPUPPCState *env, vaddr 
> eaddr,
>                                                   uint64_t *lpid, uint64_t 
> *pid)
>  {
> @@ -120,11 +129,18 @@ static bool ppc_radix64_check_prot(PowerPCCPU *cpu, int 
> rwx, uint64_t pte,
>          return true;
>      }
>  
> +    /* Check RC bits if necessary */
> +    if (!ppc_radix64_hw_rc_updates(env)) {
> +        if (!(pte & R_PTE_R) || ((rwx == 1) && !(pte & R_PTE_C))) {
> +            *fault_cause |= DSISR_ATOMIC_RC;
> +            return true;
> +        }
> +    }
> +
>      return false;
>  }
>  
> -static void ppc_radix64_set_rc(PowerPCCPU *cpu, int rwx, uint64_t pte,
> -                               hwaddr pte_addr, int *prot)
> +static uint64_t ppc_radix64_set_rc(PowerPCCPU *cpu, int rwx, uint64_t pte, 
> hwaddr pte_addr)
>  {
>      CPUState *cs = CPU(cpu);
>      uint64_t npte;
> @@ -133,17 +149,38 @@ static void ppc_radix64_set_rc(PowerPCCPU *cpu, int 
> rwx, uint64_t pte,
>  
>      if (rwx == 1) { /* Store/Write */
>          npte |= R_PTE_C; /* Set change bit */
> -    } else {
> -        /*
> -         * Treat the page as read-only for now, so that a later write
> -         * will pass through this function again to set the C bit.
> -         */
> -        *prot &= ~PAGE_WRITE;
>      }
> +    if (pte == npte) {
> +        return pte;
> +    }
> +
> +#ifdef CONFIG_ATOMIC64
> +    if (qemu_tcg_mttcg_enabled()) {
> +        uint64_t old_be = cpu_to_be32(pte);
> +        uint64_t new_be = cpu_to_be32(npte);
> +        MemTxResult result;
> +        uint64_t old_ret;
> +
> +        old_ret = address_space_cmpxchgq_notdirty(cs->as, pte_addr,
> +                                                  old_be, new_be,
> +                                                  MEMTXATTRS_UNSPECIFIED,
> +                                                  &result);
> +        if (result == MEMTX_OK) {
> +            if (old_ret != old_be && old_ret != new_be) {
> +                return 0;
> +            }
> +            return npte;
> +        }
>  
> -    if (pte ^ npte) { /* If pte has changed then write it back */
> -        stq_phys(cs->as, pte_addr, npte);
> +        /* Do we need to support this case where PTEs aren't in RAM ?
> +         *
> +         * For now fallback to non-atomic case
> +         */
>      }
> +#endif
> +
> +    stq_phys(cs->as, pte_addr, npte);
> +    return npte;
>  }
>  
>  static uint64_t ppc_radix64_walk_tree(PowerPCCPU *cpu, vaddr eaddr,
> @@ -234,6 +271,7 @@ int ppc_radix64_handle_mmu_fault(PowerPCCPU *cpu, vaddr 
> eaddr, int rwx,
>  
>      /* Walk Radix Tree from Process Table Entry to Convert EA to RA */
>      page_size = PRTBE_R_GET_RTS(prtbe0);
> + restart:
>      pte = ppc_radix64_walk_tree(cpu, eaddr & R_EADDR_MASK,
>                                  prtbe0 & PRTBE_R_RPDB, prtbe0 & PRTBE_R_RPDS,
>                                  &raddr, &page_size, &fault_cause, &pte_addr);
> @@ -244,8 +282,16 @@ int ppc_radix64_handle_mmu_fault(PowerPCCPU *cpu, vaddr 
> eaddr, int rwx,
>      }
>  
>      /* Update Reference and Change Bits */
> -    ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, &prot);
> -
> +    if (ppc_radix64_hw_rc_updates(env)) {
> +        pte = ppc_radix64_set_rc(cpu, rwx, pte, pte_addr);
> +        if (!pte) {
> +            goto restart;
> +        }
> +    }
> +    /* If the page doesn't have C, treat it as read only */
> +    if (!(pte & R_PTE_C)) {
> +        prot &= ~PAGE_WRITE;
> +    }
>      tlb_set_page(cs, eaddr & TARGET_PAGE_MASK, raddr & TARGET_PAGE_MASK,
>                   prot, mmu_idx, 1UL << page_size);
>      return 0;