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

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

[Zhang, Yang]

[-- Attachment #1: Type: text/plain, Size: 1852 bytes --]

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 <xiantao.zhang@intel.com>
Signed-off-by: Yang Zhang <yang.zhang@intel.com>
---


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
 
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;
     }
-- 
1.6.0.rc1

[-- Attachment #2: 0001-KVM-Qemu-Flush-icache-after-ide-dma-operation-in-IA64.patch --]
[-- Type: application/octet-stream, Size: 2120 bytes --]

From 3b706bda62fbddfa2bca414d8c0fb7cda904b7db Mon Sep 17 00:00:00 2001
From: Yang Zhang <Yang Zhang>
Date: Wed, 1 Apr 2009 01:50:53 -0400
Subject: [PATCH] KVM: Qemu: Flush i-cache after ide-dma operation in IA64

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 <xiantao.zhang@intel.com>
Signed-off-by: Yang Zhang <Yang Zhang>
---
 qemu/cache-utils.h |   14 ++++++++++++++
 qemu/cutils.c      |    3 +++
 2 files changed, 17 insertions(+), 0 deletions(-)

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
 
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;
     }
-- 
1.6.0.rc1

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

[Avi Kivity]

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.
>
>
>
> 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;;");
> +}
>   

What about smp?

I'm surprised the guest doesn't do this by itself?

>  
>  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 the wrong place to put this.  Once we stop bouncing 
scatter/gather DMA, we will no longer call this function.

The correct place is either in the device code itself, or in the dma api 
(dma-helpers.c).


-- 
error compiling committee.c: too many arguments to function

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

[tgingold]

Quoting Avi Kivity <avi@redhat.com>:

> 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.
> >
> >
> >
> > 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;;");
> > +}
> >

As I hit the same issue a year ago, here is my understanding:

> What about smp?

fc will broadcast to the coherence domain the cache invalidation.  So it is
SMP-ready for usual machines.

> I'm surprised the guest doesn't do this by itself?

It doesn't had to do it.  The PCI transaction will automatically invalidate
caches - but qemu doesn't emulate this (and doesn't need to do on x86).

Tristan.

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

[Avi Kivity]

tgingold@free.fr wrote:
>   
>> What about smp?
>>     
>
> fc will broadcast to the coherence domain the cache invalidation.  So it is
> SMP-ready for usual machines.
>
>   

Interesting.

>> I'm surprised the guest doesn't do this by itself?
>>     
>
> It doesn't had to do it.  The PCI transaction will automatically invalidate
> caches - but qemu doesn't emulate this (and doesn't need to do on x86).
>   

So any DMA on ia64 will flush the instruction caches?!

-- 
error compiling committee.c: too many arguments to function

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

[Avi Kivity]

Avi Kivity wrote:
>>>  
>>
>> It doesn't had to do it.  The PCI transaction will automatically 
>> invalidate
>> caches - but qemu doesn't emulate this (and doesn't need to do on x86).
>>   
>
> So any DMA on ia64 will flush the instruction caches?!
>

Or maybe, the host kernel will do it after the transaction completes?  
In our case the lack of zero-copy means the host is invalidating the 
wrong addresses (memcpy source) and leaving the real destination intact.

-- 
error compiling committee.c: too many arguments to function

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

[Zhang, Xiantao]

Avi Kivity wrote:
> tgingold@free.fr wrote:
>> 
>>> What about smp?
>>> 
>> 
>> fc will broadcast to the coherence domain the cache invalidation. 
>> So it is SMP-ready for usual machines. 
>> 
>> 
> 
> Interesting.
> 
>>> I'm surprised the guest doesn't do this by itself?
>>> 
>> 
>> It doesn't had to do it.  The PCI transaction will automatically
>> invalidate caches - but qemu doesn't emulate this (and doesn't need
>> to do on x86). 
>> 
> 
> So any DMA on ia64 will flush the instruction caches?

Yes, physical DMA should do this, but for virtual DMA operation emulated by Qemu should use explict intrusctions(fc, sync.i) to get it happen, because the data transferred by virtual DMA maybe used as instrustion streams by guest. 
Xiantao

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

[Zhang, Xiantao]

Avi Kivity wrote:
> Avi Kivity wrote:
>>>> 
>>> 
>>> It doesn't had to do it.  The PCI transaction will automatically
>>> invalidate caches - but qemu doesn't emulate this (and doesn't need
>>> to do on x86). 
>>> 
>> 
>> So any DMA on ia64 will flush the instruction caches?!
>> 
> 
> Or maybe, the host kernel will do it after the transaction completes?

Host kernel doesn't do anything about cache flush after DMA, since it thinks platform guarantees that. 

> In our case the lack of zero-copy means the host is invalidating the
> wrong addresses (memcpy source) and leaving the real destination
> intact. 

We just need to sync the target address(destination address), because only its physical address belongs to guest, and likely to be the DMA target address of guest. 

Xiantao

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

[Zhang, Xiantao]

Avi Kivity wrote:
> 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.
>> 
>> 
>> 
>> 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;;");
>> +}
>> 
> 
> What about smp?
> 
> I'm surprised the guest doesn't do this by itself?
> 
>> 
>>  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 the wrong place to put this.  Once we stop bouncing
> scatter/gather DMA, we will no longer call this function.

This patch intends to fix the issue before adopting scatter/gather DMA mode. But if we want to keep this funtion, had better to pick it to avoid such issues in future. 

> The correct place is either in the device code itself, or in the dma
> api (dma-helpers.c).

Maybe dma-helpers.c 
Xiantao

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

[Avi Kivity]

Zhang, Xiantao wrote:
>> This is the wrong place to put this.  Once we stop bouncing
>> scatter/gather DMA, we will no longer call this function.
>>     
>
> This patch intends to fix the issue before adopting scatter/gather DMA mode. But if we want to keep this funtion, had better to pick it to avoid such issues in future. 
>
>   

This function is the wrong place, it just happens to be called.

>> The correct place is either in the device code itself, or in the dma
>> api (dma-helpers.c).
>>     
>
> Maybe dma-helpers.c 

Please send a patch with this in dma-helpers.c.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Hollis Blanchard]

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 <xiantao.zhang@intel.com>
> Signed-off-by: Yang Zhang <yang.zhang@intel.com>
> ---
> 
> 
> 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