Re: [PATCH] kvm: qemu: Sync idcache after emualted DMA operations for ia64

Re: [PATCH] kvm: qemu: Sync idcache after emualted DMA operations for ia64

[Hollis Blanchard]

On Tue, 2009-04-21 at 10:08 +0000, Avi Kivity wrote:
> From: Xiantao Zhang <xiantao.zhang@intel.com>
> 
> ia64 depends on platform provides synced idcache after DMA operation.
> For virtual dma operations in qemu, it also need to provide similar
> machanism.
> 
> Signed-off-by: Xiantao Zhang  <xiantao.zhang@intel.com>
> Signed-off-by: Avi Kivity <avi@redhat.com>

I pointed out some problems with this patch a couple weeks ago:
http://article.gmane.org/gmane.comp.emulators.kvm.devel/30475

Why was it still applied?

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] kvm: qemu: Sync idcache after emualted DMA operations for ia64

[Avi Kivity]

Hollis Blanchard wrote:
> On Tue, 2009-04-21 at 10:08 +0000, Avi Kivity wrote:
>   
>> From: Xiantao Zhang <xiantao.zhang@intel.com>
>>
>> ia64 depends on platform provides synced idcache after DMA operation.
>> For virtual dma operations in qemu, it also need to provide similar
>> machanism.
>>
>> Signed-off-by: Xiantao Zhang  <xiantao.zhang@intel.com>
>> Signed-off-by: Avi Kivity <avi@redhat.com>
>>     
>
> I pointed out some problems with this patch a couple weeks ago:
> http://article.gmane.org/gmane.comp.emulators.kvm.devel/30475
>
> Why was it still applied?
>   

So I can release kvm-85.  I don't like it either.

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

Re: [PATCH] kvm: qemu: Sync idcache after emualted DMA operations for ia64

[Hollis Blanchard]

On Tue, 2009-04-21 at 20:21 +0300, Avi Kivity wrote:
> Hollis Blanchard wrote:
> > On Tue, 2009-04-21 at 10:08 +0000, Avi Kivity wrote:
> >   
> >> From: Xiantao Zhang <xiantao.zhang@intel.com>
> >>
> >> ia64 depends on platform provides synced idcache after DMA operation.
> >> For virtual dma operations in qemu, it also need to provide similar
> >> machanism.
> >>
> >> Signed-off-by: Xiantao Zhang  <xiantao.zhang@intel.com>
> >> Signed-off-by: Avi Kivity <avi@redhat.com>
> >>     
> >
> > I pointed out some problems with this patch a couple weeks ago:
> > http://article.gmane.org/gmane.comp.emulators.kvm.devel/30475
> >
> > Why was it still applied?
> >   
> 
> So I can release kvm-85.  I don't like it either.

You don't need to commit it:
     1. KVM on ia64 has made it for this long without that patch.
     2. The ia64 guys didn't even reply, much less revise the patch.
     3. The patch quality is clearly not high enough to be committed to
        qemu (at least, one would hope), so you're just going to have to
        revert it to apply a better fix anyways.

I don't see why all ia64 patches had to be applied for kvm-85,
regardless of merit.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] kvm: qemu: Sync idcache after emualted DMA operations for ia64

[Avi Kivity]

Hollis Blanchard wrote:
>> So I can release kvm-85.  I don't like it either.
>>     
>
> You don't need to commit it:
>      1. KVM on ia64 has made it for this long without that patch.
>   

Apparently, this was triggered by recent changes to the dma-api?  Xiantao?

>      2. The ia64 guys didn't even reply, much less revise the patch.
>   

See below.

>      3. The patch quality is clearly not high enough to be committed to
>         qemu (at least, one would hope), so you're just going to have to
>         revert it to apply a better fix anyways.
>   

I agree that the patch is not correct.  If it is not fixed by the end of 
this month, you may send a patch to revert this change.

> I don't see why all ia64 patches had to be applied for kvm-85,
> regardless of merit.
>   

I like to make releases that are not useless.  For that I sometimes have 
to resort to band-aids.  Comments like yours about the quality of these 
band aids will be addressed.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

