lkml.org 
[lkml]   [2010]   [Jun]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] x86: OLPC: add support for calling into OpenFirmware (v2)
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.



\
 
 \ /
  Last update: 2010-06-10 06:39    [W:0.029 / U:0.600 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site