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

[Narayana Murty N]

On 5/23/23 15:45, Greg Kurz wrote:

> 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.
>     
described in v4.
> > > 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.
>     
done
> > > > 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;
> > > >           
> > >         
> >       
>