RE: [PATCH] kvm: qemu: Sync idcache after emualted DMA operations for ia64

[Zhang, Xiantao]

Hollis Blanchard wrote:
> On Tue, 2009-04-21 at 20:21 +0300, Avi Kivity wrote:
>> Hollis Blanchard wrote:
>>> On Tue, 2009-04-21 at 10:08 +0000, Avi Kivity wrote:
>>> 
>>>> From: Xiantao Zhang <xiantao.zhang@intel.com>
>>>> 
>>>> ia64 depends on platform provides synced idcache after DMA
>>>> operation. For virtual dma operations in qemu, it also need to
>>>> provide similar machanism. 
>>>> 
>>>> Signed-off-by: Xiantao Zhang  <xiantao.zhang@intel.com>
>>>> Signed-off-by: Avi Kivity <avi@redhat.com>
>>>> 
>>> 
>>> I pointed out some problems with this patch a couple weeks ago:
>>> http://article.gmane.org/gmane.comp.emulators.kvm.devel/30475
>>> 
>>> Why was it still applied?
>>> 
>> 
>> So I can release kvm-85.  I don't like it either.

Hi, Hollis

> You don't need to commit it:
>      1. KVM on ia64 has made it for this long without that patch.

No, I don't know you had been tracing the upstream code, but I had to say this API is introduced recently, and it breaks kvm/ia64 since DMA adopts it to access guest memory. 

>      2. The ia64 guys didn't even reply, much less revise the patch. 

Sorry to forget to reply your mail!  

>      3. The patch quality is clearly not high enough to be committed
>         to qemu (at least, one would hope), so you're just going to
>         have to revert it to apply a better fix anyways.

Could you provide a better fix ?   :)

> I don't see why all ia64 patches had to be applied for kvm-85,
> regardless of merit.

We are working on kvm-85 relase from rc1, but a lot of changes related to DMA emulation in upstream Qemu introduced some problems for ia64. I don't see any side-effect about other archs with this patch, because this fucntion is defined as a blank function.
 As you suggested,  ifdef or kvm_enabled wrapper maybe better, but you may need to check the declaration of this function has been wrapped by "ifdef __ia64__ ", so I don't think it is a must to add the wrapper again. 

RE: [PATCH] kvm: qemu: Sync idcache after emualted DMA operations for ia64

[Hollis Blanchard]

On Wed, 2009-04-22 at 09:59 +0800, Zhang, Xiantao wrote:
> Hollis Blanchard wrote:
> > On Tue, 2009-04-21 at 20:21 +0300, Avi Kivity wrote:
> >> Hollis Blanchard wrote:
> >>> On Tue, 2009-04-21 at 10:08 +0000, Avi Kivity wrote:
> >>> 
> >>>> From: Xiantao Zhang <xiantao.zhang@intel.com>
> >>>> 
> >>>> ia64 depends on platform provides synced idcache after DMA
> >>>> operation. For virtual dma operations in qemu, it also need to
> >>>> provide similar machanism. 
> >>>> 
> >>>> Signed-off-by: Xiantao Zhang  <xiantao.zhang@intel.com>
> >>>> Signed-off-by: Avi Kivity <avi@redhat.com>
> >>>> 
> >>> 
> >>> I pointed out some problems with this patch a couple weeks ago:
> >>> http://article.gmane.org/gmane.comp.emulators.kvm.devel/30475
> >>> 
> >>> Why was it still applied?
> >>> 
> >> 
> >> So I can release kvm-85.  I don't like it either.
> 
> Hi, Hollis
> 
> > You don't need to commit it:
> >      1. KVM on ia64 has made it for this long without that patch.
> 
> No, I don't know you had been tracing the upstream code, but I had to
> say this API is introduced recently, and it breaks kvm/ia64 since DMA
> adopts it to access guest memory. 
> 
> >      2. The ia64 guys didn't even reply, much less revise the patch. 
> 
> Sorry to forget to reply your mail!  
> 
> >      3. The patch quality is clearly not high enough to be committed
> >         to qemu (at least, one would hope), so you're just going to
> >         have to revert it to apply a better fix anyways.
> 
> Could you provide a better fix ?   :)

