[Qemu-devel] Determining interest in PPC e500spin, yield, and openpic patches

[Qemu-devel] Determining interest in PPC e500spin, yield, and openpic patches

[alarson]

We've used older versions of QEMU for several years as a virtual
target for our OS.  Many thanks to the community for providing this
platform.

We've been working to get our OS running under QEMU 2.x and have
identified a few bugs in QEMU, have made some enhancements, and are
still tracking down some other curious behaviors.  I'm looking for
some guidance as to how, and whether, you'd like patches for the
following.

1. There is a defect in ppce500_spin.c:spin_kick() that creates an
   incorrectly sized TLB entry.  This was reported as bug
   https://bugs.launchpad.net/qemu/+bug/1587535  I can provide a
   patch if desired.

2. We have implemented the PPC "yield" instruction.  I can provide a
   patch if desired.

3. We're working on support for openpic timers.  We're not finished,
   but it would be helpful to know if a patch is desired or if we
   should expect to maintain the changes independently.

4. We're currently tracking down why in our e500 (both unicore and
   multi-core) PPC QEMU 2.5 guest (x86 host), that with interrupts
   disabled, after enabling the decrementer and issuing a "wait"
   instruction QEMU continues to "busy loop", consuming an entire host
   CPU doing apparently nothing.  As expected, issuing a "wait" prior
   to enabling the decrementer leaves the host process idle.  We found
   the bug in the PPC "wait" instruction implementation that was
   independently reported and resolved last week, but that did not fix
   the problem.  We also have our OS running on the g3beige and using
   MSR.POW which causes the host to "sleep", but we are having no joy
   with e500 and "wait".  Any pointers would be appreciated.  When we
   find something we'll report back.

Re: [Qemu-devel] [Qemu-ppc] Determining interest in PPC e500spin, yield, and openpic patches

[Mark Cave-Ayland]

On 14/06/16 00:08, alarson@ddci.com wrote:

> We've used older versions of QEMU for several years as a virtual
> target for our OS.  Many thanks to the community for providing this
> platform.
> 
> We've been working to get our OS running under QEMU 2.x and have
> identified a few bugs in QEMU, have made some enhancements, and are
> still tracking down some other curious behaviors.  I'm looking for
> some guidance as to how, and whether, you'd like patches for the
> following.
> 
> 1. There is a defect in ppce500_spin.c:spin_kick() that creates an
>    incorrectly sized TLB entry.  This was reported as bug
>    https://bugs.launchpad.net/qemu/+bug/1587535  I can provide a
>    patch if desired.
> 
> 2. We have implemented the PPC "yield" instruction.  I can provide a
>    patch if desired.
> 
> 3. We're working on support for openpic timers.  We're not finished,
>    but it would be helpful to know if a patch is desired or if we
>    should expect to maintain the changes independently.
> 
> 4. We're currently tracking down why in our e500 (both unicore and
>    multi-core) PPC QEMU 2.5 guest (x86 host), that with interrupts
>    disabled, after enabling the decrementer and issuing a "wait"
>    instruction QEMU continues to "busy loop", consuming an entire host
>    CPU doing apparently nothing.  As expected, issuing a "wait" prior
>    to enabling the decrementer leaves the host process idle.  We found
>    the bug in the PPC "wait" instruction implementation that was
>    independently reported and resolved last week, but that did not fix
>    the problem.  We also have our OS running on the g3beige and using
>    MSR.POW which causes the host to "sleep", but we are having no joy
>    with e500 and "wait".  Any pointers would be appreciated.  When we
>    find something we'll report back.

If you are able to submit patches with a valid Signed-off-by then it
would be good to see all your patches; at the very least there are some
people with some very deep knowledge of PPC CPUs around so even if the
patch is not accepted in its current form, it can be reworked into
something that can.

Don't forget to check your patches with scripts/checkpatch.pl and to
explicitly CC the correct MAINTAINERS as given by
scripts/get_maintainer.pl for submission.


