[Qemu-devel] [PATCH 0/3] Arm: honour CPU address space for image loads

[Qemu-devel] [PATCH 0/3] Arm: honour CPU address space for image loads

[Peter Maydell]

This patchset makes the Arm code for loading kernels, initrds,
etc etc honour the CPU's address space rather than loading
everything via the system address space. This makes a difference
when the image is being loaded to memory or via an alias memory
region which is implemented by an SoC container object (and
which therefore doesn't appear in the system address space).

I needed this in particular for M profile, but I'd written all
the boot.c changes before I remembered that M profile doesn't
use that code at all :-)

thanks
-- PMM

Peter Maydell (3):
  loader: Add new load_ramdisk_as()
  hw/arm/boot: Honour CPU's address space for image loads
  hw/arm/armv7m: Honour CPU's address space for image loads

 include/hw/loader.h |  12 +++++-
 hw/arm/armv7m.c     |  17 ++++++--
 hw/arm/boot.c       | 119 +++++++++++++++++++++++++++++++++-------------------
 hw/core/loader.c    |   8 +++-
 4 files changed, 108 insertions(+), 48 deletions(-)

-- 
2.16.1

[Qemu-devel] [PATCH 1/3] loader: Add new load_ramdisk_as()

[Peter Maydell]

Add a function load_ramdisk_as() which behaves like the existing
load_ramdisk() but allows the caller to specify the AddressSpace
to use. This matches the pattern we have already for various
other loader functions.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/loader.h | 12 +++++++++++-
 hw/core/loader.c    |  8 +++++++-
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/hw/loader.h b/include/hw/loader.h
index 355fe0f5a2..1fd4256782 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -163,16 +163,26 @@ int load_uimage(const char *filename, hwaddr *ep,
                 void *translate_opaque);
 
 /**
- * load_ramdisk:
+ * load_ramdisk_as:
  * @filename: Path to the ramdisk image
  * @addr: Memory address to load the ramdisk to
  * @max_sz: Maximum allowed ramdisk size (for non-u-boot ramdisks)
+ * @as: The AddressSpace to load the ELF to. The value of address_space_memory
+ *      is used if nothing is supplied here.
  *
  * Load a ramdisk image with U-Boot header to the specified memory
  * address.
  *
  * Returns the size of the loaded image on success, -1 otherwise.
  */
+int load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
+                    AddressSpace *as);
+
+/**
+ * load_ramdisk:
+ * Same as load_ramdisk_as(), but doesn't allow the caller to specify
+ * an AddressSpace.
+ */
 int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz);
 
 ssize_t gunzip(void *dst, size_t dstlen, uint8_t *src, size_t srclen);
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 91669d65aa..2b9e7394a1 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -729,9 +729,15 @@ int load_uimage_as(const char *filename, hwaddr *ep, hwaddr *loadaddr,
 
 /* Load a ramdisk.  */
 int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
+{
+    return load_ramdisk_as(filename, addr, max_sz, NULL);
+}
+
+int load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
+                    AddressSpace *as)
 {
     return load_uboot_image(filename, NULL, &addr, NULL, IH_TYPE_RAMDISK,
-                            NULL, NULL, NULL);
+                            NULL, NULL, as);
 }
 
 /* Load a gzip-compressed kernel to a dynamically allocated buffer. */
-- 
2.16.1

[Qemu-devel] [PATCH 2/3] hw/arm/boot: Honour CPU's address space for image loads

[Peter Maydell]

Instead of loading kernels, device trees, and the like to
the system address space, use the CPU's address space. This
is important if we're trying to load the file to memory or
via an alias memory region that is provided by an SoC
object and thus not mapped into the system address space.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c | 119 +++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 76 insertions(+), 43 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 9b174b982c..0eac655c98 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -35,6 +35,25 @@
 #define ARM64_TEXT_OFFSET_OFFSET    8
 #define ARM64_MAGIC_OFFSET          56
 
