[Qemu-devel] [PATCH] target-ppc: Eliminate redundant and incorrect function booke206_page_size_to_tlb

[Qemu-devel] [PATCH] target-ppc: Eliminate redundant and incorrect function booke206_page_size_to_tlb

[Aaron Larson]

Eliminate redundant and incorrect booke206_page_size_to_tlb function
from ppce500_spin.c in preference to previously existing but newly
exported definition from e500.c

Signed-off-by: Aaron Larson <alarson@ddci.com>
---
 hw/ppc/e500.c         | 2 +-
 hw/ppc/e500.h         | 2 ++
 hw/ppc/ppce500_spin.c | 7 +------
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index ee1c60b..0cd534d 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -601,7 +601,7 @@ static int ppce500_prep_device_tree(MachineState *machine,
 }
 
 /* Create -kernel TLB entries for BookE.  */
-static inline hwaddr booke206_page_size_to_tlb(uint64_t size)
+hwaddr booke206_page_size_to_tlb(uint64_t size)
 {
     return 63 - clz64(size >> 10);
 }
diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
index ef224ea..70ba1d8 100644
--- a/hw/ppc/e500.h
+++ b/hw/ppc/e500.h
@@ -26,4 +26,6 @@ typedef struct PPCE500Params {
 
 void ppce500_init(MachineState *machine, PPCE500Params *params);
 
+hwaddr booke206_page_size_to_tlb(uint64_t size);
+
 #endif
diff --git a/hw/ppc/ppce500_spin.c b/hw/ppc/ppce500_spin.c
index 225177b..22c584e 100644
--- a/hw/ppc/ppce500_spin.c
+++ b/hw/ppc/ppce500_spin.c
@@ -32,6 +32,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/sysbus.h"
 #include "sysemu/kvm.h"
+#include "e500.h"
 
 #define MAX_CPUS 32
 
@@ -72,12 +73,6 @@ static void spin_reset(void *opaque)
     }
 }
 
-/* Create -kernel TLB entries for BookE, linearly spanning 256MB.  */
-static inline hwaddr booke206_page_size_to_tlb(uint64_t size)
-{
-    return ctz32(size >> 10) >> 1;
-}
-
 static void mmubooke_create_initial_mapping(CPUPPCState *env,
                                      target_ulong va,
                                      hwaddr pa,
-- 
2.7.4

Re: [Qemu-devel] [PATCH] target-ppc: Eliminate redundant and incorrect function booke206_page_size_to_tlb

[David Gibson]

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

On Fri, Jun 24, 2016 at 12:11:00PM -0700, Aaron Larson wrote:
> 
> Eliminate redundant and incorrect booke206_page_size_to_tlb function
> from ppce500_spin.c in preference to previously existing but newly
> exported definition from e500.c
> 
> Signed-off-by: Aaron Larson <alarson@ddci.com>

Uh.. sorry.. can you provide a reference explaining why the removed
version is wrong?  Doesn't this depend on which MMU Architecture
Version we're emulating?

> ---
>  hw/ppc/e500.c         | 2 +-
>  hw/ppc/e500.h         | 2 ++
>  hw/ppc/ppce500_spin.c | 7 +------
>  3 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index ee1c60b..0cd534d 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -601,7 +601,7 @@ static int ppce500_prep_device_tree(MachineState *machine,
>  }
>  
>  /* Create -kernel TLB entries for BookE.  */
> -static inline hwaddr booke206_page_size_to_tlb(uint64_t size)
> +hwaddr booke206_page_size_to_tlb(uint64_t size)
>  {
>      return 63 - clz64(size >> 10);
>  }
> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> index ef224ea..70ba1d8 100644
> --- a/hw/ppc/e500.h
> +++ b/hw/ppc/e500.h
> @@ -26,4 +26,6 @@ typedef struct PPCE500Params {
>  
>  void ppce500_init(MachineState *machine, PPCE500Params *params);
>  
> +hwaddr booke206_page_size_to_tlb(uint64_t size);
> +
>  #endif
> diff --git a/hw/ppc/ppce500_spin.c b/hw/ppc/ppce500_spin.c
> index 225177b..22c584e 100644
> --- a/hw/ppc/ppce500_spin.c
> +++ b/hw/ppc/ppce500_spin.c
> @@ -32,6 +32,7 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/sysbus.h"
>  #include "sysemu/kvm.h"
> +#include "e500.h"
>  
>  #define MAX_CPUS 32
>  
> @@ -72,12 +73,6 @@ static void spin_reset(void *opaque)
>      }
>  }
>  
> -/* Create -kernel TLB entries for BookE, linearly spanning 256MB.  */
> -static inline hwaddr booke206_page_size_to_tlb(uint64_t size)
> -{
> -    return ctz32(size >> 10) >> 1;
> -}
> -
>  static void mmubooke_create_initial_mapping(CPUPPCState *env,
>                                       target_ulong va,
>                                       hwaddr pa,

-- 
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: 819 bytes --]