ATB,

Mark.

Re: [Qemu-devel] [Qemu-ppc] Determining interest in PPC e500spin, yield, and openpic patches

[David Gibson]

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

On Mon, Jun 13, 2016 at 06:08:30PM -0500, alarson@ddci.com wrote:
> We've used older versions of QEMU for several years as a virtual
> target for our OS.  Many thanks to the community for providing this
> platform.

Hi,

Thanks for your interest.

> We've been working to get our OS running under QEMU 2.x and have
> identified a few bugs in QEMU, have made some enhancements, and are
> still tracking down some other curious behaviors.  I'm looking for
> some guidance as to how, and whether, you'd like patches for the
> following.

Always interested in bug fixes.  The e500 support probably doesn't get
a whole lot of attention these days, but you're proof that there are
at least a few people who care.

> 1. There is a defect in ppce500_spin.c:spin_kick() that creates an
>    incorrectly sized TLB entry.  This was reported as bug
>    https://bugs.launchpad.net/qemu/+bug/1587535  I can provide a
>    patch if desired.

Absolutely.

> 2. We have implemented the PPC "yield" instruction.  I can provide a
>    patch if desired.

Sounds good.

> 3. We're working on support for openpic timers.  We're not finished,
>    but it would be helpful to know if a patch is desired or if we
>    should expect to maintain the changes independently.

I don't see a reason we wouldn't want it, unless it's horribly
invasive.  By all means post patches, and I'll review as best I can.

> 4. We're currently tracking down why in our e500 (both unicore and
>    multi-core) PPC QEMU 2.5 guest (x86 host), that with interrupts
>    disabled, after enabling the decrementer and issuing a "wait"
>    instruction QEMU continues to "busy loop", consuming an entire host
>    CPU doing apparently nothing.  As expected, issuing a "wait" prior
>    to enabling the decrementer leaves the host process idle.  We found
>    the bug in the PPC "wait" instruction implementation that was
>    independently reported and resolved last week, but that did not fix
>    the problem.  We also have our OS running on the g3beige and using
>    MSR.POW which causes the host to "sleep", but we are having no joy
>    with e500 and "wait".  Any pointers would be appreciated.  When we
>    find something we'll report back.

Ok.


So, patches for ppc related code should be sent to myself and Alex
Graf <agraf@suse.de>, co-maintainers for the target-ppc and related
code.  You should CC both qemu-ppc@nongnu.org and
qemu-devel@nongnu.org.

I have a (frequently rebased) git tree where I stage ppc patches at:
    https://github.com/dgibson/qemu/tree/ppc-for-2.7

-- 
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] [Qemu-ppc] Determining interest in PPC e500spin, yield, and openpic patches

[alarson]

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

David Gibson <david@gibson.dropbear.id.au> wrote on 06/14/2016 11:17:57 
PM:
Aaron Larson <alarson@ddci.com>

AL> 1. There is a defect in ppce500_spin.c:spin_kick() that creates an
AL>    incorrectly sized TLB entry.  This was reported as bug
AL>    https://bugs.launchpad.net/qemu/+bug/1587535  I can provide a
AL>    patch if desired.

DG> Absolutely.

OK, I'll start with this one.   Let me know if there is anything you'd 
like me to do
with that bug report.

When e500 PPC is booted multi-core, the non-boot cores are started via
the spin table.  ppce500_spin.c:spin_kick() calls
mmubooke_create_initial_mapping() to allocate a 64MB TLB entry, but
the created TLB entry is only 256KB.

The root cause is that the function computing the size of the TLB
entry, namely booke206_page_size_to_tlb assumes MAS1.TSIZE as defined
by latter PPC cores, specifically n to the power of FOUR * 1KB. The
result is then used by mmubooke_create_initial_mapping using
MAS1_TSIZE_SHIFT, but MAS1_TSIZE_SHIFT is defined assuming TLB entries
are n to the power of TWO * 1KB. I.e., a difference of shift=7 or
shift=8.

