Re: [PATCH] x86: OLPC: add support for calling into OpenFirmware (v2)

["H. Peter Anvin" <hpa@zytor.com>]

On 06/09/2010 09:14 PM, Andres Salomon wrote:
> 
> Add support for saving OFW's cif, and later calling into it to run OFW
> commands.  OFW remains resident in memory, living within virtual range
> 0xff800000 - 0xffc00000.  A single page directory entry points to the
> pgdir that OFW actually uses, so rather than saving the entire page
> table, we save that one entry (and restore it for the call into OFW).
> 
> This is currently only used by the OLPC XO; however, there's nothing
> restricting OFW's usage on other (x86) platforms.

... well, except for the fact that the protocol is insane, and not used
by anything else ...

> diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
> index 86b1506..5cba9eb 100644
> --- a/arch/x86/include/asm/setup.h
> +++ b/arch/x86/include/asm/setup.h
> @@ -21,6 +21,7 @@
>  #define OLD_CL_MAGIC		0xA33F
>  #define OLD_CL_ADDRESS		0x020	/* Relative to real mode data */
>  #define NEW_CL_POINTER		0x228	/* Relative to real mode data */
> +#define OLPC_OFW_INFO_OFFSET	0xB0	/* Relative to real mode data */

This should be added to struct boot_params as well as the various
documentation files.  I note also that this interrupts one of the
largest available spans we have in this structure, but I guess there is
very little that can be done about that.

> +#ifdef CONFIG_OLPC_OPENFIRMWARE
> +	movl $0x0, pa(olpc_ofw_cif)
> +

We just cleared BSS -- there is no point in doing this.

> +	/* Did OpenFirmware boot us? */
> +	movl $pa(boot_params) + OLPC_OFW_INFO_OFFSET, %ebp
> +	cmpl $0x2057464F, (%ebp)	/* Magic number "OFW " */
> +	jne 3f
> +

This stuff is in high memory, or otherwise protected in the memory map,
no?  If so, there is absolutely no point in doing this this early; it
can be done in C code just fine.  The only thing that needs to be done
is to same the value of %cr3 on entry (and not even the offset value of
%cr3).

> +	/* Save the callback address for calling into OFW from linux */
> +	mov 0x8(%ebp), %eax
> +	mov %eax, pa(olpc_ofw_cif)

Again, please put the definition of the entire structure into struct
boot_params as well as in the relevant documentation files.

It's really too bad that you didn't re-use the location used for struct
efi_info since that is mutually exclusive and has a signature.

I won't pick on you for not using the platform ID since that is a rather
new invention, but it would have been beneficial rather than ad hoc
inventions all along...

> +/* setup and do actual call into OFW */
> +static int setup_ofw(int *ofw_args)
> +{
> +	pgd_t *base, *pde;
> +	pgdval_t oldval;
> +	int ret;
> +
> +	/* temporarily clobber %cr3[OLPC_OFW_PDE_NR] w/ olpc_ofw_pgd */
> +	base = __va(read_cr3());
> +	pde = &base[OLPC_OFW_PDE_NR];
> +	oldval = pgd_val(*pde);
> +	set_pgd(pde, __pgd(olpc_ofw_pgd));
> +	flush_tlb();
> +
> +	/* call into ofw */
> +	ret = olpc_ofw_cif(ofw_args);
> +
> +	/* restore %cr3[OLPC_OFW_PDE_NR] */
> +	set_pgd(pde, __pgd(oldval));
> +	flush_tlb();
> +
> +	return ret;
> +}

Why are you still mucking around with swapping %cr3s?  Once you have
claimed top of the virtual address space, you can install your PGD
permanently.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.