[PATCH] ppc: fix stack usage in mmu_claim, mem_claim
The 'phys' argument to mem_claim() and 'virt' argument to mmu_claim() are now only popped from the stack if the 'align' argument is provided. Signed-off-by: Cormac O'Brien <i.am.cormac.obrien@gmail.com> --- arch/ppc/qemu/methods.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/arch/ppc/qemu/methods.c b/arch/ppc/qemu/methods.c index fd993da..d587dc7 100644 --- a/arch/ppc/qemu/methods.c +++ b/arch/ppc/qemu/methods.c @@ -168,17 +168,17 @@ DECLARE_NODE( mmu_ciface, 0, 0, "+/openprom/client-services" ); static void mem_claim( void ) { + phys_addr_t phys = -1UL; ucell align = POP(); ucell size = POP(); - ucell phys = POP(); - ucell ret = ofmem_claim_phys( phys, size, align ); - if( ret == -1 ) { - printk("MEM: claim failure\n"); - throw( -13 ); - return; + if (!align) { + phys = POP(); } - PUSH( ret ); + + phys = ofmem_claim_phys(phys, size, align); + + PUSH(phys); } /* ( phys size --- ) */ @@ -192,17 +192,18 @@ mem_release( void ) static void mmu_claim( void ) { - ucell align = POP(); - ucell size = POP(); - ucell phys = POP(); - ucell ret = ofmem_claim_virt( phys, size, align ); + ucell virt, size, align; + virt = -1UL; + align = POP(); + size = POP(); - if( ret == -1 ) { - printk("MMU: CLAIM failure\n"); - throw( -13 ); - return; + if (!align) { + virt = POP(); } - PUSH( ret ); + + virt = ofmem_claim_virt(virt, size, align); + + PUSH(virt); } /* ( phys size --- ) */ -- 2.4.3
On Jun 17, 2015, at 4:19 PM, Cormac O'Brien wrote:
The 'phys' argument to mem_claim() and 'virt' argument to mmu_claim() are now only popped from the stack if the 'align' argument is provided.
What's the reason for doing this?
On 2015-Jun-17 16:33 , Programmingkid wrote:
On Jun 17, 2015, at 4:19 PM, Cormac O'Brien wrote:
The 'phys' argument to mem_claim() and 'virt' argument to mmu_claim() are now only popped from the stack if the 'align' argument is provided. What's the reason for doing this?
Presumably to comply with page 127 of IEEE 1275, where "claim" is defined to not consume an address argument if an alignment is specified.
On Wed, Jun 17, 2015 at 3:38 PM, Tarl Neustaedter <tarl-b2@tarl.net> wrote:
On 2015-Jun-17 16:33 , Programmingkid wrote:
On Jun 17, 2015, at 4:19 PM, Cormac O'Brien wrote:
The 'phys' argument to mem_claim() and 'virt' argument to mmu_claim() are now only popped from the stack if the 'align' argument is provided. What's the reason for doing this?
Presumably to comply with page 127 of IEEE 1275, where "claim" is defined to not consume an address argument if an alignment is specified.
Correct, Mark remembered that this hadn't been applied to PPC yet and it was interfering with Apple's boot script.
On Wed, Jun 17, 2015 at 03:19:41PM -0500, Cormac O'Brien wrote:
The 'phys' argument to mem_claim() and 'virt' argument to mmu_claim() are now only popped from the stack if the 'align' argument is provided.
You probably also want to mention you no longer THROW when the return value is -1 (and explain why).
+ phys_addr_t phys = -1UL;
Is this the common style in this code? Will "long int" always fit phys_addr_t? Seems shaky. Segher
On 17/06/15 22:10, Segher Boessenkool wrote:
On Wed, Jun 17, 2015 at 03:19:41PM -0500, Cormac O'Brien wrote:
The 'phys' argument to mem_claim() and 'virt' argument to mmu_claim() are now only popped from the stack if the 'align' argument is provided.
You probably also want to mention you no longer THROW when the return value is -1 (and explain why).
I'm not too worried about missing the throw here right now. As OpenBIOS randomly switches between C and Forth at various points, not all of them pick up the throw/catch across the boundaries correctly and make it back to the caller. One of the more recent changes I managed to do was to unmap page 0 on all 3 of SPARC32, SPARC64 and PPC and still have everything boot. By combining this with a default value of -1 for the OFMEM allocations it means we die the first time a bad allocation is accessed, and it's easy to work out where this happened with CONFIG_DEBUG_OFMEM enabled and looking for values of -1.
+ phys_addr_t phys = -1UL;
Is this the common style in this code? Will "long int" always fit phys_addr_t? Seems shaky.
In the original OFMEM implementation sizeof(phys_addr_t) == sizeof(virt) == word size of machine. The only reason phys_addr_t is different here was since SPARC32 was switched over to OFMEM as SPARC32 has a 36-bit physical address space vs. a 32-bit virtual address space just to make things more interesting :) I haven't compile-tested this yet but if you can suggest a better alternative, please do. ATB, Mark.
On Wed, Jun 17, 2015 at 11:30:37PM +0100, Mark Cave-Ayland wrote:
The 'phys' argument to mem_claim() and 'virt' argument to mmu_claim() are now only popped from the stack if the 'align' argument is provided.
You probably also want to mention you no longer THROW when the return value is -1 (and explain why).
I'm not too worried about missing the throw here right now.
Neither am I, but it's a silent change; the commit message should say. I think the original code is simply wrong fwiw, the client interface shouldn't throw (but return an error).
One of the more recent changes I managed to do was to unmap page 0 on all 3 of SPARC32, SPARC64 and PPC and still have everything boot.
:-)
+ phys_addr_t phys = -1UL;
Is this the common style in this code? Will "long int" always fit phys_addr_t? Seems shaky.
In the original OFMEM implementation sizeof(phys_addr_t) == sizeof(virt) == word size of machine. The only reason phys_addr_t is different here was since SPARC32 was switched over to OFMEM as SPARC32 has a 36-bit physical address space vs. a 32-bit virtual address space just to make things more interesting :) I haven't compile-tested this yet but if you can suggest a better alternative, please do.
-1ULL should work. Plain -1 works fine, too. -1UL doesn't work correctly (if phys_addr_t is bigger than long int). So just -1 is my preference. Segher
On 18/06/15 00:37, Segher Boessenkool wrote:
On Wed, Jun 17, 2015 at 11:30:37PM +0100, Mark Cave-Ayland wrote:
The 'phys' argument to mem_claim() and 'virt' argument to mmu_claim() are now only popped from the stack if the 'align' argument is provided.
You probably also want to mention you no longer THROW when the return value is -1 (and explain why).
I'm not too worried about missing the throw here right now.
Neither am I, but it's a silent change; the commit message should say. I think the original code is simply wrong fwiw, the client interface shouldn't throw (but return an error).
Okay. How about we add a note mentioning that the throw is removed due to problems across multiple C/Forth boundaries and to keep everything consistent across all archs? If we do come up with a way to get this to work, we should do it consistently across all archs.
One of the more recent changes I managed to do was to unmap page 0 on all 3 of SPARC32, SPARC64 and PPC and still have everything boot.
:-)
+ phys_addr_t phys = -1UL;
Is this the common style in this code? Will "long int" always fit phys_addr_t? Seems shaky.
In the original OFMEM implementation sizeof(phys_addr_t) == sizeof(virt) == word size of machine. The only reason phys_addr_t is different here was since SPARC32 was switched over to OFMEM as SPARC32 has a 36-bit physical address space vs. a 32-bit virtual address space just to make things more interesting :) I haven't compile-tested this yet but if you can suggest a better alternative, please do.
-1ULL should work. Plain -1 works fine, too. -1UL doesn't work correctly (if phys_addr_t is bigger than long int). So just -1 is my preference.
Plain -1 it is then :) ATB, Mark.
On 17/06/15 21:19, Cormac O'Brien wrote:
The 'phys' argument to mem_claim() and 'virt' argument to mmu_claim() are now only popped from the stack if the 'align' argument is provided.
As per the IEEE1275 specification?
Signed-off-by: Cormac O'Brien <i.am.cormac.obrien@gmail.com>
--- arch/ppc/qemu/methods.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/arch/ppc/qemu/methods.c b/arch/ppc/qemu/methods.c index fd993da..d587dc7 100644 --- a/arch/ppc/qemu/methods.c +++ b/arch/ppc/qemu/methods.c @@ -168,17 +168,17 @@ DECLARE_NODE( mmu_ciface, 0, 0, "+/openprom/client-services" );
Looking at the current source there is a stack diagram just above the mem_claim() function - can we alter this too to indicate that the phys argument is optional with []s?
static void mem_claim( void ) { + phys_addr_t phys = -1UL; ucell align = POP(); ucell size = POP();
Can we preserve the order of the variable declarations here, e.g. first align, then size and finally phys? I know it sounds trivial but it helps the reader interpret the stack diagram the right way around. This is especially confusing when using the CIF when the Forth argument order and C argument order to opposite to each other...
- ucell phys = POP(); - ucell ret = ofmem_claim_phys( phys, size, align );
- if( ret == -1 ) { - printk("MEM: claim failure\n"); - throw( -13 ); - return; + if (!align) { + phys = POP(); } - PUSH( ret ); + + phys = ofmem_claim_phys(phys, size, align); + + PUSH(phys); }
/* ( phys size --- ) */
I know that this wasn't part of the patch, but while you're here it would be great if you could fix this stack diagram too :)
@@ -192,17 +192,18 @@ mem_release( void ) static void mmu_claim( void ) { - ucell align = POP(); - ucell size = POP(); - ucell phys = POP(); - ucell ret = ofmem_claim_virt( phys, size, align ); + ucell virt, size, align; + virt = -1UL; + align = POP(); + size = POP();
Any reason why the initialisation is different from the one above? For consistency with all the other archs I'd prefer the format you use in the above mem_claim() function.
- if( ret == -1 ) { - printk("MMU: CLAIM failure\n"); - throw( -13 ); - return; + if (!align) { + virt = POP(); } - PUSH( ret ); + + virt = ofmem_claim_virt(virt, size, align); + + PUSH(virt); }
/* ( phys size --- ) */
Also what effect does this have in your boot tests against your hacking branch? I think I got confused in our last meeting between the old branch and new branch :) ATB, Mark.
participants (5)
-
Cormac O'Brien -
Mark Cave-Ayland -
Programmingkid -
Segher Boessenkool -
Tarl Neustaedter