Re: [Qemu-devel] [PATCH 3/9] target-ppc: Add POWER7 SPRs

[Tom Musta <tommusta@gmail.com>]

On 5/21/2014 1:20 AM, Alexey Kardashevskiy wrote:
> This adds TIR/SIAR/SDAR/MMCRA/MMCR0/MMCR1.
> 
> This redefines UMMCRA (was MCCRA) and defines hypv version of if.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  target-ppc/cpu.h            | 10 +++++++++-
>  target-ppc/translate_init.c | 41 +++++++++++++++++++++++++++++++++++++----
>  2 files changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 13eea19..262cf0f 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h

[...]

> @@ -1505,9 +1511,11 @@ static inline int cpu_mmu_index (CPUPPCState *env)
>  #define SPR_UPERF9            (0x319)
>  #define SPR_UPERFA            (0x31A)
>  #define SPR_UPERFB            (0x31B)
> +#define SPR_POWER_MMCR0       (0x31B)
>  #define SPR_UPERFC            (0x31C)
>  #define SPR_UPERFD            (0x31D)
>  #define SPR_UPERFE            (0x31E)
> +#define SPR_POWER_MMCR1       (0x31E)
>  #define SPR_UPERFF            (0x31F)


Since MMCR0 and MMCR1 SPRs are assigned by the ISA, why "SPR_POWER_MMCR0" instead of just
"SPR_MMCR0" (ditto for MMCR1)?  Since you moved the outdated and unoffical 7xx encodings
out of the way in patch 1, the extra "_POWER" seems extraneous.


>  #define SPR_RCPU_MI_RA0       (0x320)
>  #define SPR_MPC_MI_DBCAM      (0x320)
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index e9c37fa..b92b447 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c

[ ... ]

>  static void gen_spr_book3s_purr(CPUPPCState *env)
> @@ -7711,16 +7716,20 @@ static void gen_spr_book3s_debug(CPUPPCState *env)
>                   SPR_NOACCESS, SPR_NOACCESS,
>                   &spr_read_cfar, &spr_write_cfar,
>                   0x00000000);
> +    spr_register_kvm(env, SPR_BOOK3S_SIAR, "SIAR",
> +                     &spr_read_generic, &spr_write_generic,
> +                     &spr_read_generic, &spr_write_generic,
> +                     KVM_REG_PPC_SIAR, 0x00000000);
> +    spr_register_kvm(env, SPR_BOOK3S_SDAR, "SDAR",
> +                     &spr_read_generic, &spr_write_generic,
> +                     &spr_read_generic, &spr_write_generic,
> +                     KVM_REG_PPC_SDAR, 0x00000000);
>  #endif
>  }

SIAR and SDAR are read-only SPRs.

>  
>  static void gen_spr_book3s_pmu(CPUPPCState *env)
>  {
>  #if !defined(CONFIG_USER_ONLY)
> -    spr_register_kvm(env, SPR_MMCRA, "SPR_MMCRA",
> -                     SPR_NOACCESS, SPR_NOACCESS,
> -                     &spr_read_generic, &spr_write_generic,
> -                     KVM_REG_PPC_MMCRA, 0x00000000);
>      spr_register_kvm(env, SPR_PMC5, "SPR_PMC5",
>                       SPR_NOACCESS, SPR_NOACCESS,
>                       &spr_read_generic, &spr_write_generic,
> @@ -7729,6 +7738,30 @@ static void gen_spr_book3s_pmu(CPUPPCState *env)
>                       SPR_NOACCESS, SPR_NOACCESS,
>                       &spr_read_generic, &spr_write_generic,
>                       KVM_REG_PPC_PMC6, 0x00000000);
> +    spr_register(env, SPR_MMCRA, "MMCRA",
> +                 &spr_read_generic, &spr_write_generic,
> +                 &spr_read_generic, &spr_write_generic,
> +                 0x00000000);
> +    spr_register_kvm(env, SPR_UMMCRA, "UMMCRA",
> +                     SPR_NOACCESS, SPR_NOACCESS,
> +                     &spr_read_generic, &spr_write_generic,
> +                     KVM_REG_PPC_MMCRA, 0x00000000);
> +    spr_register(env, SPR_POWER_MMCR0, "MMCR0",
> +                 &spr_read_generic, &spr_write_generic,
> +                 &spr_read_generic, &spr_write_generic,
> +                 0x00000000);
> +    spr_register_kvm(env, SPR_POWER_UMMCR0, "UMMCR0",
> +                     SPR_NOACCESS, SPR_NOACCESS,
> +                     &spr_read_generic, &spr_write_generic,
> +                     KVM_REG_PPC_MMCR0, 0x00000000);
> +    spr_register(env, SPR_POWER_MMCR1, "MMCR1",
> +                 &spr_read_generic, &spr_write_generic,
> +                 &spr_read_generic, &spr_write_generic,
> +                 0x00000000);
> +    spr_register_kvm(env, SPR_POWER_UMMCR1, "UMMCR1",
> +                     SPR_NOACCESS, SPR_NOACCESS,
> +                     &spr_read_generic, &spr_write_generic,
> +                     KVM_REG_PPC_MMCR1, 0x00000000);
>  #endif
>  }
>  
> 

This looks wrong to me ....  the Uxxxx SPR numbers are accessible in user space (they are Book I
SPRs) whereas the non-Uxxxx forms are not accessible from user space.