[Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Cédric Le Goater]

This is follow up to complete the serie "ppc: preparing pnv landing
(round 2)" plus a little fix on instruction privileges.

Tested on a POWER8 pserie guest and on mac99.

Benjamin Herrenschmidt (2):
  ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
  ppc: Better figure out if processor has HV mode

Cédric Le Goater (1):
  ppc: fix hrfid, tlbia and slbia privilege

 target-ppc/cpu.h            |  4 ++++
 target-ppc/excp_helper.c    |  8 ++++++--
 target-ppc/helper_regs.h    |  4 ++--
 target-ppc/translate.c      | 10 ++++++----
 target-ppc/translate_init.c | 19 +++++++++++++++----
 5 files changed, 33 insertions(+), 12 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/3] ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV

[Cédric Le Goater]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

This helper is only used by the various instructions that can alter
MSR and not interrupts. Add a comment to that effect to the interrupt
code as well in case somebody wants to change this

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 target-ppc/excp_helper.c | 8 ++++++--
 target-ppc/helper_regs.h | 4 ++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
index a37009eb2560..30e960e30b63 100644
--- a/target-ppc/excp_helper.c
+++ b/target-ppc/excp_helper.c
@@ -709,8 +709,12 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
         }
     }
 #endif
-    /* XXX: we don't use hreg_store_msr here as already have treated
-     *      any special case that could occur. Just store MSR and update hflags
+    /* We don't use hreg_store_msr here as already have treated
+     * any special case that could occur. Just store MSR and update hflags
+     *
+     * Note: We *MUST* not use hreg_store_msr() as-is anyway because it
+     * will prevent setting of the HV bit which some exceptions might need
+     * to do.
      */
     env->msr = new_msr & env->msr_mask;
     hreg_compute_hflags(env);
diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
index 57da931e3c4d..12af61cbf19b 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -114,8 +114,8 @@ static inline int hreg_store_msr(CPUPPCState *env, target_ulong value,
     excp = 0;
     value &= env->msr_mask;
 #if !defined(CONFIG_USER_ONLY)
-    if (!alter_hv) {
-        /* mtmsr cannot alter the hypervisor state */
+    /* Neither mtmsr nor guest state can alter HV */
+    if (!alter_hv || !(env->msr & MSR_HVB)) {
         value &= ~MSR_HVB;
         value |= env->msr & MSR_HVB;
     }
-- 
2.1.4

[Qemu-devel] [PATCH 2/3] ppc: Better figure out if processor has HV mode

[Cédric Le Goater]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

We use an env. flag which is set to the initial value of MSR_HVB in
the msr_mask. We also adjust the POWER8 mask to set SHV.

Also use this to adjust ctx.hv so that it is *set* when the processor
doesn't have an HV mode (970 with Apple mode for example), thus enabling
hypervisor instructions/SPRs.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
[clg: ctx.hv used to be defined only for the hypervisor kernel
      (HV=1|PR=0). It is now defined also when PR=1 and conditions are
      fixed accordingly.
      stripped unwanted tabs.]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 target-ppc/cpu.h            |  4 ++++
 target-ppc/translate.c      |  4 +++-
 target-ppc/translate_init.c | 19 +++++++++++++++----
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 98a24a50f3b5..d8f8f7e233c7 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1050,6 +1050,10 @@ struct CPUPPCState {
     hwaddr mpic_iack;
     /* true when the external proxy facility mode is enabled */
     bool mpic_proxy;
+    /* set when the processor has an HV mode, thus HV priv
+     * instructions and SPRs are diallowed if MSR:HV is 0
+     */
+    bool has_hv_mode;
 #endif
 
     /* Those resources are used only during code translation */
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index fe10bf8774cb..ad262523abca 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -11478,8 +11478,10 @@ void gen_intermediate_code(CPUPPCState *env, struct TranslationBlock *tb)
     ctx.exception = POWERPC_EXCP_NONE;
     ctx.spr_cb = env->spr_cb;
     ctx.pr = msr_pr;
-    ctx.hv = !msr_pr && msr_hv;
     ctx.mem_idx = env->dmmu_idx;
+#if !defined(CONFIG_USER_ONLY)
+    ctx.hv = msr_hv || !env->has_hv_mode;
+#endif
     ctx.insns_flags = env->insns_flags;
     ctx.insns_flags2 = env->insns_flags2;
     ctx.access_type = -1;
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 83010768ea05..55f855309806 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8450,6 +8450,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
                         PPC2_ISA205 | PPC2_ISA207S | PPC2_FP_CVT_S64 |
                         PPC2_TM;
     pcc->msr_mask = (1ull << MSR_SF) |
+                    (1ull << MSR_SHV) |
                     (1ull << MSR_TM) |
                     (1ull << MSR_VR) |
                     (1ull << MSR_VSX) |
@@ -9854,10 +9855,7 @@ static void ppc_cpu_reset(CPUState *s)
     pcc->parent_reset(s);
 
     msr = (target_ulong)0;
-    if (0) {
-        /* XXX: find a suitable condition to enable the hypervisor mode */
-        msr |= (target_ulong)MSR_HVB;
-    }
+    msr |= (target_ulong)MSR_HVB;
     msr |= (target_ulong)0 << MSR_AP; /* TO BE CHECKED */
     msr |= (target_ulong)0 << MSR_SA; /* TO BE CHECKED */
     msr |= (target_ulong)1 << MSR_EP;
@@ -9958,6 +9956,19 @@ static void ppc_cpu_initfn(Object *obj)
     env->bfd_mach = pcc->bfd_mach;
     env->check_pow = pcc->check_pow;
 
+    /* Mark HV mode as supported if the CPU has an MSR_HV bit
+     * in the msr_mask. The mask can later be cleared by PAPR
+     * mode but the hv mode support will remain, thus enforcing
+     * that we cannot use priv. instructions in guest in PAPR
+     * mode. For 970 we currently simply don't set HV in msr_mask
+     * thus simulating an "Apple mode" 970. If we ever want to
+     * support 970 HV mode, we'll have to add a processor attribute
+     * of some sort.
+     */
+#if !defined(CONFIG_USER_ONLY)
+    env->has_hv_mode = !!(env->msr_mask & MSR_HVB);
+#endif
+
 #if defined(TARGET_PPC64)
     if (pcc->sps) {
         env->sps = *pcc->sps;
-- 
2.1.4

[Qemu-devel] [PATCH 3/3] ppc: fix hrfid, tlbia and slbia privilege

[Cédric Le Goater]

commit 74693da98894 ('ppc: tlbie, tlbia and tlbisync are HV only')
introduced some extra checks on the instruction privilege. slbia was
changed wrongly and hrfid, tlbia were forgotten.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 target-ppc/translate.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index ad262523abca..776343170a53 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -4108,7 +4108,7 @@ static void gen_hrfid(DisasContext *ctx)
     gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
 #else
     /* Restore CPU state */
-    if (unlikely(!ctx->hv)) {
+    if (unlikely(ctx->pr || !ctx->hv)) {
         gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
         return;
     }
@@ -4845,7 +4845,7 @@ static void gen_tlbia(DisasContext *ctx)
 #if defined(CONFIG_USER_ONLY)
     gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
 #else
-    if (unlikely(ctx->pr)) {
+    if (unlikely(ctx->pr || !ctx->hv)) {
         gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
         return;
     }
@@ -4913,7 +4913,7 @@ static void gen_slbia(DisasContext *ctx)
 #if defined(CONFIG_USER_ONLY)
     gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
 #else
-    if (unlikely(ctx->pr || !ctx->hv)) {
+    if (unlikely(ctx->pr)) {
         gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
         return;
     }
-- 
2.1.4

Re: [Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Mark Cave-Ayland]

On 03/06/16 13:11, Cédric Le Goater wrote:

> This is follow up to complete the serie "ppc: preparing pnv landing
> (round 2)" plus a little fix on instruction privileges.
> 
> Tested on a POWER8 pserie guest and on mac99.
> 
> Benjamin Herrenschmidt (2):
>   ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>   ppc: Better figure out if processor has HV mode
> 
> Cédric Le Goater (1):
>   ppc: fix hrfid, tlbia and slbia privilege
> 
>  target-ppc/cpu.h            |  4 ++++
>  target-ppc/excp_helper.c    |  8 ++++++--
>  target-ppc/helper_regs.h    |  4 ++--
>  target-ppc/translate.c      | 10 ++++++----
>  target-ppc/translate_init.c | 19 +++++++++++++++----
>  5 files changed, 33 insertions(+), 12 deletions(-)

Hi Cédric,

I can confirm that this patchset fixes starting up OpenBIOS for both
g3beige/mac99 in my tests here. With the escc fix also applied, the only
outstanding issue is the removal of the tlb_flush() statements which
causes Darwin, MacOS X and HelenOS 0.60 to panic on boot

My only request is if it would be possible to move patch 2 "ppc: Better
figure out if processor has HV mode" to the front of this patchset which
will make the remaining patches bisectable for the Mac machines. With that:

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Does anyone know if Ben has any ideas as to why the MMU tlb_flush
changes patch is causing such problems?


ATB,

Mark.

Re: [Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Cédric Le Goater]

Hello Mark,

On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote:
> On 03/06/16 13:11, Cédric Le Goater wrote:
> 
>> This is follow up to complete the serie "ppc: preparing pnv landing
>> (round 2)" plus a little fix on instruction privileges.
>>
>> Tested on a POWER8 pserie guest and on mac99.
>>
>> Benjamin Herrenschmidt (2):
>>   ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>   ppc: Better figure out if processor has HV mode
>>
>> Cédric Le Goater (1):
>>   ppc: fix hrfid, tlbia and slbia privilege
>>
>>  target-ppc/cpu.h            |  4 ++++
>>  target-ppc/excp_helper.c    |  8 ++++++--
>>  target-ppc/helper_regs.h    |  4 ++--
>>  target-ppc/translate.c      | 10 ++++++----
>>  target-ppc/translate_init.c | 19 +++++++++++++++----
>>  5 files changed, 33 insertions(+), 12 deletions(-)
> 
> Hi Cédric,
> 
> I can confirm that this patchset fixes starting up OpenBIOS for both
> g3beige/mac99 in my tests here. With the escc fix also applied, the only
> outstanding issue is the removal of the tlb_flush() statements which
> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot

Is that just booting the CDROM or the complete OS ? because I tried that a 
couple of time with ppc-for-2.7-20160531 + the three patches above and 
did not see the issue again. I reached the device selection prompt. 

I must be doing something wrong. 

> My only request is if it would be possible to move patch 2 "ppc: Better
> figure out if processor has HV mode" to the front of this patchset which
> will make the remaining patches bisectable for the Mac machines. With that:
> 
> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Thanks,

C. 

> Does anyone know if Ben has any ideas as to why the MMU tlb_flush
> changes patch is causing such problems?
> 
> 
> ATB,
> 
> Mark.
> 

Re: [Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Mark Cave-Ayland]

On 03/06/16 15:00, Cédric Le Goater wrote:

> Hello Mark,
> 
> On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote:
>> On 03/06/16 13:11, Cédric Le Goater wrote:
>>
>>> This is follow up to complete the serie "ppc: preparing pnv landing
>>> (round 2)" plus a little fix on instruction privileges.
>>>
>>> Tested on a POWER8 pserie guest and on mac99.
>>>
>>> Benjamin Herrenschmidt (2):
>>>   ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>>   ppc: Better figure out if processor has HV mode
>>>
>>> Cédric Le Goater (1):
>>>   ppc: fix hrfid, tlbia and slbia privilege
>>>
>>>  target-ppc/cpu.h            |  4 ++++
>>>  target-ppc/excp_helper.c    |  8 ++++++--
>>>  target-ppc/helper_regs.h    |  4 ++--
>>>  target-ppc/translate.c      | 10 ++++++----
>>>  target-ppc/translate_init.c | 19 +++++++++++++++----
>>>  5 files changed, 33 insertions(+), 12 deletions(-)
>>
>> Hi Cédric,
>>
>> I can confirm that this patchset fixes starting up OpenBIOS for both
>> g3beige/mac99 in my tests here. With the escc fix also applied, the only
>> outstanding issue is the removal of the tlb_flush() statements which
>> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot
> 
> Is that just booting the CDROM or the complete OS ? because I tried that a 
> couple of time with ppc-for-2.7-20160531 + the three patches above and 
> did not see the issue again. I reached the device selection prompt. 
> 
> I must be doing something wrong. 

Hmmm not necessarily. It seems that the Darwin crash only manifests
itself on -M g3beige (the default) rather than -M mac99, although I can
confirm that HelenOS crashes on both -M g3beige and -M mac99:

./qemu-system-ppc -cdrom HelenOS-0.6.0-ppc32.iso -boot d -bios -m 256 -M
mac99

>> My only request is if it would be possible to move patch 2 "ppc: Better
>> figure out if processor has HV mode" to the front of this patchset which
>> will make the remaining patches bisectable for the Mac machines. With that:
>>
>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Thanks,
> 
> C. 

ATB,

Mark.

Re: [Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Cedric Le Goater]

On 06/03/2016 04:00 PM, Cédric Le Goater wrote:
> Hello Mark,
> 
> On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote:
>> On 03/06/16 13:11, Cédric Le Goater wrote:
>>
>>> This is follow up to complete the serie "ppc: preparing pnv landing
>>> (round 2)" plus a little fix on instruction privileges.
>>>
>>> Tested on a POWER8 pserie guest and on mac99.
>>>
>>> Benjamin Herrenschmidt (2):
>>>   ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>>   ppc: Better figure out if processor has HV mode
>>>
>>> Cédric Le Goater (1):
>>>   ppc: fix hrfid, tlbia and slbia privilege
>>>
>>>  target-ppc/cpu.h            |  4 ++++
>>>  target-ppc/excp_helper.c    |  8 ++++++--
>>>  target-ppc/helper_regs.h    |  4 ++--
>>>  target-ppc/translate.c      | 10 ++++++----
>>>  target-ppc/translate_init.c | 19 +++++++++++++++----
>>>  5 files changed, 33 insertions(+), 12 deletions(-)
>>
>> Hi Cédric,
>>
>> I can confirm that this patchset fixes starting up OpenBIOS for both
>> g3beige/mac99 in my tests here. With the escc fix also applied, the only
>> outstanding issue is the removal of the tlb_flush() statements which
>> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot
> 
> Is that just booting the CDROM or the complete OS ? because I tried that a 
> couple of time with ppc-for-2.7-20160531 + the three patches above and 
> did not see the issue again. I reached the device selection prompt. 
> 
> I must be doing something wrong. 

I was using '-cpu 750' for some reason and this is why it succeeded. It fails
without. We are getting close. 

C.

>> My only request is if it would be possible to move patch 2 "ppc: Better
>> figure out if processor has HV mode" to the front of this patchset which
>> will make the remaining patches bisectable for the Mac machines. With that:
>>
>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Thanks,
> 
> C. 
> 
>> Does anyone know if Ben has any ideas as to why the MMU tlb_flush
>> changes patch is causing such problems?
>>
>>
>> ATB,
>>
>> Mark.
>>
> 

Re: [Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Mark Cave-Ayland]

On 03/06/16 15:06, Cedric Le Goater wrote:

> On 06/03/2016 04:00 PM, Cédric Le Goater wrote:
>> Hello Mark,
>>
>> On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote:
>>> On 03/06/16 13:11, Cédric Le Goater wrote:
>>>
>>>> This is follow up to complete the serie "ppc: preparing pnv landing
>>>> (round 2)" plus a little fix on instruction privileges.
>>>>
>>>> Tested on a POWER8 pserie guest and on mac99.
>>>>
>>>> Benjamin Herrenschmidt (2):
>>>>   ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>>>   ppc: Better figure out if processor has HV mode
>>>>
>>>> Cédric Le Goater (1):
>>>>   ppc: fix hrfid, tlbia and slbia privilege
>>>>
>>>>  target-ppc/cpu.h            |  4 ++++
>>>>  target-ppc/excp_helper.c    |  8 ++++++--
>>>>  target-ppc/helper_regs.h    |  4 ++--
>>>>  target-ppc/translate.c      | 10 ++++++----
>>>>  target-ppc/translate_init.c | 19 +++++++++++++++----
>>>>  5 files changed, 33 insertions(+), 12 deletions(-)
>>>
>>> Hi Cédric,
>>>
>>> I can confirm that this patchset fixes starting up OpenBIOS for both
>>> g3beige/mac99 in my tests here. With the escc fix also applied, the only
>>> outstanding issue is the removal of the tlb_flush() statements which
>>> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot
>>
>> Is that just booting the CDROM or the complete OS ? because I tried that a 
>> couple of time with ppc-for-2.7-20160531 + the three patches above and 
>> did not see the issue again. I reached the device selection prompt. 
>>
>> I must be doing something wrong. 
> 
> I was using '-cpu 750' for some reason and this is why it succeeded. It fails
> without. We are getting close. 

Hmmm that works for -M g3beige Darwin, but not HelenOS here. Although
interestingly -M g3beige -m 256 seems to "fix" Darwin here too
(presumably because the memory allocation routines can just allocate new
RAM rather than reusing existing RAM which may be cached in the TLB).

What a strange coincidence that I've just posted a patch that fixes up
the debugging in target-ppc/mmu_helper.c ;)


ATB,

Mark.

Re: [Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Mark Cave-Ayland]

On 03/06/16 15:14, Mark Cave-Ayland wrote:

> On 03/06/16 15:06, Cedric Le Goater wrote:
> 
>> On 06/03/2016 04:00 PM, Cédric Le Goater wrote:
>>> Hello Mark,
>>>
>>> On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote:
>>>> On 03/06/16 13:11, Cédric Le Goater wrote:
>>>>
>>>>> This is follow up to complete the serie "ppc: preparing pnv landing
>>>>> (round 2)" plus a little fix on instruction privileges.
>>>>>
>>>>> Tested on a POWER8 pserie guest and on mac99.
>>>>>
>>>>> Benjamin Herrenschmidt (2):
>>>>>   ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>>>>   ppc: Better figure out if processor has HV mode
>>>>>
>>>>> Cédric Le Goater (1):
>>>>>   ppc: fix hrfid, tlbia and slbia privilege
>>>>>
>>>>>  target-ppc/cpu.h            |  4 ++++
>>>>>  target-ppc/excp_helper.c    |  8 ++++++--
>>>>>  target-ppc/helper_regs.h    |  4 ++--
>>>>>  target-ppc/translate.c      | 10 ++++++----
>>>>>  target-ppc/translate_init.c | 19 +++++++++++++++----
>>>>>  5 files changed, 33 insertions(+), 12 deletions(-)
>>>>
>>>> Hi Cédric,
>>>>
>>>> I can confirm that this patchset fixes starting up OpenBIOS for both
>>>> g3beige/mac99 in my tests here. With the escc fix also applied, the only
>>>> outstanding issue is the removal of the tlb_flush() statements which
>>>> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot
>>>
>>> Is that just booting the CDROM or the complete OS ? because I tried that a 
>>> couple of time with ppc-for-2.7-20160531 + the three patches above and 
>>> did not see the issue again. I reached the device selection prompt. 
>>>
>>> I must be doing something wrong. 
>>
>> I was using '-cpu 750' for some reason and this is why it succeeded. It fails
>> without. We are getting close. 
> 
> Hmmm that works for -M g3beige Darwin, but not HelenOS here. Although
> interestingly -M g3beige -m 256 seems to "fix" Darwin here too
> (presumably because the memory allocation routines can just allocate new
> RAM rather than reusing existing RAM which may be cached in the TLB).
> 
> What a strange coincidence that I've just posted a patch that fixes up
> the debugging in target-ppc/mmu_helper.c ;)

It also looks like you need my beta patch to convert the macio
controller over to using the DMA helpers here:
https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg04907.html.
At least that seems to progress things a little further on one of my
MacOS tests.

Looking at the DBDMA code I still see a few calls to
cpu_physical_memory_read() / cpu_physical_memory_write() scattered
around. Do these need to be switched over to dma_memory_read() /
dma_memory_write() in order to correctly invalidate the TLB upon write?


ATB,

Mark.

Re: [Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Cédric Le Goater]

On 06/03/2016 05:47 PM, Mark Cave-Ayland wrote:
> On 03/06/16 15:14, Mark Cave-Ayland wrote:
> 
>> On 03/06/16 15:06, Cedric Le Goater wrote:
>>
>>> On 06/03/2016 04:00 PM, Cédric Le Goater wrote:
>>>> Hello Mark,
>>>>
>>>> On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote:
>>>>> On 03/06/16 13:11, Cédric Le Goater wrote:
>>>>>
>>>>>> This is follow up to complete the serie "ppc: preparing pnv landing
>>>>>> (round 2)" plus a little fix on instruction privileges.
>>>>>>
>>>>>> Tested on a POWER8 pserie guest and on mac99.
>>>>>>
>>>>>> Benjamin Herrenschmidt (2):
>>>>>>   ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>>>>>   ppc: Better figure out if processor has HV mode
>>>>>>
>>>>>> Cédric Le Goater (1):
>>>>>>   ppc: fix hrfid, tlbia and slbia privilege
>>>>>>
>>>>>>  target-ppc/cpu.h            |  4 ++++
>>>>>>  target-ppc/excp_helper.c    |  8 ++++++--
>>>>>>  target-ppc/helper_regs.h    |  4 ++--
>>>>>>  target-ppc/translate.c      | 10 ++++++----
>>>>>>  target-ppc/translate_init.c | 19 +++++++++++++++----
>>>>>>  5 files changed, 33 insertions(+), 12 deletions(-)
>>>>>
>>>>> Hi Cédric,
>>>>>
>>>>> I can confirm that this patchset fixes starting up OpenBIOS for both
>>>>> g3beige/mac99 in my tests here. With the escc fix also applied, the only
>>>>> outstanding issue is the removal of the tlb_flush() statements which
>>>>> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot
>>>>
>>>> Is that just booting the CDROM or the complete OS ? because I tried that a 
>>>> couple of time with ppc-for-2.7-20160531 + the three patches above and 
>>>> did not see the issue again. I reached the device selection prompt. 
>>>>
>>>> I must be doing something wrong. 
>>>
>>> I was using '-cpu 750' for some reason and this is why it succeeded. It fails
>>> without. We are getting close. 
>>
>> Hmmm that works for -M g3beige Darwin, but not HelenOS here. Although
>> interestingly -M g3beige -m 256 seems to "fix" Darwin here too
>> (presumably because the memory allocation routines can just allocate new
>> RAM rather than reusing existing RAM which may be cached in the TLB).
>>
>> What a strange coincidence that I've just posted a patch that fixes up
>> the debugging in target-ppc/mmu_helper.c ;)
> 
> It also looks like you need my beta patch to convert the macio
> controller over to using the DMA helpers here:
> https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg04907.html.
> At least that seems to progress things a little further on one of my
> MacOS tests.
> 
> Looking at the DBDMA code I still see a few calls to
> cpu_physical_memory_read() / cpu_physical_memory_write() scattered
> around. Do these need to be switched over to dma_memory_read() /
> dma_memory_write() in order to correctly invalidate the TLB upon write?

I haven't had time to check your patches yet. I will give them a try in a 
couple of days. 

Thanks,

C. 

Re: [Qemu-devel] [PATCH 3/3] ppc: fix hrfid, tlbia and slbia privilege

[Thomas Huth]

On 03.06.2016 14:11, Cédric Le Goater wrote:
> commit 74693da98894 ('ppc: tlbie, tlbia and tlbisync are HV only')
> introduced some extra checks on the instruction privilege. slbia was
> changed wrongly and hrfid, tlbia were forgotten.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  target-ppc/translate.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index ad262523abca..776343170a53 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -4108,7 +4108,7 @@ static void gen_hrfid(DisasContext *ctx)
>      gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>  #else
>      /* Restore CPU state */
> -    if (unlikely(!ctx->hv)) {
> +    if (unlikely(ctx->pr || !ctx->hv)) {
>          gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>          return;
>      }
> @@ -4845,7 +4845,7 @@ static void gen_tlbia(DisasContext *ctx)
>  #if defined(CONFIG_USER_ONLY)
>      gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>  #else
> -    if (unlikely(ctx->pr)) {
> +    if (unlikely(ctx->pr || !ctx->hv)) {
>          gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>          return;
>      }
> @@ -4913,7 +4913,7 @@ static void gen_slbia(DisasContext *ctx)
>  #if defined(CONFIG_USER_ONLY)
>      gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>  #else
> -    if (unlikely(ctx->pr || !ctx->hv)) {
> +    if (unlikely(ctx->pr)) {
>          gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>          return;
>      }

I just double-checked the PowerISA 2.07, and you're right, hrfid and
tlbia are hypervisor-privileged, slbia is only "normal" privileged.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Cédric Le Goater]

Hello Mark,

On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote:
> On 03/06/16 13:11, Cédric Le Goater wrote:
> 
>> This is follow up to complete the serie "ppc: preparing pnv landing
>> (round 2)" plus a little fix on instruction privileges.
>>
>> Tested on a POWER8 pserie guest and on mac99.
>>
>> Benjamin Herrenschmidt (2):
>>   ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>   ppc: Better figure out if processor has HV mode
>>
>> Cédric Le Goater (1):
>>   ppc: fix hrfid, tlbia and slbia privilege
>>
>>  target-ppc/cpu.h            |  4 ++++
>>  target-ppc/excp_helper.c    |  8 ++++++--
>>  target-ppc/helper_regs.h    |  4 ++--
>>  target-ppc/translate.c      | 10 ++++++----
>>  target-ppc/translate_init.c | 19 +++++++++++++++----
>>  5 files changed, 33 insertions(+), 12 deletions(-)
> 
> Hi Cédric,
> 
> I can confirm that this patchset fixes starting up OpenBIOS for both
> g3beige/mac99 in my tests here. With the escc fix also applied, the only
> outstanding issue is the removal of the tlb_flush() statements which
> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot
> 
> My only request is if it would be possible to move patch 2 "ppc: Better
> figure out if processor has HV mode" to the front of this patchset which
> will make the remaining patches bisectable for the Mac machines. With that:
> 
> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Does anyone know if Ben has any ideas as to why the MMU tlb_flush
> changes patch is causing such problems?


Here is a fix I think. Could you give it a try ? 

Cheers,

C. 

From: Cédric Le Goater <clg@kaod.org>
Subject: [PATCH] ppc: fix tlb flushes for 32bit environment
Date: Sun, 05 Jun 2016 18:46:19 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes')
introduced an optimisation to flush TLBs only when a context
synchronizing event is reached (interrupt, rfi). This was done for
ppc64 but 32bit was forgotten on the way.

Tested on mac99 and g3beige with

    qemu-system-ppc -cdrom darwinppc-602.cdr -boot d

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---

 I think the hunk in powerpc_excp() is needed if we don't generate a
 context synchronizing event. what is best to do ?

 target-ppc/cpu.h         |    2 +-
 target-ppc/excp_helper.c |   10 ++++++++++
 target-ppc/helper_regs.h |    9 ++++++++-
 target-ppc/translate.c   |    2 +-
 4 files changed, 20 insertions(+), 3 deletions(-)

Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c
===================================================================
--- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c
+++ qemu-dgibson-for-2.7.git/target-ppc/translate.c
@@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx)
 {
 }
 
-#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
+#if !defined(CONFIG_USER_ONLY)
 static inline void gen_check_tlb_flush(DisasContext *ctx)
 {
     TCGv_i32 t = tcg_temp_new_i32();
Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h
===================================================================
--- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h
+++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h
@@ -958,9 +958,9 @@ struct CPUPPCState {
     /* PowerPC 64 SLB area */
     ppc_slb_t slb[MAX_SLB_ENTRIES];
     int32_t slb_nr;
+#endif
     /* tcg TLB needs flush (deferred slb inval instruction typically) */
     uint32_t tlb_need_flush;
-#endif
     /* segment registers */
     hwaddr htab_base;
     /* mask used to normalize hash value to PTEG index */
Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
===================================================================
--- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h
+++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
@@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS
     }
     if (((value >> MSR_IR) & 1) != msr_ir ||
         ((value >> MSR_DR) & 1) != msr_dr) {
+        /* A change of the instruction relocation bit in the MSR can
+         * cause an implicit branch in the address space. This
+         * requires a tlb flush.
+         */
+        if (env->mmu_model & POWERPC_MMU_32B) {
+            env->tlb_need_flush = 1;
+        }
         cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
     }
     if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
@@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS
     return excp;
 }
 
-#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
+#if !defined(CONFIG_USER_ONLY)
 static inline void check_tlb_flush(CPUPPCState *env)
 {
     CPUState *cs = CPU(ppc_env_get_cpu(env));
Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
===================================================================
--- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c
+++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
@@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC
         }
     }
 #endif
+    if (((new_msr >> MSR_IR) & 1) != msr_ir ||
+        ((new_msr >> MSR_DR) & 1) != msr_dr) {
+        /* A change of the instruction relocation bit in the MSR can
+         * cause an implicit branch in the address space. This
+         * requires a tlb flush.
+         */
+        if (env->mmu_model & POWERPC_MMU_32B) {
+            env->tlb_need_flush = 1;
+        }
+    }
     /* We don't use hreg_store_msr here as already have treated
      * any special case that could occur. Just store MSR and update hflags
      *

Re: [Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Mark Cave-Ayland]

On 05/06/16 18:41, Cédric Le Goater wrote:

> Hello Mark,
> 
> On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote:
>> On 03/06/16 13:11, Cédric Le Goater wrote:
>>
>>> This is follow up to complete the serie "ppc: preparing pnv landing
>>> (round 2)" plus a little fix on instruction privileges.
>>>
>>> Tested on a POWER8 pserie guest and on mac99.
>>>
>>> Benjamin Herrenschmidt (2):
>>>   ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>>   ppc: Better figure out if processor has HV mode
>>>
>>> Cédric Le Goater (1):
>>>   ppc: fix hrfid, tlbia and slbia privilege
>>>
>>>  target-ppc/cpu.h            |  4 ++++
>>>  target-ppc/excp_helper.c    |  8 ++++++--
>>>  target-ppc/helper_regs.h    |  4 ++--
>>>  target-ppc/translate.c      | 10 ++++++----
>>>  target-ppc/translate_init.c | 19 +++++++++++++++----
>>>  5 files changed, 33 insertions(+), 12 deletions(-)
>>
>> Hi Cédric,
>>
>> I can confirm that this patchset fixes starting up OpenBIOS for both
>> g3beige/mac99 in my tests here. With the escc fix also applied, the only
>> outstanding issue is the removal of the tlb_flush() statements which
>> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot
>>
>> My only request is if it would be possible to move patch 2 "ppc: Better
>> figure out if processor has HV mode" to the front of this patchset which
>> will make the remaining patches bisectable for the Mac machines. With that:
>>
>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>> Does anyone know if Ben has any ideas as to why the MMU tlb_flush
>> changes patch is causing such problems?
> 
> 
> Here is a fix I think. Could you give it a try ? 
> 
> Cheers,
> 
> C. 
> 
> From: Cédric Le Goater <clg@kaod.org>
> Subject: [PATCH] ppc: fix tlb flushes for 32bit environment
> Date: Sun, 05 Jun 2016 18:46:19 +0200
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes')
> introduced an optimisation to flush TLBs only when a context
> synchronizing event is reached (interrupt, rfi). This was done for
> ppc64 but 32bit was forgotten on the way.
> 
> Tested on mac99 and g3beige with
> 
>     qemu-system-ppc -cdrom darwinppc-602.cdr -boot d
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
>  I think the hunk in powerpc_excp() is needed if we don't generate a
>  context synchronizing event. what is best to do ?
> 
>  target-ppc/cpu.h         |    2 +-
>  target-ppc/excp_helper.c |   10 ++++++++++
>  target-ppc/helper_regs.h |    9 ++++++++-
>  target-ppc/translate.c   |    2 +-
>  4 files changed, 20 insertions(+), 3 deletions(-)
> 
> Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c
> ===================================================================
> --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c
> +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c
> @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx)
>  {
>  }
>  
> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
> +#if !defined(CONFIG_USER_ONLY)
>  static inline void gen_check_tlb_flush(DisasContext *ctx)
>  {
>      TCGv_i32 t = tcg_temp_new_i32();
> Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h
> ===================================================================
> --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h
> +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h
> @@ -958,9 +958,9 @@ struct CPUPPCState {
>      /* PowerPC 64 SLB area */
>      ppc_slb_t slb[MAX_SLB_ENTRIES];
>      int32_t slb_nr;
> +#endif
>      /* tcg TLB needs flush (deferred slb inval instruction typically) */
>      uint32_t tlb_need_flush;
> -#endif
>      /* segment registers */
>      hwaddr htab_base;
>      /* mask used to normalize hash value to PTEG index */
> Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
> ===================================================================
> --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h
> +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
> @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS
>      }
>      if (((value >> MSR_IR) & 1) != msr_ir ||
>          ((value >> MSR_DR) & 1) != msr_dr) {
> +        /* A change of the instruction relocation bit in the MSR can
> +         * cause an implicit branch in the address space. This
> +         * requires a tlb flush.
> +         */
> +        if (env->mmu_model & POWERPC_MMU_32B) {
> +            env->tlb_need_flush = 1;
> +        }
>          cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>      }
>      if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
> @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS
>      return excp;
>  }
>  
> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
> +#if !defined(CONFIG_USER_ONLY)
>  static inline void check_tlb_flush(CPUPPCState *env)
>  {
>      CPUState *cs = CPU(ppc_env_get_cpu(env));
> Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
> ===================================================================
> --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c
> +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
> @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC
>          }
>      }
>  #endif
> +    if (((new_msr >> MSR_IR) & 1) != msr_ir ||
> +        ((new_msr >> MSR_DR) & 1) != msr_dr) {
> +        /* A change of the instruction relocation bit in the MSR can
> +         * cause an implicit branch in the address space. This
> +         * requires a tlb flush.
> +         */
> +        if (env->mmu_model & POWERPC_MMU_32B) {
> +            env->tlb_need_flush = 1;
> +        }
> +    }
>      /* We don't use hreg_store_msr here as already have treated
>       * any special case that could occur. Just store MSR and update hflags
>       *
> 
> 

Hi Cédric,

I've just tried this patch on a selection of images here with both
g3beige and mac99 and as far as I can tell the crash has now gone away:

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Thanks for getting this sorted so quickly.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 3/3] ppc: fix hrfid, tlbia and slbia privilege

[David Gibson]

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

On Sat, Jun 04, 2016 at 10:24:28AM +0200, Thomas Huth wrote:
> On 03.06.2016 14:11, Cédric Le Goater wrote:
> > commit 74693da98894 ('ppc: tlbie, tlbia and tlbisync are HV only')
> > introduced some extra checks on the instruction privilege. slbia was
> > changed wrongly and hrfid, tlbia were forgotten.
> > 
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >  target-ppc/translate.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> > index ad262523abca..776343170a53 100644
> > --- a/target-ppc/translate.c
> > +++ b/target-ppc/translate.c
> > @@ -4108,7 +4108,7 @@ static void gen_hrfid(DisasContext *ctx)
> >      gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >  #else
> >      /* Restore CPU state */
> > -    if (unlikely(!ctx->hv)) {
> > +    if (unlikely(ctx->pr || !ctx->hv)) {
> >          gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >          return;
> >      }
> > @@ -4845,7 +4845,7 @@ static void gen_tlbia(DisasContext *ctx)
> >  #if defined(CONFIG_USER_ONLY)
> >      gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >  #else
> > -    if (unlikely(ctx->pr)) {
> > +    if (unlikely(ctx->pr || !ctx->hv)) {
> >          gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >          return;
> >      }
> > @@ -4913,7 +4913,7 @@ static void gen_slbia(DisasContext *ctx)
> >  #if defined(CONFIG_USER_ONLY)
> >      gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >  #else
> > -    if (unlikely(ctx->pr || !ctx->hv)) {
> > +    if (unlikely(ctx->pr)) {
> >          gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >          return;
> >      }
> 
> I just double-checked the PowerISA 2.07, and you're right, hrfid and
> tlbia are hypervisor-privileged, slbia is only "normal" privileged.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Yes, the SLB is owned by the guest - otherwise it would need
hypercalls on every context switch.  Should have caught this the first
time around, sorry.

-- 
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 0/3] ppc: complete the new HV mode

[David Gibson]

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

On Fri, Jun 03, 2016 at 02:11:17PM +0200, Cédric Le Goater wrote:
> This is follow up to complete the serie "ppc: preparing pnv landing
> (round 2)" plus a little fix on instruction privileges.
> 
> Tested on a POWER8 pserie guest and on mac99.
> 
> Benjamin Herrenschmidt (2):
>   ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>   ppc: Better figure out if processor has HV mode
> 
> Cédric Le Goater (1):
>   ppc: fix hrfid, tlbia and slbia privilege

I've applied this series to ppc-for-2.7, swapping the order of patches
1 & 2 as Marc requested.  I've also applied the extra patch to fix tlb
flushing on 32-bit cpus.  I aim to send a pull request once I've done
my usual tests.

-- 
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 0/3] ppc: complete the new HV mode

[David Gibson]

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

On Sun, Jun 05, 2016 at 07:41:50PM +0200, Cédric Le Goater wrote:
> Hello Mark,
> 
> On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote:
> > On 03/06/16 13:11, Cédric Le Goater wrote:
> > 
> >> This is follow up to complete the serie "ppc: preparing pnv landing
> >> (round 2)" plus a little fix on instruction privileges.
> >>
> >> Tested on a POWER8 pserie guest and on mac99.
> >>
> >> Benjamin Herrenschmidt (2):
> >>   ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
> >>   ppc: Better figure out if processor has HV mode
> >>
> >> Cédric Le Goater (1):
> >>   ppc: fix hrfid, tlbia and slbia privilege
> >>
> >>  target-ppc/cpu.h            |  4 ++++
> >>  target-ppc/excp_helper.c    |  8 ++++++--
> >>  target-ppc/helper_regs.h    |  4 ++--
> >>  target-ppc/translate.c      | 10 ++++++----
> >>  target-ppc/translate_init.c | 19 +++++++++++++++----
> >>  5 files changed, 33 insertions(+), 12 deletions(-)
> > 
> > Hi Cédric,
> > 
> > I can confirm that this patchset fixes starting up OpenBIOS for both
> > g3beige/mac99 in my tests here. With the escc fix also applied, the only
> > outstanding issue is the removal of the tlb_flush() statements which
> > causes Darwin, MacOS X and HelenOS 0.60 to panic on boot
> > 
> > My only request is if it would be possible to move patch 2 "ppc: Better
> > figure out if processor has HV mode" to the front of this patchset which
> > will make the remaining patches bisectable for the Mac machines. With that:
> > 
> > Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > 
> > Does anyone know if Ben has any ideas as to why the MMU tlb_flush
> > changes patch is causing such problems?
> 
> 
> Here is a fix I think. Could you give it a try ? 

So, I had applied this to ppc-for-2.7, but I've now removed it again.
BenH correctly pointed out that it basically just removes any benefit
of his original tlb_flush() patch, in a slightly more subtle way that
the last "fix".  You just set the need_flush flag whenever IR or DR
are changed, whereas the whole point of BenH's patch is that the
translation on and off modes are now different MM contexts, which
should be flagged in qemu's TLB and so not require a full flush.  We
need to work out what the real problem is here.

-- 
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 0/3] ppc: complete the new HV mode

[Benjamin Herrenschmidt]

On Mon, 2016-06-06 at 11:17 +1000, David Gibson wrote:
> On Fri, Jun 03, 2016 at 02:11:17PM +0200, Cédric Le Goater wrote:
> > 
> > This is follow up to complete the serie "ppc: preparing pnv landing
> > (round 2)" plus a little fix on instruction privileges.
> > 
> > Tested on a POWER8 pserie guest and on mac99.
> > 
> > Benjamin Herrenschmidt (2):
> >   ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
> >   ppc: Better figure out if processor has HV mode
> > 
> > Cédric Le Goater (1):
> >   ppc: fix hrfid, tlbia and slbia privilege
> I've applied this series to ppc-for-2.7, swapping the order of patches
> 1 & 2 as Marc requested.  I've also applied the extra patch to fix tlb
> flushing on 32-bit cpus.  I aim to send a pull request once I've done
> my usual tests.
> 
I'm not sure that 32-bit patch is correct. We shouldn't have to flush
on IR/DR transitions at all, that's the whole point of the split I/D
code.

I think something else is wrong.

Cheers,
Ben.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/3] ppc: complete the new HV mode

