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

[Greg Kurz]

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 ?

> 
> Alex
> 
> 



-- 
Gregory Kurz                                     address@hidden
                                                 address@hidden
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.