> -----Original Message----- > From: Christian Ehrhardt [mailto:ehrhardt@xxxxxxxxxxxxxxxxxx] > Sent: Monday, September 01, 2008 8:15 PM > To: Liu Yu-B13201 > Cc: kvm-ppc@xxxxxxxxxxxxxxx; hollisb@xxxxxxxxxx > Subject: Re: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3 > > >> + > >> + down_read(¤t->mm->mmap_sem); > >> + pcpage = gfn_to_page(vcpu->kvm, > >> + guest_pcaddr >> > >> KVM_PPCPV_MAGIC_PAGE_SHIFT); > >> + up_read(¤t->mm->mmap_sem); > >> + if (pcpage == bad_page) > >> + return -EFAULT; > >> + > >> + pcmem = kmap_atomic(pcpage, KM_USER0); > >> + if (!pcmem) > >> + return -EFAULT; > >> + pcaddr = ((unsigned long)pcmem > >> + | (vcpu->arch.pc & > >> KVM_PPCPV_MAGIC_PAGE_MASK)); > >> + BUG_ON(inst != *((u32 *)pcaddr)); > >> + create_instruction(pcaddr, newinst); > >> + kunmap_atomic(pcpage, KM_USER0); > >> + } > >> > > > > I think you are writing new instruction back here. > > Why not use kvm_write_guest_page() (kvm_main.c) directly? > > > > hmm I'm exchaning instructions in memory and the "create_instruction" > function I use is exactly doing that. > I expect that the kvm function would miss the needed sync > functionality > to ensure that all old code is dropped from the cpu caches. > Well I'm not > an expert in that, I need to look into that. > It seems there wouldn't be cache consistency problem for PowerPC as long as the memory is updated by processor. Btw, where is the definition of create_instruction? > > > >> + > >> + return err; > >> +} > >> > >> /* XXX to do: > >> * lhax > >> @@ -260,6 +415,9 @@ > >> int dcrn; > >> enum emulation_result emulated = EMULATE_DONE; > >> int advance = 1; > >> + > >> + if (kvmppc_has_pvmem(vcpu) && > >> !kvmppc_pvmem_rewrite_instruction(vcpu)) > >> + return EMULATE_DONE; > >> > > > > Rewirting instruction is one-off, so it's not that important to pay > > attention to performance. > > However, putting rewriting at the beginning of the function > add extra > > workload to other emulations. > > > > How about this way? > > > > int rewrite = 0; > > > > ....... > > case mfspr0: > > rewrite = 1; > > ...... > > > > // at the end of the function > > if(unlikely(rewrite) && kvmppc_has_pvmem(vcpu)) > > kvmppc_pvmem_rewrite_instruction(vcpu); > > > > > This would spread checks with kvmppc_has_pvmem all over the code, > because if we rewrite the instruction it is not allowed to emulate it. > On the other hand we could emulate it and change the instruction for > their "next" hit (not jump back to current pc but at pc+4 as it is > emulated). > Yeah that could work, and once most rewrite actions are done the > "unlikely" is the right thing because later on we won't hit > it anymore. > That should speed up emulation a bit (not much compared to exit/enter > guest, but it's never wrong to code things performance wise). > In fact, not only the unlikely thing, I am worried about that putting kvmppc_pvmem_rewrite_instruction() at beginning will cosume some time for normal emulations, ( a lot comparisons and jumps) especially more and more instructions would be replaced in it in the future. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
