On 02/19/2018 01:21 AM, David Gibson wrote:
> On Fri, Feb 16, 2018 at 09:45:02AM +0100, Cédric Le Goater wrote:
>> The Partition Table Control Register (PTCR) is a hypervisor privileged
>> SPR. It contains the host real address of the Partition Table and its
>> size.
>>
>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>> ---
>>
>> Changes since v1:
>>
>> - renamed partition table definitions to match ISA
>> - moved definitions under mmu-book3s-v3.h
>>
>> target/ppc/cpu.h | 2 ++
>> target/ppc/helper.h | 1 +
>> target/ppc/misc_helper.c | 12 ++++++++++++
>> target/ppc/mmu-book3s-v3.h | 6 ++++++
>> target/ppc/mmu_helper.c | 28 ++++++++++++++++++++++++++++
>> target/ppc/translate.c | 3 +++
>> target/ppc/translate_init.c | 18 ++++++++++++++++++
>> 7 files changed, 70 insertions(+)
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 9f8cbbe7aa4d..53061229a0a8 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -1314,6 +1314,7 @@ int ppc_cpu_handle_mmu_fault(CPUState *cpu, vaddr
>> address, int size, int rw,
>>
>> #if !defined(CONFIG_USER_ONLY)
>> void ppc_store_sdr1 (CPUPPCState *env, target_ulong value);
>> +void ppc_store_ptcr(CPUPPCState *env, target_ulong value);
>> #endif /* !defined(CONFIG_USER_ONLY) */
>> void ppc_store_msr (CPUPPCState *env, target_ulong value);
>>
>> @@ -1605,6 +1606,7 @@ void ppc_compat_add_property(Object *obj, const char
>> *name,
>> #define SPR_BOOKE_GIVOR13 (0x1BC)
>> #define SPR_BOOKE_GIVOR14 (0x1BD)
>> #define SPR_TIR (0x1BE)
>> +#define SPR_PTCR (0x1D0)
>> #define SPR_BOOKE_SPEFSCR (0x200)
>> #define SPR_Exxx_BBEAR (0x201)
>> #define SPR_Exxx_BBTAR (0x202)
>> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
>> index 5b739179b8b5..19453c68138a 100644
>> --- a/target/ppc/helper.h
>> +++ b/target/ppc/helper.h
>> @@ -709,6 +709,7 @@ DEF_HELPER_FLAGS_1(load_601_rtcu, TCG_CALL_NO_RWG, tl,
>> env)
>> #if !defined(CONFIG_USER_ONLY)
>> #if defined(TARGET_PPC64)
>> DEF_HELPER_FLAGS_1(load_purr, TCG_CALL_NO_RWG, tl, env)
>> +DEF_HELPER_2(store_ptcr, void, env, tl)
>> #endif
>> DEF_HELPER_2(store_sdr1, void, env, tl)
>> DEF_HELPER_2(store_pidr, void, env, tl)
>> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
>> index 0e4217821b8e..8c8cba5cc6f1 100644
>> --- a/target/ppc/misc_helper.c
>> +++ b/target/ppc/misc_helper.c
>> @@ -88,6 +88,18 @@ void helper_store_sdr1(CPUPPCState *env, target_ulong val)
>> }
>> }
>>
>> +#if defined(TARGET_PPC64)
>> +void helper_store_ptcr(CPUPPCState *env, target_ulong val)
>> +{
>> + PowerPCCPU *cpu = ppc_env_get_cpu(env);
>> +
>> + if (env->spr[SPR_PTCR] != val) {
>> + ppc_store_ptcr(env, val);
>> + tlb_flush(CPU(cpu));
>> + }
>> +}
>> +#endif /* defined(TARGET_PPC64) */
>> +
>> void helper_store_pidr(CPUPPCState *env, target_ulong val)
>> {
>> PowerPCCPU *cpu = ppc_env_get_cpu(env);
>> diff --git a/target/ppc/mmu-book3s-v3.h b/target/ppc/mmu-book3s-v3.h
>> index 56095dab522c..fdf80987d7b2 100644
>> --- a/target/ppc/mmu-book3s-v3.h
>> +++ b/target/ppc/mmu-book3s-v3.h
>> @@ -22,6 +22,12 @@
>>
>> #ifndef CONFIG_USER_ONLY
>>
>> +/*
>> + * Partition table definitions
>> + */
>> +#define PTCR_PATB 0x0FFFFFFFFFFFF000ULL /* Partition Table
>> Base */
>> +#define PTCR_PATS 0x000000000000001FULL /* Partition Table
>> Size */
>> +
>> /* Partition Table Entry Fields */
>> #define PATBE1_GR 0x8000000000000000
>>
>> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
>> index 5568d1642b34..82e63552f617 100644
>> --- a/target/ppc/mmu_helper.c
>> +++ b/target/ppc/mmu_helper.c
>> @@ -2028,6 +2028,34 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong
>> value)
>> env->spr[SPR_SDR1] = value;
>> }
>>
>> +#if defined(TARGET_PPC64)
>> +void ppc_store_ptcr(CPUPPCState *env, target_ulong value)
>> +{
>> + PowerPCCPU *cpu = ppc_env_get_cpu(env);
>> + qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value);
>> +
>> + assert(!cpu->vhyp);
>> +
>> + if (env->mmu_model & POWERPC_MMU_V3) {
>
> If it's not MMUv3, the PTCR shouldn't exist, right? So couldn't this
> just be an assert?
yes. I will change that.
>
>> + target_ulong ptcr_mask = PTCR_PATB | PTCR_PATS;
>> + target_ulong ptas = value & PTCR_PATS;
>
> Any reason it's "ptas" on the left and "PATS" on the right?
nah. the sed expression I used missed the lower case.
>> +
>> + if (value & ~ptcr_mask) {
>> + error_report("Invalid bits 0x"TARGET_FMT_lx" set in PTCR",
>> + value & ~ptcr_mask);
>> + value &= ptcr_mask;
>> + }
>> + if (ptas > 28) {
This should be 24.
>> + error_report("Invalid PTAS 0x" TARGET_FMT_lx" stored in PTCR",
>> + ptas);
>> + return;
>> + }
>
> Is masking / ignoring incorrect values correct, or should it generate
> a 0x700?
I didn't see anything in the ISA regarding incorrect values for the PTCR.
There are some build checks in the kernel though. When I have some HW,
I will try some bogus values to see what is reported.
>> + }
>> + env->spr[SPR_PTCR] = value;
>> +}
>> +
>> +#endif /* defined(TARGET_PPC64) */
>> +
>> /* Segment registers load and store */
>> target_ulong helper_load_sr(CPUPPCState *env, target_ulong sr_num)
>> {
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index 0a0c090c9978..58684d249ed9 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -7131,6 +7131,9 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f,
>> fprintf_function cpu_fprintf,
>> if (env->spr_cb[SPR_SDR1].name) { /* SDR1 Exists */
>> cpu_fprintf(f, " SDR1 " TARGET_FMT_lx " ", env->spr[SPR_SDR1]);
>> }
>> + if (env->spr_cb[SPR_PTCR].name) { /* PTCR Exists */
>> + cpu_fprintf(f, " PTCR " TARGET_FMT_lx " ", env->spr[SPR_PTCR]);
>> + }
>> cpu_fprintf(f, " DAR " TARGET_FMT_lx " DSISR " TARGET_FMT_lx "\n",
>> env->spr[SPR_DAR], env->spr[SPR_DSISR]);
>> break;
>> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
>> index cbaa343e040d..c998ac2ee405 100644
>> --- a/target/ppc/translate_init.c
>> +++ b/target/ppc/translate_init.c
>> @@ -419,6 +419,11 @@ static void spr_write_hior(DisasContext *ctx, int sprn,
>> int gprn)
>> tcg_gen_st_tl(t0, cpu_env, offsetof(CPUPPCState, excp_prefix));
>> tcg_temp_free(t0);
>> }
>> +static void spr_write_ptcr(DisasContext *ctx, int sprn, int gprn)
>> +{
>> + gen_helper_store_ptcr(cpu_env, cpu_gpr[gprn]);
>> +}
>> +
>> #endif
>> #endif
>>
>> @@ -8166,6 +8171,18 @@ static void gen_spr_power8_rpr(CPUPPCState *env)
>> #endif
>> }
>>
>> +/* Page Table */
>> +static void gen_spr_power9_ptcr(CPUPPCState *env)
>
> Is this the only POWER9 MMU related register? Otherwise renaming the
> function and putting them all here (eventually) would make sense.
I don't think there are any other POWER9 MMU SPRs but nevertheless,
a '_mmu' prefix has a better signification.
Thanks,
C.
>> +{
>> +#if !defined(CONFIG_USER_ONLY)
>> + spr_register_hv(env, SPR_PTCR, "PTCR",
>> + SPR_NOACCESS, SPR_NOACCESS,
>> + SPR_NOACCESS, SPR_NOACCESS,
>> + &spr_read_generic, &spr_write_ptcr,
>> + 0x00000000);
>> +#endif
>> +}
>> +
>> static void init_proc_book3s_common(CPUPPCState *env)
>> {
>> gen_spr_ne_601(env);
>> @@ -8758,6 +8775,7 @@ static void init_proc_POWER9(CPUPPCState *env)
>> gen_spr_power8_ic(env);
>> gen_spr_power8_book4(env);
>> gen_spr_power8_rpr(env);
>> + gen_spr_power9_ptcr(env);
>>
>> /* POWER9 Specific registers */
>> spr_register_kvm(env, SPR_TIDR, "TIDR", NULL, NULL,
>