+static AddressSpace *arm_boot_addressspace(ARMCPU *cpu,
+                                           const struct arm_boot_info *info)
+{
+    /* Return the address space to use for bootloader reads and writes.
+     * We prefer the secure address space if the CPU has it and we're
+     * going to boot the guest into it.
+     */
+    int asidx;
+    CPUState *cs = CPU(cpu);
+
+    if (arm_feature(&cpu->env, ARM_FEATURE_EL3) && info->secure_boot) {
+        asidx = ARMASIdx_S;
+    } else {
+        asidx = ARMASIdx_NS;
+    }
+
+    return cpu_get_address_space(cs, asidx);
+}
+
 typedef enum {
     FIXUP_NONE = 0,     /* do nothing */
     FIXUP_TERMINATOR,   /* end of insns */
@@ -124,7 +143,8 @@ static const ARMInsnFixup smpboot[] = {
 };
 
 static void write_bootloader(const char *name, hwaddr addr,
-                             const ARMInsnFixup *insns, uint32_t *fixupcontext)
+                             const ARMInsnFixup *insns, uint32_t *fixupcontext,
+                             AddressSpace *as)
 {
     /* Fix up the specified bootloader fragment and write it into
      * guest memory using rom_add_blob_fixed(). fixupcontext is
@@ -163,7 +183,7 @@ static void write_bootloader(const char *name, hwaddr addr,
         code[i] = tswap32(insn);
     }
 
-    rom_add_blob_fixed(name, code, len * sizeof(uint32_t), addr);
+    rom_add_blob_fixed_as(name, code, len * sizeof(uint32_t), addr, as);
 
     g_free(code);
 }
@@ -172,6 +192,7 @@ static void default_write_secondary(ARMCPU *cpu,
                                     const struct arm_boot_info *info)
 {
     uint32_t fixupcontext[FIXUP_MAX];
+    AddressSpace *as = CPU(cpu)->as;
 
     fixupcontext[FIXUP_GIC_CPU_IF] = info->gic_cpu_if_addr;
     fixupcontext[FIXUP_BOOTREG] = info->smp_bootreg_addr;
@@ -182,13 +203,14 @@ static void default_write_secondary(ARMCPU *cpu,
     }
 
     write_bootloader("smpboot", info->smp_loader_start,
-                     smpboot, fixupcontext);
+                     smpboot, fixupcontext, as);
 }
 
 void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
                                             const struct arm_boot_info *info,
                                             hwaddr mvbar_addr)
 {
+    AddressSpace *as = CPU(cpu)->as;
     int n;
     uint32_t mvbar_blob[] = {
         /* mvbar_addr: secure monitor vectors
@@ -226,22 +248,23 @@ void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
     for (n = 0; n < ARRAY_SIZE(mvbar_blob); n++) {
         mvbar_blob[n] = tswap32(mvbar_blob[n]);
     }
-    rom_add_blob_fixed("board-setup-mvbar", mvbar_blob, sizeof(mvbar_blob),
-                       mvbar_addr);
+    rom_add_blob_fixed_as("board-setup-mvbar", mvbar_blob, sizeof(mvbar_blob),
+                          mvbar_addr, as);
 
     for (n = 0; n < ARRAY_SIZE(board_setup_blob); n++) {
         board_setup_blob[n] = tswap32(board_setup_blob[n]);
     }
-    rom_add_blob_fixed("board-setup", board_setup_blob,
-                       sizeof(board_setup_blob), info->board_setup_addr);
+    rom_add_blob_fixed_as("board-setup", board_setup_blob,
+                          sizeof(board_setup_blob), info->board_setup_addr, as);
 }
 
 static void default_reset_secondary(ARMCPU *cpu,
                                     const struct arm_boot_info *info)
 {
+    AddressSpace *as = arm_boot_addressspace(cpu, info);
     CPUState *cs = CPU(cpu);
 
-    address_space_stl_notdirty(&address_space_memory, info->smp_bootreg_addr,
+    address_space_stl_notdirty(as, info->smp_bootreg_addr,
                                0, MEMTXATTRS_UNSPECIFIED, NULL);
     cpu_set_pc(cs, info->smp_loader_start);
 }
@@ -252,12 +275,12 @@ static inline bool have_dtb(const struct arm_boot_info *info)
 }
 
 #define WRITE_WORD(p, value) do { \
-    address_space_stl_notdirty(&address_space_memory, p, value, \
+    address_space_stl_notdirty(as, p, value, \
                                MEMTXATTRS_UNSPECIFIED, NULL);  \
     p += 4;                       \
 } while (0)
 
-static void set_kernel_args(const struct arm_boot_info *info)
+static void set_kernel_args(const struct arm_boot_info *info, AddressSpace *as)
 {
     int initrd_size = info->initrd_size;
     hwaddr base = info->loader_start;
@@ -288,8 +311,9 @@ static void set_kernel_args(const struct arm_boot_info *info)
         int cmdline_size;
 
         cmdline_size = strlen(info->kernel_cmdline);
-        cpu_physical_memory_write(p + 8, info->kernel_cmdline,
-                                  cmdline_size + 1);
+        address_space_write(as, p + 8, MEMTXATTRS_UNSPECIFIED,
+                            (const uint8_t *)info->kernel_cmdline,
+                            cmdline_size + 1);
         cmdline_size = (cmdline_size >> 2) + 1;
         WRITE_WORD(p, cmdline_size + 2);
         WRITE_WORD(p, 0x54410009);
@@ -303,7 +327,8 @@ static void set_kernel_args(const struct arm_boot_info *info)
         atag_board_len = (info->atag_board(info, atag_board_buf) + 3) & ~3;
         WRITE_WORD(p, (atag_board_len + 8) >> 2);
         WRITE_WORD(p, 0x414f4d50);
-        cpu_physical_memory_write(p, atag_board_buf, atag_board_len);
+        address_space_write(as, p, MEMTXATTRS_UNSPECIFIED,
+                            atag_board_buf, atag_board_len);
         p += atag_board_len;
     }
     /* ATAG_END */
@@ -311,7 +336,8 @@ static void set_kernel_args(const struct arm_boot_info *info)
     WRITE_WORD(p, 0);
 }
 
