Re: [PATCH] KVM: Qemu: Flush i-cache after ide-dma operation in

On Thu, 2009-04-02 at 10:01 +0800, Zhang, Yang wrote:
> The data from dma will include instructions. In order to exeuting the right
> instruction, we should to flush the i-cache to ensure those data can be see 
> by cpu.
> 
> Signed-off-by: Xiantao Zhang <[email protected]>
> Signed-off-by: Yang Zhang <[email protected]>
> ---
> 
> 
> diff --git a/qemu/cache-utils.h b/qemu/cache-utils.h
> index b45fde4..5e11d12 100644
> --- a/qemu/cache-utils.h
> +++ b/qemu/cache-utils.h
> @@ -33,8 +33,22 @@ static inline void flush_icache_range(unsigned long start, unsigned long stop)
>      asm volatile ("sync" : : : "memory");
>      asm volatile ("isync" : : : "memory");
>  }
> +#define qemu_sync_idcache flush_icache_range
> +#else
>  
> +#ifdef __ia64__
> +static inline void qemu_sync_idcache(unsigned long start, unsigned long stop)
> +{
> +    while (start < stop) {
> +	    asm volatile ("fc %0" :: "r"(start));
> +	    start += 32;
> +    }
> +    asm volatile (";;sync.i;;srlz.i;;");
> +}
>  #else
> +static inline void qemu_sync_idcache(unsigned long start, unsigned long stop)
> +#endif
> +
>  #define qemu_cache_utils_init(envp) do { (void) (envp); } while (0)
>  #endif

You already have flush_icache_range() in qemu/target-ia64/fake-exec.c,
so this is redundant. Moving that to cache-utils.h might make sense, but
this should be discussed on qemu-devel.

Also, flush_icache_range() is already called from
cpu_physical_memory_rw(). It would be helpful to include a comment in
this commit explaining why this path is different. (I can see that it
is, but only because I went hunting myself.)

> diff --git a/qemu/cutils.c b/qemu/cutils.c
> index 5b36cc6..7b57173 100644
> --- a/qemu/cutils.c
> +++ b/qemu/cutils.c
> @@ -23,6 +23,7 @@
>   */
>  #include "qemu-common.h"
>  #include "host-utils.h"
> +#include "cache-utils.h"
>  #include <assert.h>
>  
>  void pstrcpy(char *buf, int buf_size, const char *str)
> @@ -215,6 +216,8 @@ void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count)
>          if (copy > qiov->iov[i].iov_len)
>              copy = qiov->iov[i].iov_len;
>          memcpy(qiov->iov[i].iov_base, p, copy);
> +        qemu_sync_idcache((unsigned long)qiov->iov[i].iov_base, 
> +                    (unsigned long)(qiov->iov[i].iov_base + copy));
>          p     += copy;
>          count -= copy;
>      }

This is way too generic a call for this location. Other architectures
also need to synchronize L1 caches sometimes, but they don't need to do
it here. You need to comment and guard this call better (probably using
some combination of kvm_enabled() and ifdefs).

-- 
Hollis Blanchard
IBM Linux Technology Center