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@xxxxxxxxx> >>>>>> >>>>>> 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@xxxxxxxxx> >>>>>> Signed-off-by: Avi Kivity <avi@xxxxxxxxxx> >>>>>> >>>>> >>>>> 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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