-static void set_kernel_args_old(const struct arm_boot_info *info)
+static void set_kernel_args_old(const struct arm_boot_info *info,
+                                AddressSpace *as)
 {
     hwaddr p;
     const char *s;
@@ -379,7 +405,8 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
     }
     s = info->kernel_cmdline;
     if (s) {
-        cpu_physical_memory_write(p, s, strlen(s) + 1);
+        address_space_write(as, p, MEMTXATTRS_UNSPECIFIED,
+                            (const uint8_t *)s, strlen(s) + 1);
     } else {
         WRITE_WORD(p, 0);
     }
@@ -453,6 +480,7 @@ static void fdt_add_psci_node(void *fdt)
  * @addr:       the address to load the image at
  * @binfo:      struct describing the boot environment
  * @addr_limit: upper limit of the available memory area at @addr
+ * @as:         address space to load image to
  *
  * Load a device tree supplied by the machine or by the user  with the
  * '-dtb' command line option, and put it at offset @addr in target
@@ -469,7 +497,7 @@ static void fdt_add_psci_node(void *fdt)
  * Note: Must not be called unless have_dtb(binfo) is true.
  */
 static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
-                    hwaddr addr_limit)
+                    hwaddr addr_limit, AddressSpace *as)
 {
     void *fdt = NULL;
     int size, rc;
@@ -615,7 +643,7 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
     /* Put the DTB into the memory map as a ROM image: this will ensure
      * the DTB is copied again upon reset, even if addr points into RAM.
      */
-    rom_add_blob_fixed("dtb", fdt, size, addr);
+    rom_add_blob_fixed_as("dtb", fdt, size, addr, as);
 
     g_free(fdt);
 
@@ -702,13 +730,15 @@ static void do_cpu_reset(void *opaque)
             }
 
             if (cs == first_cpu) {
+                AddressSpace *as = arm_boot_addressspace(cpu, info);
+
                 cpu_set_pc(cs, info->loader_start);
 
                 if (!have_dtb(info)) {
                     if (old_param) {
-                        set_kernel_args_old(info);
+                        set_kernel_args_old(info, as);
                     } else {
-                        set_kernel_args(info);
+                        set_kernel_args(info, as);
                     }
                 }
             } else {
@@ -783,7 +813,7 @@ static int do_arm_linux_init(Object *obj, void *opaque)
 
 static uint64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
                              uint64_t *lowaddr, uint64_t *highaddr,
-                             int elf_machine)
+                             int elf_machine, AddressSpace *as)
 {
     bool elf_is64;
     union {
@@ -826,9 +856,9 @@ static uint64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
         }
     }
 
