Re: [OpenBIOS] [PATCH] ppc: fix stack usage in mmu_claim, mem_claim

[Mark Cave-Ayland]

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
>
> 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.