Re: [PATCH] target: ppc: Correctly initialize HILE in HID-0 for book3s processors

[Harsh Prateek Bora <harshpb@linux.ibm.com>]

Hi Narayana,

Minor comments inline below.

On 4/20/23 20:20, Narayana Murty N wrote:
> On PPC64 the HILE(Hypervisor Interrupt Little Endian) bit in HID-0
> register needs to be initialized as per isa 3.0b[1] section
> 2.10. This bit gets copied to the MSR_LE when handling interrupts that
> are handled in HV mode to establish the Endianess mode of the interrupt
> handler.
> 
> Qemu's ppc_interrupts_little_endian() depends on HILE to determine Host
> endianness which is then used to determine the endianess of the guest dump.
> 
> Currently the HILE bit is never set in the HID0 register even if the
> qemu is running in Little-Endian mode. This causes the guest dumps to be
> always taken in Big-Endian byte ordering. A guest memory dump of a
> Little-Endian guest running on Little-Endian qemu guest fails with the
> crash tool as illustrated below:
> 
> Log :
> $ virsh dump DOMAIN --memory-only dump.file
> 
> Domain 'DOMAIN' dumped to dump.file
> 
> $ crash vmlinux dump.file
> 
> <snip>
> crash 8.0.2-1.el9
> 
> WARNING: endian mismatch:
>           crash utility: little-endian
>           dump.file: big-endian
> 
> WARNING: machine type mismatch:
>           crash utility: PPC64
>           dump.file: (unknown)
> 
> crash: dump.file: not a supported file format
> <snip>
> 
> The patch fixes the issue by Setting HILE on little-endian host. HILE bit values
> are documented at [1] for POWER7, POWER8 and [2] for POWER9 onwards.
> 
> For power9 and power10:
> 	The new helper function "set_spr_default_value" added to change the default value
> 	as per host endianness in init_proc_POWER9, init_proc_POWER10
> 
> For power7 and power8 :
> 	correcting "spr_register_hv" function parameter for initial value to
> 	HID0_ISA206_INIT_VAL in register_book3s_ids_sprs()
> 
> References:
> 1. ISA 2.06, section 2.9 for POWER7,POWER8
> 2. ISA 3.0b, section 2.10 for POWER9 onwards - https://openpowerfoundation.org/specifications/isa/
> 

ISA ver and section information can be included in code comments below 
where respective macros are being introduced.

> Signed-off-by: Narayana Murty N <nnmlinux@linux.ibm.com>
> ---
>   target/ppc/cpu.h         |  9 +++++++++
>   target/ppc/cpu_init.c    | 18 +++++++++++++++++-
>   target/ppc/helper_regs.c | 18 ++++++++++++++++++
>   target/ppc/spr_common.h  |  3 +++
>   4 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 557d736dab..8c15e9cde7 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2113,6 +2113,15 @@ void ppc_compat_add_property(Object *obj, const char *name,
>   #define HID0_HILE           PPC_BIT(19) /* POWER8 */
>   #define HID0_POWER9_HILE    PPC_BIT(4)
>   
> +/* HID0 register initial value for POWER9 */
> +#if HOST_BIG_ENDIAN
> +#define HID0_ISA206_INIT_VAL    0x00000000        /* POWER7 Onwards */
> +#define HID0_ISA300_INIT_VAL    0x00000000        /* POWER9 Onwards */
> +#else
> +#define HID0_ISA206_INIT_VAL    HID0_HILE         /* POWER7 Onwards */
> +#define HID0_ISA300_INIT_VAL    HID0_POWER9_HILE  /* POWER9 Onwards */
> +#endif
> +
>   /*****************************************************************************/
>   /* PowerPC Instructions types definitions                                    */
>   enum {
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 0ce2e3c91d..5b481dc5c3 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -5372,7 +5372,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
>                    SPR_NOACCESS, SPR_NOACCESS,
>                    SPR_NOACCESS, SPR_NOACCESS,
>                    &spr_read_generic, &spr_write_generic,
> -                 0x00000000);
> +                 HID0_ISA206_INIT_VAL);
>       spr_register_hv(env, SPR_TSCR, "TSCR",
>                    SPR_NOACCESS, SPR_NOACCESS,
>                    SPR_NOACCESS, SPR_NOACCESS,
> @@ -5699,6 +5699,15 @@ static void register_power9_mmu_sprs(CPUPPCState *env)
>   #endif
>   }
>   
> +static void set_power9_default_value_sprs(CPUPPCState *env)
> +{

Naming it as set_power9_sprs_default_value (like the internal helper) 
may be more appropriate.

> +    /*
> +     * ISA 3.00, book3s ids HID0 register, HILE bit position
> +     * changed to bit HID0_POWER9_HILE
> +     */
> +    set_spr_default_value(env, SPR_HID0, HID0_ISA300_INIT_VAL);
> +}
> +
>   static void register_power10_hash_sprs(CPUPPCState *env)
>   {
>       /*
> @@ -6250,6 +6259,9 @@ static void init_proc_POWER9(CPUPPCState *env)
>       register_power8_rpr_sprs(env);
>       register_power9_mmu_sprs(env);
>   
> +    /* POWER9 Host Specific register initialization */
> +    set_power9_default_value_sprs(env);
> +
>       /* POWER9 Specific registers */
>       spr_register_kvm(env, SPR_TIDR, "TIDR", NULL, NULL,
>                        spr_read_generic, spr_write_generic,
> @@ -6424,6 +6436,10 @@ static void init_proc_POWER10(CPUPPCState *env)
>       register_power8_book4_sprs(env);
>       register_power8_rpr_sprs(env);
>       register_power9_mmu_sprs(env);
> +
> +    /* POWER10 Host Specific register initialization */
> +    set_power9_default_value_sprs(env);
> +
>       register_power10_hash_sprs(env);
>       register_power10_dexcr_sprs(env);
>   
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 779e7db513..f17e9e78c2 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -351,6 +351,24 @@ void _spr_register(CPUPPCState *env, int num, const char *name,
>   #endif
>   }
>   
> +/**
> + * set_spr_default_value
> + *
> + * sets the spr register with default value overide.
> + */
> +void set_spr_default_value(CPUPPCState *env, int num,
> +                   target_ulong default_value)

Indentation should match the starting of first arg type above.

> +{
> +    assert(num < ARRAY_SIZE(env->spr_cb));
> +    ppc_spr_t *spr = &env->spr_cb[num];
> +
> +    /* Verify the spr registered already. */
> +    assert(spr->name != NULL);
> +
> +    spr->default_value = default_value;
> +    env->spr[num] = default_value;
> +}
> +
>   /* Generic PowerPC SPRs */
>   void register_generic_sprs(PowerPCCPU *cpu)
>   {
> diff --git a/target/ppc/spr_common.h b/target/ppc/spr_common.h
> index 8437eb0340..b1d27f0138 100644
> --- a/target/ppc/spr_common.h
> +++ b/target/ppc/spr_common.h
> @@ -77,6 +77,9 @@ void _spr_register(CPUPPCState *env, int num, const char *name,
>       spr_register_kvm(env, num, name, uea_read, uea_write,                    \
>                        oea_read, oea_write, 0, ival)
>   
> +void set_spr_default_value(CPUPPCState *env, int num,
> +                   target_ulong default_value);

Ditto.

Otherwise, looks good to me.

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> +
>   /* prototypes for readers and writers for SPRs */
>   void spr_noaccess(DisasContext *ctx, int gprn, int sprn);
>   void spr_read_generic(DisasContext *ctx, int gprn, int sprn);