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

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

[Narayana Murty N]

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/

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)
+{
+    /*
+     * 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)
+{
+    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);
+
 /* 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);
-- 
2.39.2

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

[Harsh Prateek Bora]

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);

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

[Fabiano Rosas]

Narayana Murty N <nnmlinux@linux.ibm.com> writes:

> 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.
>

Not quite. We use the interrupt endianness as a proxy to guest
endianness to avoid reading MSR_LE at an inopportune moment when the
guest is switching endianness. This is not dependent on host
endianness. The HILE check is used when taking a memory dump of a
HV-capable machine such as the emulated powernv.

I think the actual issue might be that we're calling
ppc_interrupts_little_endian with hv=true for the 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:
>

Could you describe in more detail what is your setup? Specifically
whether both guests are running TCG or KVM (info kvm) and the state of
the nested-hv capability in QEMU command line.

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

[Vaibhav Jain]

Hi Fabiano,

Thanks for looking into this patch and apologies for the delayed reponse.
Fabiano Rosas <farosas@suse.de> writes:

> Narayana Murty N <nnmlinux@linux.ibm.com> writes:
>
>> 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.
>>
>
> Not quite. We use the interrupt endianness as a proxy to guest
> endianness to avoid reading MSR_LE at an inopportune moment when the
> guest is switching endianness.
Agreed

> This is not dependent on host
> endianness. The HILE check is used when taking a memory dump of a
> HV-capable machine such as the emulated powernv.

I think one concern which the patch tries to address is the guest memorydump file
generated of a BigEndian(BE) guest on a LittleEndian(LE) host is not readable on
the same LE host since 'crash' doesnt support cross endianess
dumps. Also even for a LE guest on LE host the memory dumps are marked as BE
making it not possible to analyze any guest memory dumps on the host.

However setting the HILE based on host endianess of qemu might not be
the right way to fix this problem. Based on an off mailing list discussion
with Narayana, he is working on another patch which doesnt set HILE
based on host endianess. However the problem seems to be stemming from
fact that qemu on KVM is using the HILE to set up the endianess of
memory-dump elf and since its not setup correctly the memory dumps are
in wrong endianess.

> I think the actual issue might be that we're calling
> ppc_interrupts_little_endian with hv=true for the dump.
>
Yes, that is currently the case with cpu_get_dump_info(). Excerpt from
that function below that sets the endianess of the dump:

    if (ppc_interrupts_little_endian(cpu, cpu->env.has_hv_mode)) {
        info->d_endian = ELFDATA2LSB;
    } else {
        info->d_endian = ELFDATA2MSB;
    }

for pseries kvm guest cpu->env.has_hv_mode is already set hence
ppc_interrupts_little_endian() assumes its running in 'hv' mode. The new
patch from Narayana will be addressing this.

>> 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:
>>
>
> Could you describe in more detail what is your setup? Specifically
> whether both guests are running TCG or KVM (info kvm) and the state of
> the nested-hv capability in QEMU command line.
Currently the issue is seen with any pseries KVM guest running on a PowerNV host.

-- 
Cheers
~ Vaibhav

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

[Fabiano Rosas]

Vaibhav Jain <vaibhav@linux.ibm.com> writes:

> Hi Fabiano,
>
> Thanks for looking into this patch and apologies for the delayed reponse.
> Fabiano Rosas <farosas@suse.de> writes:
>
>> Narayana Murty N <nnmlinux@linux.ibm.com> writes:
>>
>>> 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.
>>>
>>
>> Not quite. We use the interrupt endianness as a proxy to guest
>> endianness to avoid reading MSR_LE at an inopportune moment when the
>> guest is switching endianness.
> Agreed
>
>> This is not dependent on host
>> endianness. The HILE check is used when taking a memory dump of a
>> HV-capable machine such as the emulated powernv.
>
> I think one concern which the patch tries to address is the guest memorydump file
> generated of a BigEndian(BE) guest on a LittleEndian(LE) host is not readable on
> the same LE host since 'crash' doesnt support cross endianess
> dumps. Also even for a LE guest on LE host the memory dumps are marked as BE
> making it not possible to analyze any guest memory dumps on the host.
>

From QEMU's perspective there's no "host" in this equation. We'll
generate a BE dump for a BE guest and a LE dump for a LE guest. Anything
different is a bug in QEMU (as the one this patch addresses).

> However setting the HILE based on host endianess of qemu might not be
> the right way to fix this problem. Based on an off mailing list discussion
> with Narayana, he is working on another patch which doesnt set HILE
> based on host endianess. However the problem seems to be stemming from
> fact that qemu on KVM is using the HILE to set up the endianess of
> memory-dump elf and since its not setup correctly the memory dumps are
> in wrong endianess.
>
>> I think the actual issue might be that we're calling
>> ppc_interrupts_little_endian with hv=true for the dump.
>>
> Yes, that is currently the case with cpu_get_dump_info(). Excerpt from
> that function below that sets the endianess of the dump:
>
>     if (ppc_interrupts_little_endian(cpu, cpu->env.has_hv_mode)) {

This should probably be looking at cpu->vhyp or MSR_HVB since
has_hv_mode will not change after we init the cpu.

>         info->d_endian = ELFDATA2LSB;
>     } else {
>         info->d_endian = ELFDATA2MSB;
>     }
>
> for pseries kvm guest cpu->env.has_hv_mode is already set hence
> ppc_interrupts_little_endian() assumes its running in 'hv' mode. The new
> patch from Narayana will be addressing this.
>
>>> 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:
>>>
>>
>> Could you describe in more detail what is your setup? Specifically
>> whether both guests are running TCG or KVM (info kvm) and the state of
>> the nested-hv capability in QEMU command line.
> Currently the issue is seen with any pseries KVM guest running on a PowerNV host.

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

[Narayana Murty N]

[-- Attachment #1: Type: text/plain, Size: 3640 bytes --]

On 4/28/23 20:00, Fabiano Rosas wrote:
> Vaibhav Jain<vaibhav@linux.ibm.com>  writes:
>
>> Hi Fabiano,
>>
>> Thanks for looking into this patch and apologies for the delayed reponse.
>> Fabiano Rosas<farosas@suse.de>  writes:
>>
>>> Narayana Murty N<nnmlinux@linux.ibm.com>  writes:
>>>
>>>> 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.
>>>>
>>> Not quite. We use the interrupt endianness as a proxy to guest
>>> endianness to avoid reading MSR_LE at an inopportune moment when the
>>> guest is switching endianness.
>> Agreed

Agreed

>>
>>> This is not dependent on host
>>> endianness. The HILE check is used when taking a memory dump of a
>>> HV-capable machine such as the emulated powernv.
>> I think one concern which the patch tries to address is the guest memorydump file
>> generated of a BigEndian(BE) guest on a LittleEndian(LE) host is not readable on
>> the same LE host since 'crash' doesnt support cross endianess
>> dumps. Also even for a LE guest on LE host the memory dumps are marked as BE
>> making it not possible to analyze any guest memory dumps on the host.
>>
>  From QEMU's perspective there's no "host" in this equation. We'll
> generate a BE dump for a BE guest and a LE dump for a LE guest. Anything
> different is a bug in QEMU (as the one this patch addresses).
>
>> However setting the HILE based on host endianess of qemu might not be
>> the right way to fix this problem. Based on an off mailing list discussion
>> with Narayana, he is working on another patch which doesnt set HILE
>> based on host endianess. However the problem seems to be stemming from
>> fact that qemu on KVM is using the HILE to set up the endianess of
>> memory-dump elf and since its not setup correctly the memory dumps are
>> in wrong endianess.
>>
>>> I think the actual issue might be that we're calling
>>> ppc_interrupts_little_endian with hv=true for the dump.
>>>
>> Yes, that is currently the case with cpu_get_dump_info(). Excerpt from
>> that function below that sets the endianess of the dump:
>>
>>      if (ppc_interrupts_little_endian(cpu, cpu->env.has_hv_mode)) {
> This should probably be looking at cpu->vhyp or MSR_HVB since
> has_hv_mode will not change after we init the cpu.

yes, I agree. New version patch is under testing once done,
will post the patch.

>>          info->d_endian = ELFDATA2LSB;
>>      } else {
>>          info->d_endian = ELFDATA2MSB;
>>      }
>>
>> for pseries kvm guest cpu->env.has_hv_mode is already set hence
>> ppc_interrupts_little_endian() assumes its running in 'hv' mode. The new
>> patch from Narayana will be addressing this.
>>
>>>> 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:
>>>>
>>> Could you describe in more detail what is your setup? Specifically
>>> whether both guests are running TCG or KVM (info kvm) and the state of
>>> the nested-hv capability in QEMU command line.
>> Currently the issue is seen with any pseries KVM guest running on a PowerNV host.

[-- Attachment #2: Type: text/html, Size: 5746 bytes --]

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

[Nicholas Piggin]

On Sat Apr 29, 2023 at 12:30 AM AEST, Fabiano Rosas wrote:
> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
>
> > Hi Fabiano,
> >
> > Thanks for looking into this patch and apologies for the delayed reponse.
> > Fabiano Rosas <farosas@suse.de> writes:
> >
> >> Narayana Murty N <nnmlinux@linux.ibm.com> writes:
> >>
> >>> 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.
> >>>
> >>
> >> Not quite. We use the interrupt endianness as a proxy to guest
> >> endianness to avoid reading MSR_LE at an inopportune moment when the
> >> guest is switching endianness.
> > Agreed
> >
> >> This is not dependent on host
> >> endianness. The HILE check is used when taking a memory dump of a
> >> HV-capable machine such as the emulated powernv.
> >
> > I think one concern which the patch tries to address is the guest memorydump file
> > generated of a BigEndian(BE) guest on a LittleEndian(LE) host is not readable on
> > the same LE host since 'crash' doesnt support cross endianess
> > dumps. Also even for a LE guest on LE host the memory dumps are marked as BE
> > making it not possible to analyze any guest memory dumps on the host.
> >
>
> From QEMU's perspective there's no "host" in this equation. We'll
> generate a BE dump for a BE guest and a LE dump for a LE guest. Anything
> different is a bug in QEMU (as the one this patch addresses).

I'm trying to figure out what's going on here. On one hand we are
creating a dump for/in the host. The dump is just a format that
describes register metadata, the same values can be represented just
fine with either endian. Memory has no endianness (without data
structures). So from that perspective, we do want to dump host endian
format.

OTOH crash could be taught foreign-endianness in which case the
endianness of the ELF file could be useful metadata about the
target I suppose. But ILE != MSR[LE] at any given time.

ILE seems like a half way house. It doesn't always give host endian
dumps so crash won't always work. It doesn't always give the machine
operating mode either. So why is it better to take guest ILE mode than
HV ILE mode?

I guess the first thing we need is a better and precise description of
the problem and the desired resolution. PPC64 has powernv and pseries,
both of which can support guests in various ways (PR, HV, nested HV),
and then when running guests the target itself also functions as a host,
so need to make all that unambiguous and use correct terminoogy in
the changelog.

> > However setting the HILE based on host endianess of qemu might not be
> > the right way to fix this problem. Based on an off mailing list discussion
> > with Narayana, he is working on another patch which doesnt set HILE
> > based on host endianess. However the problem seems to be stemming from
> > fact that qemu on KVM is using the HILE to set up the endianess of
> > memory-dump elf and since its not setup correctly the memory dumps are
> > in wrong endianess.
> >
> >> I think the actual issue might be that we're calling
> >> ppc_interrupts_little_endian with hv=true for the dump.
> >>
> > Yes, that is currently the case with cpu_get_dump_info(). Excerpt from
> > that function below that sets the endianess of the dump:
> >
> >     if (ppc_interrupts_little_endian(cpu, cpu->env.has_hv_mode)) {
>
> This should probably be looking at cpu->vhyp or MSR_HVB since
> has_hv_mode will not change after we init the cpu.
>
> >         info->d_endian = ELFDATA2LSB;
> >     } else {
> >         info->d_endian = ELFDATA2MSB;
> >     }
> >
> > for pseries kvm guest cpu->env.has_hv_mode is already set hence
> > ppc_interrupts_little_endian() assumes its running in 'hv' mode. The new
> > patch from Narayana will be addressing this.
> >
> >>> 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:
> >>>
> >>
> >> Could you describe in more detail what is your setup? Specifically
> >> whether both guests are running TCG or KVM (info kvm) and the state of
> >> the nested-hv capability in QEMU command line.
> > Currently the issue is seen with any pseries KVM guest running on a PowerNV host.

Okay originally I thought you were talking about a powernv target
that is running a pseries guest and dumping that. But after re-reading, I
think you're talking about dumping a pseries target?

Questions still remain about why that's the best way to go. If the
target was running a nested-HV guest, is it reasonable that the guest
can change the endinaness of the target dump on a whim by changing its
ILE?

Thanks,
Nick

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

[Narayana Murty N]

[-- Attachment #1: Type: text/plain, Size: 5679 bytes --]


On 5/15/23 12:02, Nicholas Piggin wrote:
> On Sat Apr 29, 2023 at 12:30 AM AEST, Fabiano Rosas wrote:
>> Vaibhav Jain<vaibhav@linux.ibm.com>  writes:
>>
>>> Hi Fabiano,
>>>
>>> Thanks for looking into this patch and apologies for the delayed reponse.
>>> Fabiano Rosas<farosas@suse.de>  writes:
>>>
>>>> Narayana Murty N<nnmlinux@linux.ibm.com>  writes:
>>>>
>>>>> 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.
>>>>>
>>>> Not quite. We use the interrupt endianness as a proxy to guest
>>>> endianness to avoid reading MSR_LE at an inopportune moment when the
>>>> guest is switching endianness.
>>> Agreed
>>>
>>>> This is not dependent on host
>>>> endianness. The HILE check is used when taking a memory dump of a
>>>> HV-capable machine such as the emulated powernv.
>>> I think one concern which the patch tries to address is the guest memorydump file
>>> generated of a BigEndian(BE) guest on a LittleEndian(LE) host is not readable on
>>> the same LE host since 'crash' doesnt support cross endianess
>>> dumps. Also even for a LE guest on LE host the memory dumps are marked as BE
>>> making it not possible to analyze any guest memory dumps on the host.
>>>
>>  From QEMU's perspective there's no "host" in this equation. We'll
>> generate a BE dump for a BE guest and a LE dump for a LE guest. Anything
>> different is a bug in QEMU (as the one this patch addresses).
> I'm trying to figure out what's going on here. On one hand we are
> creating a dump for/in the host. The dump is just a format that
> describes register metadata, the same values can be represented just
> fine with either endian. Memory has no endianness (without data
> structures). So from that perspective, we do want to dump host endian
> format.
>
> OTOH crash could be taught foreign-endianness in which case the
> endianness of the ELF file could be useful metadata about the
> target I suppose. But ILE != MSR[LE] at any given time.
>
> ILE seems like a half way house. It doesn't always give host endian
> dumps so crash won't always work. It doesn't always give the machine
> operating mode either. So why is it better to take guest ILE mode than
> HV ILE mode?
>
> I guess the first thing we need is a better and precise description of
> the problem and the desired resolution. PPC64 has powernv and pseries,
> both of which can support guests in various ways (PR, HV, nested HV),
> and then when running guests the target itself also functions as a host,
> so need to make all that unambiguous and use correct terminoogy in
> the changelog.
>
>>> However setting the HILE based on host endianess of qemu might not be
>>> the right way to fix this problem. Based on an off mailing list discussion
>>> with Narayana, he is working on another patch which doesnt set HILE
>>> based on host endianess. However the problem seems to be stemming from
>>> fact that qemu on KVM is using the HILE to set up the endianess of
>>> memory-dump elf and since its not setup correctly the memory dumps are
>>> in wrong endianess.
>>>
>>>> I think the actual issue might be that we're calling
>>>> ppc_interrupts_little_endian with hv=true for the dump.
>>>>
>>> Yes, that is currently the case with cpu_get_dump_info(). Excerpt from
>>> that function below that sets the endianess of the dump:
>>>
>>>      if (ppc_interrupts_little_endian(cpu, cpu->env.has_hv_mode)) {
>> This should probably be looking at cpu->vhyp or MSR_HVB since
>> has_hv_mode will not change after we init the cpu.
>>
>>>          info->d_endian = ELFDATA2LSB;
>>>      } else {
>>>          info->d_endian = ELFDATA2MSB;
>>>      }
>>>
>>> for pseries kvm guest cpu->env.has_hv_mode is already set hence
>>> ppc_interrupts_little_endian() assumes its running in 'hv' mode. The new
>>> patch from Narayana will be addressing this.
>>>
>>>>> 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:
>>>>>
>>>> Could you describe in more detail what is your setup? Specifically
>>>> whether both guests are running TCG or KVM (info kvm) and the state of
>>>> the nested-hv capability in QEMU command line.
>>> Currently the issue is seen with any pseries KVM guest running on a PowerNV host.
> Okay originally I thought you were talking about a powernv target
> that is running a pseries guest and dumping that. But after re-reading, I
> think you're talking about dumping a pseries target?

yes, The qemu-memory-dump tested on different combinations like pseries
as well as powernv guests with and without -enable-kvm.

> Questions still remain about why that's the best way to go. If the
> target was running a nested-HV guest, is it reasonable that the guest
> can change the endinaness of the target dump on a whim by changing its
> ILE?
>
> Thanks,
> Nick

But the crash tool expects the endianness of the dump loaded should be same as
supplied debug kernel image endianness.
Detailed test cases we can find at V2 of this patch.
https://lore.kernel.org/all/20230509104701.12473-1-nnmlinux@linux.ibm.com/

Regards,
Narayana.

[-- Attachment #2: Type: text/html, Size: 7814 bytes --]

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

[Nicholas Piggin]

On Tue May 16, 2023 at 11:54 AM AEST, Narayana Murty N wrote:
>
> On 5/15/23 12:02, Nicholas Piggin wrote:
> > On Sat Apr 29, 2023 at 12:30 AM AEST, Fabiano Rosas wrote:
> >>>> Could you describe in more detail what is your setup? Specifically
> >>>> whether both guests are running TCG or KVM (info kvm) and the state of
> >>>> the nested-hv capability in QEMU command line.
> >>> Currently the issue is seen with any pseries KVM guest running on a PowerNV host.
> > Okay originally I thought you were talking about a powernv target
> > that is running a pseries guest and dumping that. But after re-reading, I
> > think you're talking about dumping a pseries target?
>
> yes, The qemu-memory-dump tested on different combinations like pseries
> as well as powernv guests with and without -enable-kvm.

Still not quite sure what you're talking about from that or the v2
changelog.  powernv is not a guest, it is a target machine, and it can't
use -enable-kvm. The target runs on the host which is your hardware. The
machine can emulate Linux/KVM hosting a guest but that's all *inside*
the target and not a property of the target QEMU. powernv just knows it
is executing with MSR[HV]=0, LPIDR!=0, etc.

> > Questions still remain about why that's the best way to go. If the
> > target was running a nested-HV guest, is it reasonable that the guest
> > can change the endinaness of the target dump on a whim by changing its
> > ILE?
> >
> > Thanks,
> > Nick
>
> But the crash tool expects the endianness of the dump loaded should be same as
> supplied debug kernel image endianness.

Seems like an important detail to mention in the changelog.

> Detailed test cases we can find at V2 of this patch.
> https://lore.kernel.org/all/20230509104701.12473-1-nnmlinux@linux.ibm.com/

The patch may be okay, but changelog needs work. Please explain the
cases using QEMU terminology like host and target, and describe what
the target is doing in a way that's clear you aren't talking about
the QEMU that is the subject of the patch.

Host    Target        Workload
BE      powernv       BE Linux running LE KVM guest
LE      powernv       ...
LE      pseries KVM   etc
LE      pseries TCG

Don't have to enumerate the entire matrix or put it in that format
exactly, but something clear and precise.

ILE does not always match vmlinux being run by target either, mind you.
Generally in early boot. I guess that is an unavoidable gap without
doing more stuff (either make crash work or add a way to set dump endian
or something). That's okay, ILE might be a reasonable proxy, but it
might be worth mentioning in the changelog too.

Thanks,
Nick