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

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

[Maxiwell S. Garcia]

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 <maxiwell@linux.ibm.com>
Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 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;
-- 
2.20.1

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1703 bytes --]

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 <maxiwell@linux.ibm.com>
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>

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.

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.

> ---
>  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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 <maxiwell@linux.ibm.com>
> > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> 
> 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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2254 bytes --]

On Mon, Dec 23, 2019 at 06:35:30PM -0300, Maxiwell S. Garcia wrote:
> 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 <maxiwell@linux.ibm.com>
> > > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> > 
> > 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.

Ok.

> > 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?

That function alters the LE bit in hflags, but doesn't read it.
*Nothing* reads it, so none of the places that alter it matter.

I strongly suspect we won't properly honour the LE bit in the 601 HID
register, but that's already the case.  I also suspect it's far from
the only way in which 601 emulation is broken.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Fabiano Rosas]

David Gibson <david@gibson.dropbear.id.au> writes:

> 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 see there is:

static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
{
    ...
    ctx->le_mode = !!(env->hflags & (1 << MSR_LE));
    ...
}

And we call hreg_recompute_hflags in some places:

- powerpc_excp (target/ppc/excp_helper.c)
  Called from TCG do_interrupt

- ppc_cpu_reset (target/ppc/translate_init.inc.c)
  Called from spapr_machine_reset

- hreg_store_msr (target/ppc/helper_regs.h)
  This is used for migration and for do_rfi, store_msr

- h_cede (hw/ppc/spapr_hcall.c)
  QEMU-side H_CEDE hypercall implementation 


It looks like the hflags MSR_LE is being updated correctly with TCG. But
with KVM we only touch it on system_reset (and possibly h_cede? I don't
know if it is QEMU who handles it).

So I would let hflags be.


... Actually, I don't really know the purpose of hflags. It comes from:

  commit 3f3373166227b13e762e20d2fb51eadfa6a2d653
  Author: Fabrice Bellard <fabrice@bellard.org>
  Date:   Wed Aug 20 23:02:09 2003 +0000
  
      pop ss, mov ss, x and sti disable irqs for the next instruction -
      began dispatch optimization by adding new x86 cpu 'hidden' flags
      
      
      git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@372 c046a42c-6fe2-441c-8c8c-71466251a162

Could any one clarify that?

Thanks

>> ---
>>  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;

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2823 bytes --]

On Mon, Dec 23, 2019 at 08:27:49PM -0300, Fabiano Rosas wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > 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 see there is:
> 
> static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
> {
>     ...
>     ctx->le_mode = !!(env->hflags & (1 << MSR_LE));
>     ...
> }

Ah... good point, I missed that one, sorry.  That makes all the
difference.

My guess is that this bit exists to be a universal flag for endianness
mode, generalizing across the MSR bit on modern cpus, and the old 601
which had it in the HID register.  I'm a bit dubious as to whether our
601 emulation is good enough to warrant bothering with this, but it's
probably best not to mess with it.


> And we call hreg_recompute_hflags in some places:

ITYM hreg_compute_hflags().

> - powerpc_excp (target/ppc/excp_helper.c)
>   Called from TCG do_interrupt
> 
> - ppc_cpu_reset (target/ppc/translate_init.inc.c)
>   Called from spapr_machine_reset
> 
> - hreg_store_msr (target/ppc/helper_regs.h)
>   This is used for migration and for do_rfi, store_msr

Huh... given this, I'm not sure how hflags was getting out of sync
with the MSR in the first place, which brings the initial patch into
question.

> - h_cede (hw/ppc/spapr_hcall.c)
>   QEMU-side H_CEDE hypercall implementation 
> 
> 
> It looks like the hflags MSR_LE is being updated correctly with TCG. But
> with KVM we only touch it on system_reset

Ah.. right.  I think to fix that we'd want an hreg_compute_hflags() at
the end of sucking the state out of KVM.

> (and possibly h_cede? I don't
> know if it is QEMU who handles it).

It's KVM.  If we used the qemu one it would add an awful lot of
latency to cedes.
> 
> So I would let hflags be.
> 
> 
> ... Actually, I don't really know the purpose of hflags. It comes from:
> 
>   commit 3f3373166227b13e762e20d2fb51eadfa6a2d653
>   Author: Fabrice Bellard <fabrice@bellard.org>
>   Date:   Wed Aug 20 23:02:09 2003 +0000
>   
>       pop ss, mov ss, x and sti disable irqs for the next instruction -
>       began dispatch optimization by adding new x86 cpu 'hidden' flags
>       
>       
>       git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@372 c046a42c-6fe2-441c-8c8c-71466251a162
> 
> Could any one clarify that?

Not really.  It's really, really old, in the cruft bits of TCG I don't
much understand.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Fabiano Rosas]

David Gibson <david@gibson.dropbear.id.au> writes:

>> It looks like the hflags MSR_LE is being updated correctly with TCG. But
>> with KVM we only touch it on system_reset
>
> Ah.. right.  I think to fix that we'd want an hreg_compute_hflags() at
> the end of sucking the state out of KVM.
>

Hm.. The hflags is a TCG thing that does not get used with KVM at all,
except for that one bit in the monitor disas function. I'd rather keep
it completely out of kvm_enabled code.

Couldn't we perhaps make it conditional on the acceleration type?
Using kvm_enabled() or some ifdef.

Thanks

>> (and possibly h_cede? I don't
>> know if it is QEMU who handles it).
>
> It's KVM.  If we used the qemu one it would add an awful lot of
> latency to cedes.
>> 
>> So I would let hflags be.
>> 
>> 
>> ... Actually, I don't really know the purpose of hflags. It comes from:
>> 
>>   commit 3f3373166227b13e762e20d2fb51eadfa6a2d653
>>   Author: Fabrice Bellard <fabrice@bellard.org>
>>   Date:   Wed Aug 20 23:02:09 2003 +0000
>>   
>>       pop ss, mov ss, x and sti disable irqs for the next instruction -
>>       began dispatch optimization by adding new x86 cpu 'hidden' flags
>>       
>>       
>>       git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@372 c046a42c-6fe2-441c-8c8c-71466251a162
>> 
>> Could any one clarify that?
>
> Not really.  It's really, really old, in the cruft bits of TCG I don't
> much understand.

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2110 bytes --]

On Tue, Dec 24, 2019 at 01:10:34PM -0300, Fabiano Rosas wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> >> It looks like the hflags MSR_LE is being updated correctly with TCG. But
> >> with KVM we only touch it on system_reset
> >
> > Ah.. right.  I think to fix that we'd want an hreg_compute_hflags() at
> > the end of sucking the state out of KVM.
> >
> 
> Hm.. The hflags is a TCG thing that does not get used with KVM at all,
> except for that one bit in the monitor disas function. I'd rather keep
> it completely out of kvm_enabled code.

That's a fair point.

It doesn't look like there's any reason ppc_tr_init_disas_context()
couldn't initialize ctx->le_mode directly from the MSR rather than
hflags anyway.  If we do that, we should again be able to remove the
LE bit from hflags entirely.  I think that's the way to go.

> 
> Couldn't we perhaps make it conditional on the acceleration type?
> Using kvm_enabled() or some ifdef.
> 
> Thanks
> 
> >> (and possibly h_cede? I don't
> >> know if it is QEMU who handles it).
> >
> > It's KVM.  If we used the qemu one it would add an awful lot of
> > latency to cedes.
> >> 
> >> So I would let hflags be.
> >> 
> >> 
> >> ... Actually, I don't really know the purpose of hflags. It comes from:
> >> 
> >>   commit 3f3373166227b13e762e20d2fb51eadfa6a2d653
> >>   Author: Fabrice Bellard <fabrice@bellard.org>
> >>   Date:   Wed Aug 20 23:02:09 2003 +0000
> >>   
> >>       pop ss, mov ss, x and sti disable irqs for the next instruction -
> >>       began dispatch optimization by adding new x86 cpu 'hidden' flags
> >>       
> >>       
> >>       git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@372 c046a42c-6fe2-441c-8c8c-71466251a162
> >> 
> >> Could any one clarify that?
> >
> > Not really.  It's really, really old, in the cruft bits of TCG I don't
> > much understand.
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]