[Benjamin Herrenschmidt]

On Sun, 2016-06-05 at 19:41 +0200, Cédric Le Goater wrote:
> 
> Here is a fix I think. Could you give it a try ? 

This is somewhat wrong...

> commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes')
> introduced an optimisation to flush TLBs only when a context
> synchronizing event is reached (interrupt, rfi). This was done for
> ppc64 but 32bit was forgotten on the way.

No it didn't. That commit only delays flushes on ppc64. ppc32 is
unaffected, unless I missed something. IE. It will delay flushes caused
by slb instructions (which don't exist on 32-bit)
and ppc_tlb_invalidate_one() only in the 64-bit cases.

Also what your patch does in practice is not really change that, though
you seem to try to somewhat extend the batching to 32-bit (but
incompletely), you also introduce something which effectively reverts
part of 9fb044911444fdd09f5f072ad0ca269d7f8b841d (split I/D mode).

I think that's more what's "fixing" your problem, ie, the flush in
IR/DR changes. However it shouldn't be needed.

I suspect all of that is papering over another bug somewhere else which
got exposed by the split I/D mode, since we no longer over-flush on
transitions to/from real-mode. So we must be missing flushes elsewhere,
possibly some G3 specific stuff, or there always was some kind of bug
in the TLB flushing on 32-bit that got somewhat masked by the over-
flushing we used to do.

I need a repro-case.

Cheers,
Ben.

> Tested on mac99 and g3beige with
> 
>     qemu-system-ppc -cdrom darwinppc-602.cdr -boot d
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
>  I think the hunk in powerpc_excp() is needed if we don't generate a
>  context synchronizing event. what is best to do ?
> 
>  target-ppc/cpu.h         |    2 +-
>  target-ppc/excp_helper.c |   10 ++++++++++
>  target-ppc/helper_regs.h |    9 ++++++++-
>  target-ppc/translate.c   |    2 +-
>  4 files changed, 20 insertions(+), 3 deletions(-)
> 
> Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c
> ===================================================================
> --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c
> +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c
> @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx)
>  {
>  }
>  
> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
> +#if !defined(CONFIG_USER_ONLY)
>  static inline void gen_check_tlb_flush(DisasContext *ctx)
>  {
>      TCGv_i32 t = tcg_temp_new_i32();
> Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h
> ===================================================================
> --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h
> +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h
> @@ -958,9 +958,9 @@ struct CPUPPCState {
>      /* PowerPC 64 SLB area */
>      ppc_slb_t slb[MAX_SLB_ENTRIES];
>      int32_t slb_nr;
> +#endif
>      /* tcg TLB needs flush (deferred slb inval instruction
> typically) */
>      uint32_t tlb_need_flush;
> -#endif
>      /* segment registers */
>      hwaddr htab_base;
>      /* mask used to normalize hash value to PTEG index */
> Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
> ===================================================================
> --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h
> +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
> @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS
>      }
>      if (((value >> MSR_IR) & 1) != msr_ir ||
>          ((value >> MSR_DR) & 1) != msr_dr) {
> +        /* A change of the instruction relocation bit in the MSR can
> +         * cause an implicit branch in the address space. This
> +         * requires a tlb flush.
> +         */
> +        if (env->mmu_model & POWERPC_MMU_32B) {
> +            env->tlb_need_flush = 1;
> +        }
>          cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>      }
>      if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
> @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS
>      return excp;
>  }
>  
> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
> +#if !defined(CONFIG_USER_ONLY)
>  static inline void check_tlb_flush(CPUPPCState *env)
>  {
>      CPUState *cs = CPU(ppc_env_get_cpu(env));
> Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
> ===================================================================
> --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c
> +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
> @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC
>          }
>      }
>  #endif
> +    if (((new_msr >> MSR_IR) & 1) != msr_ir ||
> +        ((new_msr >> MSR_DR) & 1) != msr_dr) {
> +        /* A change of the instruction relocation bit in the MSR can
> +         * cause an implicit branch in the address space. This
> +         * requires a tlb flush.
> +         */
> +        if (env->mmu_model & POWERPC_MMU_32B) {
> +            env->tlb_need_flush = 1;
> +        }
> +    }
>      /* We don't use hreg_store_msr here as already have treated
>       * any special case that could occur. Just store MSR and update
> hflags
>       *
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/3] ppc: complete the new HV mode

[Benjamin Herrenschmidt]

On Mon, 2016-06-06 at 13:55 +1000, Benjamin Herrenschmidt wrote:
> 
> I'm not sure that 32-bit patch is correct. We shouldn't have to flush
> on IR/DR transitions at all, that's the whole point of the split I/D
> code.
> 
> I think something else is wrong.

Note: With whatever's in this branch, OpenBIOS won't start at all

Cheers,
Ben.

Re: [Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Cédric Le Goater]

On 06/06/2016 12:26 AM, Mark Cave-Ayland wrote:
> On 05/06/16 18:41, Cédric Le Goater wrote:
> 
>> Hello Mark,
>>
>> On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote:
>>> On 03/06/16 13:11, Cédric Le Goater wrote:
>>>
>>>> This is follow up to complete the serie "ppc: preparing pnv landing
>>>> (round 2)" plus a little fix on instruction privileges.
>>>>
>>>> Tested on a POWER8 pserie guest and on mac99.
>>>>
>>>> Benjamin Herrenschmidt (2):
>>>>   ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>>>   ppc: Better figure out if processor has HV mode
>>>>
>>>> Cédric Le Goater (1):
>>>>   ppc: fix hrfid, tlbia and slbia privilege
>>>>
>>>>  target-ppc/cpu.h            |  4 ++++
>>>>  target-ppc/excp_helper.c    |  8 ++++++--
>>>>  target-ppc/helper_regs.h    |  4 ++--
>>>>  target-ppc/translate.c      | 10 ++++++----
>>>>  target-ppc/translate_init.c | 19 +++++++++++++++----
>>>>  5 files changed, 33 insertions(+), 12 deletions(-)
>>>
>>> Hi Cédric,
>>>
>>> I can confirm that this patchset fixes starting up OpenBIOS for both
>>> g3beige/mac99 in my tests here. With the escc fix also applied, the only
>>> outstanding issue is the removal of the tlb_flush() statements which
>>> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot
>>>
>>> My only request is if it would be possible to move patch 2 "ppc: Better
>>> figure out if processor has HV mode" to the front of this patchset which
>>> will make the remaining patches bisectable for the Mac machines. With that:
>>>
>>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>
>>> Does anyone know if Ben has any ideas as to why the MMU tlb_flush
>>> changes patch is causing such problems?
>>
>>
>> Here is a fix I think. Could you give it a try ? 
>>
>> Cheers,
>>
>> C. 
>>
>> From: Cédric Le Goater <clg@kaod.org>
>> Subject: [PATCH] ppc: fix tlb flushes for 32bit environment
>> Date: Sun, 05 Jun 2016 18:46:19 +0200
>> MIME-Version: 1.0
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: 8bit
>>
>> commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes')
>> introduced an optimisation to flush TLBs only when a context
>> synchronizing event is reached (interrupt, rfi). This was done for
>> ppc64 but 32bit was forgotten on the way.
>>
>> Tested on mac99 and g3beige with
>>
>>     qemu-system-ppc -cdrom darwinppc-602.cdr -boot d
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>>  I think the hunk in powerpc_excp() is needed if we don't generate a
>>  context synchronizing event. what is best to do ?
>>
>>  target-ppc/cpu.h         |    2 +-
>>  target-ppc/excp_helper.c |   10 ++++++++++
>>  target-ppc/helper_regs.h |    9 ++++++++-
>>  target-ppc/translate.c   |    2 +-
>>  4 files changed, 20 insertions(+), 3 deletions(-)
>>
>> Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c
>> ===================================================================
>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c
>> +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c
>> @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx)
>>  {
>>  }
>>  
>> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
>> +#if !defined(CONFIG_USER_ONLY)
>>  static inline void gen_check_tlb_flush(DisasContext *ctx)
>>  {
>>      TCGv_i32 t = tcg_temp_new_i32();
>> Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h
>> ===================================================================
>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h
>> +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h
>> @@ -958,9 +958,9 @@ struct CPUPPCState {
>>      /* PowerPC 64 SLB area */
>>      ppc_slb_t slb[MAX_SLB_ENTRIES];
>>      int32_t slb_nr;
>> +#endif
>>      /* tcg TLB needs flush (deferred slb inval instruction typically) */
>>      uint32_t tlb_need_flush;
>> -#endif
>>      /* segment registers */
>>      hwaddr htab_base;
>>      /* mask used to normalize hash value to PTEG index */
>> Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
>> ===================================================================
>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h
>> +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
>> @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS
>>      }
>>      if (((value >> MSR_IR) & 1) != msr_ir ||
>>          ((value >> MSR_DR) & 1) != msr_dr) {
>> +        /* A change of the instruction relocation bit in the MSR can
>> +         * cause an implicit branch in the address space. This
>> +         * requires a tlb flush.
>> +         */
>> +        if (env->mmu_model & POWERPC_MMU_32B) {
>> +            env->tlb_need_flush = 1;
>> +        }
>>          cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>>      }
>>      if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
>> @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS
>>      return excp;
>>  }
>>  
>> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
>> +#if !defined(CONFIG_USER_ONLY)
>>  static inline void check_tlb_flush(CPUPPCState *env)
>>  {
>>      CPUState *cs = CPU(ppc_env_get_cpu(env));
>> Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
>> ===================================================================
>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c
>> +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
>> @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC
>>          }
>>      }
>>  #endif
>> +    if (((new_msr >> MSR_IR) & 1) != msr_ir ||
>> +        ((new_msr >> MSR_DR) & 1) != msr_dr) {
>> +        /* A change of the instruction relocation bit in the MSR can
>> +         * cause an implicit branch in the address space. This
>> +         * requires a tlb flush.
>> +         */
>> +        if (env->mmu_model & POWERPC_MMU_32B) {
>> +            env->tlb_need_flush = 1;
>> +        }
>> +    }
>>      /* We don't use hreg_store_msr here as already have treated
>>       * any special case that could occur. Just store MSR and update hflags
>>       *
>>
>>
> 
> Hi Cédric,
> 
> I've just tried this patch on a selection of images here with both
> g3beige and mac99 and as far as I can tell the crash has now gone away:
> 
> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Super. I still need to sort out the exit path of hreg_store_msr(). 

If we have a change in IR/DR, this is a case to transform mtmsr in a 
context-synchronize instruction, for a tlb flush. This is problably 
the reason behind the POWERPC_EXCP_NONE I believe, which is then 
handled in powerpc_excp(). I need to look at that path closer.

And, now that I have a darwin guest, I have a few questions : 

1. How do I get X running ? 
2. I have an old ibook G4 from which I dd'ed the disk. openbios 
   complains for some invalid state. is that supported ?

Thanks,

C.



> Thanks for getting this sorted so quickly.
> 
> 
> ATB,
> 
> Mark.
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/3] ppc: complete the new HV mode

[Mark Cave-Ayland]

On 06/06/16 05:20, Benjamin Herrenschmidt wrote:

> On Mon, 2016-06-06 at 13:55 +1000, Benjamin Herrenschmidt wrote:
>>
>> I'm not sure that 32-bit patch is correct. We shouldn't have to flush
>> on IR/DR transitions at all, that's the whole point of the split I/D
>> code.
>>
>> I think something else is wrong.
> 
> Note: With whatever's in this branch, OpenBIOS won't start at all

Yeah, there's a patch currently pending upstream to fix the serial port
which hangs OpenBIOS upon startup:
https://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg00035.html.

The best reproducer is to run from David's ppc-for-2.7 branch with the
above patch applied manually and then try booting the following ISOs
which now panic on boot with the split I/D MMU mode enabled:

Darwin/ppc: https://opensource.apple.com/static/iso/darwinppc-602.cdr.gz
HelenOS:    http://www.helenos.org/releases/HelenOS-0.6.0-ppc32.iso

I've seen some DMA coherence issues related to macio in some of my local
tests and so I'm also testing with my beta patch to switch macio over to
use the DMA helpers here:
https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg04907.html.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Cedric Le Goater]

On 06/06/2016 08:27 AM, Cédric Le Goater wrote:
> On 06/06/2016 12:26 AM, Mark Cave-Ayland wrote:
>> On 05/06/16 18:41, Cédric Le Goater wrote:
>>
>>> Hello Mark,
>>>
>>> On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote:
>>>> On 03/06/16 13:11, Cédric Le Goater wrote:
>>>>
>>>>> This is follow up to complete the serie "ppc: preparing pnv landing
>>>>> (round 2)" plus a little fix on instruction privileges.
>>>>>
>>>>> Tested on a POWER8 pserie guest and on mac99.
>>>>>
>>>>> Benjamin Herrenschmidt (2):
>>>>>   ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>>>>   ppc: Better figure out if processor has HV mode
>>>>>
>>>>> Cédric Le Goater (1):
>>>>>   ppc: fix hrfid, tlbia and slbia privilege
>>>>>
>>>>>  target-ppc/cpu.h            |  4 ++++
>>>>>  target-ppc/excp_helper.c    |  8 ++++++--
>>>>>  target-ppc/helper_regs.h    |  4 ++--
>>>>>  target-ppc/translate.c      | 10 ++++++----
>>>>>  target-ppc/translate_init.c | 19 +++++++++++++++----
>>>>>  5 files changed, 33 insertions(+), 12 deletions(-)
>>>>
>>>> Hi Cédric,
>>>>
>>>> I can confirm that this patchset fixes starting up OpenBIOS for both
>>>> g3beige/mac99 in my tests here. With the escc fix also applied, the only
>>>> outstanding issue is the removal of the tlb_flush() statements which
>>>> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot
>>>>
>>>> My only request is if it would be possible to move patch 2 "ppc: Better
>>>> figure out if processor has HV mode" to the front of this patchset which
>>>> will make the remaining patches bisectable for the Mac machines. With that:
>>>>
>>>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>
>>>> Does anyone know if Ben has any ideas as to why the MMU tlb_flush
>>>> changes patch is causing such problems?
>>>
>>>
>>> Here is a fix I think. Could you give it a try ? 
>>>
>>> Cheers,
>>>
>>> C. 
>>>
>>> From: Cédric Le Goater <clg@kaod.org>
>>> Subject: [PATCH] ppc: fix tlb flushes for 32bit environment
>>> Date: Sun, 05 Jun 2016 18:46:19 +0200
>>> MIME-Version: 1.0
>>> Content-Type: text/plain; charset=UTF-8
>>> Content-Transfer-Encoding: 8bit
>>>
>>> commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes')
>>> introduced an optimisation to flush TLBs only when a context
>>> synchronizing event is reached (interrupt, rfi). This was done for
>>> ppc64 but 32bit was forgotten on the way.
>>>
>>> Tested on mac99 and g3beige with
>>>
>>>     qemu-system-ppc -cdrom darwinppc-602.cdr -boot d
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>
>>>  I think the hunk in powerpc_excp() is needed if we don't generate a
>>>  context synchronizing event. what is best to do ?
>>>
>>>  target-ppc/cpu.h         |    2 +-
>>>  target-ppc/excp_helper.c |   10 ++++++++++
>>>  target-ppc/helper_regs.h |    9 ++++++++-
>>>  target-ppc/translate.c   |    2 +-
>>>  4 files changed, 20 insertions(+), 3 deletions(-)
>>>
>>> Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c
>>> ===================================================================
>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c
>>> +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c
>>> @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx)
>>>  {
>>>  }
>>>  
>>> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
>>> +#if !defined(CONFIG_USER_ONLY)
>>>  static inline void gen_check_tlb_flush(DisasContext *ctx)
>>>  {
>>>      TCGv_i32 t = tcg_temp_new_i32();
>>> Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h
>>> ===================================================================
>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h
>>> +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h
>>> @@ -958,9 +958,9 @@ struct CPUPPCState {
>>>      /* PowerPC 64 SLB area */
>>>      ppc_slb_t slb[MAX_SLB_ENTRIES];
>>>      int32_t slb_nr;
>>> +#endif
>>>      /* tcg TLB needs flush (deferred slb inval instruction typically) */
>>>      uint32_t tlb_need_flush;
>>> -#endif
>>>      /* segment registers */
>>>      hwaddr htab_base;
>>>      /* mask used to normalize hash value to PTEG index */
>>> Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
>>> ===================================================================
>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h
>>> +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
>>> @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS
>>>      }
>>>      if (((value >> MSR_IR) & 1) != msr_ir ||
>>>          ((value >> MSR_DR) & 1) != msr_dr) {
>>> +        /* A change of the instruction relocation bit in the MSR can
>>> +         * cause an implicit branch in the address space. This
>>> +         * requires a tlb flush.
>>> +         */
>>> +        if (env->mmu_model & POWERPC_MMU_32B) {
>>> +            env->tlb_need_flush = 1;
>>> +        }
>>>          cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>>>      }
>>>      if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
>>> @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS
>>>      return excp;
>>>  }
>>>  
>>> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
>>> +#if !defined(CONFIG_USER_ONLY)
>>>  static inline void check_tlb_flush(CPUPPCState *env)
>>>  {
>>>      CPUState *cs = CPU(ppc_env_get_cpu(env));
>>> Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
>>> ===================================================================
>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c
>>> +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
>>> @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC
>>>          }
>>>      }
>>>  #endif
>>> +    if (((new_msr >> MSR_IR) & 1) != msr_ir ||
>>> +        ((new_msr >> MSR_DR) & 1) != msr_dr) {
>>> +        /* A change of the instruction relocation bit in the MSR can
>>> +         * cause an implicit branch in the address space. This
>>> +         * requires a tlb flush.
>>> +         */
>>> +        if (env->mmu_model & POWERPC_MMU_32B) {
>>> +            env->tlb_need_flush = 1;
>>> +        }
>>> +    }
>>>      /* We don't use hreg_store_msr here as already have treated
>>>       * any special case that could occur. Just store MSR and update hflags
>>>       *
>>>
>>>
>>
>> Hi Cédric,
>>
>> I've just tried this patch on a selection of images here with both
>> g3beige and mac99 and as far as I can tell the crash has now gone away:
>>
>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Super. I still need to sort out the exit path of hreg_store_msr(). 
> 
> If we have a change in IR/DR, this is a case to transform mtmsr in a 
> context-synchronize instruction, for a tlb flush. This is problably 
> the reason behind the POWERPC_EXCP_NONE I believe, which is then 
> handled in powerpc_excp(). I need to look at that path closer.

Forget the above as Ben just passed by :)

I still interested by what is below :

> And, now that I have a darwin guest, I have a few questions : 
> 
> 1. How do I get X running ? 
> 2. I have an old ibook G4 from which I dd'ed the disk. openbios 
>    complains for some invalid state. is that supported ?
> 
> Thanks,
> 
> C.
> 
> 
> 
>> Thanks for getting this sorted so quickly.
>>
>>
>> ATB,
>>
>> Mark.
>>
> 

Re: [Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Mark Cave-Ayland]

On 06/06/16 07:30, Cedric Le Goater wrote:

> On 06/06/2016 08:27 AM, Cédric Le Goater wrote:
>> On 06/06/2016 12:26 AM, Mark Cave-Ayland wrote:
>>> On 05/06/16 18:41, Cédric Le Goater wrote:
>>>
>>>> Hello Mark,
>>>>
>>>> On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote:
>>>>> On 03/06/16 13:11, Cédric Le Goater wrote:
>>>>>
>>>>>> This is follow up to complete the serie "ppc: preparing pnv landing
>>>>>> (round 2)" plus a little fix on instruction privileges.
>>>>>>
>>>>>> Tested on a POWER8 pserie guest and on mac99.
>>>>>>
>>>>>> Benjamin Herrenschmidt (2):
>>>>>>   ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>>>>>   ppc: Better figure out if processor has HV mode
>>>>>>
>>>>>> Cédric Le Goater (1):
>>>>>>   ppc: fix hrfid, tlbia and slbia privilege
>>>>>>
>>>>>>  target-ppc/cpu.h            |  4 ++++
>>>>>>  target-ppc/excp_helper.c    |  8 ++++++--
>>>>>>  target-ppc/helper_regs.h    |  4 ++--
>>>>>>  target-ppc/translate.c      | 10 ++++++----
>>>>>>  target-ppc/translate_init.c | 19 +++++++++++++++----
>>>>>>  5 files changed, 33 insertions(+), 12 deletions(-)
>>>>>
>>>>> Hi Cédric,
>>>>>
>>>>> I can confirm that this patchset fixes starting up OpenBIOS for both
>>>>> g3beige/mac99 in my tests here. With the escc fix also applied, the only
>>>>> outstanding issue is the removal of the tlb_flush() statements which
>>>>> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot
>>>>>
>>>>> My only request is if it would be possible to move patch 2 "ppc: Better
>>>>> figure out if processor has HV mode" to the front of this patchset which
>>>>> will make the remaining patches bisectable for the Mac machines. With that:
>>>>>
>>>>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>>
>>>>> Does anyone know if Ben has any ideas as to why the MMU tlb_flush
>>>>> changes patch is causing such problems?
>>>>
>>>>
>>>> Here is a fix I think. Could you give it a try ? 
>>>>
>>>> Cheers,
>>>>
>>>> C. 
>>>>
>>>> From: Cédric Le Goater <clg@kaod.org>
>>>> Subject: [PATCH] ppc: fix tlb flushes for 32bit environment
>>>> Date: Sun, 05 Jun 2016 18:46:19 +0200
>>>> MIME-Version: 1.0
>>>> Content-Type: text/plain; charset=UTF-8
>>>> Content-Transfer-Encoding: 8bit
>>>>
>>>> commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes')
>>>> introduced an optimisation to flush TLBs only when a context
>>>> synchronizing event is reached (interrupt, rfi). This was done for
>>>> ppc64 but 32bit was forgotten on the way.
>>>>
>>>> Tested on mac99 and g3beige with
>>>>
>>>>     qemu-system-ppc -cdrom darwinppc-602.cdr -boot d
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>
>>>>  I think the hunk in powerpc_excp() is needed if we don't generate a
>>>>  context synchronizing event. what is best to do ?
>>>>
>>>>  target-ppc/cpu.h         |    2 +-
>>>>  target-ppc/excp_helper.c |   10 ++++++++++
>>>>  target-ppc/helper_regs.h |    9 ++++++++-
>>>>  target-ppc/translate.c   |    2 +-
>>>>  4 files changed, 20 insertions(+), 3 deletions(-)
>>>>
>>>> Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c
>>>> ===================================================================
>>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c
>>>> +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c
>>>> @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx)
>>>>  {
>>>>  }
>>>>  
>>>> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
>>>> +#if !defined(CONFIG_USER_ONLY)
>>>>  static inline void gen_check_tlb_flush(DisasContext *ctx)
>>>>  {
>>>>      TCGv_i32 t = tcg_temp_new_i32();
>>>> Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h
>>>> ===================================================================
>>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h
>>>> +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h
>>>> @@ -958,9 +958,9 @@ struct CPUPPCState {
>>>>      /* PowerPC 64 SLB area */
>>>>      ppc_slb_t slb[MAX_SLB_ENTRIES];
>>>>      int32_t slb_nr;
>>>> +#endif
>>>>      /* tcg TLB needs flush (deferred slb inval instruction typically) */
>>>>      uint32_t tlb_need_flush;
>>>> -#endif
>>>>      /* segment registers */
>>>>      hwaddr htab_base;
>>>>      /* mask used to normalize hash value to PTEG index */
>>>> Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
>>>> ===================================================================
>>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h
>>>> +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
>>>> @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS
>>>>      }
>>>>      if (((value >> MSR_IR) & 1) != msr_ir ||
>>>>          ((value >> MSR_DR) & 1) != msr_dr) {
>>>> +        /* A change of the instruction relocation bit in the MSR can
>>>> +         * cause an implicit branch in the address space. This
>>>> +         * requires a tlb flush.
>>>> +         */
>>>> +        if (env->mmu_model & POWERPC_MMU_32B) {
>>>> +            env->tlb_need_flush = 1;
>>>> +        }
>>>>          cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>>>>      }
>>>>      if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
>>>> @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS
>>>>      return excp;
>>>>  }
>>>>  
>>>> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
>>>> +#if !defined(CONFIG_USER_ONLY)
>>>>  static inline void check_tlb_flush(CPUPPCState *env)
>>>>  {
>>>>      CPUState *cs = CPU(ppc_env_get_cpu(env));
>>>> Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
>>>> ===================================================================
>>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c
>>>> +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
>>>> @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC
>>>>          }
>>>>      }
>>>>  #endif
>>>> +    if (((new_msr >> MSR_IR) & 1) != msr_ir ||
>>>> +        ((new_msr >> MSR_DR) & 1) != msr_dr) {
>>>> +        /* A change of the instruction relocation bit in the MSR can
>>>> +         * cause an implicit branch in the address space. This
>>>> +         * requires a tlb flush.
>>>> +         */
>>>> +        if (env->mmu_model & POWERPC_MMU_32B) {
>>>> +            env->tlb_need_flush = 1;
>>>> +        }
>>>> +    }
>>>>      /* We don't use hreg_store_msr here as already have treated
>>>>       * any special case that could occur. Just store MSR and update hflags
>>>>       *
>>>>
>>>>
>>>
>>> Hi Cédric,
>>>
>>> I've just tried this patch on a selection of images here with both
>>> g3beige and mac99 and as far as I can tell the crash has now gone away:
>>>
>>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>> Super. I still need to sort out the exit path of hreg_store_msr(). 
>>
>> If we have a change in IR/DR, this is a case to transform mtmsr in a 
>> context-synchronize instruction, for a tlb flush. This is problably 
>> the reason behind the POWERPC_EXCP_NONE I believe, which is then 
>> handled in powerpc_excp(). I need to look at that path closer.
> 
> Forget the above as Ben just passed by :)
> 
> I still interested by what is below :
> 
>> And, now that I have a darwin guest, I have a few questions : 
>>
>> 1. How do I get X running ? 

It's not something I really test here, but shouldn't startx work? You
may need to do some configuration work if X is configured to directly
use the ATI driver.

>> 2. I have an old ibook G4 from which I dd'ed the disk. openbios 
>>    complains for some invalid state. is that supported ?

Yes, OpenBIOS should boot most things these days (MorphOS is the only
execption I know of where the bootloader won't execute correctly as it
assumes real mode). The above message means that OpenBIOS couldn't find
a bootloader, or it could but was unable to execute it, e.g. due to
incompatible architecture - which OS is your image running? Have you
tried both mac99 and g3beige machines?


ATB,

Mark.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/3] ppc: complete the new HV mode

[Benjamin Herrenschmidt]

On Mon, 2016-06-06 at 07:29 +0100, Mark Cave-Ayland wrote:
> 
> The best reproducer is to run from David's ppc-for-2.7 branch with
> the above patch applied manually and then try booting the following
> ISOs which now panic on boot with the split I/D MMU mode enabled:

So at least HelenOS is fixed by this:

--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -1969,6 +1969,11 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr)
         /* XXX: this case should be optimized,
          * giving a mask to tlb_flush_page
          */