Sorry, even if I wanted to, I couldn't test it.

> > I don't see why all ia64 patches had to be applied for kvm-85,
> > regardless of merit.
> 
> We are working on kvm-85 relase from rc1, but a lot of changes related
> to DMA emulation in upstream Qemu introduced some problems for ia64. I
> don't see any side-effect about other archs with this patch, because
> this fucntion is defined as a blank function.
>  As you suggested,  ifdef or kvm_enabled wrapper maybe better, but you
> may need to check the declaration of this function has been wrapped by
> "ifdef __ia64__ ", so I don't think it is a must to add the wrapper
> again.

As I said before, "synchronize instruction and data caches" is a
meaningful and important operation on architectures other than IA64.
*However*, that operation is not relevant on those architectures in this
particular location. As far as I know, on architectures with
non-coherent i/d caches, invalidating the icache during DMA writes is
unique to IA64.

Please re-read my original mail on this subject and let me know if my
objections are still unclear.

-- 
Hollis Blanchard
IBM Linux Technology Center

RE: [PATCH] kvm: qemu: Sync idcache after emualted DMA operations for ia64

[Zhang, Xiantao]

Hollis Blanchard wrote:
> On Wed, 2009-04-22 at 09:59 +0800, Zhang, Xiantao wrote:
>> Hollis Blanchard wrote:
>>> On Tue, 2009-04-21 at 20:21 +0300, Avi Kivity wrote:
>>>> Hollis Blanchard wrote:
>>>>> On Tue, 2009-04-21 at 10:08 +0000, Avi Kivity wrote:
>>>>> 
>>>>>> From: Xiantao Zhang <xiantao.zhang@intel.com>
>>>>>> 
>>>>>> ia64 depends on platform provides synced idcache after DMA
>>>>>> operation. For virtual dma operations in qemu, it also need to
>>>>>> provide similar machanism. 
>>>>>> 
>>>>>> Signed-off-by: Xiantao Zhang  <xiantao.zhang@intel.com>
>>>>>> Signed-off-by: Avi Kivity <avi@redhat.com>
>>>>>> 
>>>>> 
>>>>> I pointed out some problems with this patch a couple weeks ago:
>>>>> http://article.gmane.org/gmane.comp.emulators.kvm.devel/30475
>>>>> 
>>>>> Why was it still applied?
>>>>> 
>>>> 
>>>> So I can release kvm-85.  I don't like it either.
>> 
>> Hi, Hollis
>> 
>>> You don't need to commit it:
>>>      1. KVM on ia64 has made it for this long without that patch.
>> 
>> No, I don't know you had been tracing the upstream code, but I had to
>> say this API is introduced recently, and it breaks kvm/ia64 since DMA
>> adopts it to access guest memory.
>> 
>>>      2. The ia64 guys didn't even reply, much less revise the patch.
>> 
>> Sorry to forget to reply your mail!
>> 
>>>      3. The patch quality is clearly not high enough to be committed
>>>         to qemu (at least, one would hope), so you're just going to
>>>         have to revert it to apply a better fix anyways.
>> 
>> Could you provide a better fix ?   :)
> 
> Sorry, even if I wanted to, I couldn't test it.
> 
>>> I don't see why all ia64 patches had to be applied for kvm-85,
>>> regardless of merit.
>> 
>> We are working on kvm-85 relase from rc1, but a lot of changes
>> related to DMA emulation in upstream Qemu introduced some problems
>> for ia64. I don't see any side-effect about other archs with this
>> patch, because this fucntion is defined as a blank function.
>>  As you suggested,  ifdef or kvm_enabled wrapper maybe better, but
>> you may need to check the declaration of this function has been
>> wrapped by "ifdef __ia64__ ", so I don't think it is a must to add
>> the wrapper again.
> 
> As I said before, "synchronize instruction and data caches" is a
> meaningful and important operation on architectures other than IA64.
> *However*, that operation is not relevant on those architectures in
> this particular location. As far as I know, on architectures with
> non-coherent i/d caches, invalidating the icache during DMA writes is
> unique to IA64.