Re: [Qemu-devel] [PATCH] target-ppc: Eliminate redundant and incorrect function booke206_page_size_to_tlb

[alarson]

David Gibson <david@gibson.dropbear.id.au> wrote on 06/26/2016 08:36:52 
PM:

> From: David Gibson <david@gibson.dropbear.id.au>
> To: Aaron Larson <alarson@ddci.com>
> Cc: agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org
> Date: 06/26/2016 08:58 PM
> Subject: Re: [PATCH] target-ppc: Eliminate redundant and incorrect 
function booke206_page_size_to_tlb
> 
> On Fri, Jun 24, 2016 at 12:11:00PM -0700, Aaron Larson wrote:
> > 
> > Eliminate redundant and incorrect booke206_page_size_to_tlb function
> > from ppce500_spin.c in preference to previously existing but newly
> > exported definition from e500.c
> > 
> > Signed-off-by: Aaron Larson <alarson@ddci.com>
> 
> Uh.. sorry.. can you provide a reference explaining why the removed
> version is wrong?  Doesn't this depend on which MMU Architecture
> Version we're emulating?

Sure, the code is internally inconsistent (shift assumed didn't match 
shift defined).  I will provide an update commit message, similar to the 
original posting I made a few days ago with the "direction" corrected.

I didn't verify what sub-architectures/platforms are affected.  I've been 
assuming it was just ppce500.  If it is important I can do a bit of 
research, but the way code is coupled to platforms is sometimes surprising 
to me, so I'm not sure I would trust my analysis.  Let me know if you need 
this analysis as part of the updated comment.

Re: [Qemu-devel] [PATCH] target-ppc: Eliminate redundant and incorrect function booke206_page_size_to_tlb

[David Gibson]

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

On Sun, Jun 26, 2016 at 09:38:03PM -0500, alarson@ddci.com wrote:
> David Gibson <david@gibson.dropbear.id.au> wrote on 06/26/2016 08:36:52 
> PM:
> 
> > From: David Gibson <david@gibson.dropbear.id.au>
> > To: Aaron Larson <alarson@ddci.com>
> > Cc: agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org
> > Date: 06/26/2016 08:58 PM
> > Subject: Re: [PATCH] target-ppc: Eliminate redundant and incorrect 
> function booke206_page_size_to_tlb
> > 
> > On Fri, Jun 24, 2016 at 12:11:00PM -0700, Aaron Larson wrote:
> > > 
> > > Eliminate redundant and incorrect booke206_page_size_to_tlb function
> > > from ppce500_spin.c in preference to previously existing but newly
> > > exported definition from e500.c
> > > 
> > > Signed-off-by: Aaron Larson <alarson@ddci.com>
> > 
> > Uh.. sorry.. can you provide a reference explaining why the removed
> > version is wrong?  Doesn't this depend on which MMU Architecture
> > Version we're emulating?
> 
> Sure, the code is internally inconsistent (shift assumed didn't match 
> shift defined).  I will provide an update commit message, similar to the 
> original posting I made a few days ago with the "direction"
> corrected.

