Patchwork Fix a bug in mtsr/mtsrin emulation on ppc64

login
register
mail settings
Submitter David Gibson
Date May 20, 2011, 3:34 a.m.
Message ID <1305862499-12328-1-git-send-email-david@gibson.dropbear.id.au>
Download mbox | patch
Permalink /patch/96521/
State New
Headers show

Comments

David Gibson - May 20, 2011, 3:34 a.m.
Early ppc64 CPUs include a hack to partially simulate the ppc32 segment
registers, by translating writes to them into writes to the SLB.  This is
not used by any current Linux kernel, but it is used by the openbios used
in the qemu mac99 model.

Commit 81762d6dd0d430d87024f2c83e9c4dcc4329fb7d, cleaning up the SLB
handling introduced a bug in this code, breaking the openbios currently in
qemu.  Specifically, there was an off by one error bitshuffling the
register format used by mtsr into the format needed for the SLB load,
causing the flag bits to end up in the wrong place.  This caused the
storage keys to be wrong under openbios, meaning that the translation code
incorrectly thought a legitimate access was a permission violation.

This patch fixes the bug, at the same time it fixes some build bug in the
MMU debugging code (only exposed when DEBUG_MMU is enabled).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target-ppc/helper.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)
Alexander Graf - May 20, 2011, 7:40 a.m.
On 20.05.2011, at 05:34, David Gibson wrote:

> Early ppc64 CPUs include a hack to partially simulate the ppc32 segment
> registers, by translating writes to them into writes to the SLB.  This is
> not used by any current Linux kernel, but it is used by the openbios used
> in the qemu mac99 model.
> 
> Commit 81762d6dd0d430d87024f2c83e9c4dcc4329fb7d, cleaning up the SLB
> handling introduced a bug in this code, breaking the openbios currently in
> qemu.  Specifically, there was an off by one error bitshuffling the
> register format used by mtsr into the format needed for the SLB load,
> causing the flag bits to end up in the wrong place.  This caused the
> storage keys to be wrong under openbios, meaning that the translation code
> incorrectly thought a legitimate access was a permission violation.
> 
> This patch fixes the bug, at the same time it fixes some build bug in the
> MMU debugging code (only exposed when DEBUG_MMU is enabled).

Thanks, applied to ppc-next :)


Alex
Andreas Färber - May 20, 2011, 10:37 p.m.
Am 20.05.2011 um 09:40 schrieb Alexander Graf:

> On 20.05.2011, at 05:34, David Gibson wrote:
>
>> Early ppc64 CPUs include a hack to partially simulate the ppc32  
>> segment
>> registers, by translating writes to them into writes to the SLB.   
>> This is
>> not used by any current Linux kernel, but it is used by the  
>> openbios used
>> in the qemu mac99 model.
>>
>> Commit 81762d6dd0d430d87024f2c83e9c4dcc4329fb7d, cleaning up the SLB
>> handling introduced a bug in this code, breaking the openbios  
>> currently in
>> qemu.  Specifically, there was an off by one error bitshuffling the
>> register format used by mtsr into the format needed for the SLB load,
>> causing the flag bits to end up in the wrong place.  This caused the
>> storage keys to be wrong under openbios, meaning that the  
>> translation code
>> incorrectly thought a legitimate access was a permission violation.
>>
>> This patch fixes the bug, at the same time it fixes some build bug  
>> in the
>> MMU debugging code (only exposed when DEBUG_MMU is enabled).
>
> Thanks, applied to ppc-next :)

Nack, this does not fix ppc64 for me! How did you test it, Alex???

Andreas
Alexander Graf - May 21, 2011, 1:58 a.m.
Am 21.05.2011 um 00:37 schrieb Andreas Färber <andreas.faerber@web.de>:

