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

[Paolo Bonzini <pbonzini@redhat.com>]

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 <agraf@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;
>