Simply changing MAS1_TSIZE_SHIFT from 7 to 8 is not appropriate since
the macro is used elsewhere.

The following patch has a fix for that, and also raises a separate
issue that I'd be happy to resolve after getting some guidance.



[-- Attachment #2: ppce500_spin-tlb.patch --]
[-- Type: application/octet-stream, Size: 1795 bytes --]

--- qemu-2.5.0.orig/hw/ppc/ppce500_spin.c	2015-12-16 16:04:49.000000000 -0600
+++ qemu-2.5.0/hw/ppc/ppce500_spin.c	2016-06-15 14:54:36.921768400 -0500
@@ -1,7 +1,7 @@
 /*
  * QEMU PowerPC e500v2 ePAPR spinning code
  *
- * Copyright (C) 2011 Freescale Semiconductor, Inc. All rights reserved.
+ * Copyright (C) 2011, 2016 Freescale Semiconductor, Inc. All rights reserved.
  *
  * Author: Alexander Graf, <agraf@suse.de>
  *
@@ -74,7 +74,11 @@
 /* Create -kernel TLB entries for BookE, linearly spanning 256MB.  */
 static inline hwaddr booke206_page_size_to_tlb(uint64_t size)
 {
-    return ctz32(size >> 10) >> 1;
+    /* The EREF indicates that TLB pages are (4 to the power of 2)KB, which
+     * corresponds to MAS1_TSIZE_SHIFT=8, but to support legacy processors that
+     * assume TLB pages are (2 to the power of 2)KB MAS1_TSIZE_SHIFT is
+     * currently 7. */
+    return ctz32(size >> 10) >> (MAS1_TSIZE_SHIFT-7);
 }
 
 static void mmubooke_create_initial_mapping(CPUPPCState *env,
@@ -104,6 +108,16 @@
 
     cpu_synchronize_state(cpu);
     stl_p(&curspin->pir, env->spr[SPR_PIR]);
+/* The stl_p() above seems wrong to me.  First of all, it seems more appropriate
+ * in a guest ROM/BOOT code than in qemu emulation.  However, SPR_PIR is never
+ * initialized, so the effect of the stl_p() is to overwrite the curspin->pir
+ * with 0. It makes more sense to load the SPR_PIR with the curspin->pir, which
+ * is what the following does.
+ *    env->spr[SPR_PIR]=ldl_p(&curspin->pir);
+ * Alternately SPR_PIR could be initialized from SPR_BOOKE_PIR which is properly
+ * initialized, so this could also work:
+ *    env->spr[SPR_PIR] = env->spr[SPR_BOOKE_PIR]
+*/
     env->nip = ldq_p(&curspin->addr) & (map_size - 1);
     env->gpr[3] = ldq_p(&curspin->r3);
     env->gpr[4] = 0;

Re: [Qemu-devel] [Qemu-ppc] Determining interest in PPC e500spin, yield, and openpic patches

[Thomas Huth]

 Hi,

On 15.06.2016 22:12, alarson@ddci.com wrote:
[...]
> When e500 PPC is booted multi-core, the non-boot cores are started via
> the spin table.  ppce500_spin.c:spin_kick() calls
> mmubooke_create_initial_mapping() to allocate a 64MB TLB entry, but
> the created TLB entry is only 256KB.
> 
> The root cause is that the function computing the size of the TLB
> entry, namely booke206_page_size_to_tlb assumes MAS1.TSIZE as defined
> by latter PPC cores, specifically n to the power of FOUR * 1KB. The
> result is then used by mmubooke_create_initial_mapping using
> MAS1_TSIZE_SHIFT, but MAS1_TSIZE_SHIFT is defined assuming TLB entries
> are n to the power of TWO * 1KB. I.e., a difference of shift=7 or
> shift=8.
> 
> Simply changing MAS1_TSIZE_SHIFT from 7 to 8 is not appropriate since
> the macro is used elsewhere.
> 
> The following patch has a fix for that, and also raises a separate
> issue that I'd be happy to resolve after getting some guidance.

Thanks for your patch! However, patches have to follow certain rules
before they can be included in QEMU. Please read through
 http://qemu-project.org/Contribute/SubmitAPatch
to get a basic understanding of these rules first. Especially important
for your patch:

- You need to add a "Signed-off-by" line to the patch description to
  state that you've read and understood http://developercertificate.org/

- Run your patch through scripts/checkpatch.pl and the fix style issue

- Please address only one issue per patch. The issue that you've
  mentioned in the comment should be handled with a separate discussion
  (and a separate patch once there is a solution)

Thanks!

 Thomas

Re: [Qemu-devel] [Qemu-ppc] Determining interest in PPC e500spin, yield, and openpic patches

[alarson]

Thomas Huth <thuth@redhat.com> wrote on 06/16/2016 01:25:45 AM:

> Thanks for your patch! However, patches have to follow certain rules
> before they can be included in QEMU. Please read through

Sorry for the broken patch, and the long delay.  I'm not a git user
so its taken a while to climb the curve.  I didn't get 'git send-email'
working, but hopefully the next patch will be ok.

Re: [Qemu-devel] [Qemu-ppc] Determining interest in PPC e500spin, yield, and openpic patches

[David Gibson]

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

On Wed, Jun 15, 2016 at 01:12:37PM -0700, alarson@ddci.com wrote:
> David Gibson <david@gibson.dropbear.id.au> wrote on 06/14/2016 11:17:57 
> PM:
> Aaron Larson <alarson@ddci.com>
> 
> AL> 1. There is a defect in ppce500_spin.c:spin_kick() that creates an
> AL>    incorrectly sized TLB entry.  This was reported as bug
> AL>    https://bugs.launchpad.net/qemu/+bug/1587535  I can provide a
> AL>    patch if desired.
> 
> DG> Absolutely.
> 
> OK, I'll start with this one.   Let me know if there is anything you'd 
> like me to do
> with that bug report.
> 
> When e500 PPC is booted multi-core, the non-boot cores are started via
> the spin table.  ppce500_spin.c:spin_kick() calls
> mmubooke_create_initial_mapping() to allocate a 64MB TLB entry, but
> the created TLB entry is only 256KB.
> 
> The root cause is that the function computing the size of the TLB
> entry, namely booke206_page_size_to_tlb assumes MAS1.TSIZE as defined
> by latter PPC cores, specifically n to the power of FOUR * 1KB. The
> result is then used by mmubooke_create_initial_mapping using
> MAS1_TSIZE_SHIFT, but MAS1_TSIZE_SHIFT is defined assuming TLB entries
> are n to the power of TWO * 1KB. I.e., a difference of shift=7 or
> shift=8.
> 
> Simply changing MAS1_TSIZE_SHIFT from 7 to 8 is not appropriate since
> the macro is used elsewhere.
> 
> The following patch has a fix for that, and also raises a separate
> issue that I'd be happy to resolve after getting some guidance.

Ok, thanks for sending this, but there are some procedural things
you'll need to check before we can go ahead.  We generally prefer
patches to be sent inline, rather than as attachments (it makes adding
review comments much easier), and you'll need to apply a Signed-off-by
line before I could merge it.  There are more details on the
procedures for submitting patches at:
	http://wiki.qemu.org/Contribute/SubmitAPatch

-- 
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] [Qemu-ppc] Determining interest in PPC e500spin, yield, and openpic patches

[Thomas Huth]

On 15.06.2016 22:12, alarson@ddci.com wrote:
[...]
> The following patch has a fix for that, and also raises a separate
> issue that I'd be happy to resolve after getting some guidance.
[...]
@@ -104,6 +108,16 @@
 
     cpu_synchronize_state(cpu);
     stl_p(&curspin->pir, env->spr[SPR_PIR]);
+/* The stl_p() above seems wrong to me.  First of all, it seems more appropriate
+ * in a guest ROM/BOOT code than in qemu emulation.  However, SPR_PIR is never
+ * initialized, so the effect of the stl_p() is to overwrite the curspin->pir
+ * with 0. It makes more sense to load the SPR_PIR with the curspin->pir, which
+ * is what the following does.
+ *    env->spr[SPR_PIR]=ldl_p(&curspin->pir);
+ * Alternately SPR_PIR could be initialized from SPR_BOOKE_PIR which is properly
+ * initialized, so this could also work:
+ *    env->spr[SPR_PIR] = env->spr[SPR_BOOKE_PIR]
+*/
     env->nip = ldq_p(&curspin->addr) & (map_size - 1);
     env->gpr[3] = ldq_p(&curspin->r3);
     env->gpr[4] = 0;

I'm not very familiar with the e500 code, but as far as I understand the
ppce500_spin.c code, it provides the spin table facility from ePAPR for the
guests that is normally provided by the boot firmware instead. Some more
information why this has been done can be found in the original commit
message here:
  http://git.qemu.org/?p=qemu.git;a=commitdiff;h=5c145dacacad04f751c

So it's right to set up curspin->pir here (not the other way round), but
I think SPR_PIR was just a typo and should be SPR_BOOKE_PIR instead,
since the PIR register for BookE CPUs has the number 286 and not 1023.
So does it work for you if you simply replace the line with:

    stl_p(&curspin->pir, env->spr[SPR_BOOKE_PIR]);

?

 Thomas

[Qemu-devel] PPC e500spin pir improperly initialized

[alarson]

Note change of subject from "Determining interest in PPC e500spin,
yield".

Thomas Huth <address hidden> wrote on 06/16/2016 01:47:05 AM:
Aaron Larson <address hidden> wrote on 15.06.2016 22:12

in ppce500_spin.c

AL> @@ -104,6 +108,16 @@
AL> 
AL>      cpu_synchronize_state(cpu);
AL>      stl_p(&curspin->pir, env->spr[SPR_PIR]);
AL> +/* The stl_p() above seems wrong to me.  First of all, it seems more 
appropriate
AL> + * in a guest ROM/BOOT code than in qemu emulation.  However, SPR_PIR 
is never
AL> + * initialized, so the effect of the stl_p() is to overwrite the 
curspin->pir
AL> + * with 0. It makes more sense to load the SPR_PIR with the 
curspin->pir, which
AL> + * is what the following does.
AL> + *    env->spr[SPR_PIR]=ldl_p(&curspin->pir);
AL> + * Alternately SPR_PIR could be initialized from SPR_BOOKE_PIR which 
is properly
AL> + * initialized, so this could also work:
AL> + *    env->spr[SPR_PIR] = env->spr[SPR_BOOKE_PIR]
AL> +*/
AL>      env->nip = ldq_p(&curspin->addr) & (map_size - 1);
AL>      env->gpr[3] = ldq_p(&curspin->r3);
AL>      env->gpr[4] = 0;

TH> I'm not very familiar with the e500 code, but as far as I understand 
the
TH> ppce500_spin.c code, it provides the spin table facility from ePAPR 
for the
TH> guests that is normally provided by the boot firmware instead. Some 
more
TH> information why this has been done can be found in the original commit
TH> message here:
TH>   http://git.qemu.org/?p=qemu.git;a=commitdiff;h=5c145dacacad04f751c
TH> 
TH> So it's right to set up curspin->pir here (not the other way round), 
but
TH> I think SPR_PIR was just a typo and should be SPR_BOOKE_PIR instead,
TH> since the PIR register for BookE CPUs has the number 286 and not 1023.
TH> So does it work for you if you simply replace the line with:
TH> 
TH>     stl_p(&curspin->pir, env->spr[SPR_BOOKE_PIR]);

I verified that the spin table is properly initialized if
SPR_BOOKE_PIR is used.  However this is superfluous since spin_reset()
has already initialized the PV spin table pir.  I wasn't sure if there
was a desire to set the SPR_PIR as well for some reason.

I think any of the following would be acceptable:

1. If the SPR_PIR needs to be set for some reason (why I'm not sure),
   then set SPR_PIR to SPR_BOOKE_PIR.
2. Change SPR_PIR to SPR_BOOKE_PIR.
3. Delete the line that sets curspin->pir to SPR_PIR.

I'm fine submitting a patch for whatever solution folks deem
appropriate.

Re: [Qemu-devel] PPC e500spin pir improperly initialized

[Thomas Huth]

On 18.06.2016 02:50, alarson@ddci.com wrote:
> Note change of subject from "Determining interest in PPC e500spin,
> yield".
> 
> Thomas Huth <address hidden> wrote on 06/16/2016 01:47:05 AM:
> Aaron Larson <address hidden> wrote on 15.06.2016 22:12
> 
> in ppce500_spin.c
> 
> AL> @@ -104,6 +108,16 @@
> AL> 
> AL>      cpu_synchronize_state(cpu);
> AL>      stl_p(&curspin->pir, env->spr[SPR_PIR]);
> AL> +/* The stl_p() above seems wrong to me.  First of all, it seems more 
> appropriate
> AL> + * in a guest ROM/BOOT code than in qemu emulation.  However, SPR_PIR 
> is never
> AL> + * initialized, so the effect of the stl_p() is to overwrite the 
> curspin->pir
> AL> + * with 0. It makes more sense to load the SPR_PIR with the 
> curspin->pir, which
> AL> + * is what the following does.
> AL> + *    env->spr[SPR_PIR]=ldl_p(&curspin->pir);
> AL> + * Alternately SPR_PIR could be initialized from SPR_BOOKE_PIR which 
> is properly
> AL> + * initialized, so this could also work:
> AL> + *    env->spr[SPR_PIR] = env->spr[SPR_BOOKE_PIR]
> AL> +*/
> AL>      env->nip = ldq_p(&curspin->addr) & (map_size - 1);
> AL>      env->gpr[3] = ldq_p(&curspin->r3);
> AL>      env->gpr[4] = 0;
> 
> TH> I'm not very familiar with the e500 code, but as far as I understand 
> the
> TH> ppce500_spin.c code, it provides the spin table facility from ePAPR 
> for the
> TH> guests that is normally provided by the boot firmware instead. Some 
> more
> TH> information why this has been done can be found in the original commit
> TH> message here:
> TH>   http://git.qemu.org/?p=qemu.git;a=commitdiff;h=5c145dacacad04f751c
> TH> 
> TH> So it's right to set up curspin->pir here (not the other way round), 
> but
> TH> I think SPR_PIR was just a typo and should be SPR_BOOKE_PIR instead,
> TH> since the PIR register for BookE CPUs has the number 286 and not 1023.
> TH> So does it work for you if you simply replace the line with:
> TH> 
> TH>     stl_p(&curspin->pir, env->spr[SPR_BOOKE_PIR]);
> 
> I verified that the spin table is properly initialized if
> SPR_BOOKE_PIR is used.  However this is superfluous since spin_reset()
> has already initialized the PV spin table pir.  I wasn't sure if there
> was a desire to set the SPR_PIR as well for some reason.
> 
> I think any of the following would be acceptable:
> 
> 1. If the SPR_PIR needs to be set for some reason (why I'm not sure),
>    then set SPR_PIR to SPR_BOOKE_PIR.

SPR_PIR should not exist on a BookE processor, so I am pretty sure that
this is the wrong option.

> 2. Change SPR_PIR to SPR_BOOKE_PIR.
> 3. Delete the line that sets curspin->pir to SPR_PIR.

I don't know that code well enough, but I think both options 2 and 3
should be fine. Unless somebody has a better suggestion, I'd say go for
option 2, that sounds like the safest approach to me.

 Thomas