> Am 20.05.2011 um 09:40 schrieb Alexander Graf:
> 
>> On 20.05.2011, at 05:34, David Gibson wrote:
>> 
>>> Early ppc64 CPUs include a hack to partially simulate the ppc32 segment
>>> registers, by translating writes to them into writes to the SLB.  This is
>>> not used by any current Linux kernel, but it is used by the openbios used
>>> in the qemu mac99 model.
>>> 
>>> Commit 81762d6dd0d430d87024f2c83e9c4dcc4329fb7d, cleaning up the SLB
>>> handling introduced a bug in this code, breaking the openbios currently in
>>> qemu.  Specifically, there was an off by one error bitshuffling the
>>> register format used by mtsr into the format needed for the SLB load,
>>> causing the flag bits to end up in the wrong place.  This caused the
>>> storage keys to be wrong under openbios, meaning that the translation code
>>> incorrectly thought a legitimate access was a permission violation.
>>> 
>>> This patch fixes the bug, at the same time it fixes some build bug in the
>>> MMU debugging code (only exposed when DEBUG_MMU is enabled).
>> 
>> Thanks, applied to ppc-next :)
> 
> Nack, this does not fix ppc64 for me! How did you test it, Alex???

I booted a ppc64 kernel on a ppc64 Linux system with tcg and it booted fine for me. Maybe you're hitting yet another issue?

Alex

>
Andreas Färber - May 21, 2011, 9:39 a.m.
Am 21.05.2011 um 03:58 schrieb Alexander Graf:

> Am 21.05.2011 um 00:37 schrieb Andreas Färber  
> <andreas.faerber@web.de>:
>
>> Am 20.05.2011 um 09:40 schrieb Alexander Graf:
>>
>>> On 20.05.2011, at 05:34, David Gibson wrote:
>>>
>>>> Early ppc64 CPUs include a hack to partially simulate the ppc32  
>>>> segment
>>>> registers, by translating writes to them into writes to the SLB.   
>>>> This is
>>>> not used by any current Linux kernel, but it is used by the  
>>>> openbios used
>>>> in the qemu mac99 model.
>>>>
>>>> Commit 81762d6dd0d430d87024f2c83e9c4dcc4329fb7d, cleaning up the  
>>>> SLB
>>>> handling introduced a bug in this code, breaking the openbios  
>>>> currently in
>>>> qemu.  Specifically, there was an off by one error bitshuffling the
>>>> register format used by mtsr into the format needed for the SLB  
>>>> load,
>>>> causing the flag bits to end up in the wrong place.  This caused  
>>>> the
>>>> storage keys to be wrong under openbios, meaning that the  
>>>> translation code
>>>> incorrectly thought a legitimate access was a permission violation.
>>>>
>>>> This patch fixes the bug, at the same time it fixes some build  
>>>> bug in the
>>>> MMU debugging code (only exposed when DEBUG_MMU is enabled).
>>>
>>> Thanks, applied to ppc-next :)
>>
>> Nack, this does not fix ppc64 for me! How did you test it, Alex???
>
> I booted a ppc64 kernel on a ppc64 Linux system with tcg and it  
> booted fine for me. Maybe you're hitting yet another issue?

Which OpenBIOS did you use? The 32-bit version using mtsrin works now,  
but not the 64-bit HEAD version that I attached, which uses slb*  
instructions instead.

Andreas
Alexander Graf - May 21, 2011, 9:39 a.m.
On 21.05.2011, at 03:58, Alexander Graf wrote:

> 
> Am 21.05.2011 um 00:37 schrieb Andreas Färber <andreas.faerber@web.de>:
> 
>> Am 20.05.2011 um 09:40 schrieb Alexander Graf:
>> 
>>> On 20.05.2011, at 05:34, David Gibson wrote:
>>> 
>>>> Early ppc64 CPUs include a hack to partially simulate the ppc32 segment
>>>> registers, by translating writes to them into writes to the SLB.  This is
>>>> not used by any current Linux kernel, but it is used by the openbios used
>>>> in the qemu mac99 model.
>>>> 
>>>> Commit 81762d6dd0d430d87024f2c83e9c4dcc4329fb7d, cleaning up the SLB
>>>> handling introduced a bug in this code, breaking the openbios currently in
>>>> qemu.  Specifically, there was an off by one error bitshuffling the
>>>> register format used by mtsr into the format needed for the SLB load,
>>>> causing the flag bits to end up in the wrong place.  This caused the
>>>> storage keys to be wrong under openbios, meaning that the translation code
>>>> incorrectly thought a legitimate access was a permission violation.
>>>> 
>>>> This patch fixes the bug, at the same time it fixes some build bug in the
>>>> MMU debugging code (only exposed when DEBUG_MMU is enabled).
>>> 
>>> Thanks, applied to ppc-next :)
>> 
>> Nack, this does not fix ppc64 for me! How did you test it, Alex???
> 
> I booted a ppc64 kernel on a ppc64 Linux system with tcg and it booted fine for me. Maybe you're hitting yet another issue?

