Re: [Xen-devel] [PATCH RFC 19/19] Add EFI stub for ARM64

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-07-09 at 12:15 -0700, Roy Franz wrote:
> On Wed, Jul 2, 2014 at 5:34 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> > On Sat, 2014-06-28 at 02:25 +0100, Roy Franz wrote:
> >> This patch adds the real EFI application entry point in head.S,
> >> and adds the efi.c file that will contain the EFI stub implementation.
> >> The assembly wrapper is responsible for returning the processor state
> >> to what XEN expects when starting.  The EFI stub processes the
> >> XEN EFI configuration file to load the dom0 kernel, ramdisk, etc and
> >> constructs a device tree for XEN to use, then transfers control to
> >> the normal XEN image entry point.
> >>
> >> Signed-off-by: Roy Franz <roy.franz@xxxxxxxxxx>
> >> ---
> >>  xen/arch/arm/Makefile     |   2 +
> >>  xen/arch/arm/arm64/head.S |  62 ++++-
> >>  xen/arch/arm/efi-shared.c |   1 +
> >>  xen/arch/arm/efi.c        | 686 
> >> ++++++++++++++++++++++++++++++++++++++++++++++
> >>  4 files changed, 750 insertions(+), 1 deletion(-)
> >>  create mode 120000 xen/arch/arm/efi-shared.c
> >>  create mode 100644 xen/arch/arm/efi.c
> >>
> >> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> >> index 63e0460..c3ed74f 100644
> >> --- a/xen/arch/arm/Makefile
> >> +++ b/xen/arch/arm/Makefile
> >> @@ -33,6 +33,8 @@ obj-y += hvm.o
> >>  obj-y += device.o
> >>  obj-y += decode.o
> >>  obj-y += processor.o
> >> +obj-y += efi.o
> >> +obj-y += efi-shared.o
> >
> > While the latter will eventually be in xen/common/efi/something.c and
> > hence -fshort-char is taken care of I suppose efi.o needs it too?
> >
> > Probably the easiest thing to do there is to create
> > xen/arch/arm/efi/stub.c and do the fshort-char trick for that entire
> > folder, since I don't think our build system makes it especially easy to
> > change CFLAGS for individual object files.
> 
> Yes and yes.  I will likely end up with some single file directories due to 
> the
> build option differences.  Is there a XEN build system expert that I can ask
> for help if needed?  (Or should I just post to the list?)

I'm not sure anyone is an expert on this stuff but feel free to prod me
either here or on #xenarm.

I think you just need something like, in xen/arch/arm/Makefile
   subdir-$(arm64) += efi # or CONFIG_EFI etc
and in xen/arch/arm/efi/Makefile:
   CFLAGS += -fshort-wchar
   obj-y += foo.o etc.o

much the same for xen/common/efi.

> >
> >> +
> >> +     /*
> >> +      * efi_entry() will return here with device tree address in x0.
> >> +      *  Save value in register which is preserved by __flush_dcache_all.
> >> +      */
> >> +     mov     x20, x0
> >> +
> >> +     /* Turn off Dcache and MMU */
> >
> > AIUI EFI leaves us in a 1:1 mapping, hence this is safe to do.
> 
> Yes, EFI specifies a 1:1 mapping.

Could you append to the comment ", we are running on EFI's 1:1 mapping"
or something please.

>   The assembly wrapper here is the same as we
> use for Linux, so it still needs some tweaks.  Linux expects MMU/DCACHE off.

Xen's main entry point is the same. I was wondering recently about
allowing the stub to enter it with them on, I think all that would be
needed is some cache maintenance as we build the boot page tables
(which would be harmless in the zImage case).

I think that's very much a thing for a future enhancement though...

> >
> >> +     bic     x0, x0, #1 << 0 // clear SCTLR.M
> >> +     bic     x0, x0, #1 << 2 // clear SCTLR.C
> >> +     msr     sctlr_el1, x0
> >> +     isb
> >> +2:
> >> +/*   bl      __flush_dcache_all */  /* RFRANZ_TODO */
> >
> > We just don't have this function right now, correct? I suppose you are
> > running on models without cache modelling turned on?
> >
> > Eventually this will implement a total by set/way, right? That's safe
> > because we are uniprocessor at this point etc etc.
> 
> This works on the model since I'm not modeling the cache, but we do need it
> if we are going to turn the dcache off.  If we can leave the dcache enabled
> for XEN then I don't think we need to flush it here.

It's up to you I think, if you want to just fill in __flush_dcache_all
(which I expect can be cribbed from Linux) that's fine, but if you
really want to you could instead put the necessary flushes into head.S
to make it safe to run with or without mmu+caches.

> >> + * has been added to allow review and some testing of the stub code while
> >> + * the XEN kernel code is being developed.
> >> + */
> >> +#define ADD_EFI_MEMORY_TO_FDT
> >> +
> >> +#define DEVICE_TREE_GUID \
> >> +{0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 
> >> 0xe0}}
> >> +
> >> +extern CHAR16 __initdata newline[];
> >> +extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
> >> +extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdERR;
> >> +extern EFI_BOOT_SERVICES *__initdata efi_bs;
> >> +
> >> +
> >> +static EFI_HANDLE __initdata efi_ih;
> >> +
> >> +static struct file __initdata cfg;
> >> +static struct file __initdata kernel;
> >> +static struct file __initdata ramdisk;
> >> +static struct file __initdata dtb;
> >> +
> >> +static unsigned long mmap_size;
> >> +static EFI_MEMORY_DESCRIPTOR *mmap_ptr;
> >> +
> >> +static void *new_fdt;
> >> +
> >> +#ifdef ADD_EFI_MEMORY_TO_FDT
> >> +static int IsLinuxReservedRegion(int MemoryType)
> >
> > Linux? I guess this is a hangover from the Linux stub?
> >
> > (Even there I'd have thought the regions were reserved by EFI not Linux
> > though, perhaps I'm looking at it backwards though)
> 
> Since this is part of a temporary hack, I didn't rename anything :)

Ah yes, the #ifdef right above should have clued me in to that!

> >> +/*
> >> + * Set a single 'reg' property taking into account the
> >> + * configured addr and size cell sizes.
> >> + */
> >> +static int fdt_set_reg(void *fdt, int node, int addr_cells, int 
> >> size_cells,
> >> +                       uint64_t addr, uint64_t len)
> >
> > FWIW I think we have such a helper already somewhere, but perhaps this
> > file with -fshort-char etc is unable to call it?
> 
> I'll try to find it.  I think it should be callable, since wide
> characters aren't used
> within this function of in its interface.

I just had a look and I think I imagined it...

There is one in the userspace domain builder, but that is no use to you
here.

I expect that xen/arch/arm/domain_build.c could make use of this helper
too, but don't worry about that in this series.

> >> +{
> >> +    uint8_t data[16]; /* at most 2 64 bit words */
> >> +    void *p = data;
> >> +
> >> +    /* Make sure that the values provided can be represented in
> >> +     * the reg property.
> >> +     */
> >> +    if (addr_cells == 1 && (addr >> 32))
> >
> > Xen coding style puts spaces more often than you may be used to. In
> > particular inside the brackets of an if (and while etc). So here:
> >         if ( addr_cells == 1 && ( addr >> 32 ) )
> >
> I tried to be good about that :)  I was hoping for a checkpatch script
> to catch all that kind of stuff for me :)

I'm afraid I don't think we have one :-(

> I'll review this and the use of tabs again.

Thanks. 

>  Thanks Ian, Stefano and Jan for all your feedback.

Thanks for you work on this!

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel