Re: [PATCH] target/ppc: fix memory dump endianness in QEMU monitor

[Maxiwell S. Garcia]

On Mon, Dec 23, 2019 at 05:30:43PM +1100, David Gibson wrote:
> On Thu, Dec 19, 2019 at 01:38:54PM -0300, Maxiwell S. Garcia wrote:
> > The env->hflags is computed in ppc_cpu_reset(), using the MSR register
> > as input. But at the point ppc_disas_set_info() is called the MSR_LE bit
> > in env->hflags doesn't contain the same information that env->msr.
> > 
> > Signed-off-by: Maxiwell S. Garcia <address@hidden>
> > Signed-off-by: Fabiano Rosas <address@hidden>
> 
> I think the change is ok as far as it goes but,
> 
> a) the commit message should expand on what the practical effect of
> this is.  Looking, I think the only thing this affects is DEBUG_DISAS
> output (i.e. very rarely) which is worth noting.

Ok, I will do that. I got this bug using the 'x/i' command on QEMU
monitor with a LE guest.

> 
> b) AFAICT this is the *only* thing that looks for the LE bit in
> hflags. Given that, and the fact that it would be wrong in most cases,
> we should remove it from hflags entirely along with this change.
> 

I was changing the code to remove this LE bit from hflags and I found the
function 'helper_store_hid0_601()' in misc_helper.c, which manipulates the
'hflags'. The commit 056401eae6 says:

"Implement PowerPC 601 HID0 register, needed for little-endian mode support.
As a consequence, we need to merge hflags coming from MSR with other ones.
Use little-endian mode from hflags instead of MSR during code translation."

So, is the 'hflags' necessary here? Can we use MSR instead of hflags to
change the endianness in this function?

Thank you

> > ---
> >  target/ppc/translate_init.inc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target/ppc/translate_init.inc.c 
> > b/target/ppc/translate_init.inc.c
> > index d33d65dff7..a0b384da9e 100644
> > --- a/target/ppc/translate_init.inc.c
> > +++ b/target/ppc/translate_init.inc.c
> > @@ -10830,7 +10830,7 @@ static void ppc_disas_set_info(CPUState *cs, 
> > disassemble_info *info)
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> >      CPUPPCState *env = &cpu->env;
> >  
> > -    if ((env->hflags >> MSR_LE) & 1) {
> > +    if (msr_le) {
> >          info->endian = BFD_ENDIAN_LITTLE;
> >      }
> >      info->mach = env->bfd_mach;
> 
> -- 
> David Gibson                  | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au        | minimalist, thank you.  NOT _the_ 
> _other_
>                               | _way_ _around_!
> http://www.ozlabs.org/~dgibson