Sorry for not being overly precise; host userland was running in 32-bit.


Alex
Andreas Färber - May 21, 2011, 9:40 a.m.
Am 20.05.2011 um 09:40 schrieb Alexander Graf:

> On 20.05.2011, at 05:34, David Gibson wrote:
>
>> Early ppc64 CPUs include a hack to partially simulate the ppc32  
>> segment
>> registers, by translating writes to them into writes to the SLB.   
>> This is
>> not used by any current Linux kernel, but it is used by the  
>> openbios used
>> in the qemu mac99 model.
>>
>> Commit 81762d6dd0d430d87024f2c83e9c4dcc4329fb7d, cleaning up the SLB
>> handling introduced a bug in this code, breaking the openbios  
>> currently in
>> qemu.  Specifically, there was an off by one error bitshuffling the
>> register format used by mtsr into the format needed for the SLB load,
>> causing the flag bits to end up in the wrong place.  This caused the
>> storage keys to be wrong under openbios, meaning that the  
>> translation code
>> incorrectly thought a legitimate access was a permission violation.
>>
>> This patch fixes the bug, at the same time it fixes some build bug  
>> in the
>> MMU debugging code (only exposed when DEBUG_MMU is enabled).
>
> Thanks, applied to ppc-next :)

Hm. Don't you think we should split off the unrelated debug code fix  
for bisecting?

Andreas
Alexander Graf - May 21, 2011, 9:44 a.m.
On 21.05.2011, at 11:39, Andreas Färber wrote:

> Am 21.05.2011 um 03:58 schrieb Alexander Graf:
> 
>> Am 21.05.2011 um 00:37 schrieb Andreas Färber <andreas.faerber@web.de>:
>> 
>>> Am 20.05.2011 um 09:40 schrieb Alexander Graf:
>>> 
>>>> On 20.05.2011, at 05:34, David Gibson wrote:
>>>> 
>>>>> Early ppc64 CPUs include a hack to partially simulate the ppc32 segment
>>>>> registers, by translating writes to them into writes to the SLB.  This is
>>>>> not used by any current Linux kernel, but it is used by the openbios used
>>>>> in the qemu mac99 model.
>>>>> 
>>>>> Commit 81762d6dd0d430d87024f2c83e9c4dcc4329fb7d, cleaning up the SLB
>>>>> handling introduced a bug in this code, breaking the openbios currently in
>>>>> qemu.  Specifically, there was an off by one error bitshuffling the
>>>>> register format used by mtsr into the format needed for the SLB load,
>>>>> causing the flag bits to end up in the wrong place.  This caused the
>>>>> storage keys to be wrong under openbios, meaning that the translation code
>>>>> incorrectly thought a legitimate access was a permission violation.
>>>>> 
>>>>> This patch fixes the bug, at the same time it fixes some build bug in the
>>>>> MMU debugging code (only exposed when DEBUG_MMU is enabled).
>>>> 
>>>> Thanks, applied to ppc-next :)
>>> 
>>> Nack, this does not fix ppc64 for me! How did you test it, Alex???
>> 
>> I booted a ppc64 kernel on a ppc64 Linux system with tcg and it booted fine for me. Maybe you're hitting yet another issue?
> 
> Which OpenBIOS did you use? The 32-bit version using mtsrin works now, but not the 64-bit HEAD version that I attached, which uses slb* instructions instead.

Ah, I see. I tested the binary that's in pc-bios, so the 32-bit version obviously. So your 64-bit build regresses?