-    ret = load_elf(info->kernel_filename, NULL, NULL,
-                   pentry, lowaddr, highaddr, big_endian, elf_machine,
-                   1, data_swab);
+    ret = load_elf_as(info->kernel_filename, NULL, NULL,
+                      pentry, lowaddr, highaddr, big_endian, elf_machine,
+                      1, data_swab, as);
     if (ret <= 0) {
         /* The header loaded but the image didn't */
         exit(1);
@@ -838,7 +868,7 @@ static uint64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
 }
 
 static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
-                                   hwaddr *entry)
+                                   hwaddr *entry, AddressSpace *as)
 {
     hwaddr kernel_load_offset = KERNEL64_LOAD_ADDR;
     uint8_t *buffer;
@@ -873,7 +903,7 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
     }
 
     *entry = mem_base + kernel_load_offset;
-    rom_add_blob_fixed(filename, buffer, size, *entry);
+    rom_add_blob_fixed_as(filename, buffer, size, *entry, as);
 
     g_free(buffer);
 
@@ -895,6 +925,7 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
     ARMCPU *cpu = n->cpu;
     struct arm_boot_info *info =
         container_of(n, struct arm_boot_info, load_kernel_notifier);
+    AddressSpace *as = arm_boot_addressspace(cpu, info);
 
     /* The board code is not supposed to set secure_board_setup unless
      * running its code in secure mode is actually possible, and KVM
@@ -912,7 +943,7 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
              * the kernel is supposed to be loaded by the bootloader), copy the
              * DTB to the base of RAM for the bootloader to pick up.
              */
-            if (load_dtb(info->loader_start, info, 0) < 0) {
+            if (load_dtb(info->loader_start, info, 0, as) < 0) {
                 exit(1);
             }
         }
@@ -987,7 +1018,7 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
 
     /* Assume that raw images are linux kernels, and ELF images are not.  */
     kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr,
-                               &elf_high_addr, elf_machine);
+                               &elf_high_addr, elf_machine, as);
     if (kernel_size > 0 && have_dtb(info)) {
         /* If there is still some room left at the base of RAM, try and put
          * the DTB there like we do for images loaded with -bios or -pflash.
@@ -1000,25 +1031,26 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
             if (elf_low_addr < info->loader_start) {
                 elf_low_addr = 0;
             }
-            if (load_dtb(info->loader_start, info, elf_low_addr) < 0) {
+            if (load_dtb(info->loader_start, info, elf_low_addr, as) < 0) {
                 exit(1);
             }
         }
     }
     entry = elf_entry;
     if (kernel_size < 0) {
-        kernel_size = load_uimage(info->kernel_filename, &entry, NULL,
-                                  &is_linux, NULL, NULL);
+        kernel_size = load_uimage_as(info->kernel_filename, &entry, NULL,
+                                     &is_linux, NULL, NULL, as);
     }
     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size < 0) {
         kernel_size = load_aarch64_image(info->kernel_filename,
-                                         info->loader_start, &entry);
+                                         info->loader_start, &entry, as);
         is_linux = 1;
     } else if (kernel_size < 0) {
         /* 32-bit ARM */
         entry = info->loader_start + KERNEL_LOAD_ADDR;
-        kernel_size = load_image_targphys(info->kernel_filename, entry,
-                                          info->ram_size - KERNEL_LOAD_ADDR);
+        kernel_size = load_image_targphys_as(info->kernel_filename, entry,
+                                             info->ram_size - KERNEL_LOAD_ADDR,
+                                             as);
         is_linux = 1;
     }
     if (kernel_size < 0) {
@@ -1030,15 +1062,16 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
         uint32_t fixupcontext[FIXUP_MAX];
 
         if (info->initrd_filename) {
-            initrd_size = load_ramdisk(info->initrd_filename,
-                                       info->initrd_start,
-                                       info->ram_size -
-                                       info->initrd_start);
+            initrd_size = load_ramdisk_as(info->initrd_filename,
+                                          info->initrd_start,
+                                          info->ram_size - info->initrd_start,
+                                          as);
             if (initrd_size < 0) {
-                initrd_size = load_image_targphys(info->initrd_filename,
-                                                  info->initrd_start,
-                                                  info->ram_size -
-                                                  info->initrd_start);
+                initrd_size = load_image_targphys_as(info->initrd_filename,
+                                                     info->initrd_start,
+                                                     info->ram_size -
+                                                     info->initrd_start,
+                                                     as);
             }
             if (initrd_size < 0) {
                 error_report("could not load initrd '%s'",
@@ -1079,7 +1112,7 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
 
             /* Place the DTB after the initrd in memory with alignment. */
             dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size, align);
-            if (load_dtb(dtb_start, info, 0) < 0) {
+            if (load_dtb(dtb_start, info, 0, as) < 0) {
                 exit(1);
             }
             fixupcontext[FIXUP_ARGPTR] = dtb_start;
@@ -1095,7 +1128,7 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
         fixupcontext[FIXUP_ENTRYPOINT] = entry;
 
         write_bootloader("bootloader", info->loader_start,
-                         primary_loader, fixupcontext);
+                         primary_loader, fixupcontext, as);
 
         if (info->nb_cpus > 1) {
             info->write_secondary_boot(cpu, info);
-- 
2.16.1

[Qemu-devel] [PATCH 3/3] hw/arm/armv7m: Honour CPU's address space for image loads

[Peter Maydell]

Instead of loading guest images to the system address space, use the
CPU's address space.  This is important if we're trying to load the
file to memory or via an alias memory region that is provided by an
SoC object and thus not mapped into the system address space.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/armv7m.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 56770a7048..facc536b07 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -270,6 +270,9 @@ void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size)
     uint64_t entry;
     uint64_t lowaddr;
     int big_endian;
+    AddressSpace *as;
+    int asidx;
+    CPUState *cs = CPU(cpu);
 
 #ifdef TARGET_WORDS_BIGENDIAN
     big_endian = 1;
@@ -282,11 +285,19 @@ void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size)
         exit(1);
     }
 
+    if (arm_feature(&cpu->env, ARM_FEATURE_EL3)) {
+        asidx = ARMASIdx_S;
+    } else {
+        asidx = ARMASIdx_NS;
+    }
+    as = cpu_get_address_space(cs, asidx);
+
     if (kernel_filename) {
-        image_size = load_elf(kernel_filename, NULL, NULL, &entry, &lowaddr,
-                              NULL, big_endian, EM_ARM, 1, 0);
+        image_size = load_elf_as(kernel_filename, NULL, NULL, &entry, &lowaddr,
+                                 NULL, big_endian, EM_ARM, 1, 0, as);
         if (image_size < 0) {
-            image_size = load_image_targphys(kernel_filename, 0, mem_size);
+            image_size = load_image_targphys_as(kernel_filename, 0,
+                                                mem_size, as);
             lowaddr = 0;
         }
         if (image_size < 0) {
-- 
2.16.1

Re: [Qemu-devel] [PATCH 2/3] hw/arm/boot: Honour CPU's address space for image loads

[Peter Maydell]

On 15 February 2018 at 17:57, Peter Maydell <peter.maydell@linaro.org> wrote:
> Instead of loading kernels, device trees, and the like to
> the system address space, use the CPU's address space. This
> is important if we're trying to load the file to memory or
> via an alias memory region that is provided by an SoC
> object and thus not mapped into the system address space.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/boot.c | 119 +++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 76 insertions(+), 43 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 9b174b982c..0eac655c98 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -35,6 +35,25 @@
>  #define ARM64_TEXT_OFFSET_OFFSET    8
>  #define ARM64_MAGIC_OFFSET          56
>
> +static AddressSpace *arm_boot_addressspace(ARMCPU *cpu,
> +                                           const struct arm_boot_info *info)
> +{
> +    /* Return the address space to use for bootloader reads and writes.
> +     * We prefer the secure address space if the CPU has it and we're
> +     * going to boot the guest into it.
> +     */
> +    int asidx;
> +    CPUState *cs = CPU(cpu);
> +
> +    if (arm_feature(&cpu->env, ARM_FEATURE_EL3) && info->secure_boot) {
> +        asidx = ARMASIdx_S;
> +    } else {
> +        asidx = ARMASIdx_NS;
> +    }
> +
> +    return cpu_get_address_space(cs, asidx);
> +}
> +
>  typedef enum {
>      FIXUP_NONE = 0,     /* do nothing */
>      FIXUP_TERMINATOR,   /* end of insns */
> @@ -124,7 +143,8 @@ static const ARMInsnFixup smpboot[] = {
>  };
>
>  static void write_bootloader(const char *name, hwaddr addr,
> -                             const ARMInsnFixup *insns, uint32_t *fixupcontext)
> +                             const ARMInsnFixup *insns, uint32_t *fixupcontext,
> +                             AddressSpace *as)
>  {
>      /* Fix up the specified bootloader fragment and write it into
>       * guest memory using rom_add_blob_fixed(). fixupcontext is
> @@ -163,7 +183,7 @@ static void write_bootloader(const char *name, hwaddr addr,
>          code[i] = tswap32(insn);
>      }
>
> -    rom_add_blob_fixed(name, code, len * sizeof(uint32_t), addr);
> +    rom_add_blob_fixed_as(name, code, len * sizeof(uint32_t), addr, as);
>
>      g_free(code);
>  }
> @@ -172,6 +192,7 @@ static void default_write_secondary(ARMCPU *cpu,
>                                      const struct arm_boot_info *info)
>  {
>      uint32_t fixupcontext[FIXUP_MAX];
> +    AddressSpace *as = CPU(cpu)->as;

Oops. This should be
    AddressSpace *as = arm_boot_addressspace(cpu, info);

>      fixupcontext[FIXUP_GIC_CPU_IF] = info->gic_cpu_if_addr;
>      fixupcontext[FIXUP_BOOTREG] = info->smp_bootreg_addr;
> @@ -182,13 +203,14 @@ static void default_write_secondary(ARMCPU *cpu,
>      }
>
>      write_bootloader("smpboot", info->smp_loader_start,
> -                     smpboot, fixupcontext);
> +                     smpboot, fixupcontext, as);
>  }
>
>  void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
>                                              const struct arm_boot_info *info,
>                                              hwaddr mvbar_addr)
>  {
> +    AddressSpace *as = CPU(cpu)->as;

...and so should this.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/3] hw/arm/armv7m: Honour CPU's address space for image loads

[Philippe Mathieu-Daudé]

On 02/15/2018 02:57 PM, Peter Maydell wrote:
> Instead of loading guest images to the system address space, use the
> CPU's address space.  This is important if we're trying to load the
> file to memory or via an alias memory region that is provided by an
> SoC object and thus not mapped into the system address space.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/arm/armv7m.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index 56770a7048..facc536b07 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -270,6 +270,9 @@ void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size)
>      uint64_t entry;
>      uint64_t lowaddr;
>      int big_endian;
> +    AddressSpace *as;
> +    int asidx;
> +    CPUState *cs = CPU(cpu);
>  
>  #ifdef TARGET_WORDS_BIGENDIAN
>      big_endian = 1;
> @@ -282,11 +285,19 @@ void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size)
>          exit(1);
>      }
>  
> +    if (arm_feature(&cpu->env, ARM_FEATURE_EL3)) {
> +        asidx = ARMASIdx_S;
> +    } else {
> +        asidx = ARMASIdx_NS;
> +    }
> +    as = cpu_get_address_space(cs, asidx);
> +
>      if (kernel_filename) {
> -        image_size = load_elf(kernel_filename, NULL, NULL, &entry, &lowaddr,
> -                              NULL, big_endian, EM_ARM, 1, 0);
> +        image_size = load_elf_as(kernel_filename, NULL, NULL, &entry, &lowaddr,
> +                                 NULL, big_endian, EM_ARM, 1, 0, as);
>          if (image_size < 0) {
> -            image_size = load_image_targphys(kernel_filename, 0, mem_size);
> +            image_size = load_image_targphys_as(kernel_filename, 0,
> +                                                mem_size, as);
>              lowaddr = 0;
>          }
>          if (image_size < 0) {
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/3] loader: Add new load_ramdisk_as()

[Philippe Mathieu-Daudé]

On 02/15/2018 02:57 PM, Peter Maydell wrote:
> Add a function load_ramdisk_as() which behaves like the existing
> load_ramdisk() but allows the caller to specify the AddressSpace
> to use. This matches the pattern we have already for various
> other loader functions.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  include/hw/loader.h | 12 +++++++++++-
>  hw/core/loader.c    |  8 +++++++-
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 355fe0f5a2..1fd4256782 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -163,16 +163,26 @@ int load_uimage(const char *filename, hwaddr *ep,
>                  void *translate_opaque);
>  
>  /**
> - * load_ramdisk:
> + * load_ramdisk_as:
>   * @filename: Path to the ramdisk image
>   * @addr: Memory address to load the ramdisk to
>   * @max_sz: Maximum allowed ramdisk size (for non-u-boot ramdisks)
> + * @as: The AddressSpace to load the ELF to. The value of address_space_memory
> + *      is used if nothing is supplied here.
>   *
>   * Load a ramdisk image with U-Boot header to the specified memory
>   * address.
>   *
>   * Returns the size of the loaded image on success, -1 otherwise.
>   */
> +int load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
> +                    AddressSpace *as);
> +
> +/**
> + * load_ramdisk:
> + * Same as load_ramdisk_as(), but doesn't allow the caller to specify
> + * an AddressSpace.
> + */
>  int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz);
>  
>  ssize_t gunzip(void *dst, size_t dstlen, uint8_t *src, size_t srclen);
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 91669d65aa..2b9e7394a1 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -729,9 +729,15 @@ int load_uimage_as(const char *filename, hwaddr *ep, hwaddr *loadaddr,
>  
>  /* Load a ramdisk.  */
>  int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
> +{
> +    return load_ramdisk_as(filename, addr, max_sz, NULL);
> +}
> +
> +int load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
> +                    AddressSpace *as)
>  {
>      return load_uboot_image(filename, NULL, &addr, NULL, IH_TYPE_RAMDISK,
> -                            NULL, NULL, NULL);
> +                            NULL, NULL, as);
>  }
>  
>  /* Load a gzip-compressed kernel to a dynamically allocated buffer. */
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/3] hw/arm/boot: Honour CPU's address space for image loads

