[PATCH] ppc: fix reuse of MMU translation slots

[Alyssa Milburn]

The current code always picks slot 0 in the reuse case. (This doesn't seem
to matter in the cases I've tested, though.)

Signed-off-by: Alyssa Milburn 
---
 arch/ppc/qemu/ofmem.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/ppc/qemu/ofmem.c b/arch/ppc/qemu/ofmem.c
index 6f346a3..c9fe154 100644
--- a/arch/ppc/qemu/ofmem.c
+++ b/arch/ppc/qemu/ofmem.c
@@ -359,9 +359,11 @@ hash_page_64(unsigned long ea, phys_addr_t phys, ucell mode)
             found = 1;

/* otherwise use a free slot */
-    for (i = 0; !found && i < 8; i++)
-        if (!pp[i].v)
-            found = 1;
+    if (!found) {
+        for (i = 0; !found && i < 8; i++)
+            if (!pp[i].v)
+                found = 1;
+    }

/* out of slots, just evict one */
     if (!found)
@@ -414,9 +416,11 @@ hash_page_32(unsigned long ea, phys_addr_t phys, ucell mode)
             found = 1;

/* otherwise use a free slot */
-    for (i = 0; !found && i < 8; i++)
-        if (!pp[i].v)
-            found = 1;
+    if (!found) {
+        for (i = 0; !found && i < 8; i++)
+            if (!pp[i].v)
+                found = 1;
+    }

/* out of slots, just evict one */
     if (!found)

[Mark Cave-Ayland]

On 10/02/16 23:25, Alyssa Milburn wrote:
> The current code always picks slot 0 in the reuse case. (This doesn't seem
> to matter in the cases I've tested, though.)
> Signed-off-by: Alyssa Milburn
>
> arch/ppc/qemu/ofmem.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/arch/ppc/qemu/ofmem.c b/arch/ppc/qemu/ofmem.c
> index 6f346a3..c9fe154 100644
> --- a/arch/ppc/qemu/ofmem.c
> +++ b/arch/ppc/qemu/ofmem.c
> @@ -359,9 +359,11 @@ hash_page_64(unsigned long ea, phys_addr_t phys, ucell mode)
>              found = 1;
>  /* otherwise use a free slot */
> for (i = 0; !found && i < 8; i++)
>    if (!pp[i].v)
>        found = 1;
>
> if (!found) {
>    for (i = 0; !found && i < 8; i++)
>        if (!pp[i].v)
>            found = 1;
>
> }
>
> /* out of slots, just evict one */
> if (!found)
>
> @@ -414,9 +416,11 @@ hash_page_32(unsigned long ea, phys_addr_t phys, ucell mode)
>              found = 1;
>  /* otherwise use a free slot */
> for (i = 0; !found && i < 8; i++)
>    if (!pp[i].v)
>        found = 1;
>
> if (!found) {
>    for (i = 0; !found && i < 8; i++)
>        if (!pp[i].v)
>            found = 1;
>
> }
>
> /* out of slots, just evict one */
> if (!found)

Hmmm yes I think you're right there is a bug here. Assuming that we
locate the index of an old translation, then i should be set to the slot
number to replace. But even though found has been set, the subsequent
for() will always reset i to zero, even though it exits without iterating.

In theory this could mean that we have multiple entries in the hash
table for the same translation which can't be a good thing.

Alex - does this look like the right solution to you?

ATB,

Mark.

[Alexander Graf]

On 02/12/2016 12:38 PM, Mark Cave-Ayland wrote:
> On 10/02/16 23:25, Alyssa Milburn wrote:
> > The current code always picks slot 0 in the reuse case. (This doesn't seem
> > to matter in the cases I've tested, though.)
> > Signed-off-by: Alyssa Milburn
> >
> > arch/ppc/qemu/ofmem.c | 16 ++++++++++------
> >   1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/ppc/qemu/ofmem.c b/arch/ppc/qemu/ofmem.c
> > index 6f346a3..c9fe154 100644
> > --- a/arch/ppc/qemu/ofmem.c
> > +++ b/arch/ppc/qemu/ofmem.c
> > @@ -359,9 +359,11 @@ hash_page_64(unsigned long ea, phys_addr_t phys, ucell mode)
> >               found = 1;
> >   /* otherwise use a free slot */
> > for (i = 0; !found && i < 8; i++)
> >    if (!pp[i].v)
> >        found = 1;
> >
> > if (!found) {
> >    for (i = 0; !found && i < 8; i++)
> >        if (!pp[i].v)
> >            found = 1;
> >
> > }
> >
> > /* out of slots, just evict one */
> >  if (!found)
> >
> > @@ -414,9 +416,11 @@ hash_page_32(unsigned long ea, phys_addr_t phys, ucell mode)
> >               found = 1;
> >   /* otherwise use a free slot */
> > for (i = 0; !found && i < 8; i++)
> >    if (!pp[i].v)
> >        found = 1;
> >
> > if (!found) {
> >    for (i = 0; !found && i < 8; i++)
> >        if (!pp[i].v)
> >            found = 1;
> >
> > }
> >
> > /* out of slots, just evict one */
> >  if (!found)
>
> Hmmm yes I think you're right there is a bug here. Assuming that we
> locate the index of an old translation, then i should be set to the slot
> number to replace. But even though found has been set, the subsequent
> for() will always reset i to zero, even though it exits without iterating.
>
> In theory this could mean that we have multiple entries in the hash
> table for the same translation which can't be a good thing.

Yes, on real hardware machine checks can result from this.
> Alex - does this look like the right solution to you?

Looks reasonable to me.

Alex

[Mark Cave-Ayland]

On 13/02/16 20:09, Alexander Graf wrote:
> On 02/12/2016 12:38 PM, Mark Cave-Ayland wrote:
> > On 10/02/16 23:25, Alyssa Milburn wrote:
> > > The current code always picks slot 0 in the reuse case. (This doesn't
> > > seem
> > > to matter in the cases I've tested, though.)
> > > Signed-off-by: Alyssa Milburn
> > >
> > > arch/ppc/qemu/ofmem.c | 16 ++++++++++------
> > >   1 file changed, 10 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/ppc/qemu/ofmem.c b/arch/ppc/qemu/ofmem.c
> > > index 6f346a3..c9fe154 100644
> > > --- a/arch/ppc/qemu/ofmem.c
> > > +++ b/arch/ppc/qemu/ofmem.c
> > > @@ -359,9 +359,11 @@ hash_page_64(unsigned long ea, phys_addr_t phys,
> > > ucell mode)
> > >               found = 1;
> > >         /* otherwise use a free slot */
> > > for (i = 0; !found && i < 8; i++)
> > >    if (!pp[i].v)
> > >        found = 1;
> > > if (!found) {
> > >    for (i = 0; !found && i < 8; i++)
> > >        if (!pp[i].v)
> > >            found = 1;
> > > }
> > >    /* out of slots, just evict one */
> > >  if (!found)
> > >
> > > @@ -414,9 +416,11 @@ hash_page_32(unsigned long ea, phys_addr_t phys,
> > > ucell mode)
> > >               found = 1;
> > >         /* otherwise use a free slot */
> > > for (i = 0; !found && i < 8; i++)
> > >    if (!pp[i].v)
> > >        found = 1;
> > > if (!found) {
> > >    for (i = 0; !found && i < 8; i++)
> > >        if (!pp[i].v)
> > >            found = 1;
> > > }
> > >    /* out of slots, just evict one */
> > >  if (!found)
> >
> > Hmmm yes I think you're right there is a bug here. Assuming that we
> > locate the index of an old translation, then i should be set to the slot
> > number to replace. But even though found has been set, the subsequent
> > for() will always reset i to zero, even though it exits without
> > iterating.
> >
> > In theory this could mean that we have multiple entries in the hash
> > table for the same translation which can't be a good thing.
>
> Yes, on real hardware machine checks can result from this.
> > Alex - does this look like the right solution to you?
>
> Looks reasonable to me.

Thanks! Would you like to commit for a change? I get bored of seeing my
own name in the commit logs all the time ;)

ATB,

Mark.