|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 1/2] xen/arm: Rework the way to compute dom0 DTB base address
On Thu, 2013-05-30 at 15:05 +0100, Julien Grall wrote:
> If the DTB is loading right the after the kernel, on some setup, Linux will
> overwrite the DTB during the decompression step.
>
> To be sure the DTB won't be overwritten by the decompression stage, load
> the DTB near the end of the first memory bank and below 4Gib (if memory range
> is
> greater).
>
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
>
> Changes in v2:
> - Align the DTB base address to 2Mib
> - Add dtb_check_overlap to check if the kernel will overlap the DTB
> - Fix typo
> ---
> xen/arch/arm/domain_build.c | 51
> ++++++++++++++++++++++++++++++++++++++-----
> xen/arch/arm/kernel.c | 2 ++
> xen/arch/arm/kernel.h | 7 ++++++
> 3 files changed, 54 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b92c64b..515fabe 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -477,6 +477,7 @@ static int prepare_dtb(struct domain *d, struct
> kernel_info *kinfo)
> void *fdt;
> int new_size;
> int ret;
> + paddr_t end;
>
> kinfo->unassigned_mem = dom0_mem;
>
> @@ -502,17 +503,25 @@ static int prepare_dtb(struct domain *d, struct
> kernel_info *kinfo)
> goto err;
>
> /*
> - * Put the device tree at the beginning of the first bank. It
> - * must be below 4 GiB.
> + * DTB must be load below 4GiB and far enough to linux (Linux use
^from ^uses
> + * the space after it to decompress)
> + * Load the DTB at the end of the first bank or below 4Gib
and below?
Perhaps clearer to write "Load the DTB at the end of the first bank,
while ensure it is also below 4G"
> */
> - kinfo->dtb_paddr = kinfo->mem.bank[0].start + 0x100;
> - if ( kinfo->dtb_paddr + fdt_totalsize(kinfo->fdt) > (1ull << 32) )
> + end = kinfo->mem.bank[0].start + kinfo->mem.bank[0].size;
> + end = MIN(1ull << 32, end);
> + kinfo->dtb_paddr = end - fdt_totalsize(kinfo->fdt);
> + /* Linux requires the address to be aligned to 2Mb */
Does it? I don't disagree with aligning it to 2MB but I'm sure that e.g.
CONFIG_APPENDED_DTB handles the unaligned case (perhaps that is
special)?
> + kinfo->dtb_paddr &= ~((2 << 20) - 1);
> +
> + if ( fdt_totalsize(kinfo->fdt) > end )
> {
> - printk("Not enough memory below 4 GiB for the device tree.");
> + printk(XENLOG_ERR "Not enough memory in the first bank for "
> + "the device tree.");
> ret = -FDT_ERR_XEN(EINVAL);
> goto err;
> }
>
> +
> return 0;
>
> err:
> @@ -521,9 +530,36 @@ static int prepare_dtb(struct domain *d, struct
> kernel_info *kinfo)
> return -EINVAL;
> }
>
> +static int dtb_check_overlap(struct kernel_info *kinfo)
> +{
> + paddr_t zimage_start = kinfo->zimage.load_addr;
> + paddr_t zimage_end = kinfo->zimage.load_addr + kinfo->zimage.len;
> + paddr_t dtb_start = kinfo->dtb_paddr;
> + paddr_t dtb_end = kinfo->dtb_paddr + fdt_totalsize(kinfo->fdt);
> +
> + /*
> + * Check the kernel won't overlap the kernel
> + * Only when it's a ZIMAGE
> + */
> + if ( kinfo->type != KERNEL_ZIMAGE )
> + return 0;
> +
> + if ( !(MAX(zimage_start, dtb_end) < MIN(zimage_end, dtb_start)) )
> + return 0;
I had to draw quite a few pictures to convince myself this was right,
and I'm still not entirely sure ;-)
> +
> + printk(XENLOG_ERR "The kernel(0x%"PRIpaddr"-0x%"PRIpaddr
> + ") is overlapping the DTB(0x%"PRIpaddr"-0x%"PRIpaddr":\n",
Missing a closing ) on the DTB range. Also why the trailing ":"?
> + zimage_start, zimage_end, dtb_start, dtb_end);
> + xfree(kinfo->fdt);
This seems like an odd place to free this.
Returning an error here is only going to cause us to give up and panic
anyway. I'd suggest to just panic here directly and not bother
propagating an error code.
Not really sure why construct_dom0 has a return code at all, instead of
it (or the functions it calls) just panicing. But lets not worry about
that now.
> +
> + return -EINVAL;
> +}
> +
> static void dtb_load(struct kernel_info *kinfo)
> {
> void * __user dtb_virt = (void * __user)(register_t)kinfo->dtb_paddr;
Blank line here please.
> + printk("Loading dom0 DTB to 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
> + kinfo->dtb_paddr, kinfo->dtb_paddr + fdt_totalsize(kinfo->fdt));
>
> raw_copy_to_guest(dtb_virt, kinfo->fdt, fdt_totalsize(kinfo->fdt));
> xfree(kinfo->fdt);
> @@ -559,10 +595,13 @@ int construct_dom0(struct domain *d)
> if ( rc < 0 )
> return rc;
>
> + rc = dtb_check_overlap(&kinfo);
> + if ( rc < 0 )
> + return rc;
> +
> /* The following loads use the domain's p2m */
> p2m_load_VTTBR(d);
>
> - kinfo.dtb_paddr = kinfo.zimage.load_addr + kinfo.zimage.len;
> kernel_load(&kinfo);
> dtb_load(&kinfo);
>
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 8f4a60d..f8c8850 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -152,6 +152,7 @@ static int kernel_try_zimage_prepare(struct kernel_info
> *info,
>
> info->entry = info->zimage.load_addr;
> info->load = kernel_zimage_load;
> + info->type = KERNEL_ZIMAGE;
>
> return 0;
> }
> @@ -193,6 +194,7 @@ static int kernel_try_elf_prepare(struct kernel_info
> *info,
> */
> info->entry = info->elf.parms.virt_entry;
> info->load = kernel_elf_load;
> + info->type = KERNEL_ELF;
>
> return 0;
> err:
> diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
> index 1776a4d..b4ecd30 100644
> --- a/xen/arch/arm/kernel.h
> +++ b/xen/arch/arm/kernel.h
> @@ -9,6 +9,12 @@
> #include <xen/libelf.h>
> #include <xen/device_tree.h>
>
> +enum kernel_type
> +{
> + KERNEL_ELF,
> + KERNEL_ZIMAGE,
> +};
> +
> struct kernel_info {
> #ifdef CONFIG_ARM_64
> enum domain_type type;
> @@ -23,6 +29,7 @@ struct kernel_info {
>
> void *kernel_img;
> unsigned kernel_order;
> + enum kernel_type type;
>
> union {
> struct {
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |