See inline comments. > -----Original Message----- > From: kvm-ppc-owner@xxxxxxxxxxxxxxx > [mailto:kvm-ppc-owner@xxxxxxxxxxxxxxx] On Behalf Of > ehrhardt@xxxxxxxxxxxxxxxxxx > Sent: Tuesday, August 19, 2008 6:37 PM > To: kvm-ppc@xxxxxxxxxxxxxxx > Cc: hollisb@xxxxxxxxxx; ehrhardt@xxxxxxxxxxxxxxxxxx > Subject: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3 > > From: Christian Ehrhardt <ehrhardt@xxxxxxxxxxxxxxxxxx> > > This patch adds the functionality of rewriting guest code > using the magic page > mechanism. If a paravirtual guest memory (magic page) is > registered the host > rewrites trapping emulations instead of emulating them. This > only works with > instructions that can be rewritten with one instruction. > On registration of the paravirtual memory the values have to > be synced so that > the old emulated values can be found in the magic page now > and are ready to be > read by the guest. > The rewriting of guest code is guest visible (the guest > already agrees on this > by registering a magic page) and is driven by the program > interrupts usually > used to emulate these instructions in the hypervisor. > This patch replaces m[ft]spr SPRG[0-3] instructions with stw > (write) and lwz > (read) instructions. > > Signed-off-by: Christian Ehrhardt <ehrhardt@xxxxxxxxxxxxxxxxxx> > --- > > [diffstat] > arch/powerpc/kvm/booke_guest.c | 17 ++++ > arch/powerpc/kvm/emulate.c | 158 > +++++++++++++++++++++++++++++++++++++++++ > include/asm-powerpc/kvm_para.h | 27 ++++++- > 3 files changed, 200 insertions(+), 2 deletions(-) > > [diff] > diff --git a/arch/powerpc/kvm/booke_guest.c > b/arch/powerpc/kvm/booke_guest.c > --- a/arch/powerpc/kvm/booke_guest.c > +++ b/arch/powerpc/kvm/booke_guest.c > @@ -339,7 +339,7 @@ > gfn_t gfn; > > > - if (vcpu->arch.pvmem && kvmppc_is_pvmem(vcpu, eaddr)) { > + if (kvmppc_is_pvmem(vcpu, eaddr)) { > kvmppc_mmu_map(vcpu, eaddr, > vcpu->arch.pvmem_gpaddr >> > KVM_PPCPV_MAGIC_PAGE_SHIFT, > 0, KVM_PPCPV_MAGIC_PAGE_FLAGS); > @@ -536,6 +536,13 @@ > for (i = 0; i < ARRAY_SIZE(regs->gpr); i++) > regs->gpr[i] = vcpu->arch.gpr[i]; > > + if (vcpu->arch.pvmem) { > + regs->sprg0 = kvmppc_get_pvreg(vcpu, > KVM_PPCPV_OFFSET_SPRG0); > + regs->sprg1 = kvmppc_get_pvreg(vcpu, > KVM_PPCPV_OFFSET_SPRG1); > + regs->sprg2 = kvmppc_get_pvreg(vcpu, > KVM_PPCPV_OFFSET_SPRG2); > + regs->sprg3 = kvmppc_get_pvreg(vcpu, > KVM_PPCPV_OFFSET_SPRG3); > + } > + > return 0; > } > > @@ -561,6 +568,14 @@ > > for (i = 0; i < ARRAY_SIZE(vcpu->arch.gpr); i++) > vcpu->arch.gpr[i] = regs->gpr[i]; > + > + if (vcpu->arch.pvmem) { > + kvmppc_set_pvreg(vcpu, KVM_PPCPV_OFFSET_SPRG0, > regs->sprg0); > + kvmppc_set_pvreg(vcpu, KVM_PPCPV_OFFSET_SPRG1, > regs->sprg1); > + kvmppc_set_pvreg(vcpu, KVM_PPCPV_OFFSET_SPRG2, > regs->sprg2); > + kvmppc_set_pvreg(vcpu, KVM_PPCPV_OFFSET_SPRG3, > regs->sprg3); > + } > + > > return 0; > } > diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c > --- a/arch/powerpc/kvm/emulate.c > +++ b/arch/powerpc/kvm/emulate.c > @@ -29,6 +29,7 @@ > #include <asm/dcr-regs.h> > #include <asm/time.h> > #include <asm/byteorder.h> > +#include <asm/system.h> > #include <asm/kvm_ppc.h> > > #include "44x_tlb.h" > @@ -87,6 +88,37 @@ > static inline unsigned int get_d(u32 inst) > { > return inst & 0xffff; > +} > + > +static inline void set_op(u32 op, u32 *inst) > +{ > + *inst |= ((op & 0x3f) << 26); > + return; > +} > + > +static inline void set_ra(u32 ra, u32 *inst) > +{ > + *inst |= ((ra & 0x1f) << 16); > + return; > +} > + > +static inline void set_rs(u32 rs, u32 *inst) > +{ > + *inst |= ((rs & 0x1f) << 21); > + return; > +} > + > +static inline void set_rt(u32 rt, u32 *inst) > +{ > + *inst |= ((rt & 0x1f) << 21); > + return; > +} > + > +static inline void set_d(s16 d, u32 *inst) > +{ > + /* mask in last 16 signed bits */ > + *inst |= ((u32)d & 0xFFFF); > + return; > } > > static int tlbe_is_host_safe(const struct kvm_vcpu *vcpu, > @@ -212,6 +244,8 @@ > > switch (vcpu->arch.gpr[0]) { > case KVM_HCALL_RESERVE_MAGICPAGE: > + /* FIXME assert : we don't support this on smp guests */ > + > vcpu->arch.pvmem_gvaddr = vcpu->arch.gpr[3]; > vcpu->arch.pvmem_gpaddr = vcpu->arch.gpr[4]; > down_read(¤t->mm->mmap_sem); > @@ -219,6 +253,18 @@ > vcpu->arch.pvmem_gpaddr >> > KVM_PPCPV_MAGIC_PAGE_SHIFT); > up_read(¤t->mm->mmap_sem); > vcpu->arch.pvmem = kmap(pvmem_page); > + > + /* on enablement of pvmem support we need to > sync all values > + * into the pvmem area to be able so correcty > fulfill read > + * acesses that might occur prior to writes */ > + kvmppc_set_pvreg(vcpu, KVM_PPCPV_OFFSET_SPRG0, > + vcpu->arch.sprg0); > + kvmppc_set_pvreg(vcpu, KVM_PPCPV_OFFSET_SPRG1, > + vcpu->arch.sprg1); > + kvmppc_set_pvreg(vcpu, KVM_PPCPV_OFFSET_SPRG2, > + vcpu->arch.sprg2); > + kvmppc_set_pvreg(vcpu, KVM_PPCPV_OFFSET_SPRG3, > + vcpu->arch.sprg3); > break; > default: > printk(KERN_ERR "unknown hypercall %d\n", > vcpu->arch.gpr[0]); > @@ -232,6 +278,115 @@ > return ret; > } > > +int kvmppc_pvmem_rewrite_instruction(struct kvm_vcpu *vcpu) > +{ > + int err = 0; > + u32 inst = vcpu->arch.last_inst; > + int sprn; > + int rw = -1; > + > + int offset = -1; > + > + switch (get_op(inst)) { > + case 31: > + switch (get_xop(inst)) { > + case 339: > /* mfspr */ > + sprn = get_sprn(inst); > + rw = KVM_PPCPV_PVMEM_READ; > + switch (sprn) { > + case SPRN_SPRG0: > + offset = KVM_PPCPV_OFFSET_SPRG0; > + break; > + case SPRN_SPRG1: > + offset = KVM_PPCPV_OFFSET_SPRG1; > + break; > + case SPRN_SPRG2: > + offset = KVM_PPCPV_OFFSET_SPRG2; > + break; > + case SPRN_SPRG3: > + offset = KVM_PPCPV_OFFSET_SPRG3; > + break; > + default: > + err = -EFAULT; > + } > + break; > + case 467: > /* mtspr */ > + sprn = get_sprn(inst); > + rw = KVM_PPCPV_PVMEM_WRITE; > + switch (sprn) { > + case SPRN_SPRG0: > + offset = KVM_PPCPV_OFFSET_SPRG0; > + break; > + case SPRN_SPRG1: > + offset = KVM_PPCPV_OFFSET_SPRG1; > + break; > + case SPRN_SPRG2: > + offset = KVM_PPCPV_OFFSET_SPRG2; > + break; > + case SPRN_SPRG3: > + offset = KVM_PPCPV_OFFSET_SPRG3; > + break; > + default: > + err = -EFAULT; > + } > + break; > + default: > + err = -EFAULT; > + } > + break; > + default: > + err = -EFAULT; > + } > + > + if (!err) { > + u32 newinst = 0; > + struct page *pcpage; > + void *pcmem; > + unsigned long pcaddr; > + s16 displacement = 0; > + gpa_t guest_pcaddr; > + struct tlbe *gtlbe; > + > + BUG_ON(offset == -1); > + /* calculate displacement of gvaddr+offset from 0 */ > + displacement += (vcpu->arch.pvmem_gvaddr & 0xFFFF); > + displacement += offset; > + > + BUG_ON(rw == -1); > + if (rw == KVM_PPCPV_PVMEM_READ) { > + set_op(32, &newinst); /* lwz */ > + set_rt(get_rt(inst), &newinst); /* set > original rt */ > + } else { > + set_op(36, &newinst); /* stw */ > + set_rs(get_rs(inst), &newinst); /* set > original rs */ > + } > + set_ra(0, &newinst); /* ra = 0 -> base addr 0 > (no reg needed) */ > + set_d(displacement, &newinst); /* set > displacement from 0 */ > + > + gtlbe = kvmppc_44x_itlb_search(vcpu, vcpu->arch.pc); > + guest_pcaddr = tlb_xlate(gtlbe, vcpu->arch.pc); > + if (!gtlbe) > + return -EFAULT; Put this condition statement behind kvmppc_44x_itlb_search() ? > + > + 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? > + > + 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); > > switch (get_op(inst)) { > case 0: > diff --git a/include/asm-powerpc/kvm_para.h > b/include/asm-powerpc/kvm_para.h > --- a/include/asm-powerpc/kvm_para.h > +++ b/include/asm-powerpc/kvm_para.h > @@ -34,7 +34,16 @@ > */ > #define KVM_PPCPV_MAGIC_PAGE_SIZE 4096 > #define KVM_PPCPV_MAGIC_PAGE_SHIFT 12 > +#define KVM_PPCPV_MAGIC_PAGE_MASK 0xFFF > #define KVM_PPCPV_MAGIC_PAGE_FLAGS 0x3f > + > +#define KVM_PPCPV_PVMEM_READ 1 > +#define KVM_PPCPV_PVMEM_WRITE 2 > + > +#define KVM_PPCPV_OFFSET_SPRG0 0x00 > +#define KVM_PPCPV_OFFSET_SPRG1 0x04 > +#define KVM_PPCPV_OFFSET_SPRG2 0x08 > +#define KVM_PPCPV_OFFSET_SPRG3 0x0C > > static inline int kvm_para_available(void) > { > @@ -55,7 +64,23 @@ > > static inline int kvmppc_has_pvmem(struct kvm_vcpu *vcpu) > { > - return vcpu->arch.pvmem; > + return !!vcpu->arch.pvmem; > +} > + > +static inline u32 kvmppc_get_pvreg(struct kvm_vcpu *vcpu, int offset) > +{ > + u32 *pvreg; > + > + pvreg = vcpu->arch.pvmem + offset; > + return *pvreg; > +} > + > +static inline void kvmppc_set_pvreg(struct kvm_vcpu *vcpu, > int offset, u32 val) > +{ > + u32 *pvreg; > + > + pvreg = vcpu->arch.pvmem + offset; > + *pvreg = val; > } > > #endif /* __KERNEL__ */ > -- > 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 > -- 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
