Re: [Qemu-devel] [PATCH v2] roms: Flush icache when writing roms to guest memory

[Paolo Bonzini]

Il 12/12/2013 10:29, Alexander Graf ha scritto:
 We use the rom infrastructure to write firmware and/or initial kernel
 blobs into guest address space. So we're basically emulating the cache
 off phase on very early system bootup.
 
 That phase is usually responsible for clearing the instruction cache for
 anything it writes into cachable memory, to ensure that after reboot we
 don't happen to execute stale bits from the instruction cache.
 
 So we need to invalidate the icache every time we write a rom into guest
 address space. We do not need to do this for every DMA since the guest
 expects it has to flush the icache manually in that case.
 
 This fixes random reboot issues on e5500 (booke ppc) for me.
 
 Signed-off-by: Alexander Graf ag...@suse.de

Interesting way to avoid cut-and-paste.

Applied to uq/master, thanks.

Paolo

 ---
 
 v1 - v2:
 
   - extract the flush into a helper function
 ---
  exec.c| 44 +++-
  hw/core/loader.c  |  7 +++
  include/exec/cpu-common.h |  1 +
  3 files changed, 47 insertions(+), 5 deletions(-)
 
 diff --git a/exec.c b/exec.c
 index f4b9ef2..896f7b8 100644
 --- a/exec.c
 +++ b/exec.c
 @@ -50,6 +50,7 @@
  #include translate-all.h
  
  #include exec/memory-internal.h
 +#include qemu/cache-utils.h
  
  //#define DEBUG_SUBPAGE
  
 @@ -2010,9 +2011,13 @@ void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
  address_space_rw(address_space_memory, addr, buf, len, is_write);
  }
  
 -/* used for ROM loading : can write in RAM and ROM */
 -void cpu_physical_memory_write_rom(hwaddr addr,
 -   const uint8_t *buf, int len)
 +enum write_rom_type {
 +WRITE_DATA,
 +FLUSH_CACHE,
 +};
 +
 +static inline void cpu_physical_memory_write_rom_internal(
 +hwaddr addr, const uint8_t *buf, int len, enum write_rom_type type)
  {
  hwaddr l;
  uint8_t *ptr;
 @@ -2031,8 +2036,15 @@ void cpu_physical_memory_write_rom(hwaddr addr,
  addr1 += memory_region_get_ram_addr(mr);
  /* ROM/RAM case */
  ptr = qemu_get_ram_ptr(addr1);
 -memcpy(ptr, buf, l);
 -invalidate_and_set_dirty(addr1, l);
 +switch (type) {
 +case WRITE_DATA:
 +memcpy(ptr, buf, l);
 +invalidate_and_set_dirty(addr1, l);
 +break;
 +case FLUSH_CACHE:
 +flush_icache_range((uintptr_t)ptr, (uintptr_t)ptr + l);
 +break;
 +}
  }
  len -= l;
  buf += l;
 @@ -2040,6 +2052,28 @@ void cpu_physical_memory_write_rom(hwaddr addr,
  }
  }
  
 +/* used for ROM loading : can write in RAM and ROM */
 +void cpu_physical_memory_write_rom(hwaddr addr,
 +   const uint8_t *buf, int len)
 +{
 +cpu_physical_memory_write_rom_internal(addr, buf, len, WRITE_DATA);
 +}
 +
 +void cpu_flush_icache_range(hwaddr start, int len)
 +{
 +/*
 + * This function should do the same thing as an icache flush that was
 + * triggered from within the guest. For TCG we are always cache coherent,
 + * so there is no need to flush anything. For KVM / Xen we need to flush
 + * the host's instruction cache at least.
 + */
 +if (tcg_enabled()) {
 +return;
 +}
 +
 +cpu_physical_memory_write_rom_internal(start, NULL, len, FLUSH_CACHE);
 +}
 +
  typedef struct {
  MemoryRegion *mr;
  void *buffer;
 diff --git a/hw/core/loader.c b/hw/core/loader.c
 index 60d2ebd..0634bee 100644
 --- a/hw/core/loader.c
 +++ b/hw/core/loader.c
 @@ -785,6 +785,13 @@ static void rom_reset(void *unused)
  g_free(rom-data);
  rom-data = NULL;
  }
 +/*
 + * The rom loader is really on the same level as firmware in the 
 guest
 + * shadowing a ROM into RAM. Such a shadowing mechanism needs to 
 ensure
 + * that the instruction cache for that new region is clear, so that 
 the
 + * CPU definitely fetches its instructions from the just written 
 data.
 + */
 +cpu_flush_icache_range(rom-addr, rom-datasize);
  }
  }
  
 diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
 index e4996e1..8f33122 100644
 --- a/include/exec/cpu-common.h
 +++ b/include/exec/cpu-common.h
 @@ -110,6 +110,7 @@ void stq_phys(hwaddr addr, uint64_t val);
  
  void cpu_physical_memory_write_rom(hwaddr addr,
 const uint8_t *buf, int len);
 +void cpu_flush_icache_range(hwaddr start, int len);
  
  extern struct MemoryRegion io_mem_rom;
  extern struct MemoryRegion io_mem_notdirty;
 

[Qemu-devel] [PATCH v2] roms: Flush icache when writing roms to guest memory

[Alexander Graf]

We use the rom infrastructure to write firmware and/or initial kernel
blobs into guest address space. So we're basically emulating the cache
off phase on very early system bootup.

That phase is usually responsible for clearing the instruction cache for
anything it writes into cachable memory, to ensure that after reboot we
don't happen to execute stale bits from the instruction cache.