+        /* This is broken, some CPUs invalidate a whole congruence
+         * class on an even smaller subset of bits and some OSes take
+         * advantage of this. Just blow the whole thing away.
+         */
+#if 0
         tlb_flush_page(cs, addr | (0x0 << 28));
         tlb_flush_page(cs, addr | (0x1 << 28));
         tlb_flush_page(cs, addr | (0x2 << 28));
@@ -1985,6 +1990,9 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr)
         tlb_flush_page(cs, addr | (0xD << 28));
         tlb_flush_page(cs, addr | (0xE << 28));
         tlb_flush_page(cs, addr | (0xF << 28));
+#else
+        tlb_flush(cs, 1);
+#endif
         break;
 #if defined(TARGET_PPC64)
     case POWERPC_MMU_64B:

I'll check Darwin...

Cheers,
Ben.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/3] ppc: complete the new HV mode

[Benjamin Herrenschmidt]

On Mon, 2016-06-06 at 17:04 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2016-06-06 at 07:29 +0100, Mark Cave-Ayland wrote:
> > 
> > 
> > The best reproducer is to run from David's ppc-for-2.7 branch with
> > the above patch applied manually and then try booting the following
> > ISOs which now panic on boot with the split I/D MMU mode enabled:
> So at least HelenOS is fixed by this:

