Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/4] ppc64-dump: Support dump for little endian ppc64

[Alexander Graf]

On 30.04.14 11:14, Greg Kurz wrote:
> On Tue, 29 Apr 2014 11:19:40 +0200
> Alexander Graf <address@hidden> wrote:
>
> > On 28.04.14 13:29, Greg Kurz wrote:
> > > From: Bharata B Rao <address@hidden>
> > >
> > > Fix ppc64 arch specific dump code to work correctly for little endian
> > > guests.
> > >
> > > Signed-off-by: Bharata B Rao <address@hidden>
> > > [ rebased on top of current master branch,
> > >     Greg Kurz <address@hidden> ]
> > > Signed-off-by: Greg Kurz <address@hidden>
> > > ---
> > >    target-ppc/arch_dump.c |   62 
> > > ++++++++++++++++++++++++++----------------------
> > >    1 file changed, 33 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/target-ppc/arch_dump.c b/target-ppc/arch_dump.c
> > > index 9dccf1a..a85c872 100644
> > > --- a/target-ppc/arch_dump.c
> > > +++ b/target-ppc/arch_dump.c
> > > @@ -80,93 +80,95 @@ typedef struct noteStruct {
> > >    } QEMU_PACKED Note;
> > >    
> > >    
> > > -static void ppc64_write_elf64_prstatus(Note *note, PowerPCCPU *cpu)
> > > +static void ppc64_write_elf64_prstatus(Note *note, PowerPCCPU *cpu, int endian)
> > >    {
> > >        int i;
> > >        uint64_t cr;
> > >        struct PPC64ElfPrstatus *prstatus;
> > >        struct PPC64UserRegStruct *reg;
> > >    
> > > -    note->hdr.n_type = cpu_to_be32(NT_PRSTATUS);
> > > +    note->hdr.n_type = cpu_convert_to_target32(NT_PRSTATUS, endian);
> > >    
> > >        prstatus = &note->contents.prstatus;
> > >        memset(prstatus, 0, sizeof(*prstatus));
> > >        reg = &prstatus->pr_reg;
> > >    
> > >        for (i = 0; i < 32; i++) {
> > > -        reg->gpr[i] = cpu_to_be64(cpu->env.gpr[i]);
> > > +        reg->gpr[i] = cpu_convert_to_target64(cpu->env.gpr[i], endian);
> > >        }
> > > -    reg->nip = cpu_to_be64(cpu->env.nip);
> > > -    reg->msr = cpu_to_be64(cpu->env.msr);
> > > -    reg->ctr = cpu_to_be64(cpu->env.ctr);
> > > -    reg->link = cpu_to_be64(cpu->env.lr);
> > > -    reg->xer = cpu_to_be64(cpu_read_xer(&cpu->env));
> > > +    reg->nip = cpu_convert_to_target64(cpu->env.nip, endian);
> > > +    reg->msr = cpu_convert_to_target64(cpu->env.msr, endian);
> > > +    reg->ctr = cpu_convert_to_target64(cpu->env.ctr, endian);
> > > +    reg->link = cpu_convert_to_target64(cpu->env.lr, endian);
> > > +    reg->xer = cpu_convert_to_target64(cpu_read_xer(&cpu->env), endian);
> > I'm not sure we really care about performance here. Can't we just have a
> > local endian helper function that gets a Note * pointer and extracts the
> > endianness from there?
> This would be to avoid passing a 3rd argument to Note functions ?
>
> > If not, maybe we should have an encapsulation struct around the Note and
> > pass that around as token.
> Yeah we should do that since the Note does not contain anything that could
> be used to get the endianness... What about something like:
>
> typedef struct ExtNote {
>      Note note;
>      DumpState *state;
> } ExtNote;
>
> I had proposed to pass ArchDumpInfo * to the endian helpers, but I find
> DumpState * better: we end up with less users of DumpState::dump_info.
>
> Thoughts ?

I think you'll see what fits best once you start writing it, so I'll 
trust your taste here :)


Alex