So we need to invalidate the icache every time we write a rom into guest
address space. We do not need to do this for every DMA since the guest
expects it has to flush the icache manually in that case.

This fixes random reboot issues on e5500 (booke ppc) for me.

Signed-off-by: Alexander Graf ag...@suse.de

---

v1 - v2:

  - extract the flush into a helper function
---
 exec.c| 44 +++-
 hw/core/loader.c  |  7 +++
 include/exec/cpu-common.h |  1 +
 3 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/exec.c b/exec.c
index f4b9ef2..896f7b8 100644
--- a/exec.c
+++ b/exec.c
@@ -50,6 +50,7 @@
 #include translate-all.h
 
 #include exec/memory-internal.h
+#include qemu/cache-utils.h
 
 //#define DEBUG_SUBPAGE
 
@@ -2010,9 +2011,13 @@ void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
 address_space_rw(address_space_memory, addr, buf, len, is_write);
 }
 
-/* used for ROM loading : can write in RAM and ROM */
-void cpu_physical_memory_write_rom(hwaddr addr,
-   const uint8_t *buf, int len)
+enum write_rom_type {
+WRITE_DATA,
+FLUSH_CACHE,
+};
+
+static inline void cpu_physical_memory_write_rom_internal(
+hwaddr addr, const uint8_t *buf, int len, enum write_rom_type type)
 {
 hwaddr l;
 uint8_t *ptr;
@@ -2031,8 +2036,15 @@ void cpu_physical_memory_write_rom(hwaddr addr,
 addr1 += memory_region_get_ram_addr(mr);
 /* ROM/RAM case */
 ptr = qemu_get_ram_ptr(addr1);
-memcpy(ptr, buf, l);
-invalidate_and_set_dirty(addr1, l);
+switch (type) {
+case WRITE_DATA:
+memcpy(ptr, buf, l);
+invalidate_and_set_dirty(addr1, l);
+break;
+case FLUSH_CACHE:
+flush_icache_range((uintptr_t)ptr, (uintptr_t)ptr + l);
+break;
+}
 }
 len -= l;
 buf += l;
@@ -2040,6 +2052,28 @@ void cpu_physical_memory_write_rom(hwaddr addr,
 }
 }
 
+/* used for ROM loading : can write in RAM and ROM */
+void cpu_physical_memory_write_rom(hwaddr addr,
+   const uint8_t *buf, int len)
+{
+cpu_physical_memory_write_rom_internal(addr, buf, len, WRITE_DATA);
+}
+
+void cpu_flush_icache_range(hwaddr start, int len)
+{
+/*
+ * This function should do the same thing as an icache flush that was
+ * triggered from within the guest. For TCG we are always cache coherent,
+ * so there is no need to flush anything. For KVM / Xen we need to flush
+ * the host's instruction cache at least.
+ */
+if (tcg_enabled()) {
+return;
+}
+
+cpu_physical_memory_write_rom_internal(start, NULL, len, FLUSH_CACHE);
+}
+
 typedef struct {
 MemoryRegion *mr;
 void *buffer;
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 60d2ebd..0634bee 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -785,6 +785,13 @@ static void rom_reset(void *unused)
 g_free(rom-data);
 rom-data = NULL;
 }
+/*
+ * The rom loader is really on the same level as firmware in the guest
+ * shadowing a ROM into RAM. Such a shadowing mechanism needs to ensure
+ * that the instruction cache for that new region is clear, so that the
+ * CPU definitely fetches its instructions from the just written data.
+ */
+cpu_flush_icache_range(rom-addr, rom-datasize);
 }
 }
 
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index e4996e1..8f33122 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -110,6 +110,7 @@ void stq_phys(hwaddr addr, uint64_t val);
 
 void cpu_physical_memory_write_rom(hwaddr addr,
const uint8_t *buf, int len);
+void cpu_flush_icache_range(hwaddr start, int len);
 
 extern struct MemoryRegion io_mem_rom;
 extern struct MemoryRegion io_mem_notdirty;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2] roms: Flush icache when writing roms to guest memory

[Scott Wood]

On Thu, 2013-12-12 at 10:29 +0100, Alexander Graf wrote:
 We use the rom infrastructure to write firmware and/or initial kernel
 blobs into guest address space. So we're basically emulating the cache
 off phase on very early system bootup.
 
 That phase is usually responsible for clearing the instruction cache for
 anything it writes into cachable memory, to ensure that after reboot we
 don't happen to execute stale bits from the instruction cache.
 
 So we need to invalidate the icache every time we write a rom into guest
 address space. We do not need to do this for every DMA since the guest
 expects it has to flush the icache manually in that case.

Linux may do this for all DMAs (or more accurately, for all pages that
it makes executable regardless of how it loaded the data), and it's
probably a reasonable assumption for virtio, but on real hardware the
guest may often be able to get away with just invalidating the icache,
without the dcache flush step.

Whether it's worth QEMU flushing in those cases (and finding a way to
exempt virtio), versus documenting it as a known limitation, is
debatable -- but I don't think you can say that the requirements are the
same as for real hardware (on platforms that require such cache
cleaning, and excluding hardware (if any) where DMA is injected directly
into data cache without passing through main memory).  IMHO it would be
safer to start with a policy of always cleaning the cache (on relevant
platforms -- it would be a no-op on x86) when QEMU modifies guest
memory, and then for performance carve out exceptions like virtio.

Breakpoints (and any other memory modifications that might be done by a
debugger) are another situation that requires cache cleaning.

-Scott