[Philippe Mathieu-Daudé]

On 02/15/2018 03:06 PM, Peter Maydell wrote:
> On 15 February 2018 at 17:57, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Instead of loading kernels, device trees, and the like to
>> the system address space, use the CPU's address space. This
>> is important if we're trying to load the file to memory or
>> via an alias memory region that is provided by an SoC
>> object and thus not mapped into the system address space.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  hw/arm/boot.c | 119 +++++++++++++++++++++++++++++++++++++---------------------
>>  1 file changed, 76 insertions(+), 43 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 9b174b982c..0eac655c98 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -35,6 +35,25 @@
>>  #define ARM64_TEXT_OFFSET_OFFSET    8
>>  #define ARM64_MAGIC_OFFSET          56
>>
>> +static AddressSpace *arm_boot_addressspace(ARMCPU *cpu,
>> +                                           const struct arm_boot_info *info)
>> +{
>> +    /* Return the address space to use for bootloader reads and writes.
>> +     * We prefer the secure address space if the CPU has it and we're
>> +     * going to boot the guest into it.
>> +     */
>> +    int asidx;
>> +    CPUState *cs = CPU(cpu);
>> +
>> +    if (arm_feature(&cpu->env, ARM_FEATURE_EL3) && info->secure_boot) {
>> +        asidx = ARMASIdx_S;
>> +    } else {
>> +        asidx = ARMASIdx_NS;
>> +    }
>> +
>> +    return cpu_get_address_space(cs, asidx);
>> +}
>> +
>>  typedef enum {
>>      FIXUP_NONE = 0,     /* do nothing */
>>      FIXUP_TERMINATOR,   /* end of insns */
>> @@ -124,7 +143,8 @@ static const ARMInsnFixup smpboot[] = {
>>  };
>>
>>  static void write_bootloader(const char *name, hwaddr addr,
>> -                             const ARMInsnFixup *insns, uint32_t *fixupcontext)
>> +                             const ARMInsnFixup *insns, uint32_t *fixupcontext,
>> +                             AddressSpace *as)
>>  {
>>      /* Fix up the specified bootloader fragment and write it into
>>       * guest memory using rom_add_blob_fixed(). fixupcontext is
>> @@ -163,7 +183,7 @@ static void write_bootloader(const char *name, hwaddr addr,
>>          code[i] = tswap32(insn);
>>      }
>>
>> -    rom_add_blob_fixed(name, code, len * sizeof(uint32_t), addr);
>> +    rom_add_blob_fixed_as(name, code, len * sizeof(uint32_t), addr, as);
>>
>>      g_free(code);
>>  }
>> @@ -172,6 +192,7 @@ static void default_write_secondary(ARMCPU *cpu,
>>                                      const struct arm_boot_info *info)
>>  {
>>      uint32_t fixupcontext[FIXUP_MAX];
>> +    AddressSpace *as = CPU(cpu)->as;
> 
> Oops. This should be
>     AddressSpace *as = arm_boot_addressspace(cpu, info);

I was stuck there wondering why this one were using the CPU AS :)

> 
>>      fixupcontext[FIXUP_GIC_CPU_IF] = info->gic_cpu_if_addr;
>>      fixupcontext[FIXUP_BOOTREG] = info->smp_bootreg_addr;
>> @@ -182,13 +203,14 @@ static void default_write_secondary(ARMCPU *cpu,
>>      }
>>
>>      write_bootloader("smpboot", info->smp_loader_start,
>> -                     smpboot, fixupcontext);
>> +                     smpboot, fixupcontext, as);
>>  }
>>
>>  void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
>>                                              const struct arm_boot_info *info,
>>                                              hwaddr mvbar_addr)
>>  {
>> +    AddressSpace *as = CPU(cpu)->as;
> 
> ...and so should this.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> thanks
> -- PMM
>