Right, I can see that the two old versions were different, which was
clearly wrong.  What I'm looking for is an explanation of why the one
you've picked is the right one, not the other one.

-- 
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: 819 bytes --]

Re: [Qemu-devel] [PATCH] target-ppc: Eliminate redundant and incorrect function booke206_page_size_to_tlb

[alarson]

David Gibson <david@gibson.dropbear.id.au> wrote on 06/27/2016 12:32:13 
AM:

> From: David Gibson <david@gibson.dropbear.id.au>
> To: alarson@ddci.com
> Cc: agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org
> Date: 06/27/2016 12:30 AM
> Subject: Re: [PATCH] target-ppc: Eliminate redundant and incorrect 
function booke206_page_size_to_tlb
> 
> On Sun, Jun 26, 2016 at 09:38:03PM -0500, alarson@ddci.com wrote:
> > David Gibson <david@gibson.dropbear.id.au> wrote on 06/26/2016 
08:36:52 
> > PM:
> > 
> > > From: David Gibson <david@gibson.dropbear.id.au>
> > > To: Aaron Larson <alarson@ddci.com>
> > > Cc: agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org
> > > Date: 06/26/2016 08:58 PM
> > > Subject: Re: [PATCH] target-ppc: Eliminate redundant and incorrect 
> > function booke206_page_size_to_tlb
> > > 
> > > On Fri, Jun 24, 2016 at 12:11:00PM -0700, Aaron Larson wrote:
> > > > 
> > > > Eliminate redundant and incorrect booke206_page_size_to_tlb 
function
> > > > from ppce500_spin.c in preference to previously existing but newly
> > > > exported definition from e500.c
> > > > 
> > > > Signed-off-by: Aaron Larson <alarson@ddci.com>
> > > 
> > > Uh.. sorry.. can you provide a reference explaining why the removed
> > > version is wrong?  Doesn't this depend on which MMU Architecture
> > > Version we're emulating?
> > 
> > Sure, the code is internally inconsistent (shift assumed didn't match 
> > shift defined).  I will provide an update commit message, similar to 
the 
> > original posting I made a few days ago with the "direction"
> > corrected.
> 
> Right, I can see that the two old versions were different, which was
> clearly wrong.  What I'm looking for is an explanation of why the one
> you've picked is the right one, not the other one.

How's this?

Eliminate redundant and incorrect booke206_page_size_to_tlb function
from ppce500_spin.c in preference to previously existing but newly
exported definition from e500.c

The booke206_page_size_to_tlb function in e500.c was updated in commit
2bd9543cd303d9f6cbd37b7466bb03543035156b to reflect a change in the
definition of MAS1_TSIZE_SHIFT from 8 (corresponding to a min TLB page
size of 4kb) to a value of 7 (TLB page size 2k).  The
booke206_page_size_to_tlb() function defined in ppce500_spin.c was
never updated to reflect the change in MAS1_TSIZE_SHIFT.

In http://lists.nongnu.org/archive/html/qemu-ppc/2016-06/msg00533.html,
Scott Wood suggested this "root cause" explanation:

SW> The patch that changed MAS1_TSIZE_SHIFT from 8 to 7 was around the
SW> same time as the patch that added this code, which is probably why
SW> adjusting it got missed.  Commit 2bd9543cd3 did update the
SW> equivalent code in ppce500_mpc8544ds.c, which now resides in
SW> hw/ppc/e500.c and has been changed to not assume a power-of-2
SW> size.  The ppce500_spin version should be eliminated.

Re: [Qemu-devel] [PATCH] target-ppc: Eliminate redundant and incorrect function booke206_page_size_to_tlb

