Re: [PATCH v3] target: ppc: Use MSR_HVB bit to get the target endianness for memory dump

[Greg Kurz <groug@kaod.org>]

On Tue, 23 May 2023 12:20:17 +0530
Narayana Murty N <nnmlinux@linux.vnet.ibm.com> wrote:

> 
> On 5/22/23 23:50, Greg Kurz wrote:
> > On Mon, 22 May 2023 12:02:42 -0400
> > Narayana Murty N<nnmlinux@linux.ibm.com>  wrote:
> >
> >> Currently on PPC64 qemu always dumps the guest memory in
> >> Big Endian (BE) format even though the guest running in Little Endian
> >> (LE) mode. So crash tool fails to load the dump 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>
> >>
> >> This happens because cpu_get_dump_info() passes cpu->env->has_hv_mode
> >> to function ppc_interrupts_little_endian(), the cpu->env->has_hv_mode
> >> always set for powerNV even though the guest is not running in hv mode.
> >> The hv mode should be taken from msr_mask MSR_HVB bit
> >> (cpu->env.msr_mask & MSR_HVB). This patch fixes the issue by passing
> >> MSR_HVB value to ppc_interrupts_little_endian() in order to determine
> >> the guest endianness.
> >>
> >> The crash tool also expects guest kernel endianness should match the
> >> endianness of the dump.
> >>
> >> The patch was tested on POWER9 box booted with Linux as host in
> >> following cases:
> >>
> >> Host-Endianess Qemu-Target-Machine Qemu-Guest-Endianess  Qemu-Generated-Guest
> >>                                                            Memory-Dump-Format
> >> BE             powernv             LE KVM guest                 LE
> >> BE             powernv             BE KVM guest                 BE
> >> LE             powernv             LE KVM guest                 LE
> >> LE             powernv             BE KVM guest                 BE
> > I don't quite understand why KVM is mentioned with the powernv machine.
> 
> guest running mode was mentioned.
> 

QEMU cannot use KVM on the host to run a powernv machine. The
guest is thus necessarily running in TCG mode.

Please describe your setup and what exactly you are testing.

> >
> > Also have you tried to dump at various moments, e.g. during skiboot
> > and when guest is booted, as in [1] which introduced the code this
> > patch is changing ?
> >
> > [1]https://github.com/qemu/qemu/commit/5609400a422809c89ea788e4d0e13124a617582e.
> >
> >> LE             pseries KVM         LE KVM guest                 LE
> >> LE             pseries TCG         LE guest                     LE
> >>
> > Fixes: 5609400a4228 ("target/ppc: Set the correct endianness for powernv memory dumps")
> 
> I agree, commit 5609400a4228 fixes endianness detection only for initial stage (skiboot) till endianness switch happens.
> However, has_hv_mode is just a capability flag which is always set based on command-line param and doesnt really represent current hv state.
> With this patch, it relies on the current state of the hv state based on the MSR_HVB of the msr_mask.
> 

Yes I see what your patch is doing. The 'Fixes: 5609400a4228 ...' line is
intended to the changelog because it is supposedly a fix to this commit.

> >
> >> Signed-off-by: Narayana Murty N<nnmlinux@linux.ibm.com>
> >> ---
> >> Changes since V2:
> >> commit message modified as per feedbak from Nicholas Piggin.
> >> Changes since V1:
> >> https://lore.kernel.org/qemu-devel/20230420145055.10196-1-nnmlinux@linux.ibm.com/
> >> The approach to solve the issue was changed based on feedback from
> >> Fabiano Rosas on patch V1.
> >> ---
> >>   target/ppc/arch_dump.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/target/ppc/arch_dump.c b/target/ppc/arch_dump.c
> >> index f58e6359d5..a8315659d9 100644
> >> --- a/target/ppc/arch_dump.c
> >> +++ b/target/ppc/arch_dump.c
> >> @@ -237,7 +237,7 @@ int cpu_get_dump_info(ArchDumpInfo *info,
> >>       info->d_machine = PPC_ELF_MACHINE;
> >>       info->d_class = ELFCLASS;
> >>   
> >> -    if (ppc_interrupts_little_endian(cpu, cpu->env.has_hv_mode)) {
> >> +    if (ppc_interrupts_little_endian(cpu, !!(cpu->env.msr_mask & MSR_HVB))) {
> >>           info->d_endian = ELFDATA2LSB;
> >>       } else {
> >>           info->d_endian = ELFDATA2MSB;