Re: [Qemu-ppc] [RFC PATCH 10/17] target/ppc/POWER9: Add POWER9 mmu fault handler

[David Gibson]

On Fri, Jan 13, 2017 at 05:28:16PM +1100, Suraj Jitindar Singh wrote:
> Add a new mmu fault handler for the POWER9 cpu and add it as the handler
> for the POWER9 cpu definition.
> 
> This handler checks if the guest is radix or hash based on the value in the
> partition table entry and calls the correct fault handler accordingly.
> 
> The hash fault handling code has also been updated to check if the
> partition is using segment tables.
> 
> Currently only legacy hash (no segment tables) is supported.
> 
> Signed-off-by: Suraj Jitindar Singh <address@hidden>
> ---
>  target/ppc/mmu-hash64.c     |  8 ++++++++
>  target/ppc/mmu.h            |  8 ++++++++
>  target/ppc/mmu_helper.c     | 47 
> +++++++++++++++++++++++++++++++++++++++++++++
>  target/ppc/translate_init.c |  2 +-
>  4 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index b9d4f4e..b476b3f 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -73,6 +73,14 @@ static ppc_slb_t *slb_lookup(PowerPCCPU *cpu, target_ulong 
> eaddr)
>          }
>      }
>  
> +    /* Check if in-memory segment tables are in use */
> +    if (ppc64_use_proc_tbl(cpu)) {
> +        /* TODO - Unsupported */
> +        qemu_log_mask(LOG_UNIMP, "%s: unimplemented - segment table 
> support\n",
> +                      __func__);
> +        /* Not much we can do here, caller will generate a segment interrupt 
> */
> +    }

I think this logic would be better in the fault handler.  For the
fault path in the segment table case, I don't think we want to even
model the SLB (in hardware the SLB is an important optimization, but I
don't think the software SLB will be notably better than just looking
up the seg table directly).  I think the other users of slb_lookup()
are in contexts that only make sense in the context of a software
managed SLB anyway.

>      return NULL;
>  }
>  
> diff --git a/target/ppc/mmu.h b/target/ppc/mmu.h
> index c7967c3..e07b128 100644
> --- a/target/ppc/mmu.h
> +++ b/target/ppc/mmu.h
> @@ -3,6 +3,10 @@
>  
>  #ifndef CONFIG_USER_ONLY
>  
> +/* Common Partition Table Entry Fields */
> +#define PATBE0_HR                0x8000000000000000
> +#define PATBE1_GR                0x8000000000000000
> +
>  /* Partition Table Entry */
>  struct patb_entry {
>      uint64_t patbe0, patbe1;
> @@ -11,6 +15,10 @@ struct patb_entry {
>  #ifdef TARGET_PPC64
>  
>  void ppc64_set_external_patb(PowerPCCPU *cpu, void *patb, Error **errp);
> +bool ppc64_use_proc_tbl(PowerPCCPU *cpu);
> +bool ppc64_radix_guest(PowerPCCPU *cpu);
> +int ppc64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr, int rwx,
> +                           int mmu_idx);
>  
>  #endif /* TARGET_PPC64 */
>  
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index bc6c117..612f407 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -28,6 +28,7 @@
>  #include "exec/cpu_ldst.h"
>  #include "exec/log.h"
>  #include "helper_regs.h"
> +#include "qemu/error-report.h"
>  #include "mmu.h"
>  
>  //#define DEBUG_MMU
> @@ -1281,6 +1282,17 @@ void dump_mmu(FILE *f, fprintf_function cpu_fprintf, 
> CPUPPCState *env)
>      case POWERPC_MMU_2_07a:
>          dump_slb(f, cpu_fprintf, ppc_env_get_cpu(env));
>          break;
> +    case POWERPC_MMU_3_00:
> +        if (ppc64_radix_guest(ppc_env_get_cpu(env))) {
> +            /* TODO - Unsupported */
> +        } else {
> +            if (ppc64_use_proc_tbl(ppc_env_get_cpu(env))) {
> +                /* TODO - Unsupported */
> +            } else {
> +                dump_slb(f, cpu_fprintf, ppc_env_get_cpu(env));
> +                break;
> +            }
> +        }
>  #endif
>      default:
>          qemu_log_mask(LOG_UNIMP, "%s: unimplemented\n", __func__);
> @@ -1422,6 +1434,17 @@ hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr 
> addr)
>      case POWERPC_MMU_2_07:
>      case POWERPC_MMU_2_07a:
>          return ppc_hash64_get_phys_page_debug(cpu, addr);
> +    case POWERPC_MMU_3_00:
> +        if (ppc64_radix_guest(ppc_env_get_cpu(env))) {
> +            /* TODO - Unsupported */
> +        } else {
> +            if (ppc64_use_proc_tbl(ppc_env_get_cpu(env))) {
> +                /* TODO - Unsupported */
> +            } else {
> +                return ppc_hash64_get_phys_page_debug(cpu, addr);
> +            }
> +        }
> +        break;
>  #endif
>  
>      case POWERPC_MMU_32B:
> @@ -2919,3 +2942,27 @@ void ppc64_set_external_patb(PowerPCCPU *cpu, void 
> *patb, Error **errp)
>  
>      env->external_patbe = (struct patb_entry *) patb;
>  }
> +
> +inline bool ppc64_use_proc_tbl(PowerPCCPU *cpu)

There's basically no point putting an inline on functions that are in
a .c rather than a .h (it will only be inlined for callers in this .o,
not elsewhere).  These are simple enough that I think they do belong
in the .h instead.

> +{
> +    return !!(cpu->env.spr[SPR_LPCR] & LPCR_UPRT);
> +}
> +
> +inline bool ppc64_radix_guest(PowerPCCPU *cpu)
> +{
> +    struct patb_entry *patbe = cpu->env.external_patbe;
> +
> +    return patbe && (patbe->patbe1 & PATBE1_GR);
> +}
> +
> +int ppc64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr, int rwx,
> +                           int mmu_idx)

I think this name is too generic, since it's really only right for
POWER9 / MMU v3.

> +{
> +    if (ppc64_radix_guest(cpu)) { /* Guest uses radix */
> +        /* TODO - Unsupported */
> +        error_report("Guest Radix Support Unimplemented\n");
> +        abort();
> +    } else { /* Guest uses hash */
> +        return ppc_hash64_handle_mmu_fault(cpu, eaddr, rwx, mmu_idx);
> +    }
> +}
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index c771ba3..87297a7 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8850,7 +8850,7 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
>                      (1ull << MSR_LE);
>      pcc->mmu_model = POWERPC_MMU_3_00;
>  #if defined(CONFIG_SOFTMMU)
> -    pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault;
> +    pcc->handle_mmu_fault = ppc64_handle_mmu_fault;
>      /* segment page size remain the same */
>      pcc->sps = &POWER7_POWER8_sps;
>  #endif

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature