[PATCH v3 00/14] xen: arm: 64-bit guest support and domU FDT autogeneration

[PATCH v3 00/14] xen: arm: 64-bit guest support and domU FDT autogeneration

[Ian Campbell]

I've now tested on 32-bit and found that I broke dom0 boot by not
setting dtb_paddr early enough. Patch #3 has been updated to fix this. I
also downgraded some overzealous LOG(INFO, ....) from the dtb building
code.

I can now boot 32- and 64-bit guests without using an appended DTB.

Ian J, I've CC you here since I realised some of the later tools patches
are not 100% ARM specific and could do with an eye from you.

Cheers,
Ian.

[PATCH v3 01/14] xen: arm: Report aarch64 capability.

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/setup.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 1081b43..cdcc2e7 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -731,6 +731,10 @@ void arch_get_xen_caps(xen_capabilities_info_t *info)
 
     (*info)[0] = '\0';
 
+#ifdef CONFIG_ARM_64
+    snprintf(s, sizeof(s), "xen-%d.%d-aarch64 ", major, minor);
+    safe_strcat(*info, s);
+#endif
     snprintf(s, sizeof(s), "xen-%d.%d-armv7l ", major, minor);
     safe_strcat(*info, s);
 }
-- 
1.7.10.4

[PATCH v3 02/14] xen: arm: Add comment regard arm64 zImage v0 vs v1

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/kernel.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 315d12c..7036d94 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -139,6 +139,7 @@ static int kernel_try_zimage64_prepare(struct kernel_info *info,
         uint64_t text_offset;  /* Image load offset */
         uint64_t res1;
         uint64_t res2;
+        /* zImage V1 only from here */
         uint64_t res3;
         uint64_t res4;
         uint64_t res5;
-- 
1.7.10.4

[PATCH v3 03/14] xen: arm: allocate dom0 memory separately from preparing the dtb

[Ian Campbell]

Mixing these two together is a pain, it forces us to prepare the dtb before
processing the kernel which means we don't know whether the guest is 32- or
64-bit while we construct its DTB.

Instead split out the memory allocation (including 1:1 workaround handling)
and p2m setup into a separate phase and then create a memory node in the DTB
based on the result.

This allows us to move kernel parsing before DTB setup.

As part of this it was also necessary to rework where the decision regarding
the placement of the DTB and initrd in RAM was made. It is now made when
loading the kernel, which allows it to make use of the zImage/ELF specific
information and therefore to make decisions based on complete knowledge and do
it right rather than guessing in prepare_dtb and relying on a later check to
see if things worked.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v3: Also rework module placement, v2 broke boot because dtb_paddr wasn't set
    soon enough. This ends up cleaner anyway.
v2: Fixed typo in the commit log
    Handle multiple memory nodes as well as individual nodes with several
    entries in them.
    Strip the original memory node and recreate rather than trying to modify.
---
 xen/arch/arm/domain_build.c |  198 ++++++++++++++++++++++---------------------
 xen/arch/arm/kernel.c       |   80 +++++++++++------
 xen/arch/arm/kernel.h       |    2 -
 3 files changed, 155 insertions(+), 125 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c644be2..ed09f5c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -63,11 +63,8 @@ struct vcpu *__init alloc_dom0_vcpu0(void)
     return alloc_vcpu(dom0, 0, 0);
 }
 
-static int set_memory_reg_11(struct domain *d, struct kernel_info *kinfo,
-                             const struct dt_property *pp,
-                             const struct dt_device_node *np, __be32 *new_cell)
+static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo)
 {
-    int reg_size = dt_cells_to_size(dt_n_addr_cells(np) + dt_n_size_cells(np));
     paddr_t start;
     paddr_t size;
     struct page_info *pg = NULL;
@@ -98,53 +95,61 @@ static int set_memory_reg_11(struct domain *d, struct kernel_info *kinfo,
     if ( res )
         panic("Unable to add pages in DOM0: %d\n", res);
 
-    dt_set_range(&new_cell, np, start, size);
-
     kinfo->mem.bank[0].start = start;
     kinfo->mem.bank[0].size = size;
     kinfo->mem.nr_banks = 1;
 
-    return reg_size;
+    kinfo->unassigned_mem -= size;
 }
 
-static int set_memory_reg(struct domain *d, struct kernel_info *kinfo,
-                          const struct dt_property *pp,
-                          const struct dt_device_node *np, __be32 *new_cell)
+static void allocate_memory(struct domain *d, struct kernel_info *kinfo)
 {
-    int reg_size = dt_cells_to_size(dt_n_addr_cells(np) + dt_n_size_cells(np));
-    int l = 0;
+
+    struct dt_device_node *memory = NULL;
+    const void *reg;
+    u32 reg_len, reg_size;
     unsigned int bank = 0;
-    u64 start;
-    u64 size;
-    int ret;
 
     if ( platform_has_quirk(PLATFORM_QUIRK_DOM0_MAPPING_11) )
-        return set_memory_reg_11(d, kinfo, pp, np, new_cell);
+        return allocate_memory_11(d, kinfo);
 
-    while ( kinfo->unassigned_mem > 0 && l + reg_size <= pp->length
-            && kinfo->mem.nr_banks < NR_MEM_BANKS )
+    while ( (memory = dt_find_node_by_type(memory, "memory")) )
     {
-        ret = dt_device_get_address(np, bank, &start, &size);
-        if ( ret )
-            panic("Unable to retrieve the bank %u for %s\n",
-                  bank, dt_node_full_name(np));
-
-        if ( size > kinfo->unassigned_mem )
-            size = kinfo->unassigned_mem;
-        dt_set_range(&new_cell, np, start, size);
-
-        printk("Populate P2M %#"PRIx64"->%#"PRIx64"\n", start, start + size);
-        if ( p2m_populate_ram(d, start, start + size) < 0 )
-            panic("Failed to populate P2M\n");
-        kinfo->mem.bank[kinfo->mem.nr_banks].start = start;
-        kinfo->mem.bank[kinfo->mem.nr_banks].size = size;
-        kinfo->mem.nr_banks++;
-        kinfo->unassigned_mem -= size;
-
-        l += reg_size;
-    }
+        int l;
+
+        DPRINT("memory node\n");
+
+        reg_size = dt_cells_to_size(dt_n_addr_cells(memory) + dt_n_size_cells(memory));
 
-    return l;
+        reg = dt_get_property(memory, "reg", &reg_len);
+        if ( reg == NULL )
+            panic("Memory node has no reg property!\n");
+
+        for ( l = 0;
+              kinfo->unassigned_mem > 0 && l + reg_size <= reg_len
+                  && kinfo->mem.nr_banks < NR_MEM_BANKS;
+              l += reg_size )
+        {
+            paddr_t start, size;
+
+            if ( dt_device_get_address(memory, bank, &start, &size) )
+                panic("Unable to retrieve the bank %u for %s\n",
+                      bank, dt_node_full_name(memory));
+
+            if ( size > kinfo->unassigned_mem )
+                size = kinfo->unassigned_mem;
+
+            printk("Populate P2M %#"PRIx64"->%#"PRIx64"\n",
+                   start, start + size);
+            if ( p2m_populate_ram(d, start, start + size) < 0 )
+                panic("Failed to populate P2M\n");
+            kinfo->mem.bank[kinfo->mem.nr_banks].start = start;
+            kinfo->mem.bank[kinfo->mem.nr_banks].size = size;
+            kinfo->mem.nr_banks++;
+
+            kinfo->unassigned_mem -= size;
+        }
+    }
 }
 
 static int write_properties(struct domain *d, struct kernel_info *kinfo,
@@ -193,23 +198,6 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
                 continue;
             }
         }
-        /*
-         * In a memory node: adjust reg property.
-         * TODO: handle properly memory node (ie: device_type = "memory")
-         */
-        else if ( dt_node_name_is_equal(np, "memory") )
-        {
-            if ( dt_property_name_is_equal(pp, "reg") )
-            {
-                new_data = xzalloc_bytes(pp->length);
-                if ( new_data  == NULL )
-                    return -FDT_ERR_XEN(ENOMEM);
-
-                prop_len = set_memory_reg(d, kinfo, pp, np,
-                                          (__be32 *)new_data);
-                prop_data = new_data;
-            }
-        }
 
         res = fdt_property(kinfo->fdt, pp->name, prop_data, prop_len);
 
@@ -286,6 +274,46 @@ static int fdt_property_interrupts(void *fdt, gic_interrupt_t *intr,
     return res;
 }
 
+static int make_memory_node(struct domain *d,
+                            void *fdt, const struct dt_device_node *np,
+                            const struct kernel_info *kinfo)
+{
+    int res, i;
+    int reg_size = dt_cells_to_size(dt_n_addr_cells(np) + dt_n_size_cells(np));
+    __be32 reg[reg_size*kinfo->mem.nr_banks];
+    __be32 *cells;
+
+    DPRINT("Create memory node\n");
+
+    /* ePAPR 3.4 */
+    res = fdt_begin_node(fdt, "memory");
+    if ( res )
+        return res;
+
+    res = fdt_property_string(fdt, "device_type", "memory");
+    if ( res )
+        return res;
+
+    cells = &reg[0];
+    for ( i = 0 ; i < kinfo->mem.nr_banks; i++ )
+    {
+        u64 start = kinfo->mem.bank[i].start;
+        u64 size = kinfo->mem.bank[i].size;
+
+        DPRINT("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
+                i, start, start + size);
+
+        dt_set_range(&cells, np, start, size);
+    }
+
+    res = fdt_property(fdt, "reg", reg, reg_size*kinfo->mem.nr_banks);
+    if ( res )
+        return res;
+
+    res = fdt_end_node(fdt);
+
+    return res;
+}
 
 static int make_hypervisor_node(void *fdt, const struct dt_device_node *parent)
 {
@@ -690,6 +718,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
         DT_MATCH_COMPATIBLE("xen,multiboot-module"),
         DT_MATCH_COMPATIBLE("arm,psci"),
         DT_MATCH_PATH("/cpus"),
+        DT_MATCH_TYPE("memory"),
         DT_MATCH_GIC,
         DT_MATCH_TIMER,
         { /* sentinel */ },
@@ -754,6 +783,10 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
 
     if ( np == dt_host )
     {
+        res = make_memory_node(d, kinfo->fdt, np, kinfo);
+        if ( res )
+            return res;
+
         res = make_hypervisor_node(kinfo->fdt, np);
         if ( res )
             return res;
@@ -785,14 +818,9 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
     const void *fdt;
     int new_size;
     int ret;
-    paddr_t end;
-    paddr_t initrd_len;
-    paddr_t dtb_len;
 
     ASSERT(dt_host && (dt_host->sibling == NULL));
 
-    kinfo->unassigned_mem = dom0_mem;
-
     fdt = device_tree_flattened;
 
     new_size = fdt_totalsize(fdt) + DOM0_FDT_EXTRA_SIZE;
@@ -814,36 +842,6 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
     if ( ret < 0 )
         goto err;
 
-    /* Align DTB and initrd size to 2Mb. Linux only requires 4 byte alignment */
-    initrd_len = ROUNDUP(early_info.modules.module[MOD_INITRD].size, MB(2));
-    dtb_len = ROUNDUP(fdt_totalsize(kinfo->fdt), MB(2));
-    new_size = initrd_len + dtb_len;
-
-    /*
-     * DTB must be loaded such that it does not conflict with the
-     * kernel decompressor. For 32-bit Linux Documentation/arm/Booting
-     * recommends just after the 128MB boundary while for 64-bit Linux
-     * the recommendation in Documentation/arm64/booting.txt is below
-     * 512MB. Place at 128MB, (or, if we have less RAM, as high as
-     * possible) in order to satisfy both.
-     * If the bootloader provides an initrd, it will be loaded just
-     * after the DTB.
-     */
-    end = kinfo->mem.bank[0].start + kinfo->mem.bank[0].size;
-    end = MIN(kinfo->mem.bank[0].start + (128<<20) + new_size, end);
-
-    kinfo->initrd_paddr = end - initrd_len;
-    kinfo->dtb_paddr = kinfo->initrd_paddr - dtb_len;
-
-    if ( kinfo->dtb_paddr < kinfo->mem.bank[0].start ||
-         kinfo->mem.bank[0].start + new_size > end )
-    {
-        printk(XENLOG_ERR "Not enough memory in the first bank for "
-               "the device tree.");
-        ret = -FDT_ERR_XEN(EINVAL);
-        goto err;
-    }
-
     return 0;
 
   err:
@@ -938,11 +936,19 @@ int construct_dom0(struct domain *d)
 
     d->max_pages = ~0U;
 
-    rc = prepare_dtb(d, &kinfo);
+    kinfo.unassigned_mem = dom0_mem;
+
+    allocate_memory(d, &kinfo);
+
+    rc = kernel_prepare(&kinfo);
     if ( rc < 0 )
         return rc;
 
-    rc = kernel_prepare(&kinfo);
+#ifdef CONFIG_ARM_64
+    d->arch.type = kinfo.type;
+#endif
+
+    rc = prepare_dtb(d, &kinfo);
     if ( rc < 0 )
         return rc;
 
@@ -950,9 +956,6 @@ int construct_dom0(struct domain *d)
     if ( rc < 0 )
         return rc;
 
-    if ( kinfo.check_overlap )
-        kinfo.check_overlap(&kinfo);
-
     /* The following loads use the domain's p2m */
     p2m_load_VTTBR(d);
 #ifdef CONFIG_ARM_64
@@ -963,6 +966,10 @@ int construct_dom0(struct domain *d)
         WRITE_SYSREG(READ_SYSREG(HCR_EL2) | HCR_RW, HCR_EL2);
 #endif
 
+    /*
+     * kernel_load will determine the placement of the initrd & fdt in
+     * RAM, so call it first.
+     */
     kernel_load(&kinfo);
     /* initrd_load will fix up the fdt, so call it before dtb_load */
     initrd_load(&kinfo);
@@ -977,7 +984,6 @@ int construct_dom0(struct domain *d)
 
     regs->pc = (register_t)kinfo.entry;
 
-
     if ( is_pv32_domain(d) )
     {
         regs->cpsr = PSR_GUEST32_INIT;
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 7036d94..9046797 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -68,26 +68,56 @@ void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len, int attrindx)
     clear_fixmap(FIXMAP_MISC);
 }
 
-static void kernel_zimage_check_overlap(struct kernel_info *info)
+static void place_modules(struct kernel_info *info,
+                         paddr_t kernel_start,
+                         paddr_t kernel_end)
 {
-    paddr_t zimage_start = info->zimage.load_addr;
-    paddr_t zimage_end = info->zimage.load_addr + info->zimage.len;
-    paddr_t start = info->dtb_paddr;
-    paddr_t end;
+    /* Align DTB and initrd size to 2Mb. Linux only requires 4 byte alignment */
+    const paddr_t initrd_len =
+        ROUNDUP(early_info.modules.module[MOD_INITRD].size, MB(2));
+    const paddr_t dtb_len = ROUNDUP(fdt_totalsize(info->fdt), MB(2));
+    const paddr_t total = initrd_len + dtb_len;
 
-    end = info->initrd_paddr + early_info.modules.module[MOD_INITRD].size;
+    /* Convenient */
+    const paddr_t mem_start = info->mem.bank[0].start;
+    const paddr_t mem_size = info->mem.bank[0].size;
+    const paddr_t mem_end = mem_start + mem_size;
+    const paddr_t kernel_size = kernel_end - kernel_start;
+
+    paddr_t addr;
+
+    if ( total + kernel_size > mem_size )
+        panic("Not enough memory in the first bank for the dtb+initrd.");
 
     /*
-     * In the dom0 memory, the initrd will be just after the DTB. So we
-     * only need to check if the zImage range will overlap the
-     * DTB-initrd range.
+     * DTB must be loaded such that it does not conflict with the
+     * kernel decompressor. For 32-bit Linux Documentation/arm/Booting
+     * recommends just after the 128MB boundary while for 64-bit Linux
+     * the recommendation in Documentation/arm64/booting.txt is below
+     * 512MB.
+     *
+     * If the bootloader provides an initrd, it will be loaded just
+     * after the DTB.
+     *
+     * We try to place dtb+initrd at 128MB, (or, if we have less RAM,
+     * as high as possible). If there is no space then fallback to
+     * just after the kernel, if there is room, otherwise just before.
      */
-    if ( (start > zimage_end) || (end < zimage_start) )
+
+    if ( kernel_end < MIN(mem_start + MB(128), mem_end - total) )
+        addr = MIN(mem_start + MB(128), mem_end - total);
+    else if ( mem_end - ROUNDUP(kernel_end, MB(2)) >= total )
+        addr = ROUNDUP(kernel_end, MB(2));
+    else if ( kernel_start - mem_start >= total )
+        addr = kernel_start - total;
+    else
+    {
+        panic("Unable to find suitable location for dtb+initrd.");
         return;
+    }
 
-    panic(XENLOG_ERR "The kernel(0x%"PRIpaddr"-0x%"PRIpaddr
-          ") is overlapping the DTB-initrd(0x%"PRIpaddr"-0x%"PRIpaddr")\n",
-          zimage_start, zimage_end, start, end);
+    info->dtb_paddr = addr;
+    info->initrd_paddr = info->dtb_paddr + dtb_len;
 }
 
 static void kernel_zimage_load(struct kernel_info *info)
@@ -98,6 +128,8 @@ static void kernel_zimage_load(struct kernel_info *info)
     paddr_t len = info->zimage.len;
     unsigned long offs;
 
+    place_modules(info, load_addr, load_addr + len);
+
     printk("Loading zImage from %"PRIpaddr" to %"PRIpaddr"-%"PRIpaddr"\n",
            paddr, load_addr, load_addr + len);
     for ( offs = 0; offs < len; )
@@ -176,7 +208,6 @@ static int kernel_try_zimage64_prepare(struct kernel_info *info,
 
     info->entry = info->zimage.load_addr;
     info->load = kernel_zimage_load;
-    info->check_overlap = kernel_zimage_check_overlap;
 
     info->type = DOMAIN_PV64;
 
@@ -236,17 +267,9 @@ static int kernel_try_zimage32_prepare(struct kernel_info *info,
         paddr_t load_end;
 
         load_end = info->mem.bank[0].start + info->mem.bank[0].size;
-        load_end = MIN(info->mem.bank[0].start + (128<<20), load_end);
-
-        /*
-         * FDT is loaded above 128M or as high as possible, so the
-         * only way we can clash is if we have <=128MB, in which case
-         * FDT will be right at the end and so dtb_paddr will be below
-         * the proposed kernel load address. Move the kernel down if
-         * necessary.
-         */
-        if ( load_end >= info->dtb_paddr )
-            load_end = info->dtb_paddr;
+        load_end = MIN(info->mem.bank[0].start + MB(128), load_end);
+
+        load_end += MB(2);
 
         info->zimage.load_addr = load_end - end;
         /* Align to 2MB */
@@ -258,7 +281,6 @@ static int kernel_try_zimage32_prepare(struct kernel_info *info,
 
     info->entry = info->zimage.load_addr;
     info->load = kernel_zimage_load;
-    info->check_overlap = kernel_zimage_check_overlap;
 
 #ifdef CONFIG_ARM_64
     info->type = DOMAIN_PV32;
@@ -269,10 +291,15 @@ static int kernel_try_zimage32_prepare(struct kernel_info *info,
 
 static void kernel_elf_load(struct kernel_info *info)
 {
+    place_modules(info,
+                  info->elf.parms.virt_kstart,
+                  info->elf.parms.virt_kend);
+
     printk("Loading ELF image into guest memory\n");
     info->elf.elf.dest_base = (void*)(unsigned long)info->elf.parms.virt_kstart;
     info->elf.elf.dest_size =
          info->elf.parms.virt_kend - info->elf.parms.virt_kstart;
+
     elf_load_binary(&info->elf.elf);
 
     printk("Free temporary kernel buffer\n");
@@ -321,7 +348,6 @@ static int kernel_try_elf_prepare(struct kernel_info *info,
      */
     info->entry = info->elf.parms.virt_entry;
     info->load = kernel_elf_load;
-    info->check_overlap = NULL;
 
     if ( elf_check_broken(&info->elf.elf) )
         printk("Xen: warning: ELF kernel broken: %s\n",
diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
index debf590..b48c2c9 100644
--- a/xen/arch/arm/kernel.h
+++ b/xen/arch/arm/kernel.h
@@ -40,8 +40,6 @@ struct kernel_info {
     };
 
     void (*load)(struct kernel_info *info);
-    /* Callback to check overlap between the kernel and the device tree */
-    void (*check_overlap)(struct kernel_info *kinfo);
     int load_attr;
 };
 
-- 
1.7.10.4

Re: [PATCH v3 03/14] xen: arm: allocate dom0 memory separately from preparing the dtb

[Julien Grall]

On 11/07/2013 08:44 AM, Ian Campbell wrote:
> Mixing these two together is a pain, it forces us to prepare the dtb before
> processing the kernel which means we don't know whether the guest is 32- or
> 64-bit while we construct its DTB.
>
> Instead split out the memory allocation (including 1:1 workaround handling)
> and p2m setup into a separate phase and then create a memory node in the DTB
> based on the result.

Your solution to create the memory node won't work in some case. From 
the EPAR, memory nodes can be everywhere. So we can have a device tree 
like that:

/ {
   motherboard
   {
      #address-cells = 2
      #size-cells = 2
      memory {
         device_type = "memory";
         reg = < ... >
      }
   }
}

Here, the root (/) has #address-cells = 2 and #size-cells = 1, that is 
the default value. As you will create the memory node in slash, you will 
loose 1 cell of the size.

> This allows us to move kernel parsing before DTB setup.

Why do you want to move the kernel parsing earlier? Xen don't use 
d->arch.type during dom0 building.

> As part of this it was also necessary to rework where the decision regarding
> the placement of the DTB and initrd in RAM was made. It is now made when
> loading the kernel, which allows it to make use of the zImage/ELF specific
> information and therefore to make decisions based on complete knowledge and do
> it right rather than guessing in prepare_dtb and relying on a later check to
> see if things worked.

> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v3: Also rework module placement, v2 broke boot because dtb_paddr wasn't set
>      soon enough. This ends up cleaner anyway.
> v2: Fixed typo in the commit log
>      Handle multiple memory nodes as well as individual nodes with several
>      entries in them.
>      Strip the original memory node and recreate rather than trying to modify.
> ---
>   xen/arch/arm/domain_build.c |  198 ++++++++++++++++++++++---------------------
>   xen/arch/arm/kernel.c       |   80 +++++++++++------
>   xen/arch/arm/kernel.h       |    2 -
>   3 files changed, 155 insertions(+), 125 deletions(-)
>

[..]

>   static void kernel_elf_load(struct kernel_info *info)
>   {
> +    place_modules(info,
> +                  info->elf.parms.virt_kstart,
> +                  info->elf.parms.virt_kend);
> +
>       printk("Loading ELF image into guest memory\n");
>       info->elf.elf.dest_base = (void*)(unsigned long)info->elf.parms.virt_kstart;
>       info->elf.elf.dest_size =
>            info->elf.parms.virt_kend - info->elf.parms.virt_kstart;
> +

spurious line?

>       elf_load_binary(&info->elf.elf);
>
>       printk("Free temporary kernel buffer\n");
> @@ -321,7 +348,6 @@ static int kernel_try_elf_prepare(struct kernel_info *info,
>        */
>       info->entry = info->elf.parms.virt_entry;
>       info->load = kernel_elf_load;
> -    info->check_overlap = NULL;
>
>       if ( elf_check_broken(&info->elf.elf) )
>           printk("Xen: warning: ELF kernel broken: %s\n",
> diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
> index debf590..b48c2c9 100644
> --- a/xen/arch/arm/kernel.h
> +++ b/xen/arch/arm/kernel.h
> @@ -40,8 +40,6 @@ struct kernel_info {
>       };
>
>       void (*load)(struct kernel_info *info);
> -    /* Callback to check overlap between the kernel and the device tree */
> -    void (*check_overlap)(struct kernel_info *kinfo);
>       int load_attr;
>   };
>
>

-- 
Julien Grall

Re: [PATCH v3 03/14] xen: arm: allocate dom0 memory separately from preparing the dtb

[Ian Campbell]

On Thu, 2013-11-07 at 23:18 -0800, Julien Grall wrote:
> 
> On 11/07/2013 08:44 AM, Ian Campbell wrote:
> > Mixing these two together is a pain, it forces us to prepare the dtb before
> > processing the kernel which means we don't know whether the guest is 32- or
> > 64-bit while we construct its DTB.
> >
> > Instead split out the memory allocation (including 1:1 workaround handling)
> > and p2m setup into a separate phase and then create a memory node in the DTB
> > based on the result.
> 
> Your solution to create the memory node won't work in some case. From 
> the EPAR, memory nodes can be everywhere. So we can have a device tree 
> like that:
> 
> / {
>    motherboard
>    {
>       #address-cells = 2
>       #size-cells = 2
>       memory {
>          device_type = "memory";
>          reg = < ... >
>       }
>    }
> }
> 
> Here, the root (/) has #address-cells = 2 and #size-cells = 1, that is 
> the default value. As you will create the memory node in slash, you will 
> loose 1 cell of the size.

Urk yes.

I think
	/ {
		"memory" {
			#address-cells = 2;
	 		#size-cells = 2;
			device_type = "memory";
			reg = <...>;
		}
	}

Won't work because the #foo-cells only applies to children.

I could do
	/ {
		"memory" {
			#address-cells = 2;
	        	#size-cells = 2;
			"memory@foo" {
				device_type = "memory"
				reg = <...>;
	 		}
	  	}
	}

which puts the size under my control.

Or I could just remember the root sizes and hope the ram addresses fit,
but that would be fragile (so not an option IMHO).

Or I could leave a placeholder in the original location which I try to
find again later and fill in, but that would suck in practice I think.

> > This allows us to move kernel parsing before DTB setup.
> 
> Why do you want to move the kernel parsing earlier? Xen don't use 
> d->arch.type during dom0 building.

In this series prepare_dtb needs to know which kind of guest it is,
which requires us to have parsed the kernel.

I also think it is an independently worthwhile change to separate
prepping the dtb from the memory allocation, since it makes things
cleaner overall (i.e. we can drop the overlap_check() which is quite a
hacky way to do things which came about because of the intertwining of
the dtb and memory alloc.

> >   static void kernel_elf_load(struct kernel_info *info)
> >   {
> > +    place_modules(info,
> > +                  info->elf.parms.virt_kstart,
> > +                  info->elf.parms.virt_kend);
> > +
> >       printk("Loading ELF image into guest memory\n");
> >       info->elf.elf.dest_base = (void*)(unsigned long)info->elf.parms.virt_kstart;
> >       info->elf.elf.dest_size =
> >            info->elf.parms.virt_kend - info->elf.parms.virt_kstart;
> > +
> 
> spurious line?

Just aesthetics I think. Could drop it I guess.

Ian.

Re: [PATCH v3 03/14] xen: arm: allocate dom0 memory separately from preparing the dtb

[Julien Grall]

On 11/08/2013 09:36 AM, Ian Campbell wrote:
> On Thu, 2013-11-07 at 23:18 -0800, Julien Grall wrote:
> I think
> 	/ {
> 		"memory" {
> 			#address-cells = 2;
> 	 		#size-cells = 2;
> 			device_type = "memory";
> 			reg = <...>;
> 		}
> 	}
> 
> Won't work because the #foo-cells only applies to children.
> 
> I could do
> 	/ {
> 		"memory" {
> 			#address-cells = 2;
> 	        	#size-cells = 2;
> 			"memory@foo" {
> 				device_type = "memory"
> 				reg = <...>;
> 	 		}
> 	  	}
> 	}
> 
> which puts the size under my control.

I have noticed that we have the same issue on the other nodes (gic,
timer,...). Perhaps we can create a node "xen" which contains all the
devices?

"xen" {
   #address-cells = 2;
   #size-cells = 2;
   memory {
     ...
   }
   timer {
     ...
   }
}

>>> This allows us to move kernel parsing before DTB setup.
>>
>> Why do you want to move the kernel parsing earlier? Xen don't use 
>> d->arch.type during dom0 building.
> 
> In this series prepare_dtb needs to know which kind of guest it is,
> which requires us to have parsed the kernel.

Thanks, I didn't pay attention that patch #4 uses the kernel type via
is_pv64_domain.

> 
> I also think it is an independently worthwhile change to separate
> prepping the dtb from the memory allocation, since it makes things
> cleaner overall (i.e. we can drop the overlap_check() which is quite a
> hacky way to do things which came about because of the intertwining of
> the dtb and memory alloc.
> 
>>>   static void kernel_elf_load(struct kernel_info *info)
>>>   {
>>> +    place_modules(info,
>>> +                  info->elf.parms.virt_kstart,
>>> +                  info->elf.parms.virt_kend);
>>> +
>>>       printk("Loading ELF image into guest memory\n");
>>>       info->elf.elf.dest_base = (void*)(unsigned long)info->elf.parms.virt_kstart;
>>>       info->elf.elf.dest_size =
>>>            info->elf.parms.virt_kend - info->elf.parms.virt_kstart;
>>> +
>>
>> spurious line?
> 
> Just aesthetics I think. Could drop it I guess.

Ok.


-- 
Julien Grall

Re: [PATCH v3 03/14] xen: arm: allocate dom0 memory separately from preparing the dtb

[Ian Campbell]

On Mon, 2013-11-11 at 12:08 +0000, Julien Grall wrote:
> On 11/08/2013 09:36 AM, Ian Campbell wrote:
> > On Thu, 2013-11-07 at 23:18 -0800, Julien Grall wrote:
> > I think
> > 	/ {
> > 		"memory" {
> > 			#address-cells = 2;
> > 	 		#size-cells = 2;
> > 			device_type = "memory";
> > 			reg = <...>;
> > 		}
> > 	}
> > 
> > Won't work because the #foo-cells only applies to children.
> > 
> > I could do
> > 	/ {
> > 		"memory" {
> > 			#address-cells = 2;
> > 	        	#size-cells = 2;
> > 			"memory@foo" {
> > 				device_type = "memory"
> > 				reg = <...>;
> > 	 		}
> > 	  	}
> > 	}
> > 
> > which puts the size under my control.
> 
> I have noticed that we have the same issue on the other nodes (gic,
> timer,...).

The timer node at least doesn't have any regions, does it? But for gic
yes this is trouble.

> Perhaps we can create a node "xen" which contains all the
> devices?

Would it be bogus to put them in the existing hypervisor node?

But maybe it is best to keep that separate. A "xen" node is somewhat
analogous to a "soc" or "motherboard" node I guess.

> "xen" {
>    #address-cells = 2;
>    #size-cells = 2;
>    memory {
>      ...
>    }
>    timer {
>      ...
>    }
> }

Re: [PATCH v3 03/14] xen: arm: allocate dom0 memory separately from preparing the dtb

[Julien Grall]

On 11/11/2013 12:12 PM, Ian Campbell wrote:
> On Mon, 2013-11-11 at 12:08 +0000, Julien Grall wrote:
>> On 11/08/2013 09:36 AM, Ian Campbell wrote:
>>> On Thu, 2013-11-07 at 23:18 -0800, Julien Grall wrote:
>>> I think
>>> 	/ {
>>> 		"memory" {
>>> 			#address-cells = 2;
>>> 	 		#size-cells = 2;
>>> 			device_type = "memory";
>>> 			reg = <...>;
>>> 		}
>>> 	}
>>>
>>> Won't work because the #foo-cells only applies to children.
>>>
>>> I could do
>>> 	/ {
>>> 		"memory" {
>>> 			#address-cells = 2;
>>> 	        	#size-cells = 2;
>>> 			"memory@foo" {
>>> 				device_type = "memory"
>>> 				reg = <...>;
>>> 	 		}
>>> 	  	}
>>> 	}
>>>
>>> which puts the size under my control.
>>
>> I have noticed that we have the same issue on the other nodes (gic,
>> timer,...).
> 
> The timer node at least doesn't have any regions, does it? But for gic
> yes this is trouble.
> 
>> Perhaps we can create a node "xen" which contains all the
>> devices?
> 
> Would it be bogus to put them in the existing hypervisor node?

I don't think so.

-- 
Julien Grall

Re: [PATCH v3 03/14] xen: arm: allocate dom0 memory separately from preparing the dtb

[Stefano Stabellini]

On Mon, 11 Nov 2013, Julien Grall wrote:
> On 11/11/2013 12:12 PM, Ian Campbell wrote:
> > On Mon, 2013-11-11 at 12:08 +0000, Julien Grall wrote:
> >> On 11/08/2013 09:36 AM, Ian Campbell wrote:
> >>> On Thu, 2013-11-07 at 23:18 -0800, Julien Grall wrote:
> >>> I think
> >>> 	/ {
> >>> 		"memory" {
> >>> 			#address-cells = 2;
> >>> 	 		#size-cells = 2;
> >>> 			device_type = "memory";
> >>> 			reg = <...>;
> >>> 		}
> >>> 	}
> >>>
> >>> Won't work because the #foo-cells only applies to children.
> >>>
> >>> I could do
> >>> 	/ {
> >>> 		"memory" {
> >>> 			#address-cells = 2;
> >>> 	        	#size-cells = 2;
> >>> 			"memory@foo" {
> >>> 				device_type = "memory"
> >>> 				reg = <...>;
> >>> 	 		}
> >>> 	  	}
> >>> 	}
> >>>
> >>> which puts the size under my control.
> >>
> >> I have noticed that we have the same issue on the other nodes (gic,
> >> timer,...).
> > 
> > The timer node at least doesn't have any regions, does it? But for gic
> > yes this is trouble.
> > 
> >> Perhaps we can create a node "xen" which contains all the
> >> devices?
> > 
> > Would it be bogus to put them in the existing hypervisor node?
> 
> I don't think so.

I don't like the idea: the hypervisor node is for hypervisor properties.
If you need another node to represent the soc, you can introduce a new
one. Maybe call it soc-xen or something like that, it could be

compatible = "simple-bus";

like many soc and motherboard nodes.
> -- 
> Julien Grall
> 

[PATCH v3 04/14] xen: arm: add enable-method to cpu nodes for arm64 guests.

[Ian Campbell]

This is required by the Linux arm64 boot protocol.

We use PSCI.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/domain_build.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index ed09f5c..f27dfd3 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -485,6 +485,13 @@ static int make_cpus_node(const struct domain *d, void *fdt,
         if ( res )
             return res;
 
+        if ( is_pv64_domain(d) )
+        {
+            res = fdt_property_string(fdt, "enable-method", "psci");
+            if ( res )
+                return res;
+        }
+
         res = fdt_end_node(fdt);
         if ( res )
             return res;
-- 
1.7.10.4

[PATCH v3 05/14] xen: arm: implement XEN_DOMCTL_set_address_size

[Ian Campbell]

This is subarch specific to plumb through to arm32 and arm64 versions.

The toolstack uses this to select 32- vs 64-bit guests (or rather it does on
x86 and soon will for arm too).

I noticed that domctl.c and sysctl.c weren't including xen/hypercall.h to get
their arch_do_fooctl prototypes, so fix that.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/arm32/Makefile     |    1 +
 xen/arch/arm/arm32/domctl.c     |   35 +++++++++++++++++++++++
 xen/arch/arm/arm64/Makefile     |    1 +
 xen/arch/arm/arm64/domctl.c     |   59 +++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/domctl.c           |    7 ++++-
 xen/arch/arm/sysctl.c           |    1 +
 xen/include/asm-arm/hypercall.h |    3 ++
 7 files changed, 106 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/arm/arm32/domctl.c
 create mode 100644 xen/arch/arm/arm64/domctl.c

diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
index aacdcb9..65ecff4 100644
--- a/xen/arch/arm/arm32/Makefile
+++ b/xen/arch/arm/arm32/Makefile
@@ -7,5 +7,6 @@ obj-y += traps.o
 obj-y += domain.o
 obj-y += vfp.o
 obj-y += smpboot.o
+obj-y += domctl.o
 
 obj-$(EARLY_PRINTK) += debug.o
diff --git a/xen/arch/arm/arm32/domctl.c b/xen/arch/arm/arm32/domctl.c
new file mode 100644
index 0000000..c2ca4d3
--- /dev/null
+++ b/xen/arch/arm/arm32/domctl.c
@@ -0,0 +1,35 @@
+/******************************************************************************
+ * Subarch-specific domctl.c
+ *
+ * Copyright (c) 2013, Citrix Systems
+ */
+
+#include <xen/config.h>
+#include <xen/types.h>
+#include <xen/lib.h>
+#include <xen/errno.h>
+#include <xen/sched.h>
+#include <xen/hypercall.h>
+#include <public/domctl.h>
+
+long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
+               XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
+{
+    switch ( domctl->cmd )
+    {
+    case XEN_DOMCTL_set_address_size:
+        return domctl->u.address_size.size == 32 ? 0 : -EINVAL;
+    default:
+        return -ENOSYS;
+    }
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
index 5d28bad..d2d5875 100644
--- a/xen/arch/arm/arm64/Makefile
+++ b/xen/arch/arm/arm64/Makefile
@@ -6,5 +6,6 @@ obj-y += traps.o
 obj-y += domain.o
 obj-y += vfp.o
 obj-y += smpboot.o
+obj-y += domctl.o
 
 obj-$(EARLY_PRINTK) += debug.o
diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
new file mode 100644
index 0000000..e2b4617
--- /dev/null
+++ b/xen/arch/arm/arm64/domctl.c
@@ -0,0 +1,59 @@
+/******************************************************************************
+ * Subarch-specific domctl.c
+ *
+ * Copyright (c) 2013, Citrix Systems
+ */
+
+#include <xen/config.h>
+#include <xen/types.h>
+#include <xen/lib.h>
+#include <xen/errno.h>
+#include <xen/sched.h>
+#include <xen/hypercall.h>
+#include <public/domctl.h>
+
+static long switch_mode(struct domain *d, enum domain_type type)
+{
+    if ( d == NULL )
+        return -EINVAL;
+    if ( d->tot_pages != 0 )
+        return -EBUSY;
+    if ( d->arch.type == type )
+        return 0;
+
+    d->arch.type = type;
+
+    return 0;
+}
+
+long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
+                       XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
+{
+    switch ( domctl->cmd )
+    {
+    case XEN_DOMCTL_set_address_size:
+        switch ( domctl->u.address_size.size )
+        {
+        case 32:
+            return switch_mode(d, DOMAIN_PV32);
+        case 64:
+            return switch_mode(d, DOMAIN_PV64);
+        default:
+            return -EINVAL;
+        }
+        break;
+
+    default:
+        return -ENOSYS;
+    }
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index 851ee40..546e86b 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -9,12 +9,17 @@
 #include <xen/lib.h>
 #include <xen/errno.h>
 #include <xen/sched.h>
+#include <xen/hypercall.h>
 #include <public/domctl.h>
 
 long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
                     XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 {
-    return -ENOSYS;
+    switch ( domctl->cmd )
+    {
+    default:
+        return subarch_do_domctl(domctl, d, u_domctl);
+    }
 }
 
 void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
index 6388204..98bab6a 100644
--- a/xen/arch/arm/sysctl.c
+++ b/xen/arch/arm/sysctl.c
@@ -10,6 +10,7 @@
 #include <xen/types.h>
 #include <xen/lib.h>
 #include <xen/errno.h>
+#include <xen/hypercall.h>
 #include <public/sysctl.h>
 
 void arch_do_physinfo(xen_sysctl_physinfo_t *pi) { }
diff --git a/xen/include/asm-arm/hypercall.h b/xen/include/asm-arm/hypercall.h
index 3327a96..94a92d4 100644
--- a/xen/include/asm-arm/hypercall.h
+++ b/xen/include/asm-arm/hypercall.h
@@ -6,6 +6,9 @@ int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
 
 long do_arm_vcpu_op(int cmd, int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg);
 
+long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
+                       XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
+
 #endif /* __ASM_ARM_HYPERCALL_H__ */
 /*
  * Local variables:
-- 
1.7.10.4

Re: [PATCH v3 05/14] xen: arm: implement XEN_DOMCTL_set_address_size

[Julien Grall]

On 11/07/2013 04:44 PM, Ian Campbell wrote:
> This is subarch specific to plumb through to arm32 and arm64 versions.
> 
> The toolstack uses this to select 32- vs 64-bit guests (or rather it does on
> x86 and soon will for arm too).
> 
> I noticed that domctl.c and sysctl.c weren't including xen/hypercall.h to get
> their arch_do_fooctl prototypes, so fix that.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/arm32/Makefile     |    1 +
>  xen/arch/arm/arm32/domctl.c     |   35 +++++++++++++++++++++++
>  xen/arch/arm/arm64/Makefile     |    1 +
>  xen/arch/arm/arm64/domctl.c     |   59 +++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/domctl.c           |    7 ++++-
>  xen/arch/arm/sysctl.c           |    1 +
>  xen/include/asm-arm/hypercall.h |    3 ++
>  7 files changed, 106 insertions(+), 1 deletion(-)
>  create mode 100644 xen/arch/arm/arm32/domctl.c
>  create mode 100644 xen/arch/arm/arm64/domctl.c
> 
> diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
> index aacdcb9..65ecff4 100644
> --- a/xen/arch/arm/arm32/Makefile
> +++ b/xen/arch/arm/arm32/Makefile
> @@ -7,5 +7,6 @@ obj-y += traps.o
>  obj-y += domain.o
>  obj-y += vfp.o
>  obj-y += smpboot.o
> +obj-y += domctl.o
>  
>  obj-$(EARLY_PRINTK) += debug.o
> diff --git a/xen/arch/arm/arm32/domctl.c b/xen/arch/arm/arm32/domctl.c
> new file mode 100644
> index 0000000..c2ca4d3
> --- /dev/null
> +++ b/xen/arch/arm/arm32/domctl.c
> @@ -0,0 +1,35 @@
> +/******************************************************************************
> + * Subarch-specific domctl.c
> + *
> + * Copyright (c) 2013, Citrix Systems
> + */
> +
> +#include <xen/config.h>
> +#include <xen/types.h>
> +#include <xen/lib.h>
> +#include <xen/errno.h>
> +#include <xen/sched.h>
> +#include <xen/hypercall.h>
> +#include <public/domctl.h>
> +
> +long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +               XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> +{
> +    switch ( domctl->cmd )
> +    {
> +    case XEN_DOMCTL_set_address_size:
> +        return domctl->u.address_size.size == 32 ? 0 : -EINVAL;
> +    default:
> +        return -ENOSYS;
> +    }
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
> index 5d28bad..d2d5875 100644
> --- a/xen/arch/arm/arm64/Makefile
> +++ b/xen/arch/arm/arm64/Makefile
> @@ -6,5 +6,6 @@ obj-y += traps.o
>  obj-y += domain.o
>  obj-y += vfp.o
>  obj-y += smpboot.o
> +obj-y += domctl.o
>  
>  obj-$(EARLY_PRINTK) += debug.o
> diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
> new file mode 100644
> index 0000000..e2b4617
> --- /dev/null
> +++ b/xen/arch/arm/arm64/domctl.c
> @@ -0,0 +1,59 @@
> +/******************************************************************************
> + * Subarch-specific domctl.c
> + *
> + * Copyright (c) 2013, Citrix Systems
> + */
> +
> +#include <xen/config.h>
> +#include <xen/types.h>
> +#include <xen/lib.h>
> +#include <xen/errno.h>
> +#include <xen/sched.h>
> +#include <xen/hypercall.h>
> +#include <public/domctl.h>
> +
> +static long switch_mode(struct domain *d, enum domain_type type)
> +{
> +    if ( d == NULL )
> +        return -EINVAL;
> +    if ( d->tot_pages != 0 )
> +        return -EBUSY;
> +    if ( d->arch.type == type )
> +        return 0;
> +
> +    d->arch.type = type;
> +
> +    return 0;
> +}
> +
> +long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +                       XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> +{
> +    switch ( domctl->cmd )
> +    {
> +    case XEN_DOMCTL_set_address_size:
> +        switch ( domctl->u.address_size.size )
> +        {
> +        case 32:
> +            return switch_mode(d, DOMAIN_PV32);
> +        case 64:
> +            return switch_mode(d, DOMAIN_PV64);
> +        default:
> +            return -EINVAL;
> +        }
> +        break;
> +
> +    default:
> +        return -ENOSYS;
> +    }
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index 851ee40..546e86b 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -9,12 +9,17 @@
>  #include <xen/lib.h>
>  #include <xen/errno.h>
>  #include <xen/sched.h>
> +#include <xen/hypercall.h>
>  #include <public/domctl.h>
>  
>  long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>                      XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>  {
> -    return -ENOSYS;
> +    switch ( domctl->cmd )
> +    {
> +    default:
> +        return subarch_do_domctl(domctl, d, u_domctl);
> +    }
>  }
>  
>  void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
> diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
> index 6388204..98bab6a 100644
> --- a/xen/arch/arm/sysctl.c
> +++ b/xen/arch/arm/sysctl.c
> @@ -10,6 +10,7 @@
>  #include <xen/types.h>
>  #include <xen/lib.h>
>  #include <xen/errno.h>
> +#include <xen/hypercall.h>

I think this line should be in a separate patch.

The other part looks good:
Acked-by: Julien Grall <julien.grall@linaro.org>

-- 
Julien Grall

[PATCH v3 06/14] xen: arm: implement arch_set_info_guest for 64-bit vcpus

[Ian Campbell]

This all seems too easy...

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/domain.c |   64 ++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 50 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index cb0424d..5ff7adf 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -550,7 +550,7 @@ void arch_domain_destroy(struct domain *d)
     free_xenheap_page(d->shared_info);
 }
 
-static int is_guest_psr(uint32_t psr)
+static int is_guest_pv32_psr(uint32_t psr)
 {
     switch (psr & PSR_MODE_MASK)
     {
@@ -569,6 +569,29 @@ static int is_guest_psr(uint32_t psr)
     }
 }
 
+
+#ifdef CONFIG_ARM_64
+static int is_guest_pv64_psr(uint32_t psr)
+{
+    if ( psr & PSR_MODE_BIT )
+        return 0;
+
+    switch (psr & PSR_MODE_MASK)
+    {
+    case PSR_MODE_EL1h:
+    case PSR_MODE_EL1t:
+    case PSR_MODE_EL0t:
+        return 1;
+    case PSR_MODE_EL3h:
+    case PSR_MODE_EL3t:
+    case PSR_MODE_EL2h:
+    case PSR_MODE_EL2t:
+    default:
+        return 0;
+    }
+}
+#endif
+
 /*
  * Initialise VCPU state. The context can be supplied by either the
  * toolstack (XEN_DOMCTL_setvcpucontext) or the guest
@@ -580,19 +603,32 @@ int arch_set_info_guest(
     struct vcpu_guest_context *ctxt = c.nat;
     struct vcpu_guest_core_regs *regs = &c.nat->user_regs;
 
-    if ( !is_guest_psr(regs->cpsr) )
-        return -EINVAL;
-
-    if ( regs->spsr_svc && !is_guest_psr(regs->spsr_svc) )
-        return -EINVAL;
-    if ( regs->spsr_abt && !is_guest_psr(regs->spsr_abt) )
-        return -EINVAL;
-    if ( regs->spsr_und && !is_guest_psr(regs->spsr_und) )
-        return -EINVAL;
-    if ( regs->spsr_irq && !is_guest_psr(regs->spsr_irq) )
-        return -EINVAL;
-    if ( regs->spsr_fiq && !is_guest_psr(regs->spsr_fiq) )
-        return -EINVAL;
+    if ( is_pv32_domain(v->domain) )
+    {
+        if ( !is_guest_pv32_psr(regs->cpsr) )
+            return -EINVAL;
+
+        if ( regs->spsr_svc && !is_guest_pv32_psr(regs->spsr_svc) )
+            return -EINVAL;
+        if ( regs->spsr_abt && !is_guest_pv32_psr(regs->spsr_abt) )
+            return -EINVAL;
+        if ( regs->spsr_und && !is_guest_pv32_psr(regs->spsr_und) )
+            return -EINVAL;
+        if ( regs->spsr_irq && !is_guest_pv32_psr(regs->spsr_irq) )
+            return -EINVAL;
+        if ( regs->spsr_fiq && !is_guest_pv32_psr(regs->spsr_fiq) )
+            return -EINVAL;
+    }
+#ifdef CONFIG_ARM_64
+    else
+    {
+        if ( !is_guest_pv64_psr(regs->cpsr) )
+            return -EINVAL;
+
+        if ( regs->spsr_el1 && !is_guest_pv64_psr(regs->spsr_el1) )
+            return -EINVAL;
+    }
+#endif
 
     vcpu_regs_user_to_hyp(v, regs);
 
-- 
1.7.10.4

Re: [PATCH v3 06/14] xen: arm: implement arch_set_info_guest for 64-bit vcpus

[Julien Grall]

On 11/07/2013 08:44 AM, Ian Campbell wrote:
> This all seems too easy...
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>

> ---
>   xen/arch/arm/domain.c |   64 ++++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 50 insertions(+), 14 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index cb0424d..5ff7adf 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -550,7 +550,7 @@ void arch_domain_destroy(struct domain *d)
>       free_xenheap_page(d->shared_info);
>   }
>
> -static int is_guest_psr(uint32_t psr)
> +static int is_guest_pv32_psr(uint32_t psr)
>   {
>       switch (psr & PSR_MODE_MASK)
>       {
> @@ -569,6 +569,29 @@ static int is_guest_psr(uint32_t psr)
>       }
>   }
>
> +
> +#ifdef CONFIG_ARM_64
> +static int is_guest_pv64_psr(uint32_t psr)
> +{
> +    if ( psr & PSR_MODE_BIT )
> +        return 0;
> +
> +    switch (psr & PSR_MODE_MASK)
> +    {
> +    case PSR_MODE_EL1h:
> +    case PSR_MODE_EL1t:
> +    case PSR_MODE_EL0t:
> +        return 1;
> +    case PSR_MODE_EL3h:
> +    case PSR_MODE_EL3t:
> +    case PSR_MODE_EL2h:
> +    case PSR_MODE_EL2t:
> +    default:
> +        return 0;
> +    }
> +}
> +#endif
> +
>   /*
>    * Initialise VCPU state. The context can be supplied by either the
>    * toolstack (XEN_DOMCTL_setvcpucontext) or the guest
> @@ -580,19 +603,32 @@ int arch_set_info_guest(
>       struct vcpu_guest_context *ctxt = c.nat;
>       struct vcpu_guest_core_regs *regs = &c.nat->user_regs;
>
> -    if ( !is_guest_psr(regs->cpsr) )
> -        return -EINVAL;
> -
> -    if ( regs->spsr_svc && !is_guest_psr(regs->spsr_svc) )
> -        return -EINVAL;
> -    if ( regs->spsr_abt && !is_guest_psr(regs->spsr_abt) )
> -        return -EINVAL;
> -    if ( regs->spsr_und && !is_guest_psr(regs->spsr_und) )
> -        return -EINVAL;
> -    if ( regs->spsr_irq && !is_guest_psr(regs->spsr_irq) )
> -        return -EINVAL;
> -    if ( regs->spsr_fiq && !is_guest_psr(regs->spsr_fiq) )
> -        return -EINVAL;
> +    if ( is_pv32_domain(v->domain) )
> +    {
> +        if ( !is_guest_pv32_psr(regs->cpsr) )
> +            return -EINVAL;
> +
> +        if ( regs->spsr_svc && !is_guest_pv32_psr(regs->spsr_svc) )
> +            return -EINVAL;
> +        if ( regs->spsr_abt && !is_guest_pv32_psr(regs->spsr_abt) )
> +            return -EINVAL;
> +        if ( regs->spsr_und && !is_guest_pv32_psr(regs->spsr_und) )
> +            return -EINVAL;
> +        if ( regs->spsr_irq && !is_guest_pv32_psr(regs->spsr_irq) )
> +            return -EINVAL;
> +        if ( regs->spsr_fiq && !is_guest_pv32_psr(regs->spsr_fiq) )
> +            return -EINVAL;
> +    }
> +#ifdef CONFIG_ARM_64
> +    else
> +    {
> +        if ( !is_guest_pv64_psr(regs->cpsr) )
> +            return -EINVAL;
> +
> +        if ( regs->spsr_el1 && !is_guest_pv64_psr(regs->spsr_el1) )
> +            return -EINVAL;
> +    }
> +#endif
>
>       vcpu_regs_user_to_hyp(v, regs);
>
>

-- 
Julien Grall

[PATCH v3 07/14] tools: check for libfdt when building for ARM

[Ian Campbell]

libxl is going to want this to aid in the creation of guest device tree blobs.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
Rerun autogen.sh when committing
---
 tools/config.h.in  |    3 +++
 tools/configure    |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/configure.ac |    6 ++++++
 3 files changed, 61 insertions(+)

diff --git a/tools/config.h.in b/tools/config.h.in
index b1c9531..015f2a1 100644
--- a/tools/config.h.in
+++ b/tools/config.h.in
@@ -9,6 +9,9 @@
 /* Define to 1 if you have the `crypto' library (-lcrypto). */
 #undef HAVE_LIBCRYPTO
 
+/* Define to 1 if you have the `fdt' library (-lfdt). */
+#undef HAVE_LIBFDT
+
 /* Define to 1 if you have the `yajl' library (-lyajl). */
 #undef HAVE_LIBYAJL
 
diff --git a/tools/configure b/tools/configure
index 1da8652..2503990 100755
--- a/tools/configure
+++ b/tools/configure
@@ -7941,6 +7941,58 @@ fi
 
 
 
+# FDT is needed only on ARM
+case "$host_cpu" in
+arm*|aarch64)
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for fdt_create in -lfdt" >&5
+$as_echo_n "checking for fdt_create in -lfdt... " >&6; }
+if ${ac_cv_lib_fdt_fdt_create+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  ac_check_lib_save_LIBS=$LIBS
+LIBS="-lfdt  $LIBS"
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+/* Override any GCC internal prototype to avoid an error.
+   Use char because int might match the return type of a GCC
+   builtin and then its argument prototype would still apply.  */
+#ifdef __cplusplus
+extern "C"
+#endif
+char fdt_create ();
+int
+main ()
+{
+return fdt_create ();
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+  ac_cv_lib_fdt_fdt_create=yes
+else
+  ac_cv_lib_fdt_fdt_create=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+    conftest$ac_exeext conftest.$ac_ext
+LIBS=$ac_check_lib_save_LIBS
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_fdt_fdt_create" >&5
+$as_echo "$ac_cv_lib_fdt_fdt_create" >&6; }
+if test "x$ac_cv_lib_fdt_fdt_create" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_LIBFDT 1
+_ACEOF
+
+  LIBS="-lfdt $LIBS"
+
+else
+  as_fn_error $? "Could not find libfdt" "$LINENO" 5
+fi
+
+esac
+
 # Checks for header files.
 for ac_header in yajl/yajl_version.h sys/eventfd.h
 do :
diff --git a/tools/configure.ac b/tools/configure.ac
index 4f3c33a..c366efb 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -216,6 +216,12 @@ AC_CHECK_LIB([z], [deflateCopy], [], [AC_MSG_ERROR([Could not find zlib])])
 AC_CHECK_LIB([iconv], [libiconv_open], [libiconv="y"], [libiconv="n"])
 AC_SUBST(libiconv)
 
+# FDT is needed only on ARM
+case "$host_cpu" in
+arm*|aarch64)
+AC_CHECK_LIB([fdt], [fdt_create], [], [AC_MSG_ERROR([Could not find libfdt])])
+esac
+
 # Checks for header files.
 AC_CHECK_HEADERS([yajl/yajl_version.h sys/eventfd.h])
 
-- 
1.7.10.4

Re: [PATCH v3 07/14] tools: check for libfdt when building for ARM

[Ian Jackson]

Ian Campbell writes ("[PATCH v3 07/14] tools: check for libfdt when building for ARM"):
> libxl is going to want this to aid in the creation of guest device tree blobs.

Looks plausible.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH v3 08/14] xen: arm: define guest virtual platform in API headers

[Ian Campbell]

The tools and the hypervisor need to agree on various aspects of the guest
environment, such as interrupt numbers, memory layout, initial register values
for registers which are implementation defined etc. Therefore move the
associated defines into the public interface headers, or create them as
necessary. However these should not be exposed to guests, they should find
these things out via device tree or should not be relying on implementation
defined defaults.

Various bits of the hypervisor needed to change to configure dom0 with the real
platform values while using the virtual platform configuration for guests.
Arrange for this where appropriate and plumb through as needed.

We also need to expose some 64-bit values (e.g. PSR_GUEST64_INIT) for the
benefit of 32 bit toolstacks building 64 bit guests.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_dom_arm.c        |    4 +--
 xen/arch/arm/domain.c           |    8 ++++--
 xen/arch/arm/domain_build.c     |   13 +++++----
 xen/arch/arm/gic.c              |   21 +++++++++-----
 xen/arch/arm/psci.c             |    2 +-
 xen/arch/arm/traps.c            |    2 +-
 xen/arch/arm/vtimer.c           |   13 ++++++---
 xen/include/asm-arm/domain.h    |    1 +
 xen/include/asm-arm/event.h     |    3 +-
 xen/include/asm-arm/gic.h       |    3 --
 xen/include/asm-arm/processor.h |    7 -----
 xen/include/asm-arm/psci.h      |    5 ----
 xen/include/public/arch-arm.h   |   60 +++++++++++++++++++++++++++++++--------
 13 files changed, 91 insertions(+), 51 deletions(-)

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index df59ffb..9f3fdd3 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -126,13 +126,13 @@ static int vcpu_arm(struct xc_dom_image *dom, void *ptr)
      */
     ctxt->user_regs.r2_usr = 0xffffffff;
 
-    ctxt->sctlr = /* #define SCTLR_BASE */0x00c50078;
+    ctxt->sctlr = SCTLR_GUEST_INIT;
 
     ctxt->ttbr0 = 0;
     ctxt->ttbr1 = 0;
     ctxt->ttbcr = 0; /* Defined Reset Value */
 
-    ctxt->user_regs.cpsr = PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC;
+    ctxt->user_regs.cpsr = PSR_GUEST32_INIT;
 
     ctxt->flags = VGCF_online;
 
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 5ff7adf..2f57d01 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -461,7 +461,8 @@ int vcpu_initialise(struct vcpu *v)
     if ( is_idle_vcpu(v) )
         return rc;
 
-    v->arch.sctlr = SCTLR_BASE;
+    v->arch.sctlr = SCTLR_GUEST_INIT;
+
     /*
      * By default exposes an SMP system with AFF0 set to the VCPU ID
      * TODO: Handle multi-threading processor and cluster
@@ -525,6 +526,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
     if ( (rc = vcpu_domain_init(d)) != 0 )
         goto fail;
 
+    /* XXX dom0 needs more intelligent selection of PPI */
+    d->arch.evtchn_irq = GUEST_EVTCHN_PPI;
+
     /*
      * Virtual UART is only used by linux early printk and decompress code.
      * Only use it for dom0 because the linux kernel may not support
@@ -740,7 +744,7 @@ void vcpu_mark_events_pending(struct vcpu *v)
     if ( already_pending )
         return;
 
-    vgic_vcpu_inject_irq(v, VGIC_IRQ_EVTCHN_CALLBACK, 1);
+    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq, 1);
 }
 
 /*
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index f27dfd3..4f1b54a 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -315,7 +315,8 @@ static int make_memory_node(struct domain *d,
     return res;
 }
 
-static int make_hypervisor_node(void *fdt, const struct dt_device_node *parent)
+static int make_hypervisor_node(struct domain *d,
+                                void *fdt, const struct dt_device_node *parent)
 {
     const char compat[] =
         "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
@@ -363,8 +364,8 @@ static int make_hypervisor_node(void *fdt, const struct dt_device_node *parent)
      *
      * TODO: Handle correctly the cpumask
      */
-    DPRINT("  Event channel interrupt to %u\n", VGIC_IRQ_EVTCHN_CALLBACK);
-    set_interrupt_ppi(intr, VGIC_IRQ_EVTCHN_CALLBACK, 0xf,
+    DPRINT("  Event channel interrupt to %u\n", d->arch.evtchn_irq);
+    set_interrupt_ppi(intr, d->arch.evtchn_irq, 0xf,
                    DT_IRQ_TYPE_LEVEL_LOW);
 
     res = fdt_property_interrupts(fdt, &intr, 1);
@@ -395,11 +396,11 @@ static int make_psci_node(void *fdt, const struct dt_device_node *parent)
     if ( res )
         return res;
 
-    res = fdt_property_cell(fdt, "cpu_off", __PSCI_cpu_off);
+    res = fdt_property_cell(fdt, "cpu_off", PSCI_cpu_off);
     if ( res )
         return res;
 
-    res = fdt_property_cell(fdt, "cpu_on", __PSCI_cpu_on);
+    res = fdt_property_cell(fdt, "cpu_on", PSCI_cpu_on);
     if ( res )
         return res;
 
@@ -794,7 +795,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
         if ( res )
             return res;
 
-        res = make_hypervisor_node(kinfo->fdt, np);
+        res = make_hypervisor_node(d, kinfo->fdt, np);
         if ( res )
             return res;
 
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 74575cd..33c6b8d 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -717,7 +717,7 @@ int gic_events_need_delivery(void)
 void gic_inject(void)
 {
     if ( vcpu_info(current, evtchn_upcall_pending) )
-        vgic_vcpu_inject_irq(current, VGIC_IRQ_EVTCHN_CALLBACK, 1);
+        vgic_vcpu_inject_irq(current, current->domain->arch.evtchn_irq, 1);
 
     gic_restore_pending_irqs(current);
     if (!gic_events_need_delivery())
@@ -823,13 +823,20 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
 
 int gicv_setup(struct domain *d)
 {
-    /* TODO: Retrieve distributor and CPU guest base address from the
-     * guest DTS
-     * For the moment we use dom0 DTS
+    /*
+     * Domain 0 gets the hardware address.
+     * Guests get the virtual platform layout.
      */
-    d->arch.vgic.dbase = gic.dbase;
-    d->arch.vgic.cbase = gic.cbase;
-
+    if ( d == dom0 )
+    {
+        d->arch.vgic.dbase = gic.dbase;
+        d->arch.vgic.cbase = gic.cbase;
+    }
+    else
+    {
+        d->arch.vgic.dbase = GUEST_GICD_BASE;
+        d->arch.vgic.cbase = GUEST_GICC_BASE;
+    }
 
     d->arch.vgic.nr_lines = 0;
 
diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 6c3be47..c82884f 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -43,7 +43,7 @@ int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point)
 
     memset(ctxt, 0, sizeof(*ctxt));
     ctxt->user_regs.pc64 = (u64) entry_point;
-    ctxt->sctlr = SCTLR_BASE;
+    ctxt->sctlr = SCTLR_GUEST_INIT;
     ctxt->ttbr0 = 0;
     ctxt->ttbr1 = 0;
     ctxt->ttbcr = 0; /* Defined Reset Value */
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 287dd7b..f64b2b8 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -870,7 +870,7 @@ typedef struct {
 } arm_psci_t;
 
 #define PSCI(_name, _nr_args)                                  \
-    [ __PSCI_ ## _name ] =  {                                  \
+    [ PSCI_ ## _name ] =  {                                    \
         .fn = (arm_psci_fn_t) &do_psci_ ## _name,              \
         .nr_args = _nr_args,                                   \
     }
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index d58a630..f323453 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -54,21 +54,26 @@ int vcpu_domain_init(struct domain *d)
 int vcpu_vtimer_init(struct vcpu *v)
 {
     struct vtimer *t = &v->arch.phys_timer;
+    bool_t d0 = (v->domain == dom0);
 
-    /* TODO: Retrieve physical and virtual timer IRQ from the guest
-     * DT. For the moment we use dom0 DT
+    /*
+     * Domain 0 uses the hardware interrupts, guests get the virtual platform.
      */
 
     init_timer(&t->timer, phys_timer_expired, t, v->processor);
     t->ctl = 0;
     t->cval = NOW();
-    t->irq = timer_dt_irq(TIMER_PHYS_NONSECURE_PPI)->irq;
+    t->irq = d0
+        ? timer_dt_irq(TIMER_PHYS_NONSECURE_PPI)->irq
+        : GUEST_TIMER_PHYS_NS_PPI;
     t->v = v;
 
     t = &v->arch.virt_timer;
     init_timer(&t->timer, virt_timer_expired, t, v->processor);
     t->ctl = 0;
-    t->irq = timer_dt_irq(TIMER_VIRT_PPI)->irq;
+    t->irq = d0
+        ? timer_dt_irq(TIMER_VIRT_PPI)->irq
+        : GUEST_TIMER_VIRT_PPI;
     t->v = v;
 
     return 0;
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 67bfbbc..53847a9 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -112,6 +112,7 @@ struct arch_domain
         spinlock_t                  lock;
     } vuart;
 
+    int evtchn_irq;
 }  __cacheline_aligned;
 
 struct arch_vcpu
diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
index 04d854f..dd3ad13 100644
--- a/xen/include/asm-arm/event.h
+++ b/xen/include/asm-arm/event.h
@@ -15,7 +15,8 @@ static inline int vcpu_event_delivery_is_enabled(struct vcpu *v)
 
 static inline int local_events_need_delivery_nomask(void)
 {
-    struct pending_irq *p = irq_to_pending(current, VGIC_IRQ_EVTCHN_CALLBACK);
+    struct pending_irq *p = irq_to_pending(current,
+                                           current->domain->arch.evtchn_irq);
 
     /* XXX: if the first interrupt has already been delivered, we should
      * check whether any other interrupts with priority higher than the
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 0a890be..41f0b3b 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -129,9 +129,6 @@
 #define GICH_LR_CPUID_SHIFT     9
 #define GICH_VTR_NRLRGS         0x3f
 
-/* XXX: write this into the DT */
-#define VGIC_IRQ_EVTCHN_CALLBACK 31
-
 #ifndef __ASSEMBLY__
 #include <xen/device_tree.h>
 
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 5294421..3da3a3d 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -48,15 +48,8 @@
 #define SCTLR_A         (1<<1)
 #define SCTLR_M         (1<<0)
 
-#define SCTLR_BASE        0x00c50078
 #define HSCTLR_BASE       0x30c51878
 
-#define PSR_GUEST32_INIT  (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC)
-
-#ifdef CONFIG_ARM_64
-#define PSR_GUEST64_INIT (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_EL1h)
-#endif
-
 /* HCR Hyp Configuration Register */
 #define HCR_RW          (1<<31) /* Register Width, ARM64 only */
 #define HCR_TGE         (1<<27) /* Trap General Exceptions */
diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
index fdba636..67d4c35 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -6,11 +6,6 @@
 #define PSCI_EINVAL  -2
 #define PSCI_DENIED  -3
 
-#define __PSCI_cpu_suspend 0
-#define __PSCI_cpu_off     1
-#define __PSCI_cpu_on      2
-#define __PSCI_migrate     3
-
 int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point);
 int do_psci_cpu_off(uint32_t power_state);
 int do_psci_cpu_suspend(uint32_t power_state, register_t entry_point);
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 5d359af..70a0766 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -268,8 +268,21 @@ typedef uint64_t xen_callback_t;
 
 #endif
 
+#if defined(__XEN__) || defined(__XEN_TOOLS__)
+
 /* PSR bits (CPSR, SPSR)*/
 
+#define PSR_THUMB       (1<<5)        /* Thumb Mode enable */
+#define PSR_FIQ_MASK    (1<<6)        /* Fast Interrupt mask */
+#define PSR_IRQ_MASK    (1<<7)        /* Interrupt mask */
+#define PSR_ABT_MASK    (1<<8)        /* Asynchronous Abort mask */
+#define PSR_BIG_ENDIAN  (1<<9)        /* Big Endian Mode */
+#ifdef __aarch64__ /* For Aarch64 bit 9 is repurposed. */
+#define PSR_DBG_MASK    (1<<9)
+#endif
+#define PSR_IT_MASK     (0x0600fc00)  /* Thumb If-Then Mask */
+#define PSR_JAZELLE     (1<<24)       /* Jazelle Mode */
+
 /* 32 bit modes */
 #define PSR_MODE_USR 0x10
 #define PSR_MODE_FIQ 0x11
@@ -281,8 +294,6 @@ typedef uint64_t xen_callback_t;
 #define PSR_MODE_UND 0x1b
 #define PSR_MODE_SYS 0x1f
 
-/* 64 bit modes */
-#ifdef __aarch64__
 #define PSR_MODE_BIT  0x10 /* Set iff AArch32 */
 #define PSR_MODE_EL3h 0x0d
 #define PSR_MODE_EL3t 0x0c
@@ -291,18 +302,43 @@ typedef uint64_t xen_callback_t;
 #define PSR_MODE_EL1h 0x05
 #define PSR_MODE_EL1t 0x04
 #define PSR_MODE_EL0t 0x00
-#endif
 
-#define PSR_THUMB       (1<<5)        /* Thumb Mode enable */
-#define PSR_FIQ_MASK    (1<<6)        /* Fast Interrupt mask */
-#define PSR_IRQ_MASK    (1<<7)        /* Interrupt mask */
-#define PSR_ABT_MASK    (1<<8)        /* Asynchronous Abort mask */
-#define PSR_BIG_ENDIAN  (1<<9)        /* Big Endian Mode */
-#ifdef __aarch64__ /* For Aarch64 bit 9 is repurposed. */
-#define PSR_DBG_MASK    (1<<9)
+#define PSR_GUEST64_INIT (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_EL1h)
+
+#define SCTLR_GUEST_INIT    0x00c50078
+
+/*
+ * Virtual machine platform (memory layout, interrupts)
+ *
+ * These are defined for consistency between the tools and the
+ * hypervisor. Guests must not rely on these hardcoded values but
+ * should instead use the FDT.
+ */
+
+/* Physical Address Space */
+#define GUEST_GICD_BASE   0x2c001000ULL
+#define GUEST_GICD_SIZE   0x1000ULL
+#define GUEST_GICC_BASE   0x2c002000ULL
+#define GUEST_GICC_SIZE   0x100ULL
+
+#define GUEST_RAM_BASE    0x80000000ULL
+
+#define GUEST_GNTTAB_BASE 0xb0000000ULL
+#define GUEST_GNTTAB_SIZE 0x00020000ULL
+
+/* Interrupts */
+#define GUEST_TIMER_VIRT_PPI    27
+#define GUEST_TIMER_PHYS_S_PPI  29
+#define GUEST_TIMER_PHYS_NS_PPI 30
+#define GUEST_EVTCHN_PPI        31
+
+/* PSCI functions */
+#define PSCI_cpu_suspend 0
+#define PSCI_cpu_off     1
+#define PSCI_cpu_on      2
+#define PSCI_migrate     3
+
 #endif
-#define PSR_IT_MASK     (0x0600fc00)  /* Thumb If-Then Mask */
-#define PSR_JAZELLE     (1<<24)       /* Jazelle Mode */
 
 #endif /*  __XEN_PUBLIC_ARCH_ARM_H__ */
 
-- 
1.7.10.4

Re: [PATCH v3 08/14] xen: arm: define guest virtual platform in API headers

[Julien Grall]

On 11/07/2013 08:44 AM, Ian Campbell wrote:
> The tools and the hypervisor need to agree on various aspects of the guest
> environment, such as interrupt numbers, memory layout, initial register values
> for registers which are implementation defined etc. Therefore move the
> associated defines into the public interface headers, or create them as
> necessary. However these should not be exposed to guests, they should find
> these things out via device tree or should not be relying on implementation
> defined defaults.
>
> Various bits of the hypervisor needed to change to configure dom0 with the real
> platform values while using the virtual platform configuration for guests.
> Arrange for this where appropriate and plumb through as needed.
>
> We also need to expose some 64-bit values (e.g. PSR_GUEST64_INIT) for the
> benefit of 32 bit toolstacks building 64 bit guests.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>   tools/libxc/xc_dom_arm.c        |    4 +--
>   xen/arch/arm/domain.c           |    8 ++++--
>   xen/arch/arm/domain_build.c     |   13 +++++----
>   xen/arch/arm/gic.c              |   21 +++++++++-----
>   xen/arch/arm/psci.c             |    2 +-
>   xen/arch/arm/traps.c            |    2 +-
>   xen/arch/arm/vtimer.c           |   13 ++++++---
>   xen/include/asm-arm/domain.h    |    1 +
>   xen/include/asm-arm/event.h     |    3 +-
>   xen/include/asm-arm/gic.h       |    3 --
>   xen/include/asm-arm/processor.h |    7 -----
>   xen/include/asm-arm/psci.h      |    5 ----
>   xen/include/public/arch-arm.h   |   60 +++++++++++++++++++++++++++++++--------
>   13 files changed, 91 insertions(+), 51 deletions(-)
>

[..]

>   /* HCR Hyp Configuration Register */
>   #define HCR_RW          (1<<31) /* Register Width, ARM64 only */
>   #define HCR_TGE         (1<<27) /* Trap General Exceptions */
> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
> index fdba636..67d4c35 100644
> --- a/xen/include/asm-arm/psci.h
> +++ b/xen/include/asm-arm/psci.h
> @@ -6,11 +6,6 @@
>   #define PSCI_EINVAL  -2
>   #define PSCI_DENIED  -3
>
> -#define __PSCI_cpu_suspend 0
> -#define __PSCI_cpu_off     1
> -#define __PSCI_cpu_on      2
> -#define __PSCI_migrate     3
> -
>   int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point);
>   int do_psci_cpu_off(uint32_t power_state);
>   int do_psci_cpu_suspend(uint32_t power_state, register_t entry_point);
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 5d359af..70a0766 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -268,8 +268,21 @@ typedef uint64_t xen_callback_t;
>
>   #endif
>
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +
>   /* PSR bits (CPSR, SPSR)*/
>
> +#define PSR_THUMB       (1<<5)        /* Thumb Mode enable */
> +#define PSR_FIQ_MASK    (1<<6)        /* Fast Interrupt mask */
> +#define PSR_IRQ_MASK    (1<<7)        /* Interrupt mask */
> +#define PSR_ABT_MASK    (1<<8)        /* Asynchronous Abort mask */
> +#define PSR_BIG_ENDIAN  (1<<9)        /* Big Endian Mode */
> +#ifdef __aarch64__ /* For Aarch64 bit 9 is repurposed. */

If you keep __aarch64__, you won't be able to have dom0 in 32 bits on arm64.
BTW, what does prevent the developer to use arm64 defines on arm64?

> +#define PSR_DBG_MASK    (1<<9)
> +#endif
> +#define PSR_IT_MASK     (0x0600fc00)  /* Thumb If-Then Mask */
> +#define PSR_JAZELLE     (1<<24)       /* Jazelle Mode */
> +
>   /* 32 bit modes */
>   #define PSR_MODE_USR 0x10
>   #define PSR_MODE_FIQ 0x11
> @@ -281,8 +294,6 @@ typedef uint64_t xen_callback_t;
>   #define PSR_MODE_UND 0x1b
>   #define PSR_MODE_SYS 0x1f
>
> -/* 64 bit modes */

I would keep this commit. It's good to know that it's ARM64 specific.

> -#ifdef __aarch64__
>   #define PSR_MODE_BIT  0x10 /* Set iff AArch32 */
>   #define PSR_MODE_EL3h 0x0d
>   #define PSR_MODE_EL3t 0x0c
> @@ -291,18 +302,43 @@ typedef uint64_t xen_callback_t;
>   #define PSR_MODE_EL1h 0x05
>   #define PSR_MODE_EL1t 0x04
>   #define PSR_MODE_EL0t 0x00
> -#endif
>
> -#define PSR_THUMB       (1<<5)        /* Thumb Mode enable */
> -#define PSR_FIQ_MASK    (1<<6)        /* Fast Interrupt mask */
> -#define PSR_IRQ_MASK    (1<<7)        /* Interrupt mask */
> -#define PSR_ABT_MASK    (1<<8)        /* Asynchronous Abort mask */
> -#define PSR_BIG_ENDIAN  (1<<9)        /* Big Endian Mode */
> -#ifdef __aarch64__ /* For Aarch64 bit 9 is repurposed. */
> -#define PSR_DBG_MASK    (1<<9)
> +#define PSR_GUEST64_INIT (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_EL1h)
> +
> +#define SCTLR_GUEST_INIT    0x00c50078
> +
> +/*
> + * Virtual machine platform (memory layout, interrupts)
> + *
> + * These are defined for consistency between the tools and the
> + * hypervisor. Guests must not rely on these hardcoded values but
> + * should instead use the FDT.
> + */
> +

I don't like the solution to hardcode GIC, IRQs (timer + evtchn) in the 
public interface. If we keep this solution, we will need to modify, 
again, the ABI for ARM, why can't we add/extend some hypercalls to give 
theses values to the hypervisor?

> +/* Physical Address Space */
> +#define GUEST_GICD_BASE   0x2c001000ULL
> +#define GUEST_GICD_SIZE   0x1000ULL
> +#define GUEST_GICC_BASE   0x2c002000ULL
> +#define GUEST_GICC_SIZE   0x100ULL
> +
> +#define GUEST_RAM_BASE    0x80000000ULL

This define should be libxc/libxl specific. Xen should not use this value.

> +
> +#define GUEST_GNTTAB_BASE 0xb0000000ULL
> +#define GUEST_GNTTAB_SIZE 0x00020000ULL
> +
> +/* Interrupts */
> +#define GUEST_TIMER_VIRT_PPI    27
> +#define GUEST_TIMER_PHYS_S_PPI  29
> +#define GUEST_TIMER_PHYS_NS_PPI 30
> +#define GUEST_EVTCHN_PPI        31
> +
> +/* PSCI functions */
> +#define PSCI_cpu_suspend 0
> +#define PSCI_cpu_off     1
> +#define PSCI_cpu_on      2
> +#define PSCI_migrate     3
> +
>   #endif
> -#define PSR_IT_MASK     (0x0600fc00)  /* Thumb If-Then Mask */
> -#define PSR_JAZELLE     (1<<24)       /* Jazelle Mode */
>
>   #endif /*  __XEN_PUBLIC_ARCH_ARM_H__ */
>
>

-- 
Julien Grall

Re: [PATCH v3 08/14] xen: arm: define guest virtual platform in API headers

[Ian Campbell]

On Thu, 2013-11-07 at 23:29 -0800, Julien Grall wrote:

> > +#define PSR_THUMB       (1<<5)        /* Thumb Mode enable */
> > +#define PSR_FIQ_MASK    (1<<6)        /* Fast Interrupt mask */
> > +#define PSR_IRQ_MASK    (1<<7)        /* Interrupt mask */
> > +#define PSR_ABT_MASK    (1<<8)        /* Asynchronous Abort mask */
> > +#define PSR_BIG_ENDIAN  (1<<9)        /* Big Endian Mode */
> > +#ifdef __aarch64__ /* For Aarch64 bit 9 is repurposed. */
> 
> If you keep __aarch64__, you won't be able to have dom0 in 32 bits on arm64.

Oops. I removed the others but missed this one. Nothing about this
particular #if would prevent you having a 32 bit dom0 on arm64.

But I'll remove the #define and add a comment anyway.

> BTW, what does prevent the developer to use arm64 defines on arm64?

Did you mean s/64/32/ in one of those? The answer is nothing other than
code review, getting bug reports etc. I don't think there is any way we
can prevent it programmatically. Maybe separate enums for 32 vs 64-bit
PSR values, but I think that would get nasty fast.

> 
> > +#define PSR_DBG_MASK    (1<<9)
> > +#endif
> > +#define PSR_IT_MASK     (0x0600fc00)  /* Thumb If-Then Mask */
> > +#define PSR_JAZELLE     (1<<24)       /* Jazelle Mode */
> > +
> >   /* 32 bit modes */
> >   #define PSR_MODE_USR 0x10
> >   #define PSR_MODE_FIQ 0x11
> > @@ -281,8 +294,6 @@ typedef uint64_t xen_callback_t;
> >   #define PSR_MODE_UND 0x1b
> >   #define PSR_MODE_SYS 0x1f
> >
> > -/* 64 bit modes */
> 
> I would keep this commit. It's good to know that it's ARM64 specific.

Yes.

> 
> > -#ifdef __aarch64__
> >   #define PSR_MODE_BIT  0x10 /* Set iff AArch32 */
> >   #define PSR_MODE_EL3h 0x0d
> >   #define PSR_MODE_EL3t 0x0c
> > @@ -291,18 +302,43 @@ typedef uint64_t xen_callback_t;
> >   #define PSR_MODE_EL1h 0x05
> >   #define PSR_MODE_EL1t 0x04
> >   #define PSR_MODE_EL0t 0x00
> > -#endif
> >
> > -#define PSR_THUMB       (1<<5)        /* Thumb Mode enable */
> > -#define PSR_FIQ_MASK    (1<<6)        /* Fast Interrupt mask */
> > -#define PSR_IRQ_MASK    (1<<7)        /* Interrupt mask */
> > -#define PSR_ABT_MASK    (1<<8)        /* Asynchronous Abort mask */
> > -#define PSR_BIG_ENDIAN  (1<<9)        /* Big Endian Mode */
> > -#ifdef __aarch64__ /* For Aarch64 bit 9 is repurposed. */
> > -#define PSR_DBG_MASK    (1<<9)
> > +#define PSR_GUEST64_INIT (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_EL1h)
> > +
> > +#define SCTLR_GUEST_INIT    0x00c50078
> > +
> > +/*
> > + * Virtual machine platform (memory layout, interrupts)
> > + *
> > + * These are defined for consistency between the tools and the
> > + * hypervisor. Guests must not rely on these hardcoded values but
> > + * should instead use the FDT.
> > + */
> > +
> 
> I don't like the solution to hardcode GIC, IRQs (timer + evtchn) in the 
> public interface.

Note that this is inside
        #if defined(__XEN__) || defined(__XEN_TOOLS__)
so it is not exposed to guests and therefore not part of the guest ABI.
The tools ABI is completely fungible and we can change it at will.

>  If we keep this solution, we will need to modify, 
> again, the ABI for ARM, why can't we add/extend some hypercalls to give 
> theses values to the hypervisor?

That would be overkill until (if!) we get to the point of needing a more
dynamic guest layout.

I'm not going to implement that now. I thought I'd expressed this in the
commit message or the cover letter but it seems I forgot. I'll add some
words to the commit message for the next posting.

> > +/* Physical Address Space */
> > +#define GUEST_GICD_BASE   0x2c001000ULL
> > +#define GUEST_GICD_SIZE   0x1000ULL
> > +#define GUEST_GICC_BASE   0x2c002000ULL
> > +#define GUEST_GICC_SIZE   0x100ULL
> > +
> > +#define GUEST_RAM_BASE    0x80000000ULL
> 
> This define should be libxc/libxl specific. Xen should not use this value.

No, it is part of the guest ABI which Xen is presenting, even if Xen
isn't actually using it. It needs to fit in with the other physical
address properties. It is consistent for it to be defined here.

Ian.

Re: [PATCH v3 08/14] xen: arm: define guest virtual platform in API headers

[Julien Grall]

On 11/08/2013 09:48 AM, Ian Campbell wrote:
> On Thu, 2013-11-07 at 23:29 -0800, Julien Grall wrote:
> 
>>> +#define PSR_THUMB       (1<<5)        /* Thumb Mode enable */
>>> +#define PSR_FIQ_MASK    (1<<6)        /* Fast Interrupt mask */
>>> +#define PSR_IRQ_MASK    (1<<7)        /* Interrupt mask */
>>> +#define PSR_ABT_MASK    (1<<8)        /* Asynchronous Abort mask */
>>> +#define PSR_BIG_ENDIAN  (1<<9)        /* Big Endian Mode */
>>> +#ifdef __aarch64__ /* For Aarch64 bit 9 is repurposed. */
>>
>> If you keep __aarch64__, you won't be able to have dom0 in 32 bits on arm64.
> 
> Oops. I removed the others but missed this one. Nothing about this
> particular #if would prevent you having a 32 bit dom0 on arm64.
> 
> But I'll remove the #define and add a comment anyway.
> 
>> BTW, what does prevent the developer to use arm64 defines on arm64?
> 
> Did you mean s/64/32/ in one of those? The answer is nothing other than
> code review, getting bug reports etc. I don't think there is any way we
> can prevent it programmatically. Maybe separate enums for 32 vs 64-bit
> PSR values, but I think that would get nasty fast.

What about defining arm64 specific stuff only if we are in xen tools or
in xen and build for arm64?

ie:
XEN_TOOLS || (XEN && ARM64)

>>
>>> +#define PSR_DBG_MASK    (1<<9)
>>> +#endif
>>> +#define PSR_IT_MASK     (0x0600fc00)  /* Thumb If-Then Mask */
>>> +#define PSR_JAZELLE     (1<<24)       /* Jazelle Mode */
>>> +
>>>   /* 32 bit modes */
>>>   #define PSR_MODE_USR 0x10
>>>   #define PSR_MODE_FIQ 0x11
>>> @@ -281,8 +294,6 @@ typedef uint64_t xen_callback_t;
>>>   #define PSR_MODE_UND 0x1b
>>>   #define PSR_MODE_SYS 0x1f
>>>
>>> -/* 64 bit modes */
>>
>> I would keep this commit. It's good to know that it's ARM64 specific.
> 
> Yes.
> 
>>
>>> -#ifdef __aarch64__
>>>   #define PSR_MODE_BIT  0x10 /* Set iff AArch32 */
>>>   #define PSR_MODE_EL3h 0x0d
>>>   #define PSR_MODE_EL3t 0x0c
>>> @@ -291,18 +302,43 @@ typedef uint64_t xen_callback_t;
>>>   #define PSR_MODE_EL1h 0x05
>>>   #define PSR_MODE_EL1t 0x04
>>>   #define PSR_MODE_EL0t 0x00
>>> -#endif
>>>
>>> -#define PSR_THUMB       (1<<5)        /* Thumb Mode enable */
>>> -#define PSR_FIQ_MASK    (1<<6)        /* Fast Interrupt mask */
>>> -#define PSR_IRQ_MASK    (1<<7)        /* Interrupt mask */
>>> -#define PSR_ABT_MASK    (1<<8)        /* Asynchronous Abort mask */
>>> -#define PSR_BIG_ENDIAN  (1<<9)        /* Big Endian Mode */
>>> -#ifdef __aarch64__ /* For Aarch64 bit 9 is repurposed. */
>>> -#define PSR_DBG_MASK    (1<<9)
>>> +#define PSR_GUEST64_INIT (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_EL1h)
>>> +
>>> +#define SCTLR_GUEST_INIT    0x00c50078
>>> +
>>> +/*
>>> + * Virtual machine platform (memory layout, interrupts)
>>> + *
>>> + * These are defined for consistency between the tools and the
>>> + * hypervisor. Guests must not rely on these hardcoded values but
>>> + * should instead use the FDT.
>>> + */
>>> +
>>
>> I don't like the solution to hardcode GIC, IRQs (timer + evtchn) in the 
>> public interface.
> 
> Note that this is inside
>         #if defined(__XEN__) || defined(__XEN_TOOLS__)
> so it is not exposed to guests and therefore not part of the guest ABI.
> The tools ABI is completely fungible and we can change it at will.
> 
>>  If we keep this solution, we will need to modify, 
>> again, the ABI for ARM, why can't we add/extend some hypercalls to give 
>> theses values to the hypervisor?
> 
> That would be overkill until (if!) we get to the point of needing a more
> dynamic guest layout.

The current solution has some memory limitations. The layout doesn't
allow to allocate more than 768MB (space between the GUEST_RAM_BASE and
GUEST_GNTTAB_BASE).

> 
> I'm not going to implement that now. I thought I'd expressed this in the
> commit message or the cover letter but it seems I forgot. I'll add some
> words to the commit message for the next posting.

Ok. Can you document somewhere the guest memory layout?

-- 
Julien Grall

Re: [PATCH v3 08/14] xen: arm: define guest virtual platform in API headers

[Ian Campbell]

On Mon, 2013-11-11 at 14:25 +0000, Julien Grall wrote:
> On 11/08/2013 09:48 AM, Ian Campbell wrote:
> > On Thu, 2013-11-07 at 23:29 -0800, Julien Grall wrote:
> > 
> >>> +#define PSR_THUMB       (1<<5)        /* Thumb Mode enable */
> >>> +#define PSR_FIQ_MASK    (1<<6)        /* Fast Interrupt mask */
> >>> +#define PSR_IRQ_MASK    (1<<7)        /* Interrupt mask */
> >>> +#define PSR_ABT_MASK    (1<<8)        /* Asynchronous Abort mask */
> >>> +#define PSR_BIG_ENDIAN  (1<<9)        /* Big Endian Mode */
> >>> +#ifdef __aarch64__ /* For Aarch64 bit 9 is repurposed. */
> >>
> >> If you keep __aarch64__, you won't be able to have dom0 in 32 bits on arm64.
> > 
> > Oops. I removed the others but missed this one. Nothing about this
> > particular #if would prevent you having a 32 bit dom0 on arm64.
> > 
> > But I'll remove the #define and add a comment anyway.
> > 
> >> BTW, what does prevent the developer to use arm64 defines on arm64?
> > 
> > Did you mean s/64/32/ in one of those? The answer is nothing other than
> > code review, getting bug reports etc. I don't think there is any way we
> > can prevent it programmatically. Maybe separate enums for 32 vs 64-bit
> > PSR values, but I think that would get nasty fast.
> 
> What about defining arm64 specific stuff only if we are in xen tools or
> in xen and build for arm64?
> 
> ie:
> XEN_TOOLS || (XEN && ARM64)

That could work I suppose. Is it really worth it for this one bit?

> >> I don't like the solution to hardcode GIC, IRQs (timer + evtchn) in the 
> >> public interface.
> > 
> > Note that this is inside
> >         #if defined(__XEN__) || defined(__XEN_TOOLS__)
> > so it is not exposed to guests and therefore not part of the guest ABI.
> > The tools ABI is completely fungible and we can change it at will.
> > 
> >>  If we keep this solution, we will need to modify, 
> >> again, the ABI for ARM, why can't we add/extend some hypercalls to give 
> >> theses values to the hypervisor?
> > 
> > That would be overkill until (if!) we get to the point of needing a more
> > dynamic guest layout.
> 
> The current solution has some memory limitations. The layout doesn't
> allow to allocate more than 768MB (space between the GUEST_RAM_BASE and
> GUEST_GNTTAB_BASE).

Yes. We should fix this, but it is out of scope for this series.

> > I'm not going to implement that now. I thought I'd expressed this in the
> > commit message or the cover letter but it seems I forgot. I'll add some
> > words to the commit message for the next posting.
> 
> Ok. Can you document somewhere the guest memory layout?

The stuff which  this patch adds is supposed to be that document.
Specifically:

+/* Physical Address Space */
+#define GUEST_GICD_BASE   0x2c001000ULL
+#define GUEST_GICD_SIZE   0x1000ULL
+#define GUEST_GICC_BASE   0x2c002000ULL
+#define GUEST_GICC_SIZE   0x100ULL
+
+#define GUEST_RAM_BASE    0x80000000ULL
+
+#define GUEST_GNTTAB_BASE 0xb0000000ULL
+#define GUEST_GNTTAB_SIZE 0x00020000ULL

I could duplicate those numbers in a comment but it seems a bit
pointless and prone to skew.

Ian.

[PATCH v3 09/14] libxc: arm: rename various bits of zimage load with 32 suffix

[Ian Campbell]

Making room for a 64 bit implementation.

Also fix a typo and stop refering to it as a bzImage, which is an x86-ism.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
---
 tools/libxc/xc_dom_armzimageloader.c |   44 ++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/tools/libxc/xc_dom_armzimageloader.c b/tools/libxc/xc_dom_armzimageloader.c
index 54728b8..b693390 100644
--- a/tools/libxc/xc_dom_armzimageloader.c
+++ b/tools/libxc/xc_dom_armzimageloader.c
@@ -36,12 +36,6 @@
  */
 #define GUEST_RAM_BASE 0x80000000
 
-#define ZIMAGE_MAGIC_OFFSET 0x24
-#define ZIMAGE_START_OFFSET 0x28
-#define ZIMAGE_END_OFFSET   0x2c
-
-#define ZIMAGE_MAGIC 0x016f2818
-
 struct minimal_dtb_header {
     uint32_t magic;
     uint32_t total_size;
@@ -50,7 +44,17 @@ struct minimal_dtb_header {
 
 #define DTB_MAGIC 0xd00dfeed
 
-static int xc_dom_probe_zimage_kernel(struct xc_dom_image *dom)
+/* ------------------------------------------------------------ */
+/* 32-bit zImage Support                                        */
+/* ------------------------------------------------------------ */
+
+#define ZIMAGE32_MAGIC_OFFSET 0x24
+#define ZIMAGE32_START_OFFSET 0x28
+#define ZIMAGE32_END_OFFSET   0x2c
+
+#define ZIMAGE32_MAGIC 0x016f2818
+
+static int xc_dom_probe_zimage32_kernel(struct xc_dom_image *dom)
 {
     uint32_t *zimage;
     uint32_t end;
@@ -69,13 +73,13 @@ static int xc_dom_probe_zimage_kernel(struct xc_dom_image *dom)
     }
 
     zimage = (uint32_t *)dom->kernel_blob;
-    if ( zimage[ZIMAGE_MAGIC_OFFSET/4] != ZIMAGE_MAGIC )
+    if ( zimage[ZIMAGE32_MAGIC_OFFSET/4] != ZIMAGE32_MAGIC )
     {
-        xc_dom_printf(dom->xch, "%s: kernel is not a bzImage", __FUNCTION__);
+        xc_dom_printf(dom->xch, "%s: kernel is not an arm32 zImage", __FUNCTION__);
         return -EINVAL;
     }
 
-    end = zimage[ZIMAGE_END_OFFSET/4];
+    end = zimage[ZIMAGE32_END_OFFSET/4];
 
     /*
      * Check for an appended DTB.
@@ -94,7 +98,7 @@ static int xc_dom_probe_zimage_kernel(struct xc_dom_image *dom)
     return 0;
 }
 
-static int xc_dom_parse_zimage_kernel(struct xc_dom_image *dom)
+static int xc_dom_parse_zimage32_kernel(struct xc_dom_image *dom)
 {
     uint32_t *zimage;
     uint32_t start, entry_addr;
@@ -111,7 +115,7 @@ static int xc_dom_parse_zimage_kernel(struct xc_dom_image *dom)
     v_start = rambase + 0x8000;
     v_end = v_start + dom->kernel_size;
 
-    start = zimage[ZIMAGE_START_OFFSET/4];
+    start = zimage[ZIMAGE32_START_OFFSET/4];
 
     if (start == 0)
         entry_addr = v_start;
@@ -134,6 +138,10 @@ static int xc_dom_parse_zimage_kernel(struct xc_dom_image *dom)
     return 0;
 }
 
+/* ------------------------------------------------------------ */
+/* Common zImage Support                                        */
+/* ------------------------------------------------------------ */
+
 static int xc_dom_load_zimage_kernel(struct xc_dom_image *dom)
 {
     void *dst;
@@ -148,7 +156,7 @@ static int xc_dom_load_zimage_kernel(struct xc_dom_image *dom)
         return -1;
     }
 
-    DOMPRINTF("%s: kernel sed %#"PRIx64"-%#"PRIx64,
+    DOMPRINTF("%s: kernel seg %#"PRIx64"-%#"PRIx64,
               __func__, dom->kernel_seg.vstart, dom->kernel_seg.vend);
     DOMPRINTF("%s: copy %zd bytes from blob %p to dst %p",
               __func__, dom->kernel_size, dom->kernel_blob, dst);
@@ -158,16 +166,16 @@ static int xc_dom_load_zimage_kernel(struct xc_dom_image *dom)
     return 0;
 }
 
-static struct xc_dom_loader zimage_loader = {
-    .name = "Linux zImage (ARM)",
-    .probe = xc_dom_probe_zimage_kernel,
-    .parser = xc_dom_parse_zimage_kernel,
+static struct xc_dom_loader zimage32_loader = {
+    .name = "Linux zImage (ARM32)",
+    .probe = xc_dom_probe_zimage32_kernel,
+    .parser = xc_dom_parse_zimage32_kernel,
     .loader = xc_dom_load_zimage_kernel,
 };
 
 static void __init register_loader(void)
 {
-    xc_dom_register_loader(&zimage_loader);
+    xc_dom_register_loader(&zimage32_loader);
 }
 
 /*
-- 
1.7.10.4

[PATCH v3 10/14] libxc: allow caller to specify guest rambase rather than hardcoding

[Ian Campbell]

It's still hardcoded but it could now be plausibly be made variable.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_dom.h                 |    1 +
 tools/libxc/xc_dom_armzimageloader.c |   12 +-----------
 tools/libxc/xc_dom_core.c            |    8 ++++++++
 tools/libxl/libxl_dom.c              |    6 ++++++
 4 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h
index 86e23ee..ed49aa7 100644
--- a/tools/libxc/xc_dom.h
+++ b/tools/libxc/xc_dom.h
@@ -197,6 +197,7 @@ struct xc_dom_image *xc_dom_allocate(xc_interface *xch,
                                      const char *cmdline, const char *features);
 void xc_dom_release_phys(struct xc_dom_image *dom);
 void xc_dom_release(struct xc_dom_image *dom);
+int xc_dom_rambase_init(struct xc_dom_image *dom, uint64_t rambase);
 int xc_dom_mem_init(struct xc_dom_image *dom, unsigned int mem_mb);
 
 /* Set this larger if you have enormous ramdisks/kernels. Note that
diff --git a/tools/libxc/xc_dom_armzimageloader.c b/tools/libxc/xc_dom_armzimageloader.c
index b693390..4e3f7ae 100644
--- a/tools/libxc/xc_dom_armzimageloader.c
+++ b/tools/libxc/xc_dom_armzimageloader.c
@@ -30,12 +30,6 @@
 
 #include <arpa/inet.h> /* XXX ntohl is not the right function... */
 
-/*
- * Guest virtual RAM starts here. This must be consistent with the DTB
- * appended to the guest kernel.
- */
-#define GUEST_RAM_BASE 0x80000000
-
 struct minimal_dtb_header {
     uint32_t magic;
     uint32_t total_size;
@@ -103,14 +97,12 @@ static int xc_dom_parse_zimage32_kernel(struct xc_dom_image *dom)
     uint32_t *zimage;
     uint32_t start, entry_addr;
     uint64_t v_start, v_end;
-    uint64_t rambase = GUEST_RAM_BASE;
+    uint64_t rambase = dom->rambase_pfn << XC_PAGE_SHIFT;
 
     DOMPRINTF_CALLED(dom->xch);
 
     zimage = (uint32_t *)dom->kernel_blob;
 
-    dom->rambase_pfn = rambase >> XC_PAGE_SHIFT;
-
     /* Do not load kernel at the very first RAM address */
     v_start = rambase + 0x8000;
     v_end = v_start + dom->kernel_size;
@@ -130,8 +122,6 @@ static int xc_dom_parse_zimage32_kernel(struct xc_dom_image *dom)
     dom->parms.virt_base = rambase;
 
     dom->guest_type = "xen-3.0-armv7l";
-    DOMPRINTF("%s: %s: RAM starts at %"PRI_xen_pfn,
-              __FUNCTION__, dom->guest_type, dom->rambase_pfn);
     DOMPRINTF("%s: %s: 0x%" PRIx64 " -> 0x%" PRIx64 "",
               __FUNCTION__, dom->guest_type,
               dom->kernel_seg.vstart, dom->kernel_seg.vend);
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index 3bf51ef..6aa0176 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -785,6 +785,14 @@ int xc_dom_parse_image(struct xc_dom_image *dom)
     return -1;
 }
 
+int xc_dom_rambase_init(struct xc_dom_image *dom, uint64_t rambase)
+{
+    dom->rambase_pfn = rambase >> XC_PAGE_SHIFT;
+    DOMPRINTF("%s: RAM starts at %"PRI_xen_pfn,
+              __FUNCTION__, dom->rambase_pfn);
+    return 0;
+}
+
 int xc_dom_mem_init(struct xc_dom_image *dom, unsigned int mem_mb)
 {
     unsigned int page_shift;
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 1812bdc..10f976c 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -390,6 +390,12 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
         LOGE(ERROR, "xc_dom_boot_xen_init failed");
         goto out;
     }
+#ifdef GUEST_RAM_BASE
+    if ( (ret = xc_dom_rambase_init(dom, GUEST_RAM_BASE)) != 0 ) {
+        LOGE(ERROR, "xc_dom_rambase failed");
+        goto out;
+    }
+#endif
     if ( (ret = xc_dom_parse_image(dom)) != 0 ) {
         LOGE(ERROR, "xc_dom_parse_image failed");
         goto out;
-- 
1.7.10.4

Re: [PATCH v3 10/14] libxc: allow caller to specify guest rambase rather than hardcoding

[Ian Jackson]

Ian Campbell writes ("[PATCH v3 10/14] libxc: allow caller to specify guest rambase rather than hardcoding"):
> It's still hardcoded but it could now be plausibly be made variable.
...
> +#ifdef GUEST_RAM_BASE
> +    if ( (ret = xc_dom_rambase_init(dom, GUEST_RAM_BASE)) != 0 ) {
> +        LOGE(ERROR, "xc_dom_rambase failed");
> +        goto out;
> +    }
> +#endif

This is weird.  Where is the definition of GUEST_RAM_BASE ?

Ian.

Re: [PATCH v3 10/14] libxc: allow caller to specify guest rambase rather than hardcoding

[Ian Campbell]

On Thu, 2013-11-07 at 17:06 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH v3 10/14] libxc: allow caller to specify guest rambase rather than hardcoding"):
> > It's still hardcoded but it could now be plausibly be made variable.
> ...
> > +#ifdef GUEST_RAM_BASE
> > +    if ( (ret = xc_dom_rambase_init(dom, GUEST_RAM_BASE)) != 0 ) {
> > +        LOGE(ERROR, "xc_dom_rambase failed");
> > +        goto out;
> > +    }
> > +#endif
> 
> This is weird.  Where is the definition of GUEST_RAM_BASE ?

In "xen: arm: define guest virtual platform in API headers" which is in
patch #8.

Ian.

Re: [PATCH v3 10/14] libxc: allow caller to specify guest rambase rather than hardcoding

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v3 10/14] libxc: allow caller to specify guest rambase rather than hardcoding"):
> On Thu, 2013-11-07 at 17:06 +0000, Ian Jackson wrote:
> > This is weird.  Where is the definition of GUEST_RAM_BASE ?
> 
> In "xen: arm: define guest virtual platform in API headers" which is in
> patch #8.

Oh, OK, sorry.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH v3 11/14] libxc: allow passing a device tree blob to the guest

[Ian Campbell]

TODO: placement is rather simplistic

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_dom.h      |    8 ++++++++
 tools/libxc/xc_dom_arm.c  |   15 +++++++++++++-
 tools/libxc/xc_dom_core.c |   48 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h
index ed49aa7..bf56436 100644
--- a/tools/libxc/xc_dom.h
+++ b/tools/libxc/xc_dom.h
@@ -54,9 +54,12 @@ struct xc_dom_image {
     size_t kernel_size;
     void *ramdisk_blob;
     size_t ramdisk_size;
+    void *devicetree_blob;
+    size_t devicetree_size;
 
     size_t max_kernel_size;
     size_t max_ramdisk_size;
+    size_t max_devicetree_size;
 
     /* arguments and parameters */
     char *cmdline;
@@ -217,6 +220,8 @@ int xc_dom_kernel_max_size(struct xc_dom_image *dom, size_t sz);
 int xc_dom_ramdisk_check_size(struct xc_dom_image *dom, size_t sz);
 int xc_dom_ramdisk_max_size(struct xc_dom_image *dom, size_t sz);
 
+int xc_dom_devicetree_max_size(struct xc_dom_image *dom, size_t sz);
+
 size_t xc_dom_check_gzip(xc_interface *xch,
                      void *blob, size_t ziplen);
 int xc_dom_do_gunzip(xc_interface *xch,
@@ -229,6 +234,9 @@ int xc_dom_kernel_mem(struct xc_dom_image *dom, const void *mem,
                       size_t memsize);
 int xc_dom_ramdisk_mem(struct xc_dom_image *dom, const void *mem,
                        size_t memsize);
+int xc_dom_devicetree_file(struct xc_dom_image *dom, const char *filename);
+int xc_dom_devicetree_mem(struct xc_dom_image *dom, const void *mem,
+                          size_t memsize);
 
 int xc_dom_parse_image(struct xc_dom_image *dom);
 struct xc_dom_arch *xc_dom_find_arch_hooks(xc_interface *xch, char *guest_type);
diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 9f3fdd3..ee2c9cc 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -124,7 +124,8 @@ static int vcpu_arm(struct xc_dom_image *dom, void *ptr)
      * using CONFIG_ARM_APPENDED_DTB. Ensure that r2 does not look
      * like a valid pointer to a set of ATAGS or a DTB.
      */
-    ctxt->user_regs.r2_usr = 0xffffffff;
+    ctxt->user_regs.r2_usr = dom->devicetree_blob ?
+        dom->devicetree_seg.vstart : 0xffffffff;
 
     ctxt->sctlr = SCTLR_GUEST_INIT;
 
@@ -191,6 +192,18 @@ int arch_setup_meminit(struct xc_dom_image *dom)
             0, 0, &dom->p2m_host[i]);
     }
 
+    if ( dom->devicetree_blob )
+    {
+        uint64_t rambase = dom->rambase_pfn << XC_PAGE_SHIFT;
+        /* XXX intelligent placement */
+        dom->devicetree_seg.vstart = rambase + 127*1024*1024;
+        dom->devicetree_seg.vend =
+            dom->devicetree_seg.vstart + dom->devicetree_size;
+        DOMPRINTF("%s: devicetree: 0x%" PRIx64 " -> 0x%" PRIx64 "",
+                  __FUNCTION__,
+                  dom->devicetree_seg.vstart, dom->devicetree_seg.vend);
+    }
+
     return 0;
 }
 
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index 6aa0176..338c9a1 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -671,6 +671,7 @@ struct xc_dom_image *xc_dom_allocate(xc_interface *xch,
 
     dom->max_kernel_size = XC_DOM_DECOMPRESS_MAX;
     dom->max_ramdisk_size = XC_DOM_DECOMPRESS_MAX;
+    dom->max_devicetree_size = XC_DOM_DECOMPRESS_MAX;
 
     if ( cmdline )
         dom->cmdline = xc_dom_strdup(dom, cmdline);
@@ -706,6 +707,13 @@ int xc_dom_ramdisk_max_size(struct xc_dom_image *dom, size_t sz)
     return 0;
 }
 
+int xc_dom_devicetree_max_size(struct xc_dom_image *dom, size_t sz)
+{
+    DOMPRINTF("%s: devicetree_max_size=%zx", __FUNCTION__, sz);
+    dom->max_devicetree_size = sz;
+    return 0;
+}
+
 int xc_dom_kernel_file(struct xc_dom_image *dom, const char *filename)
 {
     DOMPRINTF("%s: filename=\"%s\"", __FUNCTION__, filename);
@@ -729,6 +737,18 @@ int xc_dom_ramdisk_file(struct xc_dom_image *dom, const char *filename)
     return 0;
 }
 
+int xc_dom_devicetree_file(struct xc_dom_image *dom, const char *filename)
+{
+    DOMPRINTF("%s: filename=\"%s\"", __FUNCTION__, filename);
+    dom->devicetree_blob =
+        xc_dom_malloc_filemap(dom, filename, &dom->devicetree_size,
+                              dom->max_devicetree_size);
+
+    if ( dom->devicetree_blob == NULL )
+        return -1;
+    return 0;
+}
+
 int xc_dom_kernel_mem(struct xc_dom_image *dom, const void *mem, size_t memsize)
 {
     DOMPRINTF_CALLED(dom->xch);
@@ -747,6 +767,15 @@ int xc_dom_ramdisk_mem(struct xc_dom_image *dom, const void *mem,
     return 0;
 }
 
+int xc_dom_devicetree_mem(struct xc_dom_image *dom, const void *mem,
+                          size_t memsize)
+{
+    DOMPRINTF_CALLED(dom->xch);
+    dom->devicetree_blob = (void *)mem;
+    dom->devicetree_size = memsize;
+    return 0;
+}
+
 int xc_dom_parse_image(struct xc_dom_image *dom)
 {
     int i;
@@ -916,6 +945,25 @@ int xc_dom_build_image(struct xc_dom_image *dom)
             memcpy(ramdiskmap, dom->ramdisk_blob, dom->ramdisk_size);
     }
 
+    /* load devicetree */
+    if ( dom->devicetree_blob )
+    {
+        void *devicetreemap;
+
+        if ( xc_dom_alloc_segment(dom, &dom->devicetree_seg, "devicetree",
+                                  dom->devicetree_seg.vstart,
+                                  dom->devicetree_size) != 0 )
+            goto err;
+        devicetreemap = xc_dom_seg_to_ptr(dom, &dom->devicetree_seg);
+        if ( devicetreemap == NULL )
+        {
+            DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &dom->devicetree_seg) => NULL",
+                      __FUNCTION__);
+            goto err;
+        }
+        memcpy(devicetreemap, dom->devicetree_blob, dom->devicetree_size);
+    }
+
     /* allocate other pages */
     if ( dom->arch_hooks->alloc_magic_pages(dom) != 0 )
         goto err;
-- 
1.7.10.4

Re: [PATCH v3 11/14] libxc: allow passing a device tree blob to the guest

[Ian Jackson]

Ian Campbell writes ("[PATCH v3 11/14] libxc: allow passing a device tree blob to the guest"):
> TODO: placement is rather simplistic

I guess this will do.

But AFAICT you fail to reject DTBs on x86, and don't arrange to
provide them to the kernel either.

Ian.

Re: [PATCH v3 11/14] libxc: allow passing a device tree blob to the guest

[Ian Campbell]

On Thu, 2013-11-07 at 17:09 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH v3 11/14] libxc: allow passing a device tree blob to the guest"):
> > TODO: placement is rather simplistic
> 
> I guess this will do.
> 
> But AFAICT you fail to reject DTBs on x86, and don't arrange to
> provide them to the kernel either.

It'd have to reject it, since DTB isn't used much on x86 outside of
specific embedded use case, would an ifdef be acceptable?

> 
> Ian.

Re: [PATCH v3 11/14] libxc: allow passing a device tree blob to the guest

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v3 11/14] libxc: allow passing a device tree blob to the guest"):
> On Thu, 2013-11-07 at 17:09 +0000, Ian Jackson wrote:
> > But AFAICT you fail to reject DTBs on x86, and don't arrange to
> > provide them to the kernel either.
> 
> It'd have to reject it, since DTB isn't used much on x86 outside of
> specific embedded use case, would an ifdef be acceptable?

I don't particularly mind how it's done.  You could define an
always-failing DTB function in the x86 libxc code, alternatively.

Ian.

Re: [PATCH v3 11/14] libxc: allow passing a device tree blob to the guest

[Ian Campbell]

On Thu, 2013-11-07 at 17:31 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH v3 11/14] libxc: allow passing a device tree blob to the guest"):
> > On Thu, 2013-11-07 at 17:09 +0000, Ian Jackson wrote:
> > > But AFAICT you fail to reject DTBs on x86, and don't arrange to
> > > provide them to the kernel either.
> > 
> > It'd have to reject it, since DTB isn't used much on x86 outside of
> > specific embedded use case, would an ifdef be acceptable?
> 
> I don't particularly mind how it's done.  You could define an
> always-failing DTB function in the x86 libxc code, alternatively.

I'll have a dig and see which feels more natural in the context.

Ian.

Re: [PATCH v3 11/14] libxc: allow passing a device tree blob to the guest

[Ian Campbell]

On Thu, 2013-11-07 at 17:42 +0000, Ian Campbell wrote:
> On Thu, 2013-11-07 at 17:31 +0000, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [PATCH v3 11/14] libxc: allow passing a device tree blob to the guest"):
> > > On Thu, 2013-11-07 at 17:09 +0000, Ian Jackson wrote:
> > > > But AFAICT you fail to reject DTBs on x86, and don't arrange to
> > > > provide them to the kernel either.
> > > 
> > > It'd have to reject it, since DTB isn't used much on x86 outside of
> > > specific embedded use case, would an ifdef be acceptable?
> > 
> > I don't particularly mind how it's done.  You could define an
> > always-failing DTB function in the x86 libxc code, alternatively.
> 
> I'll have a dig and see which feels more natural in the context.

I went with the ifdef since it seemed most natural to keep most of this
functionality in xc_dom_core alongside all the other stuff for kernels
and initrds etc.

Ian.

[PATCH v3 12/14] libxc: support for arm64 Image format

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_dom_armzimageloader.c |   88 ++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/tools/libxc/xc_dom_armzimageloader.c b/tools/libxc/xc_dom_armzimageloader.c
index 4e3f7ae..609d3a0 100644
--- a/tools/libxc/xc_dom_armzimageloader.c
+++ b/tools/libxc/xc_dom_armzimageloader.c
@@ -129,6 +129,86 @@ static int xc_dom_parse_zimage32_kernel(struct xc_dom_image *dom)
 }
 
 /* ------------------------------------------------------------ */
+/* 64-bit zImage Support                                        */
+/* ------------------------------------------------------------ */
+
+#define ZIMAGE64_MAGIC_V0 0x14000008
+#define ZIMAGE64_MAGIC_V1 0x644d5241 /* "ARM\x64" */
+
+/* linux/Documentation/arm64/booting.txt */
+struct zimage64_hdr {
+    uint32_t magic0;
+    uint32_t res0;
+    uint64_t text_offset;  /* Image load offset */
+    uint64_t res1;
+    uint64_t res2;
+    /* zImage V1 only from here */
+    uint64_t res3;
+    uint64_t res4;
+    uint64_t res5;
+    uint32_t magic1;
+    uint32_t res6;
+};
+static int xc_dom_probe_zimage64_kernel(struct xc_dom_image *dom)
+{
+    struct zimage64_hdr *zimage;
+
+    if ( dom->kernel_blob == NULL )
+    {
+        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
+                     "%s: no kernel image loaded", __FUNCTION__);
+        return -EINVAL;
+    }
+
+    if ( dom->kernel_size < sizeof(*zimage) )
+    {
+        xc_dom_printf(dom->xch, "%s: kernel image too small", __FUNCTION__);
+        return -EINVAL;
+    }
+
+    zimage =  dom->kernel_blob;
+    if ( zimage->magic0 != ZIMAGE64_MAGIC_V0 &&
+         zimage->magic1 != ZIMAGE64_MAGIC_V1 )
+    {
+        xc_dom_printf(dom->xch, "%s: kernel is not an arm64 Image", __FUNCTION__);
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
+static int xc_dom_parse_zimage64_kernel(struct xc_dom_image *dom)
+{
+    struct zimage64_hdr *zimage;
+    uint32_t entry_addr;
+    uint64_t v_start, v_end;
+    uint64_t rambase = dom->rambase_pfn << XC_PAGE_SHIFT;
+
+    DOMPRINTF_CALLED(dom->xch);
+
+    zimage = dom->kernel_blob;
+
+    v_start = rambase + zimage->text_offset;
+    v_end = v_start + dom->kernel_size;
+
+    entry_addr = v_start;
+
+    /* find kernel segment */
+    dom->kernel_seg.vstart = v_start;
+    dom->kernel_seg.vend   = v_start + dom->kernel_size;
+
+    dom->parms.virt_entry = entry_addr;
+    dom->parms.virt_base = rambase;
+
+    dom->guest_type = "xen-3.0-aarch64";
+    DOMPRINTF("%s: %s: 0x%" PRIx64 " -> 0x%" PRIx64 "",
+              __FUNCTION__, dom->guest_type,
+              dom->kernel_seg.vstart, dom->kernel_seg.vend);
+
+    return 0;
+}
+
+/* ------------------------------------------------------------ */
 /* Common zImage Support                                        */
 /* ------------------------------------------------------------ */
 
@@ -163,9 +243,17 @@ static struct xc_dom_loader zimage32_loader = {
     .loader = xc_dom_load_zimage_kernel,
 };
 
+static struct xc_dom_loader zimage64_loader = {
+    .name = "Linux zImage (ARM64)",
+    .probe = xc_dom_probe_zimage64_kernel,
+    .parser = xc_dom_parse_zimage64_kernel,
+    .loader = xc_dom_load_zimage_kernel,
+};
+
 static void __init register_loader(void)
 {
     xc_dom_register_loader(&zimage32_loader);
+    xc_dom_register_loader(&zimage64_loader);
 }
 
 /*
-- 
1.7.10.4

Re: [PATCH v3 12/14] libxc: support for arm64 Image format

[Julien Grall]

On 11/07/2013 04:44 PM, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxc/xc_dom_armzimageloader.c |   88 ++++++++++++++++++++++++++++++++++
>  1 file changed, 88 insertions(+)
> 
> diff --git a/tools/libxc/xc_dom_armzimageloader.c b/tools/libxc/xc_dom_armzimageloader.c
> index 4e3f7ae..609d3a0 100644
> --- a/tools/libxc/xc_dom_armzimageloader.c
> +++ b/tools/libxc/xc_dom_armzimageloader.c
> @@ -129,6 +129,86 @@ static int xc_dom_parse_zimage32_kernel(struct xc_dom_image *dom)
>  }
>  
>  /* ------------------------------------------------------------ */
> +/* 64-bit zImage Support                                        */
> +/* ------------------------------------------------------------ */
> +
> +#define ZIMAGE64_MAGIC_V0 0x14000008
> +#define ZIMAGE64_MAGIC_V1 0x644d5241 /* "ARM\x64" */
> +
> +/* linux/Documentation/arm64/booting.txt */
> +struct zimage64_hdr {
> +    uint32_t magic0;
> +    uint32_t res0;
> +    uint64_t text_offset;  /* Image load offset */
> +    uint64_t res1;
> +    uint64_t res2;
> +    /* zImage V1 only from here */
> +    uint64_t res3;
> +    uint64_t res4;
> +    uint64_t res5;
> +    uint32_t magic1;
> +    uint32_t res6;
> +};
> +static int xc_dom_probe_zimage64_kernel(struct xc_dom_image *dom)
> +{
> +    struct zimage64_hdr *zimage;
> +
> +    if ( dom->kernel_blob == NULL )
> +    {
> +        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> +                     "%s: no kernel image loaded", __FUNCTION__);
> +        return -EINVAL;
> +    }
> +
> +    if ( dom->kernel_size < sizeof(*zimage) )
> +    {
> +        xc_dom_printf(dom->xch, "%s: kernel image too small", __FUNCTION__);
> +        return -EINVAL;
> +    }
> +
> +    zimage =  dom->kernel_blob;
> +    if ( zimage->magic0 != ZIMAGE64_MAGIC_V0 &&
> +         zimage->magic1 != ZIMAGE64_MAGIC_V1 )
> +    {
> +        xc_dom_printf(dom->xch, "%s: kernel is not an arm64 Image", __FUNCTION__);
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +static int xc_dom_parse_zimage64_kernel(struct xc_dom_image *dom)
> +{
> +    struct zimage64_hdr *zimage;
> +    uint32_t entry_addr;
> +    uint64_t v_start, v_end;
> +    uint64_t rambase = dom->rambase_pfn << XC_PAGE_SHIFT;
> +
> +    DOMPRINTF_CALLED(dom->xch);
> +
> +    zimage = dom->kernel_blob;
> +
> +    v_start = rambase + zimage->text_offset;
> +    v_end = v_start + dom->kernel_size;

You define v_end which is not use in the function.

-- 
Julien Grall

[PATCH v3 13/14] libxc: arm64 vcpu initialisation

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_dom_arm.c |   90 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 88 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index ee2c9cc..03db256 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -105,7 +105,7 @@ static int shared_info_arm(struct xc_dom_image *dom, void *ptr)
 
 /* ------------------------------------------------------------------------ */
 
-static int vcpu_arm(struct xc_dom_image *dom, void *ptr)
+static int vcpu_arm32(struct xc_dom_image *dom, void *ptr)
 {
     vcpu_guest_context_t *ctxt = ptr;
 
@@ -143,6 +143,41 @@ static int vcpu_arm(struct xc_dom_image *dom, void *ptr)
     return 0;
 }
 
+static int vcpu_arm64(struct xc_dom_image *dom, void *ptr)
+{
+    vcpu_guest_context_t *ctxt = ptr;
+
+    DOMPRINTF_CALLED(dom->xch);
+    /* clear everything */
+    memset(ctxt, 0, sizeof(*ctxt));
+
+    ctxt->user_regs.pc64 = dom->parms.virt_entry;
+
+    /* Linux boot protocol. See linux.Documentation/arm/Booting. */
+    ctxt->user_regs.x0 = dom->devicetree_blob ?
+        dom->devicetree_seg.vstart : 0xffffffff;
+    ctxt->user_regs.x1 = 0;
+    ctxt->user_regs.x2 = 0;
+    ctxt->user_regs.x3 = 0;
+
+    DOMPRINTF("DTB %"PRIx64, ctxt->user_regs.x0);
+
+    ctxt->sctlr = SCTLR_GUEST_INIT;
+
+    ctxt->ttbr0 = 0;
+    ctxt->ttbr1 = 0;
+    ctxt->ttbcr = 0; /* Defined Reset Value */
+
+    ctxt->user_regs.cpsr = PSR_GUEST64_INIT;
+
+    ctxt->flags = VGCF_online;
+
+    DOMPRINTF("Initial state CPSR %#"PRIx32" PC %#"PRIx64,
+           ctxt->user_regs.cpsr, ctxt->user_regs.pc64);
+
+    return 0;
+}
+
 /* ------------------------------------------------------------------------ */
 
 static struct xc_dom_arch xc_dom_32 = {
@@ -155,12 +190,59 @@ static struct xc_dom_arch xc_dom_32 = {
     .setup_pgtables = setup_pgtables_arm,
     .start_info = start_info_arm,
     .shared_info = shared_info_arm,
-    .vcpu = vcpu_arm,
+    .vcpu = vcpu_arm32,
+};
+
+static struct xc_dom_arch xc_dom_64 = {
+    .guest_type = "xen-3.0-aarch64",
+    .native_protocol = XEN_IO_PROTO_ABI_ARM,
+    .page_shift = PAGE_SHIFT_ARM,
+    .sizeof_pfn = 8,
+    .alloc_magic_pages = alloc_magic_pages,
+    .count_pgtables = count_pgtables_arm,
+    .setup_pgtables = setup_pgtables_arm,
+    .start_info = start_info_arm,
+    .shared_info = shared_info_arm,
+    .vcpu = vcpu_arm64,
 };
 
 static void __init register_arch_hooks(void)
 {
     xc_dom_register_arch_hooks(&xc_dom_32);
+    xc_dom_register_arch_hooks(&xc_dom_64);
+}
+
+static int set_mode(xc_interface *xch, domid_t domid, char *guest_type)
+{
+    static const struct {
+        char           *guest;
+        uint32_t        size;
+    } types[] = {
+        { "xen-3.0-aarch64", 64 },
+        { "xen-3.0-armv7l",  32 },
+    };
+    DECLARE_DOMCTL;
+    int i,rc;
+
+    domctl.domain = domid;
+    domctl.cmd    = XEN_DOMCTL_set_address_size;
+    for ( i = 0; i < sizeof(types)/sizeof(types[0]); i++ )
+        if ( !strcmp(types[i].guest, guest_type) )
+            domctl.u.address_size.size = types[i].size;
+    if ( domctl.u.address_size.size == 0 )
+    {
+        xc_dom_printf(xch, "%s: warning: unknown guest type %s",
+                      __FUNCTION__, guest_type);
+        return -EINVAL;
+    }
+
+    xc_dom_printf(xch, "%s: guest %s, address size %" PRId32 "", __FUNCTION__,
+                  guest_type, domctl.u.address_size.size);
+    rc = do_domctl(xch, &domctl);
+    if ( rc != 0 )
+        xc_dom_printf(xch, "%s: warning: failed (rc=%d)",
+                      __FUNCTION__, rc);
+    return rc;
 }
 
 int arch_setup_meminit(struct xc_dom_image *dom)
@@ -168,6 +250,10 @@ int arch_setup_meminit(struct xc_dom_image *dom)
     int rc;
     xen_pfn_t pfn, allocsz, i;
 
+    rc = set_mode(dom->xch, dom->guest_domid, dom->guest_type);
+    if ( rc )
+        return rc;
+
     dom->shadow_enabled = 1;
 
     dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->total_pages);
-- 
1.7.10.4

Re: [PATCH v3 13/14] libxc: arm64 vcpu initialisation

[Ian Jackson]

Ian Campbell writes ("[PATCH v3 13/14] libxc: arm64 vcpu initialisation"):
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

(on the basis that it's arm-specific, rather than any review.)

Re: [PATCH v3 13/14] libxc: arm64 vcpu initialisation

[Julien Grall]

On 11/07/2013 04:44 PM, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
> ---
>  tools/libxc/xc_dom_arm.c |   90 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 88 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index ee2c9cc..03db256 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -105,7 +105,7 @@ static int shared_info_arm(struct xc_dom_image *dom, void *ptr)
>  
>  /* ------------------------------------------------------------------------ */
>  
> -static int vcpu_arm(struct xc_dom_image *dom, void *ptr)
> +static int vcpu_arm32(struct xc_dom_image *dom, void *ptr)
>  {
>      vcpu_guest_context_t *ctxt = ptr;
>  
> @@ -143,6 +143,41 @@ static int vcpu_arm(struct xc_dom_image *dom, void *ptr)
>      return 0;
>  }
>  
> +static int vcpu_arm64(struct xc_dom_image *dom, void *ptr)
> +{
> +    vcpu_guest_context_t *ctxt = ptr;
> +
> +    DOMPRINTF_CALLED(dom->xch);
> +    /* clear everything */
> +    memset(ctxt, 0, sizeof(*ctxt));
> +
> +    ctxt->user_regs.pc64 = dom->parms.virt_entry;
> +
> +    /* Linux boot protocol. See linux.Documentation/arm/Booting. */
> +    ctxt->user_regs.x0 = dom->devicetree_blob ?
> +        dom->devicetree_seg.vstart : 0xffffffff;
> +    ctxt->user_regs.x1 = 0;
> +    ctxt->user_regs.x2 = 0;
> +    ctxt->user_regs.x3 = 0;
> +
> +    DOMPRINTF("DTB %"PRIx64, ctxt->user_regs.x0);
> +
> +    ctxt->sctlr = SCTLR_GUEST_INIT;
> +
> +    ctxt->ttbr0 = 0;
> +    ctxt->ttbr1 = 0;
> +    ctxt->ttbcr = 0; /* Defined Reset Value */
> +
> +    ctxt->user_regs.cpsr = PSR_GUEST64_INIT;
> +
> +    ctxt->flags = VGCF_online;
> +
> +    DOMPRINTF("Initial state CPSR %#"PRIx32" PC %#"PRIx64,
> +           ctxt->user_regs.cpsr, ctxt->user_regs.pc64);
> +
> +    return 0;
> +}
> +
>  /* ------------------------------------------------------------------------ */
>  
>  static struct xc_dom_arch xc_dom_32 = {
> @@ -155,12 +190,59 @@ static struct xc_dom_arch xc_dom_32 = {
>      .setup_pgtables = setup_pgtables_arm,
>      .start_info = start_info_arm,
>      .shared_info = shared_info_arm,
> -    .vcpu = vcpu_arm,
> +    .vcpu = vcpu_arm32,
> +};
> +
> +static struct xc_dom_arch xc_dom_64 = {
> +    .guest_type = "xen-3.0-aarch64",
> +    .native_protocol = XEN_IO_PROTO_ABI_ARM,
> +    .page_shift = PAGE_SHIFT_ARM,
> +    .sizeof_pfn = 8,
> +    .alloc_magic_pages = alloc_magic_pages,
> +    .count_pgtables = count_pgtables_arm,
> +    .setup_pgtables = setup_pgtables_arm,
> +    .start_info = start_info_arm,
> +    .shared_info = shared_info_arm,
> +    .vcpu = vcpu_arm64,
>  };
>  
>  static void __init register_arch_hooks(void)
>  {
>      xc_dom_register_arch_hooks(&xc_dom_32);
> +    xc_dom_register_arch_hooks(&xc_dom_64);
> +}
> +
> +static int set_mode(xc_interface *xch, domid_t domid, char *guest_type)
> +{
> +    static const struct {
> +        char           *guest;
> +        uint32_t        size;
> +    } types[] = {
> +        { "xen-3.0-aarch64", 64 },
> +        { "xen-3.0-armv7l",  32 },
> +    };
> +    DECLARE_DOMCTL;
> +    int i,rc;
> +
> +    domctl.domain = domid;
> +    domctl.cmd    = XEN_DOMCTL_set_address_size;
> +    for ( i = 0; i < sizeof(types)/sizeof(types[0]); i++ )
> +        if ( !strcmp(types[i].guest, guest_type) )
> +            domctl.u.address_size.size = types[i].size;
> +    if ( domctl.u.address_size.size == 0 )
> +    {
> +        xc_dom_printf(xch, "%s: warning: unknown guest type %s",
> +                      __FUNCTION__, guest_type);
> +        return -EINVAL;
> +    }
> +
> +    xc_dom_printf(xch, "%s: guest %s, address size %" PRId32 "", __FUNCTION__,
> +                  guest_type, domctl.u.address_size.size);
> +    rc = do_domctl(xch, &domctl);
> +    if ( rc != 0 )
> +        xc_dom_printf(xch, "%s: warning: failed (rc=%d)",
> +                      __FUNCTION__, rc);
> +    return rc;
>  }
>  
>  int arch_setup_meminit(struct xc_dom_image *dom)
> @@ -168,6 +250,10 @@ int arch_setup_meminit(struct xc_dom_image *dom)
>      int rc;
>      xen_pfn_t pfn, allocsz, i;
>  
> +    rc = set_mode(dom->xch, dom->guest_domid, dom->guest_type);
> +    if ( rc )
> +        return rc;
> +
>      dom->shadow_enabled = 1;
>  
>      dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->total_pages);
> 


-- 
Julien Grall

[PATCH v3 14/14] libxl: build a device tree for ARM guests

[Ian Campbell]

Uses xc_dom_devicetree_mem which was just added. The call to this needs to be
carefully sequenced to be after xc_dom_parse_image (so we can tell which kind
of guest we are building, although we don't use this yet) and before
xc_dom_mem_init which tries to decide where to place the FDT in guest RAM.

Removes libxl_noarch which would only have been used by IA64 after this
change. Remove IA64 as part of this patch.

There is no attempt to expose this as a configuration setting for the user.

Includes a debug hook to dump the dtb to a file for inspection.

TODO:
- v7 CPU compat is hardcoded to cortex-a15 -- may need to define something more
  generic via mach-virt dt bindngs?

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: base addresses, irq, evtchn etc stuff is now from public API headers,
    avoiding the need to introduce domctls etc until we want to make them
    dynamic.
    fix memory node
    Improve libfdt error handling, especially for FDT_ERR_NOSPACE.
    Derive guest CPU and timer compatiblity nodes from the guest type.
---
 tools/libxl/Makefile       |    6 +-
 tools/libxl/libxl_arch.h   |    4 +
 tools/libxl/libxl_arm.c    |  553 ++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_dom.c    |    4 +
 tools/libxl/libxl_noarch.c |    8 -
 tools/libxl/libxl_x86.c    |    7 +
 6 files changed, 572 insertions(+), 10 deletions(-)
 create mode 100644 tools/libxl/libxl_arm.c
 delete mode 100644 tools/libxl/libxl_noarch.c

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index cf214bb..d8495bb 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -28,9 +28,12 @@ CFLAGS_LIBXL += $(CFLAGS_libxenstore)
 CFLAGS_LIBXL += $(CFLAGS_libblktapctl) 
 CFLAGS_LIBXL += -Wshadow
 
+LIBXL_LIBS-$(CONFIG_ARM) += -lfdt
+
 CFLAGS += $(PTHREAD_CFLAGS)
 LDFLAGS += $(PTHREAD_LDFLAGS)
 LIBXL_LIBS += $(PTHREAD_LIBS)
+LIBXL_LIBS += $(LIBXL_LIBS-y)
 
 LIBXLU_LIBS =
 
@@ -41,8 +44,7 @@ else
 LIBXL_OBJS-y += libxl_noblktap2.o
 endif
 LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o
-LIBXL_OBJS-$(CONFIG_IA64) += libxl_nocpuid.o libxl_noarch.o
-LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_noarch.o
+LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o
 
 ifeq ($(CONFIG_NetBSD),y)
 LIBXL_OBJS-y += libxl_netbsd.o
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index abe6685..3de2d76 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -19,4 +19,8 @@
 int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
                uint32_t domid);
 
+/* */
+int libxl__arch_domain_configure(libxl__gc *gc,
+                                 libxl_domain_build_info *info,
+                                 struct xc_dom_image *dom);
 #endif
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
new file mode 100644
index 0000000..6bc3536
--- /dev/null
+++ b/tools/libxl/libxl_arm.c
@@ -0,0 +1,553 @@
+#include "libxl_internal.h"
+#include "libxl_arch.h"
+
+#include <xc_dom.h>
+#include <libfdt.h>
+#include <assert.h>
+
+int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
+                              uint32_t domid)
+{
+    return 0;
+}
+
+static struct arch_info {
+    const char *guest_type;
+    const char *timer_compat;
+    const char *cpu_compat;
+} arch_info[] = {
+    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15" },
+    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" },
+};
+
+enum {
+    PHANDLE_NONE = 0,
+    PHANDLE_GIC,
+};
+
+typedef uint32_t __be32;
+typedef __be32 gic_interrupt_t[3];
+
+#define ROOT_ADDRESS_CELLS 2
+#define ROOT_SIZE_CELLS 2
+
+static void set_cell(__be32 **cellp, int size, uint64_t val)
+{
+    int cells = size;
+
+    while ( size-- )
+    {
+        (*cellp)[size] = cpu_to_fdt32(val);
+        val >>= 32;
+    }
+
+    (*cellp) += cells;
+}
+
+static void set_interrupt_ppi(gic_interrupt_t interrupt, unsigned int irq,
+                              unsigned int cpumask, unsigned int level)
+{
+    __be32 *cells = interrupt;
+
+    /* See linux Documentation/devictree/bindings/arm/gic.txt */
+    set_cell(&cells, 1, 1); /* is a PPI */
+    set_cell(&cells, 1, irq - 16); /* PPIs start at 16 */
+    set_cell(&cells, 1, (cpumask << 8) | level);
+}
+
+static void set_range(__be32 **cellp,
+                      int address_cells, int size_cells,
+                      uint64_t address, uint64_t size)
+{
+    set_cell(cellp, address_cells, address);
+    set_cell(cellp, size_cells, size);
+}
+
+static int fdt_property_compat(libxl__gc *gc, void *fdt, unsigned nr_compat, ...)
+{
+    const char *compats[nr_compat];
+    int i;
+    size_t sz;
+    va_list ap;
+    char *compat, *p;
+
+    va_start(ap, nr_compat);
+    sz = 0;
+    for (i = 0; i < nr_compat; i++) {
+        const char *c = va_arg(ap, const char *);
+        compats[i] = c;
+        sz += strlen(compats[i]) + 1;
+    }
+    va_end(ap);
+
+    p = compat = libxl__zalloc(gc, sz);
+    for (i = 0; i < nr_compat; i++) {
+        strcpy(p, compats[i]);
+        p += strlen(compats[i]) + 1;
+    }
+
+    fdt_property(fdt, "compatible", compat, sz);
+
+    return 0;
+}
+
+static int fdt_property_interrupts(libxl__gc *gc, void *fdt,
+                                   gic_interrupt_t *intr,
+                                   unsigned num_irq)
+{
+    int res;
+
+    res = fdt_property(fdt, "interrupts", intr, sizeof (intr[0]) * num_irq);
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "interrupt-parent", PHANDLE_GIC);
+
+    return res;
+}
+
+static int fdt_property_regs(libxl__gc *gc, void *fdt,
+                             unsigned addr_cells,
+                             unsigned size_cells,
+                             unsigned num_regs, ...)
+{
+    uint32_t regs[num_regs*(addr_cells+size_cells)];
+    __be32 *cells = &regs[0];
+    int i;
+    va_list ap;
+    uint64_t base, size;
+
+    va_start(ap, num_regs);
+    for (i = 0 ; i < num_regs; i++) {
+        base = addr_cells ? va_arg(ap, uint64_t) : 0;
+        size = size_cells ? va_arg(ap, uint64_t) : 0;
+        set_range(&cells, addr_cells, size_cells, base, size);
+    }
+    va_end(ap);
+
+    return fdt_property(fdt, "reg", regs, sizeof(regs));
+}
+
+static int make_root_properties(libxl__gc *gc,
+                                const libxl_version_info *vers,
+                                void *fdt)
+{
+    int res;
+
+    res = fdt_property_string(fdt, "model", GCSPRINTF("XENVM-%d.%d",
+                                                      vers->xen_version_major,
+                                                      vers->xen_version_minor));
+    if ( res )
+        return res;
+
+    res = fdt_property_compat(gc, fdt, 2,
+                              GCSPRINTF("xen,xenvm-%d.%d",
+                                        vers->xen_version_major,
+                                        vers->xen_version_minor),
+                              "xen,xenvm");
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "interrupt-parent", PHANDLE_GIC);
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "#address-cells", ROOT_ADDRESS_CELLS);
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "#size-cells", ROOT_SIZE_CELLS);
+    if ( res )
+        return res;
+
+    return 0;
+}
+
+static int make_chosen_node(libxl__gc *gc, void *fdt,
+                            const libxl_domain_build_info *info)
+{
+    int res;
+
+    /* See linux Documentation/devicetree/... */
+    res = fdt_begin_node(fdt, "chosen");
+    if ( res )
+        return res;
+
+    res = fdt_property_string(fdt, "bootargs", info->u.pv.cmdline);
+    if ( res )
+        return res;
+
+    res = fdt_end_node(fdt);
+
+    return res;
+}
+
+static int make_cpus_node(libxl__gc *gc, void *fdt, int nr_cpus,
+                          const struct arch_info *ainfo)
+{
+    int res, i;
+
+    res = fdt_begin_node(fdt, "cpus");
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "#address-cells", 1);
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "#size-cells", 0);
+    if ( res )
+        return res;
+
+    for (i = 0; i < nr_cpus; i++) {
+        char name[strlen("cpu@") + 2 + 1];
+
+        if ( snprintf(name, sizeof(name)-1, "cpu@%d", i) < 0 )
+        {
+            perror("snprintf");
+            exit(1);
+        }
+
+        res = fdt_begin_node(fdt, name);
+        if ( res )
+            return res;
+
+        res = fdt_property_string(fdt, "device_type", "cpu");
+        if ( res )
+            return res;
+
+        res = fdt_property_compat(gc, fdt, 1, ainfo->cpu_compat);
+        if ( res )
+            return res;
+
+        res = fdt_property_string(fdt, "enable-method", "psci");
+        if ( res )
+            return res;
+
+        res = fdt_property_regs(gc, fdt, 1, 0, 1, (uint64_t)i);
+        if ( res )
+            return res;
+
+        res = fdt_end_node(fdt);
+        if ( res )
+            return res;
+
+    }
+
+    res = fdt_end_node(fdt);
+
+    return res;
+}
+
+static int make_psci_node(libxl__gc *gc, void *fdt)
+{
+    int res;
+
+    res = fdt_begin_node(fdt, "psci");
+    if ( res )
+        return res;
+
+    res = fdt_property_compat(gc, fdt, 1, "arm,psci");
+    if ( res )
+        return res;
+
+    res = fdt_property_string(fdt, "method", "hvc");
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "cpu_off", PSCI_cpu_off);
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "cpu_on", PSCI_cpu_on);
+    if ( res )
+        return res;
+
+    res = fdt_end_node(fdt);
+
+    return res;
+}
+
+static int make_memory_node(libxl__gc *gc, void *fdt,
+                            unsigned long long base,
+                            unsigned long long size)
+{
+    int res;
+    char name[strlen("memory@") + 8 + 1];
+
+    snprintf(name, sizeof(name), "memory@%08llx", base);
+
+    res = fdt_begin_node(fdt, name);
+    if ( res )
+        return res;
+
+    res = fdt_property_string(fdt, "device_type", "memory");
+    if ( res )
+        return res;
+
+    res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
+                            1, (uint64_t)base, (uint64_t)size);
+    if ( res )
+        return res;
+
+    res = fdt_end_node(fdt);
+
+    return res;
+}
+
+static int make_intc_node(libxl__gc *gc, void *fdt,
+                          unsigned long long gicd_base,
+                          unsigned long long gicd_size,
+                          unsigned long long gicc_base,
+                          unsigned long long gicc_size)
+{
+    int res;
+    char name[strlen("interrupt-controller@") + 8 + 1];
+
+    snprintf(name, sizeof(name), "interrupt-controller@%08llx", gicd_base);
+
+    res = fdt_begin_node(fdt, name);
+    if ( res )
+        return res;
+
+    res = fdt_property_compat(gc, fdt, 2,
+                              "arm,cortex-a15-gic",
+                              "arm,cortex-a9-gic");
+    if ( res )
+        return res;
+
+
+    res = fdt_property_cell(fdt, "#interrupt-cells", 3);
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "#address-cells", 0);
+    if ( res )
+        return res;
+
+    res = fdt_property(fdt, "interrupt-controller", NULL, 0);
+    if ( res )
+        return res;
+
+    res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
+                            2,
+                            (uint64_t)gicd_base, (uint64_t)gicd_size,
+                            (uint64_t)gicc_base, (uint64_t)gicc_size);
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "linux,phandle", PHANDLE_GIC);
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "phandle", PHANDLE_GIC);
+    if ( res )
+        return res;
+
+    res = fdt_end_node(fdt);
+
+    return res;
+}
+
+static int make_timer_node(libxl__gc *gc, void *fdt, const struct arch_info *ainfo)
+{
+    int res;
+    gic_interrupt_t ints[3];
+
+    res = fdt_begin_node(fdt, "timer");
+    if ( res )
+        return res;
+
+    res = fdt_property_compat(gc, fdt, 1, ainfo->timer_compat);
+    if ( res )
+        return res;
+
+    set_interrupt_ppi(ints[0], GUEST_TIMER_PHYS_S_PPI, 0xf, 0x8);
+    set_interrupt_ppi(ints[1], GUEST_TIMER_PHYS_NS_PPI, 0xf, 0x8);
+    set_interrupt_ppi(ints[2], GUEST_TIMER_VIRT_PPI, 0xf, 0x8);
+
+    res = fdt_property_interrupts(gc, fdt, ints, 3);
+    if ( res )
+        return res;
+
+    res = fdt_end_node(fdt);
+
+    return res;
+}
+
+static int make_hypervisor_node(libxl__gc *gc, void *fdt,
+                                const libxl_version_info *vers)
+{
+    int res;
+    gic_interrupt_t intr;
+
+    /* See linux Documentation/devicetree/bindings/arm/xen.txt */
+    res = fdt_begin_node(fdt, "hypervisor");
+    if ( res )
+        return res;
+
+    res = fdt_property_compat(gc, fdt, 2,
+                              GCSPRINTF("xen,xen-%d.%d",
+                                        vers->xen_version_major,
+                                        vers->xen_version_minor),
+                              "xen,xen");
+    if ( res )
+        return res;
+
+    /* reg 0 is grant table space */
+    res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
+                            1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
+    if ( res )
+        return res;
+
+    /*
+     * interrupts is evtchn upcall:
+     *  - Active-low level-sensitive
+     *  - All cpus
+     */
+    set_interrupt_ppi(intr, GUEST_EVTCHN_PPI, 0xf, 0x8);
+
+    res = fdt_property_interrupts(gc, fdt, &intr, 1);
+    if ( res )
+        return res;
+
+    res = fdt_end_node(fdt);
+
+    return res;
+}
+
+static const struct arch_info *get_arch_info(libxl__gc *gc,
+                                             const struct xc_dom_image *dom)
+{
+    int i;
+
+    for (i=0; i < ARRAY_SIZE(arch_info); i++) {
+        const struct arch_info *info = &arch_info[i];
+        if (!strcmp(dom->guest_type, info->guest_type))
+            return info;
+    }
+    LOG(ERROR, "Unable to find arch FDT info for %s\n", dom->guest_type);
+    return NULL;
+}
+
+#define FDT_MAX_SIZE (1<<20)
+
+/*
+ * Call "call" handling FDR_ERR_*. Will either:
+ * - loop back to retry_resize
+ * - set rc and goto out
+ * - fall through successfully
+ *
+ * On FDT_ERR_NOSPACE we start again from scratch rather than
+ * realloc+libfdt_open_into because "call" may have failed half way
+ * through a series of steps leaving the partial tree in an
+ * inconsistent state, e.g. leaving a node open.
+ */
+#define FDT( call ) do {                                        \
+    int fdt_res = (call);                                       \
+    if (fdt_res == -FDT_ERR_NOSPACE && fdt_size < FDT_MAX_SIZE) \
+        goto retry_resize;                                      \
+    else if (fdt_res < 0) {                                     \
+        LOG(ERROR, "FDT: %s failed: %d = %s",                   \
+            #call, fdt_res, fdt_strerror(fdt_res));             \
+        rc = ERROR_FAIL;                                        \
+        goto out;                                               \
+    }                                                           \
+} while(0)
+
+int libxl__arch_domain_configure(libxl__gc *gc,
+                                 libxl_domain_build_info *info,
+                                 struct xc_dom_image *dom)
+{
+    void *fdt;
+    int rc, res, fd;
+    size_t fdt_size = 0;
+
+    const libxl_version_info *vers;
+    const struct arch_info *ainfo;
+
+    assert(info->type == LIBXL_DOMAIN_TYPE_PV);
+
+    vers = libxl_get_version_info(CTX);
+    if (vers == NULL) return ERROR_FAIL;
+
+    ainfo = get_arch_info(gc, dom);
+    if (ainfo == NULL) return ERROR_FAIL;
+
+    LOG(DEBUG, "constructing DTB for Xen version %d.%d guest",
+        vers->xen_version_major, vers->xen_version_minor);
+
+retry_resize:
+    if (fdt_size) {
+        fdt_size <<= 1;
+        LOG(DEBUG, "Increasing FDT size to %zd and retrying", fdt_size);
+    } else {
+        fdt_size = 4096;
+    }
+
+    fdt = libxl__zalloc(gc, fdt_size);
+
+    FDT( fdt_create(fdt, fdt_size) );
+
+    fdt_finish_reservemap(fdt);
+
+    FDT( fdt_begin_node(fdt, "") );
+
+    FDT( make_root_properties(gc, vers, fdt) );
+    FDT( make_chosen_node(gc, fdt, info) );
+    FDT( make_cpus_node(gc, fdt, info->max_vcpus, ainfo) );
+    FDT( make_psci_node(gc, fdt) );
+
+    FDT( make_memory_node(gc, fdt,
+                          dom->rambase_pfn << XC_PAGE_SHIFT,
+                          info->target_memkb * 1024) );
+    FDT( make_intc_node(gc, fdt,
+                        GUEST_GICD_BASE, GUEST_GICD_SIZE,
+                        GUEST_GICC_BASE, GUEST_GICD_SIZE) );
+
+    FDT( make_timer_node(gc, fdt, ainfo) );
+    FDT( make_hypervisor_node(gc, fdt, vers) );
+
+    FDT( fdt_end_node(fdt) );
+
+    FDT( fdt_finish(fdt) );
+
+    LOG(DEBUG, "fdt total size %d", fdt_totalsize(fdt));
+
+    if ( (res = xc_dom_devicetree_mem(dom, fdt, fdt_totalsize(fdt))) != 0 ) {
+        LOGE(ERROR, "xc_dom_devicetree_file failed");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+#ifndef NDEBUG
+    if ( getenv("LIBXL_DEBUG_DUMP_DTB") ) {
+        const char *dtb = getenv("LIBXL_DEBUG_DUMP_DTB");
+
+        fd = open(dtb, O_CREAT|O_TRUNC|O_WRONLY, 0666);
+        if ( fd < 0 ) {
+            LOGE(DEBUG, "cannot open %s for LIBXL_DEBUG_DUMP_DTB", dtb);
+            goto no_debug_dump;
+        }
+
+        rc = libxl_write_exactly(CTX, fd, fdt, fdt_totalsize(fdt),
+                            dtb, "dtb");
+        if ( rc < 0 ) {
+            LOG(DEBUG, "unable to write DTB debug dump output %d", rc);
+            goto no_debug_dump;
+        }
+
+        res = close(fd);
+        if ( res < 0 ) {
+            LOGE(DEBUG, "unable to close DTB debug dump output");
+            goto no_debug_dump;
+        }
+    }
+no_debug_dump:
+#endif
+
+    rc = 0;
+
+out:
+    return rc;
+}
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 10f976c..6351ebd 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -400,6 +400,10 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
         LOGE(ERROR, "xc_dom_parse_image failed");
         goto out;
     }
+    if ( (ret = libxl__arch_domain_configure(gc, info, dom)) != 0 ) {
+        LOGE(ERROR, "libxl__arch_domain_configure failed");
+        goto out;
+    }
     if ( (ret = xc_dom_mem_init(dom, info->target_memkb / 1024)) != 0 ) {
         LOGE(ERROR, "xc_dom_mem_init failed");
         goto out;
diff --git a/tools/libxl/libxl_noarch.c b/tools/libxl/libxl_noarch.c
deleted file mode 100644
index 7893535..0000000
--- a/tools/libxl/libxl_noarch.c
+++ /dev/null
@@ -1,8 +0,0 @@
-#include "libxl_internal.h"
-#include "libxl_arch.h"
-
-int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
-                              uint32_t domid)
-{
-    return 0;
-}
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index a78c91d..dd13c45 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -308,3 +308,10 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
 
     return ret;
 }
+
+int libxl__arch_domain_configure(libxl__gc *gc,
+                                 libxl_domain_build_info *info,
+                                 struct xc_dom_image *dom)
+{
+    return 0;
+}
-- 
1.7.10.4

Re: [PATCH v3 14/14] libxl: build a device tree for ARM guests

[Ian Jackson]

Ian Campbell writes ("[PATCH v3 14/14] libxl: build a device tree for ARM guests"):
> Uses xc_dom_devicetree_mem which was just added. The call to this needs to be
> carefully sequenced to be after xc_dom_parse_image (so we can tell which kind
> of guest we are building, although we don't use this yet) and before
> xc_dom_mem_init which tries to decide where to place the FDT in guest RAM.
...
> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
> index abe6685..3de2d76 100644
> --- a/tools/libxl/libxl_arch.h
> +++ b/tools/libxl/libxl_arch.h
> @@ -19,4 +19,8 @@
>  int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
>                 uint32_t domid);
>  
> +/* */
> +int libxl__arch_domain_configure(libxl__gc *gc,
> +                                 libxl_domain_build_info *info,
> +                                 struct xc_dom_image *dom);

Missing content for comment ?  Or maybe the comment just wants to go.

> +typedef uint32_t __be32;
> +typedef __be32 gic_interrupt_t[3];

Identifiers starting with __, and ones ending _t, are reserved for the
C implementation.

> +static void set_cell(__be32 **cellp, int size, uint64_t val)
> +{
> +    int cells = size;
> +
> +    while ( size-- )

The spaces inside ( ) aren't expected inside libxl.

> +static int make_memory_node(libxl__gc *gc, void *fdt,
> +                            unsigned long long base,
> +                            unsigned long long size)
> +{
> +    int res;
> +    char name[strlen("memory@") + 8 + 1];
> +
> +    snprintf(name, sizeof(name), "memory@%08llx", base);

Why not use GCSPRINTF ?

> +/*
> + * Call "call" handling FDR_ERR_*. Will either:
> + * - loop back to retry_resize
> + * - set rc and goto out
> + * - fall through successfully
> + *
> + * On FDT_ERR_NOSPACE we start again from scratch rather than
> + * realloc+libfdt_open_into because "call" may have failed half way
> + * through a series of steps leaving the partial tree in an
> + * inconsistent state, e.g. leaving a node open.
> + */
> +#define FDT( call ) do {                                        \
> +    int fdt_res = (call);                                       \
> +    if (fdt_res == -FDT_ERR_NOSPACE && fdt_size < FDT_MAX_SIZE) \
> +        goto retry_resize;                                      \
> +    else if (fdt_res < 0) {                                     \
> +        LOG(ERROR, "FDT: %s failed: %d = %s",                   \
> +            #call, fdt_res, fdt_strerror(fdt_res));             \
> +        rc = ERROR_FAIL;                                        \
> +        goto out;                                               \
> +    }                                                           \
> +} while(0)

My preference would be for this to be defined inside
libxl__arch_domain_configure and #undef'd at the end, so as to limit
the scope of the implicit reference to the label retry_resize.

> +int libxl__arch_domain_configure(libxl__gc *gc,
> +                                 libxl_domain_build_info *info,
> +                                 struct xc_dom_image *dom)
> +{
> +    void *fdt;
> +    int rc, res, fd;
> +    size_t fdt_size = 0;
> +
> +    const libxl_version_info *vers;
> +    const struct arch_info *ainfo;
> +
> +    assert(info->type == LIBXL_DOMAIN_TYPE_PV);
> +
> +    vers = libxl_get_version_info(CTX);
> +    if (vers == NULL) return ERROR_FAIL;
> +
> +    ainfo = get_arch_info(gc, dom);
> +    if (ainfo == NULL) return ERROR_FAIL;
> +
> +    LOG(DEBUG, "constructing DTB for Xen version %d.%d guest",
> +        vers->xen_version_major, vers->xen_version_minor);
> +
> +retry_resize:

For form's sake, how about making this an explicit loop ?  e.g.

       for (;;) {
           next_resize:;
           blah blah
           if (something)
               goto next_resize;

Then the end of the loop would be more obvious.  You'd have to write
"break".  (And you can #undef FDT at the end of the loop rather than
the end of the function, as the label becomes semantically void after
the end of the loop.)

(If C had labelled loops you could do
       for resize (;;) {
           blah blah
           next resize;
but it doesn't.  Emulating this with goto is a lot nicer IMO than
the naked goto.)

> +    if (fdt_size) {
> +        fdt_size <<= 1;
> +        LOG(DEBUG, "Increasing FDT size to %zd and retrying", fdt_size);
> +    } else {
> +        fdt_size = 4096;
> +    }

Can we rule out this loop getting out of control ?

> +#ifndef NDEBUG

Please don't key off NDEBUG.  libxl ought never to be compiled
with NDEBUG.  I don't see why you can't just have this code present
all the time.

> +    if ( getenv("LIBXL_DEBUG_DUMP_DTB") ) {
> +        const char *dtb = getenv("LIBXL_DEBUG_DUMP_DTB");
> +
> +        fd = open(dtb, O_CREAT|O_TRUNC|O_WRONLY, 0666);
> +        if ( fd < 0 ) {
> +            LOGE(DEBUG, "cannot open %s for LIBXL_DEBUG_DUMP_DTB", dtb);
> +            goto no_debug_dump;

This construction with "goto" is pretty ugly.  Perhaps if you made
the whole thing a separate function it would be less so.

> +        }
> +
> +        rc = libxl_write_exactly(CTX, fd, fdt, fdt_totalsize(fdt),
> +                            dtb, "dtb");
> +        if ( rc < 0 ) {
> +            LOG(DEBUG, "unable to write DTB debug dump output %d", rc);
> +            goto no_debug_dump;

For example, if you had made it a separate function with the "goto
out" error handling style you wouldn't leak fd in this particular
error case :-).  (Also, no spaces inside ( ) in ifs, please.)

> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 10f976c..6351ebd 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -400,6 +400,10 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>          LOGE(ERROR, "xc_dom_parse_image failed");
>          goto out;
>      }
> +    if ( (ret = libxl__arch_domain_configure(gc, info, dom)) != 0 ) {
> +        LOGE(ERROR, "libxl__arch_domain_configure failed");
> +        goto out;
> +    }
>      if ( (ret = xc_dom_mem_init(dom, info->target_memkb / 1024)) != 0 ) {
>          LOGE(ERROR, "xc_dom_mem_init failed");
>          goto out;

The style in this function is anomalous:

  * spaces inside ( )

  * use of
      if ((ret = blah) !=0 )
    which hides the meat in the if and has !=0 clobber, instead of
      rc = blah;
      if (rc) goto out;

  * use of "ret" for a libxl error code, rather than rc.

But I won't insist on you fixing it.  Adding an extra occurrence is
probably better than having one call which is different to all the
others.


Thanks,
Ian.

Re: [PATCH v3 14/14] libxl: build a device tree for ARM guests

[Ian Campbell]

On Thu, 2013-11-07 at 17:30 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH v3 14/14] libxl: build a device tree for ARM guests"):
> > Uses xc_dom_devicetree_mem which was just added. The call to this needs to be
> > carefully sequenced to be after xc_dom_parse_image (so we can tell which kind
> > of guest we are building, although we don't use this yet) and before
> > xc_dom_mem_init which tries to decide where to place the FDT in guest RAM.
> ...
> > diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
> > index abe6685..3de2d76 100644
> > --- a/tools/libxl/libxl_arch.h
> > +++ b/tools/libxl/libxl_arch.h
> > @@ -19,4 +19,8 @@
> >  int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
> >                 uint32_t domid);
> >  
> > +/* */
> > +int libxl__arch_domain_configure(libxl__gc *gc,
> > +                                 libxl_domain_build_info *info,
> > +                                 struct xc_dom_image *dom);
> 
> Missing content for comment ?  Or maybe the comment just wants to go.

I'll nuke it.

> 
> > +typedef uint32_t __be32;
> > +typedef __be32 gic_interrupt_t[3];
> 
> Identifiers starting with __, and ones ending _t, are reserved for the
> C implementation.

Oops, types carried over from the h/v side.

> > +static int make_memory_node(libxl__gc *gc, void *fdt,
> > +                            unsigned long long base,
> > +                            unsigned long long size)
> > +{
> > +    int res;
> > +    char name[strlen("memory@") + 8 + 1];
> > +
> > +    snprintf(name, sizeof(name), "memory@%08llx", base);
> 
> Why not use GCSPRINTF ?

Another hypervisor-ism which I carried over without enough thought.

> 
> > +/*
> > + * Call "call" handling FDR_ERR_*. Will either:
> > + * - loop back to retry_resize
> > + * - set rc and goto out
> > + * - fall through successfully
> > + *
> > + * On FDT_ERR_NOSPACE we start again from scratch rather than
> > + * realloc+libfdt_open_into because "call" may have failed half way
> > + * through a series of steps leaving the partial tree in an
> > + * inconsistent state, e.g. leaving a node open.
> > + */
> > +#define FDT( call ) do {                                        \
> > +    int fdt_res = (call);                                       \
> > +    if (fdt_res == -FDT_ERR_NOSPACE && fdt_size < FDT_MAX_SIZE) \
> > +        goto retry_resize;                                      \
> > +    else if (fdt_res < 0) {                                     \
> > +        LOG(ERROR, "FDT: %s failed: %d = %s",                   \
> > +            #call, fdt_res, fdt_strerror(fdt_res));             \
> > +        rc = ERROR_FAIL;                                        \
> > +        goto out;                                               \
> > +    }                                                           \
> > +} while(0)
> 
> My preference would be for this to be defined inside
> libxl__arch_domain_configure and #undef'd at the end, so as to limit
> the scope of the implicit reference to the label retry_resize.

Good idea.

> > +int libxl__arch_domain_configure(libxl__gc *gc,
> > +                                 libxl_domain_build_info *info,
> > +                                 struct xc_dom_image *dom)
> > +{
> > +    void *fdt;
> > +    int rc, res, fd;
> > +    size_t fdt_size = 0;
> > +
> > +    const libxl_version_info *vers;
> > +    const struct arch_info *ainfo;
> > +
> > +    assert(info->type == LIBXL_DOMAIN_TYPE_PV);
> > +
> > +    vers = libxl_get_version_info(CTX);
> > +    if (vers == NULL) return ERROR_FAIL;
> > +
> > +    ainfo = get_arch_info(gc, dom);
> > +    if (ainfo == NULL) return ERROR_FAIL;
> > +
> > +    LOG(DEBUG, "constructing DTB for Xen version %d.%d guest",
> > +        vers->xen_version_major, vers->xen_version_minor);
> > +
> > +retry_resize:
> 
> For form's sake, how about making this an explicit loop ?  e.g.
> 
>        for (;;) {
>            next_resize:;
>            blah blah
>            if (something)
>                goto next_resize;
> 
> Then the end of the loop would be more obvious.  You'd have to write
> "break".  (And you can #undef FDT at the end of the loop rather than
> the end of the function, as the label becomes semantically void after
> the end of the loop.)
> 
> (If C had labelled loops you could do
>        for resize (;;) {
>            blah blah
>            next resize;
> but it doesn't.  Emulating this with goto is a lot nicer IMO than
> the naked goto.)

I agree, I'll change things along these lines.

> 
> > +    if (fdt_size) {
> > +        fdt_size <<= 1;
> > +        LOG(DEBUG, "Increasing FDT size to %zd and retrying", fdt_size);
> > +    } else {
> > +        fdt_size = 4096;
> > +    }
> 
> Can we rule out this loop getting out of control ?

The FDT macro has a check and error if it gets too big. I'll see about
moving the <<= into the macro, which I suspect will seem more natural
with a loop called "retry".

> 
> > +#ifndef NDEBUG
> 
> Please don't key off NDEBUG.  libxl ought never to be compiled
> with NDEBUG.  I don't see why you can't just have this code present
> all the time.

I wasn't entirely sure about keeping it at all, I wrote it for my own
debugging purposes. I'm happy with it unconditional if you are.

> > +    if ( getenv("LIBXL_DEBUG_DUMP_DTB") ) {
> > +        const char *dtb = getenv("LIBXL_DEBUG_DUMP_DTB");
> > +
> > +        fd = open(dtb, O_CREAT|O_TRUNC|O_WRONLY, 0666);
> > +        if ( fd < 0 ) {
> > +            LOGE(DEBUG, "cannot open %s for LIBXL_DEBUG_DUMP_DTB", dtb);
> > +            goto no_debug_dump;
> 
> This construction with "goto" is pretty ugly.  Perhaps if you made
> the whole thing a separate function it would be less so.

Yes, I suspect it will.

> > +        }
> > +
> > +        rc = libxl_write_exactly(CTX, fd, fdt, fdt_totalsize(fdt),
> > +                            dtb, "dtb");
> > +        if ( rc < 0 ) {
> > +            LOG(DEBUG, "unable to write DTB debug dump output %d", rc);
> > +            goto no_debug_dump;
> 
> For example, if you had made it a separate function with the "goto
> out" error handling style you wouldn't leak fd in this particular
> error case :-).

:-)

>   (Also, no spaces inside ( ) in ifs, please.)

Ack. Will do a thorough sweep.

> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index 10f976c..6351ebd 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -400,6 +400,10 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
> >          LOGE(ERROR, "xc_dom_parse_image failed");
> >          goto out;
> >      }
> > +    if ( (ret = libxl__arch_domain_configure(gc, info, dom)) != 0 ) {
> > +        LOGE(ERROR, "libxl__arch_domain_configure failed");
> > +        goto out;
> > +    }
> >      if ( (ret = xc_dom_mem_init(dom, info->target_memkb / 1024)) != 0 ) {
> >          LOGE(ERROR, "xc_dom_mem_init failed");
> >          goto out;
> 
> The style in this function is anomalous:
> 
>   * spaces inside ( )
> 
>   * use of
>       if ((ret = blah) !=0 )
>     which hides the meat in the if and has !=0 clobber, instead of
>       rc = blah;
>       if (rc) goto out;
> 
>   * use of "ret" for a libxl error code, rather than rc.
> 
> But I won't insist on you fixing it.  Adding an extra occurrence is
> probably better than having one call which is different to all the
> others.

I agree.

Ian.

Re: [PATCH v3 14/14] libxl: build a device tree for ARM guests

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v3 14/14] libxl: build a device tree for ARM guests"):
> On Thu, 2013-11-07 at 17:30 +0000, Ian Jackson wrote:
> > > +    if (fdt_size) {
> > > +        fdt_size <<= 1;
> > > +        LOG(DEBUG, "Increasing FDT size to %zd and retrying", fdt_size);
> > > +    } else {
> > > +        fdt_size = 4096;
> > > +    }
> > 
> > Can we rule out this loop getting out of control ?
> 
> The FDT macro has a check and error if it gets too big. I'll see about
> moving the <<= into the macro, which I suspect will seem more natural
> with a loop called "retry".

Oh, please don't do that.  I much prefer to keep anything out of a
macro that doesn't have to be there.

The fdt_size increase is part of the loop control.  Now you can't put
it inside the for(;;) brackets but putting it at the top of the loop
will make it nice and obvious.

Personally I normally do something like this
     fdt_size += 4096;
     fdt_size <<= 2;
which avoids the if (fdt_size).  The only downside is that the
answers aren't uniformly powers of 2.

> > Please don't key off NDEBUG.  libxl ought never to be compiled
> > with NDEBUG.  I don't see why you can't just have this code present
> > all the time.
> 
> I wasn't entirely sure about keeping it at all, I wrote it for my own
> debugging purposes. I'm happy with it unconditional if you are.

Sure.

> > The style in this function is anomalous:
> > 
> >   * spaces inside ( )
> > 
> >   * use of
> >       if ((ret = blah) !=0 )
> >     which hides the meat in the if and has !=0 clobber, instead of
> >       rc = blah;
> >       if (rc) goto out;
> > 
> >   * use of "ret" for a libxl error code, rather than rc.
> > 
> > But I won't insist on you fixing it.  Adding an extra occurrence is
> > probably better than having one call which is different to all the
> > others.
> 
> I agree.

Thanks,
Ian.

Re: [PATCH v3 14/14] libxl: build a device tree for ARM guests

[Ian Campbell]

On Thu, 2013-11-07 at 17:47 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH v3 14/14] libxl: build a device tree for ARM guests"):
> > On Thu, 2013-11-07 at 17:30 +0000, Ian Jackson wrote:
> > > > +    if (fdt_size) {
> > > > +        fdt_size <<= 1;
> > > > +        LOG(DEBUG, "Increasing FDT size to %zd and retrying", fdt_size);
> > > > +    } else {
> > > > +        fdt_size = 4096;
> > > > +    }
> > > 
> > > Can we rule out this loop getting out of control ?
> > 
> > The FDT macro has a check and error if it gets too big. I'll see about
> > moving the <<= into the macro, which I suspect will seem more natural
> > with a loop called "retry".
> 
> Oh, please don't do that.  I much prefer to keep anything out of a
> macro that doesn't have to be there.

OK.

> The fdt_size increase is part of the loop control.  Now you can't put
> it inside the for(;;) brackets but putting it at the top of the loop
> will make it nice and obvious.
> 
> Personally I normally do something like this
>      fdt_size += 4096;
>      fdt_size <<= 2;
> which avoids the if (fdt_size).  The only downside is that the
> answers aren't uniformly powers of 2.

TBH I could just go with += and avoid the <<=, I don't actually expect
an FDT to be so large that exponentially increasing size is that
important.

How would
	for(fdt_size = 4096; fdt_size < FDT_SIZE_MAX; fdt_size <<= 2)
and use continue on error in the macro, with a break at the bottom of
the loop (success) sit with you?

Ian.

Re: [PATCH v3 14/14] libxl: build a device tree for ARM guests

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v3 14/14] libxl: build a device tree for ARM guests"):
> On Thu, 2013-11-07 at 17:47 +0000, Ian Jackson wrote:
> > Personally I normally do something like this
> >      fdt_size += 4096;
> >      fdt_size <<= 2;
> > which avoids the if (fdt_size).  The only downside is that the
> > answers aren't uniformly powers of 2.
> 
> TBH I could just go with += and avoid the <<=, I don't actually expect
> an FDT to be so large that exponentially increasing size is that
> important.

Bear in mind that each of these iterations dumps a bunch of garbage
into the gc.

> How would
> 	for(fdt_size = 4096; fdt_size < FDT_SIZE_MAX; fdt_size <<= 2)
> and use continue on error in the macro, with a break at the bottom of
> the loop (success) sit with you?

That would be OK, but using "continue" makes the macro unsuitable to
use in a loop, which is less good.  I think it's worth a goto to make
the macro loop-safe.

If you prefer "continue" you can do "goto resize_continue" and put
resize_continue at the end of the loop.

Ian.

Re: [PATCH v3 14/14] libxl: build a device tree for ARM guests

[Ian Campbell]

On Fri, 2013-11-08 at 15:14 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH v3 14/14] libxl: build a device tree for ARM guests"):
> > On Thu, 2013-11-07 at 17:47 +0000, Ian Jackson wrote:
> > > Personally I normally do something like this
> > >      fdt_size += 4096;
> > >      fdt_size <<= 2;
> > > which avoids the if (fdt_size).  The only downside is that the
> > > answers aren't uniformly powers of 2.
> > 
> > TBH I could just go with += and avoid the <<=, I don't actually expect
> > an FDT to be so large that exponentially increasing size is that
> > important.
> 
> Bear in mind that each of these iterations dumps a bunch of garbage
> into the gc.

True. We have a bunch of such loops over libxl though.

Or was you concern the larger than usual allocation for the fdt itself
more than the various sundry allocations during creation? Perhaps I
should use libxl__realloc for the big one anyway.

> 
> > How would
> > 	for(fdt_size = 4096; fdt_size < FDT_SIZE_MAX; fdt_size <<= 2)
> > and use continue on error in the macro, with a break at the bottom of
> > the loop (success) sit with you?
> 
> That would be OK, but using "continue" makes the macro unsuitable to
> use in a loop, which is less good.  I think it's worth a goto to make
> the macro loop-safe.

OK.

> 
> If you prefer "continue" you can do "goto resize_continue" and put
> resize_continue at the end of the loop.
> 
> Ian.

Re: [PATCH v3 14/14] libxl: build a device tree for ARM guests

[Ian Campbell]

On Thu, 2013-11-07 at 17:30 +0000, Ian Jackson wrote:
> > +typedef uint32_t __be32;
> > +typedef __be32 gic_interrupt_t[3];
> 
> Identifiers starting with __, and ones ending _t, are reserved for the
> C implementation.

I couldn't find any standards defined types for endian specific types so
I went with be32. If you know of a standard type please let me know...

gic_interrupt for the other one.