[David Gibson]

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

On Mon, Jun 27, 2016 at 10:55:03AM -0500, alarson@ddci.com wrote:
> David Gibson <david@gibson.dropbear.id.au> wrote on 06/27/2016 12:32:13 
> AM:
> 
> > From: David Gibson <david@gibson.dropbear.id.au>
> > To: alarson@ddci.com
> > Cc: agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org
> > Date: 06/27/2016 12:30 AM
> > Subject: Re: [PATCH] target-ppc: Eliminate redundant and incorrect 
> function booke206_page_size_to_tlb
> > 
> > On Sun, Jun 26, 2016 at 09:38:03PM -0500, alarson@ddci.com wrote:
> > > David Gibson <david@gibson.dropbear.id.au> wrote on 06/26/2016 
> 08:36:52 
> > > PM:
> > > 
> > > > From: David Gibson <david@gibson.dropbear.id.au>
> > > > To: Aaron Larson <alarson@ddci.com>
> > > > Cc: agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org
> > > > Date: 06/26/2016 08:58 PM
> > > > Subject: Re: [PATCH] target-ppc: Eliminate redundant and incorrect 
> > > function booke206_page_size_to_tlb
> > > > 
> > > > On Fri, Jun 24, 2016 at 12:11:00PM -0700, Aaron Larson wrote:
> > > > > 
> > > > > Eliminate redundant and incorrect booke206_page_size_to_tlb 
> function
> > > > > from ppce500_spin.c in preference to previously existing but newly
> > > > > exported definition from e500.c
> > > > > 
> > > > > Signed-off-by: Aaron Larson <alarson@ddci.com>
> > > > 
> > > > Uh.. sorry.. can you provide a reference explaining why the removed
> > > > version is wrong?  Doesn't this depend on which MMU Architecture
> > > > Version we're emulating?
> > > 
> > > Sure, the code is internally inconsistent (shift assumed didn't match 
> > > shift defined).  I will provide an update commit message, similar to 
> the 
> > > original posting I made a few days ago with the "direction"
> > > corrected.
> > 
> > Right, I can see that the two old versions were different, which was
> > clearly wrong.  What I'm looking for is an explanation of why the one
> > you've picked is the right one, not the other one.
> 
> How's this?
> 
> Eliminate redundant and incorrect booke206_page_size_to_tlb function
> from ppce500_spin.c in preference to previously existing but newly
> exported definition from e500.c
> 
> The booke206_page_size_to_tlb function in e500.c was updated in commit
> 2bd9543cd303d9f6cbd37b7466bb03543035156b to reflect a change in the
> definition of MAS1_TSIZE_SHIFT from 8 (corresponding to a min TLB page
> size of 4kb) to a value of 7 (TLB page size 2k).  The
> booke206_page_size_to_tlb() function defined in ppce500_spin.c was
> never updated to reflect the change in MAS1_TSIZE_SHIFT.

Ah, yes, that looks good.  Except that the convention when citing
other commits is to just use the first 7 characters of the commit id,
but to include the 1 line description.  So:

   ...commit 2bd9543 "ppc: booke206: use MAV=2.0 TSIZE definition, fix
      4G pages" to reflect...

> In http://lists.nongnu.org/archive/html/qemu-ppc/2016-06/msg00533.html,
> Scott Wood suggested this "root cause" explanation:
> 
> SW> The patch that changed MAS1_TSIZE_SHIFT from 8 to 7 was around the
> SW> same time as the patch that added this code, which is probably why
> SW> adjusting it got missed.  Commit 2bd9543cd3 did update the
> SW> equivalent code in ppce500_mpc8544ds.c, which now resides in
> SW> hw/ppc/e500.c and has been changed to not assume a power-of-2
> SW> size.  The ppce500_spin version should be eliminated.
> 

-- 
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: 819 bytes --]