Alex
Alexander Graf - May 21, 2011, 9:46 a.m.
On 21.05.2011, at 11:40, Andreas Färber wrote:

> Am 20.05.2011 um 09:40 schrieb Alexander Graf:
> 
>> On 20.05.2011, at 05:34, David Gibson wrote:
>> 
>>> Early ppc64 CPUs include a hack to partially simulate the ppc32 segment
>>> registers, by translating writes to them into writes to the SLB.  This is
>>> not used by any current Linux kernel, but it is used by the openbios used
>>> in the qemu mac99 model.
>>> 
>>> Commit 81762d6dd0d430d87024f2c83e9c4dcc4329fb7d, cleaning up the SLB
>>> handling introduced a bug in this code, breaking the openbios currently in
>>> qemu.  Specifically, there was an off by one error bitshuffling the
>>> register format used by mtsr into the format needed for the SLB load,
>>> causing the flag bits to end up in the wrong place.  This caused the
>>> storage keys to be wrong under openbios, meaning that the translation code
>>> incorrectly thought a legitimate access was a permission violation.
>>> 
>>> This patch fixes the bug, at the same time it fixes some build bug in the
>>> MMU debugging code (only exposed when DEBUG_MMU is enabled).
>> 
>> Thanks, applied to ppc-next :)
> 
> Hm. Don't you think we should split off the unrelated debug code fix for bisecting?

Not sure if it's worth the effort. If you were bisecting before that, you probably had DEBUG_MMU disabled anyways, because you'd otherwise get build breakages before that specific commit anyways, so the commit behaves as if it's only the SLB fix.


Alex
Andreas Färber - May 21, 2011, 9:46 a.m.
Am 20.05.2011 um 05:34 schrieb David Gibson:

> Early ppc64 CPUs include a hack to partially simulate the ppc32  
> segment
> registers, by translating writes to them into writes to the SLB.   
> This is
> not used by any current Linux kernel, but it is used by the openbios  
> used
> in the qemu mac99 model.
>
> Commit 81762d6dd0d430d87024f2c83e9c4dcc4329fb7d, cleaning up the SLB
> handling introduced a bug in this code, breaking the openbios  
> currently in
> qemu.  Specifically, there was an off by one error bitshuffling the
> register format used by mtsr into the format needed for the SLB load,
> causing the flag bits to end up in the wrong place.  This caused the
> storage keys to be wrong under openbios, meaning that the  
> translation code
> incorrectly thought a legitimate access was a permission violation.
>
> This patch fixes the bug, at the same time it fixes some build bug  
> in the
> MMU debugging code (only exposed when DEBUG_MMU is enabled).
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> target-ppc/helper.c |    9 ++++-----
> 1 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
> index 4238be6..4700632 100644
> --- a/target-ppc/helper.c
> +++ b/target-ppc/helper.c

> @@ -2140,7 +2139,7 @@ void ppc_store_sr (CPUPPCState *env, int  
> srnum, target_ulong value)
>         /* VSID = VSID */
>         rs |= (value & 0xfffffff) << 12;
>         /* flags = flags */
> -        rs |= ((value >> 27) & 0xf) << 9;
> +        rs |= ((value >> 27) & 0xf) << 8;
>
>         ppc_store_slb(env, rb, rs);
>     } else

This part fixes OpenBIOS legacy ppc64 support.

Acked-by: Andreas Färber <andreas.faerber@web.de>

