| Message ID | 10C63FAD690C13458F0B32BCED571F140F98ED4B@pdsmsx502.ccr.corp.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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).
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?!
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.
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-- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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).
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; }