See inline comments.
> -----Original Message-----
> From: [EMAIL PROTECTED]
> [mailto:[EMAIL PROTECTED] On Behalf Of
> [EMAIL PROTECTED]
> Sent: Tuesday, August 19, 2008 6:37 PM
> To: [email protected]
> Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]
> Subject: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3
>
> From: Christian Ehrhardt <[EMAIL PROTECTED]>
>
> 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 <[EMAIL PROTECTED]>
> ---
>
> [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 [EMAIL PROTECTED]
> 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