Andreas
David Gibson - May 21, 2011, 11:13 a.m.
On Sat, May 21, 2011 at 11:46:12AM +0200, Alexander Graf wrote:
> 
> On 21.05.2011, at 11:40, Andreas Färber wrote:
> 
> > Am 20.05.2011 um 09:40 schrieb Alexander Graf:
> > 
> >> On 20.05.2011, at 05:34, David Gibson wrote:
> >> 
> >>> Early ppc64 CPUs include a hack to partially simulate the ppc32 segment
> >>> registers, by translating writes to them into writes to the SLB.  This is
> >>> not used by any current Linux kernel, but it is used by the openbios used
> >>> in the qemu mac99 model.
> >>> 
> >>> Commit 81762d6dd0d430d87024f2c83e9c4dcc4329fb7d, cleaning up the SLB
> >>> handling introduced a bug in this code, breaking the openbios currently in
> >>> qemu.  Specifically, there was an off by one error bitshuffling the
> >>> register format used by mtsr into the format needed for the SLB load,
> >>> causing the flag bits to end up in the wrong place.  This caused the
> >>> storage keys to be wrong under openbios, meaning that the translation code
> >>> incorrectly thought a legitimate access was a permission violation.
> >>> 
> >>> This patch fixes the bug, at the same time it fixes some build bug in the
> >>> MMU debugging code (only exposed when DEBUG_MMU is enabled).
> >> 
> >> Thanks, applied to ppc-next :)
> > 
> > Hm. Don't you think we should split off the unrelated debug code fix for bisecting?
> 
> Not sure if it's worth the effort. If you were bisecting before
> that, you probably had DEBUG_MMU disabled anyways, because you'd
> otherwise get build breakages before that specific commit anyways,
> so the commit behaves as if it's only the SLB fix.

Right, I don't see how the debug fixes break bisect in any way.  Or
least not in any way they werem't already broken.
Alexander Graf - May 21, 2011, 11:22 a.m.
On 21.05.2011, at 13:13, David Gibson wrote:

> On Sat, May 21, 2011 at 11:46:12AM +0200, Alexander Graf wrote:
>> 
>> On 21.05.2011, at 11:40, Andreas Färber wrote:
>> 
>>> Am 20.05.2011 um 09:40 schrieb Alexander Graf:
>>> 
>>>> On 20.05.2011, at 05:34, David Gibson wrote:
>>>> 
>>>>> Early ppc64 CPUs include a hack to partially simulate the ppc32 segment
>>>>> registers, by translating writes to them into writes to the SLB.  This is
>>>>> not used by any current Linux kernel, but it is used by the openbios used
>>>>> in the qemu mac99 model.
>>>>> 
>>>>> Commit 81762d6dd0d430d87024f2c83e9c4dcc4329fb7d, cleaning up the SLB
>>>>> handling introduced a bug in this code, breaking the openbios currently in
>>>>> qemu.  Specifically, there was an off by one error bitshuffling the
>>>>> register format used by mtsr into the format needed for the SLB load,
>>>>> causing the flag bits to end up in the wrong place.  This caused the
>>>>> storage keys to be wrong under openbios, meaning that the translation code
>>>>> incorrectly thought a legitimate access was a permission violation.
>>>>> 
>>>>> This patch fixes the bug, at the same time it fixes some build bug in the
>>>>> MMU debugging code (only exposed when DEBUG_MMU is enabled).
>>>> 
>>>> Thanks, applied to ppc-next :)
>>> 
>>> Hm. Don't you think we should split off the unrelated debug code fix for bisecting?
>> 
>> Not sure if it's worth the effort. If you were bisecting before
>> that, you probably had DEBUG_MMU disabled anyways, because you'd
>> otherwise get build breakages before that specific commit anyways,
>> so the commit behaves as if it's only the SLB fix.
> 
> Right, I don't see how the debug fixes break bisect in any way.  Or
> least not in any way they werem't already broken.

Yup, let's just make sure to have you submit separate patches next time, so everyone is happy :)


Alex
Alexander Graf - May 21, 2011, 12:16 p.m.
On 21.05.2011, at 11:39, Andreas Färber wrote:

