| Message ID | 201606241911.u5OJB05n006837@linux03a.ddci.com |
|---|---|
| State | New |
| Headers | show |
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 <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.
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 <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.
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. >
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,
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(-)