Note that Cedric is right, the TLB flush batching isn't done for 32-
bit, and we could do it, it would probably improve performances, but it
needs to be done properly :-) I'll look into it next.

> --- a/target-ppc/mmu_helper.c
> +++ b/target-ppc/mmu_helper.c
> @@ -1969,6 +1969,11 @@ void ppc_tlb_invalidate_one(CPUPPCState *env,
> target_ulong addr)
>          /* XXX: this case should be optimized,
>           * giving a mask to tlb_flush_page
>           */
> +        /* This is broken, some CPUs invalidate a whole congruence
> +         * class on an even smaller subset of bits and some OSes
> take
> +         * advantage of this. Just blow the whole thing away.
> +         */
> +#if 0
>          tlb_flush_page(cs, addr | (0x0 << 28));
>          tlb_flush_page(cs, addr | (0x1 << 28));
>          tlb_flush_page(cs, addr | (0x2 << 28));
> @@ -1985,6 +1990,9 @@ void ppc_tlb_invalidate_one(CPUPPCState *env,
> target_ulong addr)
>          tlb_flush_page(cs, addr | (0xD << 28));
>          tlb_flush_page(cs, addr | (0xE << 28));
>          tlb_flush_page(cs, addr | (0xF << 28));
> +#else
> +        tlb_flush(cs, 1);
> +#endif
>          break;
>  #if defined(TARGET_PPC64)
>      case POWERPC_MMU_64B:
> 
> I'll check Darwin...
> 
> Cheers,
> Ben.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/3] ppc: complete the new HV mode

