Re: [Xen-devel] [PATCH ARM v5 19/20] mini-os: initial ARM support

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-06-30 at 22:08 +0100, Julien Grall wrote:

> >>> diff --git a/extras/mini-os/arch/arm/arm32.S
> >>> b/extras/mini-os/arch/arm/arm32.S
> >>> new file mode 100644
> >>> index 0000000..de74ed9
> >>> --- /dev/null
> >>> +++ b/extras/mini-os/arch/arm/arm32.S
> >>> @@ -0,0 +1,266 @@
> >>> +@ Virtual address of the start of RAM (any value will do, but it must be
> >>> +@ section-aligned). Update the lds script if changed.
> >>> +#define VIRT_BASE 0x400000
> >>> +
> >>> +@ Offset of the kernel within the RAM. This is a zImage convention which
> >>> we
> >>> +@ rely on.
> >>> +#define ZIMAGE_KERNEL_OFFSET 0x8000
> >>
> >>
> >> Hmmm... this is not a zImage convention... IIRC Linux is using this offset
> >> to have enough space to create startup page table during boot.
> >
> > OK, so this is a convention of Linux only? I found this reference:
> >
> > http://www.simtec.co.uk/products/SWLINUX/files/booting_article.html
> >
> > "Despite the ability to place zImage anywhere within memory,
> > convention has it that it is loaded at the base of physical RAM plus
> > an offset of 0x8000 (32K). This leaves space for the parameter block
> > usually placed at offset 0x100, zero page exception vectors and page
> > tables. This convention is *very* common."

Documentation/arm/Booting is the more canonical reference here.

> I think this convention is for kernel which are able to load only in a 
> specific address (i.e CONFIG_AUTO_ZRELADDR=n). Linux Xen guest need to 
> have this option set to be able to boot. So Linux will calculate the 
> relocation address itself:
> 
> #ifdef CONFIG_AUTO_ZRELADDR
>                  @ determine final kernel image address
>                  mov     r4, pc
>                  and     r4, r4, #0xf8000000
>                  add     r4, r4, #TEXT_OFFSET
> #else
>                  ldr     r4, =zreladdr
> #endif
> 
> TEXT_OFFSET is equal to 0x8000 here.
> 
> This code lives in Linux since at least 2010 (last commit in git blame 
> arch/arm/boot/compressed/head.S).
> 
> Futhermore, for DOM0 we are using a 2MB-aligned address. I think it's 
> time to make the same thing for the guest

Guest OSes are entitled to expect that we will load them in accordance
with the Linux zImage protocol, nothing more or less. Likewise a guest
which relies on something which is not guaranteed by that spec is buggy.

(By spec I mean the combination of Documentation/arm/Booting and the
various comments in the Linux code, which are the closest we come to a
formal spec here)

So it would be wrong for a guest kernel to indicate (via offset 0x24 in
the header) that it is relocatable while at the same time assuming that
it is loaded at either a 0x8000 offset or a 2MB aligned address. It's
either PIC on entry or it isn't (in which case offset 0x24 should give
an explicit load address).

(Xen itself probably violates this, by assuming 2MB alignment. We are in
the wrong and I expect that eventually it will bite us in the ass on
some platform/bootloader and then we'll have to fix it, not complain
that it used to work with older platforms/bootloaders. The same is true
of any guests which make similar assumptions IMHO -- i.e. it's their bug
when it breaks because we changed Xen)

>  (see the patch I sent earlier 
> today: https://patches.linaro.org/32742/).
> 
> This will also help to add support more easily for Xen in new OS.
> 
> >> The Linux zImage is able to relocate itself in the memory to respect this
> >> convention. But the zImage itself can be loaded anywhere in the memory.
> >>
> >> By looking to your code below your are relying that the kernel will be
> >> loaded at 0xXXXX8000 which is incorrect. This offset is odd and make other
> >> kernel (such as FreeBSD) requiring the same trick which is not part of the
> >> Linux boot protocol.
> >
> > Sorry, I don't understand this. You're saying that Xen's choice of
> > 0x8000 forces FreeBSD to support this offset too? But that isn't
> > caused by anything in Mini-OS.

I don't know what Julien was saying, but as far as I'm concerned FreeBSD
needs to either be PIC on entry or give a specific load address (or make
buggy assumptions about Xen's current guest loader behaviour).

> >>> +                fdt_node_check_compatible(device_tree, node,
> >>> "arm,cortex-a7-gic")) {
> >>> +                printk("Skipping incompatible interrupt-controller
> >>> node\n");
> >>> +                continue;
> >>> +            }
> >>> +
> >>> +            const uint64_t *reg = fdt_getprop(device_tree, node, "reg",
> >>> &len);
> >>> +            if (reg == NULL || len != 32) {
> >>
> >>
> >> As asked on the previous version, if you plan to assume specific range size
> >> for the time-being, please explain the 32.
> >
> > OK, so we have two registers (GICC and GICD), each of which contains
> > two parts (an address and a size), each of which is a 64-bit value (8
> > bytes). So 2 * 2 * 8 = 32. Should I check this as a minimum, or
> > require it to match exactly?
> 
> Match exactly is better. It will catch easily any change in the device 
> tree provided by the toolstack.

It would be perfectly reasonable in the future for a DT binding to be
enhanced such that a new compatiblity string was defined which had a
superset of the registers of the existing string.

You should check for whatever minimum the compatibility string you
actually know about says and ignore any additional registers.

(I don't know how likely this is for the GIC, but as a general principal
it's what should be done)

Ian.


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