For ia64 in native system, processors depend on that platforms issue snoop cycles to invalidate the cachable memory pages that are touched by DMA to ensure the cache coherence from viewpoint of processors.
But for virtual machine, the DMA is emulated by memcpy in userspace, so have to use explict instructions to emulate the snoop cycles to invalidate icache after virtual DMAs, otherwise, processor may see old icache and lead to disaster. 


> Please re-read my original mail on this subject and let me know if my
> objections are still unclear.

> 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.

Agree to discuss the code move in qemu-devel, and this is what we want to push. 

> 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.)

Good suggestion to add the comment here. 

RE: [PATCH] kvm: qemu: Sync idcache after emualted DMA operations for ia64

[Hollis Blanchard]

On Fri, 2009-04-24 at 15:45 +0800, Zhang, Xiantao wrote:
> 
> > As I said before, "synchronize instruction and data caches" is a
> > meaningful and important operation on architectures other than IA64.
> > *However*, that operation is not relevant on those architectures in
> > this particular location. As far as I know, on architectures with
> > non-coherent i/d caches, invalidating the icache during DMA writes
> is
> > unique to IA64.
> 
> For ia64 in native system, processors depend on that platforms issue
> snoop cycles to invalidate the cachable memory pages that are touched
> by DMA to ensure the cache coherence from viewpoint of processors.
> But for virtual machine, the DMA is emulated by memcpy in userspace,
> so have to use explict instructions to emulate the snoop cycles to
> invalidate icache after virtual DMAs, otherwise, processor may see old
> icache and lead to disaster. 

That is exactly the sort of background that should have been in the
commit message in the first place.

Commit message aside, without comments in the code, an outside developer
might very well wonder why other architectures (e.g. PowerPC) *don't*
implement qemu_sync_idcache().

Are you starting to understand my objections to this patch?

-- 
Hollis Blanchard
IBM Linux Technology Center

RE: [PATCH] kvm: qemu: Sync idcache after emualted DMA operations for ia64

[Zhang, Xiantao]

Hollis Blanchard wrote:
> On Fri, 2009-04-24 at 15:45 +0800, Zhang, Xiantao wrote:
>> 
>>> As I said before, "synchronize instruction and data caches" is a
>>> meaningful and important operation on architectures other than IA64.
>>> *However*, that operation is not relevant on those architectures in
>>> this particular location. As far as I know, on architectures with
>>> non-coherent i/d caches, invalidating the icache during DMA writes
>>> is unique to IA64.
>> 
>> For ia64 in native system, processors depend on that platforms issue
>> snoop cycles to invalidate the cachable memory pages that are touched
>> by DMA to ensure the cache coherence from viewpoint of processors.
>> But for virtual machine, the DMA is emulated by memcpy in userspace,
>> so have to use explict instructions to emulate the snoop cycles to
>> invalidate icache after virtual DMAs, otherwise, processor may see
>> old icache and lead to disaster.
> 
> That is exactly the sort of background that should have been in the
> commit message in the first place.
> 
> Commit message aside, without comments in the code, an outside
> developer might very well wonder why other architectures (e.g.
> PowerPC) *don't* implement qemu_sync_idcache().

Yes. Maybe I can submit a patch to add the comments later.

> Are you starting to understand my objections to this patch?
Sure. :)

Xiantao

[PATCH] Revert "Sync idcache after emualted DMA operations for ia64"

[Hollis Blanchard]

This reverts commit 9dc99a28236161a5a1b4c58f1e9c4ec6179cb976.
Aside from the other issues discussed on kvm-devel, this commit breaks the
PowerPC build.

Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>

---
Bad mailing list address on my previous mail.

 cache-utils.h |   14 --------------
 cutils.c      |    3 ---
 dma-helpers.c |   12 ------------
 exec.c        |    3 ---
 4 files changed, 0 insertions(+), 32 deletions(-)

diff --git a/cache-utils.h b/cache-utils.h
index 2a07fbd..b45fde4 100644
--- a/cache-utils.h
+++ b/cache-utils.h
@@ -33,22 +33,8 @@ 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/cutils.c b/cutils.c
index 4bf4fcd..5b36cc6 100644
--- a/cutils.c
+++ b/cutils.c
@@ -23,7 +23,6 @@
  */
 #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)