[Cédric Le Goater]

On 06/06/2016 06:17 AM, Benjamin Herrenschmidt wrote:
> On Sun, 2016-06-05 at 19:41 +0200, Cédric Le Goater wrote:
>>  
>> Here is a fix I think. Could you give it a try ? 
> 
> This is somewhat wrong...
> 
>> commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes')
>> introduced an optimisation to flush TLBs only when a context
>> synchronizing event is reached (interrupt, rfi). This was done for
>> ppc64 but 32bit was forgotten on the way.
> 
> No it didn't. That commit only delays flushes on ppc64. ppc32 is
> unaffected, unless I missed something. IE. It will delay flushes caused
> by slb instructions (which don't exist on 32-bit)
> and ppc_tlb_invalidate_one() only in the 64-bit cases.
> 
> Also what your patch does in practice is not really change that, though
> you seem to try to somewhat extend the batching to 32-bit (but
> incompletely), you also introduce something which effectively reverts
> part of 9fb044911444fdd09f5f072ad0ca269d7f8b841d (split I/D mode).
> 
> I think that's more what's "fixing" your problem, ie, the flush in
> IR/DR changes. However it shouldn't be needed.

OK. I thought that was needed because of what the 32b specs say in 
"Synchronization Requirements for Special Registers and for Lookaside 
Buffers", a "Context-synchronizing instruction" is required after a 
mtmsr({I,D}R) and also because changing IR can break implicit branching.

But I might just misunderstand all of it as I am discovering.

> I suspect all of that is papering over another bug somewhere else which
> got exposed by the split I/D mode, since we no longer over-flush on
> transitions to/from real-mode. So we must be missing flushes elsewhere,
> possibly some G3 specific stuff, or there always was some kind of bug
> in the TLB flushing on 32-bit that got somewhat masked by the over-
> flushing we used to do.

>From what I see, darwin loops on tlbie :

	0x000952fc:  mtctr   r0
	0x00095300:  tlbie   r6
	0x00095304:  addi    r6,r6,4096
	0x00095308:  bdnz+   0x95300
	0x0009530c:  mtcrf   128,r11
	0x00095310:  sync    
	0x00095314:  eieio
	0x00095318:  bns-    0x95328

and this is done on the G4, but not necessarily on the G3, it depends on
r11 which contains some bits of SPRG2 :

	  0x0009531c:  tlbsync
	  0x00095320:  sync    
	  0x00095324:  isync

HID0 is also read and written to but to control cache bits.

> I need a repro-case.

Booting the darwin CD is enough.

Cheers,

C. 
 
> Cheers,
> Ben.
> 
>> Tested on mac99 and g3beige with
>>
>>     qemu-system-ppc -cdrom darwinppc-602.cdr -boot d
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>>  I think the hunk in powerpc_excp() is needed if we don't generate a
>>  context synchronizing event. what is best to do ?
>>
>>  target-ppc/cpu.h         |    2 +-
>>  target-ppc/excp_helper.c |   10 ++++++++++
>>  target-ppc/helper_regs.h |    9 ++++++++-
>>  target-ppc/translate.c   |    2 +-
>>  4 files changed, 20 insertions(+), 3 deletions(-)
>>
>> Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c
>> ===================================================================
>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c
>> +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c
>> @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx)
>>  {
>>  }
>>  
>> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
>> +#if !defined(CONFIG_USER_ONLY)
>>  static inline void gen_check_tlb_flush(DisasContext *ctx)
>>  {
>>      TCGv_i32 t = tcg_temp_new_i32();
>> Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h
>> ===================================================================
>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h
>> +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h
>> @@ -958,9 +958,9 @@ struct CPUPPCState {
>>      /* PowerPC 64 SLB area */
>>      ppc_slb_t slb[MAX_SLB_ENTRIES];
>>      int32_t slb_nr;
>> +#endif
>>      /* tcg TLB needs flush (deferred slb inval instruction
>> typically) */
>>      uint32_t tlb_need_flush;
>> -#endif
>>      /* segment registers */
>>      hwaddr htab_base;
>>      /* mask used to normalize hash value to PTEG index */
>> Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
>> ===================================================================
>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h
>> +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
>> @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS
>>      }
>>      if (((value >> MSR_IR) & 1) != msr_ir ||
>>          ((value >> MSR_DR) & 1) != msr_dr) {
>> +        /* A change of the instruction relocation bit in the MSR can
>> +         * cause an implicit branch in the address space. This
>> +         * requires a tlb flush.
>> +         */
>> +        if (env->mmu_model & POWERPC_MMU_32B) {
>> +            env->tlb_need_flush = 1;
>> +        }
>>          cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>>      }
>>      if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
>> @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS
>>      return excp;
>>  }
>>  
>> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
>> +#if !defined(CONFIG_USER_ONLY)
>>  static inline void check_tlb_flush(CPUPPCState *env)
>>  {
>>      CPUState *cs = CPU(ppc_env_get_cpu(env));
>> Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
>> ===================================================================
>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c
>> +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
>> @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC
>>          }
>>      }
>>  #endif
>> +    if (((new_msr >> MSR_IR) & 1) != msr_ir ||
>> +        ((new_msr >> MSR_DR) & 1) != msr_dr) {
>> +        /* A change of the instruction relocation bit in the MSR can
>> +         * cause an implicit branch in the address space. This
>> +         * requires a tlb flush.
>> +         */
>> +        if (env->mmu_model & POWERPC_MMU_32B) {
>> +            env->tlb_need_flush = 1;
>> +        }
>> +    }
>>      /* We don't use hreg_store_msr here as already have treated
>>       * any special case that could occur. Just store MSR and update
>> hflags
>>       *
>>

Re: [Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Cédric Le Goater]

On 06/06/2016 08:38 AM, Mark Cave-Ayland wrote:
> On 06/06/16 07:30, Cedric Le Goater wrote:
> 
>> On 06/06/2016 08:27 AM, Cédric Le Goater wrote:
>>> On 06/06/2016 12:26 AM, Mark Cave-Ayland wrote:
>>>> On 05/06/16 18:41, Cédric Le Goater wrote:
>>>>
>>>>> Hello Mark,
>>>>>
>>>>> On 06/03/2016 03:52 PM, Mark Cave-Ayland wrote:
>>>>>> On 03/06/16 13:11, Cédric Le Goater wrote:
>>>>>>
>>>>>>> This is follow up to complete the serie "ppc: preparing pnv landing
>>>>>>> (round 2)" plus a little fix on instruction privileges.
>>>>>>>
>>>>>>> Tested on a POWER8 pserie guest and on mac99.
>>>>>>>
>>>>>>> Benjamin Herrenschmidt (2):
>>>>>>>   ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>>>>>>   ppc: Better figure out if processor has HV mode
>>>>>>>
>>>>>>> Cédric Le Goater (1):
>>>>>>>   ppc: fix hrfid, tlbia and slbia privilege
>>>>>>>
>>>>>>>  target-ppc/cpu.h            |  4 ++++
>>>>>>>  target-ppc/excp_helper.c    |  8 ++++++--
>>>>>>>  target-ppc/helper_regs.h    |  4 ++--
>>>>>>>  target-ppc/translate.c      | 10 ++++++----
>>>>>>>  target-ppc/translate_init.c | 19 +++++++++++++++----
>>>>>>>  5 files changed, 33 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> Hi Cédric,
>>>>>>
>>>>>> I can confirm that this patchset fixes starting up OpenBIOS for both
>>>>>> g3beige/mac99 in my tests here. With the escc fix also applied, the only
>>>>>> outstanding issue is the removal of the tlb_flush() statements which
>>>>>> causes Darwin, MacOS X and HelenOS 0.60 to panic on boot
>>>>>>
>>>>>> My only request is if it would be possible to move patch 2 "ppc: Better
>>>>>> figure out if processor has HV mode" to the front of this patchset which
>>>>>> will make the remaining patches bisectable for the Mac machines. With that:
>>>>>>
>>>>>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>>>
>>>>>> Does anyone know if Ben has any ideas as to why the MMU tlb_flush
>>>>>> changes patch is causing such problems?
>>>>>
>>>>>
>>>>> Here is a fix I think. Could you give it a try ? 
>>>>>
>>>>> Cheers,
>>>>>
>>>>> C. 
>>>>>
>>>>> From: Cédric Le Goater <clg@kaod.org>
>>>>> Subject: [PATCH] ppc: fix tlb flushes for 32bit environment
>>>>> Date: Sun, 05 Jun 2016 18:46:19 +0200
>>>>> MIME-Version: 1.0
>>>>> Content-Type: text/plain; charset=UTF-8
>>>>> Content-Transfer-Encoding: 8bit
>>>>>
>>>>> commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes')
>>>>> introduced an optimisation to flush TLBs only when a context
>>>>> synchronizing event is reached (interrupt, rfi). This was done for
>>>>> ppc64 but 32bit was forgotten on the way.
>>>>>
>>>>> Tested on mac99 and g3beige with
>>>>>
>>>>>     qemu-system-ppc -cdrom darwinppc-602.cdr -boot d
>>>>>
>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>> ---
>>>>>
>>>>>  I think the hunk in powerpc_excp() is needed if we don't generate a
>>>>>  context synchronizing event. what is best to do ?
>>>>>
>>>>>  target-ppc/cpu.h         |    2 +-
>>>>>  target-ppc/excp_helper.c |   10 ++++++++++
>>>>>  target-ppc/helper_regs.h |    9 ++++++++-
>>>>>  target-ppc/translate.c   |    2 +-
>>>>>  4 files changed, 20 insertions(+), 3 deletions(-)
>>>>>
>>>>> Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c
>>>>> ===================================================================
>>>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c
>>>>> +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c
>>>>> @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx)
>>>>>  {
>>>>>  }
>>>>>  
>>>>> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
>>>>> +#if !defined(CONFIG_USER_ONLY)
>>>>>  static inline void gen_check_tlb_flush(DisasContext *ctx)
>>>>>  {
>>>>>      TCGv_i32 t = tcg_temp_new_i32();
>>>>> Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h
>>>>> ===================================================================
>>>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h
>>>>> +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h
>>>>> @@ -958,9 +958,9 @@ struct CPUPPCState {
>>>>>      /* PowerPC 64 SLB area */
>>>>>      ppc_slb_t slb[MAX_SLB_ENTRIES];
>>>>>      int32_t slb_nr;
>>>>> +#endif
>>>>>      /* tcg TLB needs flush (deferred slb inval instruction typically) */
>>>>>      uint32_t tlb_need_flush;
>>>>> -#endif
>>>>>      /* segment registers */
>>>>>      hwaddr htab_base;
>>>>>      /* mask used to normalize hash value to PTEG index */
>>>>> Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
>>>>> ===================================================================
>>>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h
>>>>> +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h
>>>>> @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS
>>>>>      }
>>>>>      if (((value >> MSR_IR) & 1) != msr_ir ||
>>>>>          ((value >> MSR_DR) & 1) != msr_dr) {
>>>>> +        /* A change of the instruction relocation bit in the MSR can
>>>>> +         * cause an implicit branch in the address space. This
>>>>> +         * requires a tlb flush.
>>>>> +         */
>>>>> +        if (env->mmu_model & POWERPC_MMU_32B) {
>>>>> +            env->tlb_need_flush = 1;
>>>>> +        }
>>>>>          cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>>>>>      }
>>>>>      if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
>>>>> @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS
>>>>>      return excp;
>>>>>  }
>>>>>  
>>>>> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
>>>>> +#if !defined(CONFIG_USER_ONLY)
>>>>>  static inline void check_tlb_flush(CPUPPCState *env)
>>>>>  {
>>>>>      CPUState *cs = CPU(ppc_env_get_cpu(env));
>>>>> Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
>>>>> ===================================================================
>>>>> --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c
>>>>> +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c
>>>>> @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC
>>>>>          }
>>>>>      }
>>>>>  #endif
>>>>> +    if (((new_msr >> MSR_IR) & 1) != msr_ir ||
>>>>> +        ((new_msr >> MSR_DR) & 1) != msr_dr) {
>>>>> +        /* A change of the instruction relocation bit in the MSR can
>>>>> +         * cause an implicit branch in the address space. This
>>>>> +         * requires a tlb flush.
>>>>> +         */
>>>>> +        if (env->mmu_model & POWERPC_MMU_32B) {
>>>>> +            env->tlb_need_flush = 1;
>>>>> +        }
>>>>> +    }
>>>>>      /* We don't use hreg_store_msr here as already have treated
>>>>>       * any special case that could occur. Just store MSR and update hflags
>>>>>       *
>>>>>
>>>>>
>>>>
>>>> Hi Cédric,
>>>>
>>>> I've just tried this patch on a selection of images here with both
>>>> g3beige and mac99 and as far as I can tell the crash has now gone away:
>>>>
>>>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>
>>> Super. I still need to sort out the exit path of hreg_store_msr(). 
>>>
>>> If we have a change in IR/DR, this is a case to transform mtmsr in a 
>>> context-synchronize instruction, for a tlb flush. This is problably 
>>> the reason behind the POWERPC_EXCP_NONE I believe, which is then 
>>> handled in powerpc_excp(). I need to look at that path closer.
>>
>> Forget the above as Ben just passed by :)
>>
>> I still interested by what is below :
>>
>>> And, now that I have a darwin guest, I have a few questions : 
>>>
>>> 1. How do I get X running ? 
> 
> It's not something I really test here, but shouldn't startx work? You
> may need to do some configuration work if X is configured to directly
> use the ATI driver.
> 
>>> 2. I have an old ibook G4 from which I dd'ed the disk. openbios 
>>>    complains for some invalid state. is that supported ?
> 
> Yes, OpenBIOS should boot most things these days (MorphOS is the only
> execption I know of where the bootloader won't execute correctly as it
> assumes real mode). The above message means that OpenBIOS couldn't find
> a bootloader, or it could but was unable to execute it, e.g. due to
> incompatible architecture - which OS is your image running? 

I am pretty sure it is a Mac OS X v10.5. I still have the hardware but it
is running Linux now : 

# cat /proc/cpuinfo 
processor	: 0
cpu		: 7447A, altivec supported
clock		: 1333.333000MHz
revision	: 1.5 (pvr 8003 0105)
bogomips	: 36.86

total bogomips	: 36.86
timebase	: 18432000
platform	: PowerMac
model		: PowerBook6,7
machine		: PowerBook6,7
motherboard	: PowerBook6,7 MacRISC3 Power Macintosh 
detected as	: 287 (iBook G4)
pmac flags	: 0000001a
L2 cache	: 512K unified
pmac-generation	: NewWorld
Memory		: 1024 MB

# lsprop  /proc/device-tree/rom@ff800000/boot-rom@fff00000/
reg              fff00000 00100000
info             fff00000 00003f00 000493f0 20050705
		 815fda85 fff08000 00078001 000493f0
		 20050705 23765c6c fff80000 00080002
		 000493f0 20050705 b3364dca fff03f00
		 00000083 000493f0 20050705 c2b72d61
		 fff03f80 00000084 e24a68ca 15a82001
		 ffffffff fff04000 00004005 6e767261
		 6d000000 00000000 00000000 00000000
		 [140 bytes total]
name             "boot-rom"
security-modes   6e6f6e65 2c206675 6c6c2c20 636f6d6d
		 616e642c 206e6f2d 70617373 776f7264
image            00080000 (524288)
model            "Apple PowerBook6,7 4.9.3f0 BootROM built on 07/05/05 at 11:14:11"
write-characteristic
		 "flash"
hwi-flags        402a1220 (1076498976)
BootROM-version  "$0004.93f0"
BootROM-build-date
		 "07/05/05 at 11:14:11"
linux,phandle    ff89cb08
has-config-block


> Have you tried both mac99 and g3beige machines?

yes.

C.

Re: [Qemu-devel] [PATCH 0/3] ppc: complete the new HV mode

[Mark Cave-Ayland]

On 07/06/16 08:04, Cédric Le Goater wrote:

>>>> 2. I have an old ibook G4 from which I dd'ed the disk. openbios 
>>>>    complains for some invalid state. is that supported ?
>>
>> Yes, OpenBIOS should boot most things these days (MorphOS is the only
>> execption I know of where the bootloader won't execute correctly as it
>> assumes real mode). The above message means that OpenBIOS couldn't find
>> a bootloader, or it could but was unable to execute it, e.g. due to
>> incompatible architecture - which OS is your image running? 
> 
> I am pretty sure it is a Mac OS X v10.5. I still have the hardware but it
> is running Linux now : 
> 
> # cat /proc/cpuinfo 
> processor	: 0
> cpu		: 7447A, altivec supported
> clock		: 1333.333000MHz
> revision	: 1.5 (pvr 8003 0105)
> bogomips	: 36.86
> 
> total bogomips	: 36.86
> timebase	: 18432000
> platform	: PowerMac
> model		: PowerBook6,7
> machine		: PowerBook6,7
> motherboard	: PowerBook6,7 MacRISC3 Power Macintosh 
> detected as	: 287 (iBook G4)
> pmac flags	: 0000001a
> L2 cache	: 512K unified
> pmac-generation	: NewWorld
> Memory		: 1024 MB
> 
> # lsprop  /proc/device-tree/rom@ff800000/boot-rom@fff00000/
> reg              fff00000 00100000
> info             fff00000 00003f00 000493f0 20050705
> 		 815fda85 fff08000 00078001 000493f0
> 		 20050705 23765c6c fff80000 00080002
> 		 000493f0 20050705 b3364dca fff03f00
> 		 00000083 000493f0 20050705 c2b72d61
> 		 fff03f80 00000084 e24a68ca 15a82001
> 		 ffffffff fff04000 00004005 6e767261
> 		 6d000000 00000000 00000000 00000000
> 		 [140 bytes total]
> name             "boot-rom"
> security-modes   6e6f6e65 2c206675 6c6c2c20 636f6d6d
> 		 616e642c 206e6f2d 70617373 776f7264
> image            00080000 (524288)
> model            "Apple PowerBook6,7 4.9.3f0 BootROM built on 07/05/05 at 11:14:11"
> write-characteristic
> 		 "flash"
> hwi-flags        402a1220 (1076498976)
> BootROM-version  "$0004.93f0"
> BootROM-build-date
> 		 "07/05/05 at 11:14:11"
> linux,phandle    ff89cb08
> has-config-block
> 
> 
>> Have you tried both mac99 and g3beige machines?
> 
> yes.

(I'm wondering if we should start a new thread here either just on
qemu-ppc or over on the OpenBIOS list)

OpenBIOS will read hfs/hfsplus filesystems fine - I wonder if maybe it
can't locate a suitable partition in the Apple Partition Map?

If you boot to the Forth prompt with -prom-env 'auto-boot?=false' can
you try the following to see if you can list the disk contents:

dir hd:,\

If that doesn't work it means that the partition auto-detection is
failing, so try manually forcing the partition number until you find one
that works e.g.

dir hd:0,\
dir hd:1,\
etc.

up until around partition 10? Once you find one that works it should be
possible to boot that partition directly e.g.

boot hd:1


ATB,

Mark.