> Am 21.05.2011 um 03:58 schrieb Alexander Graf:
> 
>> Am 21.05.2011 um 00:37 schrieb Andreas Färber <andreas.faerber@web.de>:
>> 
>>> Am 20.05.2011 um 09:40 schrieb Alexander Graf:
>>> 
>>>> On 20.05.2011, at 05:34, David Gibson wrote:
>>>> 
>>>>> Early ppc64 CPUs include a hack to partially simulate the ppc32 segment
>>>>> registers, by translating writes to them into writes to the SLB.  This is
>>>>> not used by any current Linux kernel, but it is used by the openbios used
>>>>> in the qemu mac99 model.
>>>>> 
>>>>> Commit 81762d6dd0d430d87024f2c83e9c4dcc4329fb7d, cleaning up the SLB
>>>>> handling introduced a bug in this code, breaking the openbios currently in
>>>>> qemu.  Specifically, there was an off by one error bitshuffling the
>>>>> register format used by mtsr into the format needed for the SLB load,
>>>>> causing the flag bits to end up in the wrong place.  This caused the
>>>>> storage keys to be wrong under openbios, meaning that the translation code
>>>>> incorrectly thought a legitimate access was a permission violation.
>>>>> 
>>>>> This patch fixes the bug, at the same time it fixes some build bug in the
>>>>> MMU debugging code (only exposed when DEBUG_MMU is enabled).
>>>> 
>>>> Thanks, applied to ppc-next :)
>>> 
>>> Nack, this does not fix ppc64 for me! How did you test it, Alex???
>> 
>> I booted a ppc64 kernel on a ppc64 Linux system with tcg and it booted fine for me. Maybe you're hitting yet another issue?
> 
> Which OpenBIOS did you use? The 32-bit version using mtsrin works now, but not the 64-bit HEAD version that I attached, which uses slb* instructions instead.

After some debugging, we tracked the 64-bit issue down to a bug in OpenBIOS, so things are fine in Qemu land now :)


Alex

Patch

diff --git a/target-ppc/helper.c b/target-ppc/helper.c
index 4238be6..4700632 100644
--- a/target-ppc/helper.c
+++ b/target-ppc/helper.c
@@ -606,7 +606,7 @@  static inline int _find_pte(CPUState *env, mmu_ctx_t *ctx, int is_64b, int h,
             r = pte64_check(ctx, pte0, pte1, h, rw, type);
             LOG_MMU("Load pte from " TARGET_FMT_lx " => " TARGET_FMT_lx " "
                     TARGET_FMT_lx " %d %d %d " TARGET_FMT_lx "\n",
-                    pteg_base + (i * 16), pte0, pte1, (int)(pte0 & 1), h,
+                    pteg_off + (i * 16), pte0, pte1, (int)(pte0 & 1), h,
                     (int)((pte0 >> 1) & 1), ctx->ptem);
         } else
 #endif
@@ -621,7 +621,7 @@  static inline int _find_pte(CPUState *env, mmu_ctx_t *ctx, int is_64b, int h,
             r = pte32_check(ctx, pte0, pte1, h, rw, type);
             LOG_MMU("Load pte from " TARGET_FMT_lx " => " TARGET_FMT_lx " "
                     TARGET_FMT_lx " %d %d %d " TARGET_FMT_lx "\n",
-                    pteg_base + (i * 8), pte0, pte1, (int)(pte0 >> 31), h,
+                    pteg_off + (i * 8), pte0, pte1, (int)(pte0 >> 31), h,
                     (int)((pte0 >> 6) & 1), ctx->ptem);
         }
         switch (r) {
@@ -918,8 +918,7 @@  static inline int get_segment(CPUState *env, mmu_ctx_t *ctx,
                     if (eaddr != 0xEFFFFFFF)
                         LOG_MMU("1 htab=" TARGET_FMT_plx "/" TARGET_FMT_plx
                                 " vsid=" TARGET_FMT_lx " api=" TARGET_FMT_lx
-                                " hash=" TARGET_FMT_plx " pg_addr="
-                                TARGET_FMT_plx "\n", env->htab_base,
+                                " hash=" TARGET_FMT_plx "\n", env->htab_base,
                                 env->htab_mask, vsid, ctx->ptem, ctx->hash[1]);
                     ret2 = find_pte(env, ctx, 1, rw, type,
                                     target_page_bits);
@@ -2140,7 +2139,7 @@  void ppc_store_sr (CPUPPCState *env, int srnum, target_ulong value)
         /* VSID = VSID */
         rs |= (value & 0xfffffff) << 12;
         /* flags = flags */
-        rs |= ((value >> 27) & 0xf) << 9;
+        rs |= ((value >> 27) & 0xf) << 8;
 
         ppc_store_slb(env, rb, rs);
     } else