@@ -216,8 +215,6 @@ 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;
     }
diff --git a/dma-helpers.c b/dma-helpers.c
index f71ca3b..f9eb224 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -9,7 +9,6 @@
 
 #include "dma.h"
 #include "block_int.h"
-#include "cache-utils.h"
 
 static AIOPool dma_aio_pool;
 
@@ -138,8 +137,6 @@ static BlockDriverAIOCB *dma_bdrv_io(
     BlockDriverCompletionFunc *cb, void *opaque,
     int is_write)
 {
-    int i;
-    QEMUIOVector *qiov;
     DMAAIOCB *dbs =  qemu_aio_get_pool(&dma_aio_pool, bs, cb, opaque);
 
     dbs->acb = NULL;
@@ -152,15 +149,6 @@ static BlockDriverAIOCB *dma_bdrv_io(
     dbs->bh = NULL;
     qemu_iovec_init(&dbs->iov, sg->nsg);
     dma_bdrv_cb(dbs, 0);
-
-    if (!is_write) {
-        qiov = &dbs->iov;
-        for (i = 0; i < qiov->niov; ++i) {
-           qemu_sync_idcache((unsigned long)qiov->iov[i].iov_base,
-                 (unsigned long)(qiov->iov[i].iov_base + qiov->iov[i].iov_len));
-	}
-    }
-
     if (!dbs->acb) {
         qemu_aio_release(dbs);
         return NULL;
diff --git a/exec.c b/exec.c
index 16d3cf8..262c72f 100644
--- a/exec.c
+++ b/exec.c
@@ -3400,9 +3400,6 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len,
                 addr1 += l;
                 access_len -= l;
             }
-            if (kvm_enabled())
-	        flush_icache_range((unsigned long)buffer,
-				(unsigned long)buffer + access_len);
         }
         return;
     }
-- 
1.6.0.6

Re: [PATCH] Revert "Sync idcache after emualted DMA operations for ia64"

[Avi Kivity]

Hollis Blanchard wrote:
> This reverts commit 9dc99a28236161a5a1b4c58f1e9c4ec6179cb976.
> Aside from the other issues discussed on kvm-devel, this commit breaks the
> PowerPC build.
>
>   

Applied, thanks.

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

RE: [PATCH] Revert "Sync idcache after emualted DMA operations for ia64"

[Zhang, Xiantao]

Avi Kivity wrote:
> Hollis Blanchard wrote:
>> This reverts commit 9dc99a28236161a5a1b4c58f1e9c4ec6179cb976.
>> Aside from the other issues discussed on kvm-devel, this commit
>> breaks the PowerPC build. 
>> 
>> 
> 
> Applied, thanks.

Hollis, 
   Could you explain why this patch breaks the powerpc build?  qemu_sync_icache has the definition for non-ai64 case, so shoudn't break any arch-specific build.  
Xiantao

RE: [PATCH] Revert "Sync idcache after emualted DMA operations foria64"

[Hollis Blanchard]

On Mon, 2009-05-04 at 09:39 +0800, Zhang, Xiantao wrote:
>    Could you explain why this patch breaks the powerpc build?
> qemu_sync_icache has the definition for non-ai64 case, so shoudn't
> break any arch-specific build.  

cutils.o: In function `qemu_iovec_from_buffer':
/home/hollisb/source/qemu-kvm.git/cutils.c:175: undefined reference to
`qemu_cache_conf'
/home/hollisb/source/qemu-kvm.git/cutils.c:171: undefined reference to
`qemu_cache_conf'
cutils.o: In function `qemu_iovec_from_buffer':
/home/hollisb/source/qemu-kvm.git/cache-utils.h:18: undefined reference
to `qemu_cache_conf'

However, to restate my point: the build error is not the biggest problem
with this patch. The bigger problems are all the other issues I've
repeatedly described. The build break is the icing on the cake.

-- 
Hollis Blanchard
IBM Linux Technology Center