* Re: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3
2008-08-19 10:36 [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3 ehrhardt
@ 2008-08-20 17:42 ` Hollis Blanchard
2008-08-21 13:19 ` Christian Ehrhardt
` (8 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Hollis Blanchard @ 2008-08-20 17:42 UTC (permalink / raw)
To: kvm-ppc
Some initial comments:
On Tue, 2008-08-19 at 12:36 +0200, ehrhardt@linux.vnet.ibm.com wrote:
> + 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 */
> + }
I think it's time we replaced these magic numbers (32, 36 above, but
also in the opcode switch statements) with named constants.
> +int kvmppc_pvmem_rewrite_instruction(struct kvm_vcpu *vcpu)
> +{
> ...
> + 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);
This suffers from the same kunmap_atomic() problem you pointed out to me
elsewhere in my own code. :) (You're passing pcpage to kunmap_atomic()
instead of pcaddr.)
--
Hollis Blanchard
IBM Linux Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3
2008-08-19 10:36 [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3 ehrhardt
2008-08-20 17:42 ` Hollis Blanchard
@ 2008-08-21 13:19 ` Christian Ehrhardt
2008-08-21 18:54 ` Hollis Blanchard
` (7 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Christian Ehrhardt @ 2008-08-21 13:19 UTC (permalink / raw)
To: kvm-ppc
Hollis Blanchard wrote:
> Some initial comments:
>
> On Tue, 2008-08-19 at 12:36 +0200, ehrhardt@linux.vnet.ibm.com wrote:
>
>> + 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 */
>> + }
>>
>
> I think it's time we replaced these magic numbers (32, 36 above, but
> also in the opcode switch statements) with named constants.
>
>
Yes, but I wanted to keep it in sync with the emulation switch code here.
Once we agree on this patch series it is easy to throw a numer->name
patch on top.
Or do you want me to put a renaming patch into the paravirtualization
queue now ?
>> +int kvmppc_pvmem_rewrite_instruction(struct kvm_vcpu *vcpu)
>> +{
>> ...
>> + 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);
>>
>
> This suffers from the same kunmap_atomic() problem you pointed out to me
> elsewhere in my own code. :) (You're passing pcpage to kunmap_atomic()
> instead of pcaddr.)
>
>
damn - stepped in my own trap ;-)
thanks for the hint - fixed
--
Grüsse / regards,
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3
2008-08-19 10:36 [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3 ehrhardt
2008-08-20 17:42 ` Hollis Blanchard
2008-08-21 13:19 ` Christian Ehrhardt
@ 2008-08-21 18:54 ` Hollis Blanchard
2008-08-29 2:20 ` Liu Yu
` (6 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Hollis Blanchard @ 2008-08-21 18:54 UTC (permalink / raw)
To: kvm-ppc
On Thu, 2008-08-21 at 15:19 +0200, Christian Ehrhardt wrote:
> Hollis Blanchard wrote:
> > Some initial comments:
> >
> > On Tue, 2008-08-19 at 12:36 +0200,
> ehrhardt@linux.vnet.ibm.com wrote:
> >
> >> + 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 */
> >> + }
> >>
> >
> > I think it's time we replaced these magic numbers (32, 36 above, but
> > also in the opcode switch statements) with named constants.
> >
> >
> Yes, but I wanted to keep it in sync with the emulation switch code
> here.
> Once we agree on this patch series it is easy to throw a numer->name
> patch on top.
> Or do you want me to put a renaming patch into the paravirtualization
> queue now ?
In general I think the philosophy should be to commit patches that we
all agree on *first*, and then patches that need more thought/discussion
later. That way we don't need to wait for consensus on all outstanding
patches before committing any of them.
--
Hollis Blanchard
IBM Linux Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread* RE: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3
2008-08-19 10:36 [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3 ehrhardt
` (2 preceding siblings ...)
2008-08-21 18:54 ` Hollis Blanchard
@ 2008-08-29 2:20 ` Liu Yu
2008-08-29 2:58 ` Liu Yu
` (5 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Liu Yu @ 2008-08-29 2:20 UTC (permalink / raw)
To: kvm-ppc
See inline comments.
> -----Original Message-----
> From: kvm-ppc-owner@vger.kernel.org
> [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of
> ehrhardt@linux.vnet.ibm.com
> Sent: Tuesday, August 19, 2008 6:37 PM
> To: kvm-ppc@vger.kernel.org
> Cc: hollisb@us.ibm.com; ehrhardt@linux.vnet.ibm.com
> Subject: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3
>
> From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
>
> 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@linux.vnet.ibm.com>
> ---
>
> [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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 11+ messages in thread* RE: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3
2008-08-19 10:36 [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3 ehrhardt
` (3 preceding siblings ...)
2008-08-29 2:20 ` Liu Yu
@ 2008-08-29 2:58 ` Liu Yu
2008-09-01 12:15 ` Christian Ehrhardt
` (4 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Liu Yu @ 2008-08-29 2:58 UTC (permalink / raw)
To: kvm-ppc
> -----Original Message-----
> From: kvm-ppc-owner@vger.kernel.org
> [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of
> ehrhardt@linux.vnet.ibm.com
> Sent: Tuesday, August 19, 2008 6:37 PM
> To: kvm-ppc@vger.kernel.org
> Cc: hollisb@us.ibm.com; ehrhardt@linux.vnet.ibm.com
> Subject: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3
>
> From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
>
> 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@linux.vnet.ibm.com>
> ---
>
> [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;
> +
> + 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);
> + }
> +
> + 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;
>
> 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
Maintaining copies is painful.
Is there any method to use the sole storage?
For exampe, allocate a page beforehand, and initialize kvm_arch_vcpu on
the page.
After guest request para support, then map this page to guest.
>
> 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3
2008-08-19 10:36 [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3 ehrhardt
` (4 preceding siblings ...)
2008-08-29 2:58 ` Liu Yu
@ 2008-09-01 12:15 ` Christian Ehrhardt
2008-09-01 12:26 ` Christian Ehrhardt
` (3 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Christian Ehrhardt @ 2008-09-01 12:15 UTC (permalink / raw)
To: kvm-ppc
Liu Yu wrote:
> See inline comments.
>
>
>> -----Original Message-----
>> From: kvm-ppc-owner@vger.kernel.org
>> [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of
>> ehrhardt@linux.vnet.ibm.com
>> Sent: Tuesday, August 19, 2008 6:37 PM
>> To: kvm-ppc@vger.kernel.org
>> Cc: hollisb@us.ibm.com; ehrhardt@linux.vnet.ibm.com
>> Subject: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3
>>
>> From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
>>
>> 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@linux.vnet.ibm.com>
>> ---
>>
>> [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() ?
>
>
yes, thanks
>> +
>> + 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.
>
>> +
>> + 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).
thanks, I'll check that out.
>
>>
>> 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@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
--
Grüsse / regards,
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3
2008-08-19 10:36 [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3 ehrhardt
` (5 preceding siblings ...)
2008-09-01 12:15 ` Christian Ehrhardt
@ 2008-09-01 12:26 ` Christian Ehrhardt
2008-09-02 3:02 ` Liu Yu-B13201
` (2 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Christian Ehrhardt @ 2008-09-01 12:26 UTC (permalink / raw)
To: kvm-ppc
Liu Yu wrote:
>> -----Original Message-----
>> From: kvm-ppc-owner@vger.kernel.org
>> [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of
>> ehrhardt@linux.vnet.ibm.com
>> Sent: Tuesday, August 19, 2008 6:37 PM
>> To: kvm-ppc@vger.kernel.org
>> Cc: hollisb@us.ibm.com; ehrhardt@linux.vnet.ibm.com
>> Subject: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3
>>
>> From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
>>
>> 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@linux.vnet.ibm.com>
>> ---
>>
>> [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;
>> +
>> + 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);
>> + }
>> +
>> + 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;
>>
>> 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
>>
>
> Maintaining copies is painful.
> Is there any method to use the sole storage?
>
> For exampe, allocate a page beforehand, and initialize kvm_arch_vcpu on
> the page.
> After guest request para support, then map this page to guest.
>
We discussed that in the past and came out that guest allocation of that
memory is really preferable.
Allocating an additional piece just for that thing that is only
optionally used creates a lot of pain coding it.
You would also need to split the vcpu.arch structure for that to only
map the accessible part to keep security e.g. for effective msr, ...
There are just a lot of little issues and corner cases if we would share
that memory, which I believe in the end might be more complicated in
code&runtime complexity than just keeping a few pv copies.
For example once changed sprg0-3 is only held in pvmem and not
interesting to the host in any way. And esr, dear, ... are only changed
and synced to pvmem if we are on the irq delivery path anyway.
Feel free to catch us up on #kvm on freenode (You, I & Hollis) if you
want to discuss it in detail (nick = paelzer).
>>
>> 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@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
--
Grüsse / regards,
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization
^ permalink raw reply [flat|nested] 11+ messages in thread* RE: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3
2008-08-19 10:36 [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3 ehrhardt
` (6 preceding siblings ...)
2008-09-01 12:26 ` Christian Ehrhardt
@ 2008-09-02 3:02 ` Liu Yu-B13201
2008-09-02 7:30 ` Christian Ehrhardt
2008-09-16 6:27 ` ehrhardt
9 siblings, 0 replies; 11+ messages in thread
From: Liu Yu-B13201 @ 2008-09-02 3:02 UTC (permalink / raw)
To: kvm-ppc
> -----Original Message-----
> From: Christian Ehrhardt [mailto:ehrhardt@linux.vnet.ibm.com]
> Sent: Monday, September 01, 2008 8:15 PM
> To: Liu Yu-B13201
> Cc: kvm-ppc@vger.kernel.org; hollisb@us.ibm.com
> 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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3
2008-08-19 10:36 [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3 ehrhardt
` (7 preceding siblings ...)
2008-09-02 3:02 ` Liu Yu-B13201
@ 2008-09-02 7:30 ` Christian Ehrhardt
2008-09-16 6:27 ` ehrhardt
9 siblings, 0 replies; 11+ messages in thread
From: Christian Ehrhardt @ 2008-09-02 7:30 UTC (permalink / raw)
To: kvm-ppc
Liu Yu-B13201 wrote:
>> -----Original Message-----
>> From: Christian Ehrhardt [mailto:ehrhardt@linux.vnet.ibm.com]
>> Sent: Monday, September 01, 2008 8:15 PM
>> To: Liu Yu-B13201
>> Cc: kvm-ppc@vger.kernel.org; hollisb@us.ibm.com
>> 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?
>
>
-> asm/system.h:521
>>>
>>>
>>>> +
>>>> + 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.
>
>
Sure, I think I already got your point. This is what I described with
"emulate & rewrite it" allowing us to put the
kvmppc_pvmem_rewrite_instruction() at the end guarded by an additional
if that will eventually reduce the overhead to nearly zero once the
trapping code is rewritten.
The code flow I have in mind is that:
emulate instruction
if this is a rewritable one set flag
increase program counter because instruction is emulated
if unlikely(rewritable) flag is set and kvm_has_pvmem call
kvmppc_pvmem_rewrite_instruction
-> The next time guest executes that code it will already use the new
rewritten instruction
-> non rewritable instructions that are emulated don't set the flag and
therefore don't call kvmppc_pvmem_rewrite_instruction saving overhead
I'll submit an updated patch series somewhen this week. Then you can
check if I got your hint right :-)
--
Grüsse / regards,
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3
2008-08-19 10:36 [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3 ehrhardt
` (8 preceding siblings ...)
2008-09-02 7:30 ` Christian Ehrhardt
@ 2008-09-16 6:27 ` ehrhardt
9 siblings, 0 replies; 11+ messages in thread
From: ehrhardt @ 2008-09-16 6:27 UTC (permalink / raw)
To: kvm-ppc
From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
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
accessed by the guest.
For speed optimization the rewriting is done only if normal emulation set a
rewritable flag, saving unneccesary calls and checks for every emulation. On the
other hand this implies that the normal emulation was done with out of sync
(non-pv) values. Therefor a second emulation based on pv values is called
prior to the instruction being rewritten. This simplifies the frequent path of
normal instruction emulation by increasing the path when an instruction is
rewritten which is only executed once per rewritable instruction in the guest
code.
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@linux.vnet.ibm.com>
---
[diffstat]
arch/powerpc/kvm/booke_guest.c | 14 ++
arch/powerpc/kvm/emulate.c | 279 +++++++++++++++++++++++++++++++++++++++--
include/asm-powerpc/kvm_para.h | 25 +++
3 files changed, 310 insertions(+), 8 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
@@ -544,6 +544,13 @@
for (i = 0; i < ARRAY_SIZE(regs->gpr); i++)
regs->gpr[i] = vcpu->arch.gpr[i];
+ if (kvmppc_has_pvmem(vcpu)) {
+ 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;
}
@@ -569,6 +576,13 @@
for (i = 0; i < ARRAY_SIZE(vcpu->arch.gpr); i++)
vcpu->arch.gpr[i] = regs->gpr[i];
+
+ if (kvmppc_has_pvmem(vcpu)) {
+ 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,11 +244,25 @@
switch (vcpu->arch.gpr[11]) {
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];
pvmem_page = gfn_to_page(vcpu->kvm,
vcpu->arch.pvmem_gpaddr >> KVM_PPCPV_MAGIC_PAGE_SHIFT);
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[11]);
@@ -226,6 +272,200 @@
vcpu->arch.gpr[3] = ret;
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);
+ if (!gtlbe)
+ return -EFAULT;
+ guest_pcaddr = tlb_xlate(gtlbe, vcpu->arch.pc);
+
+ 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(pcmem, KM_USER0);
+ }
+
+ return err;
+}
+
+/*
+ * This is the last emulation before the instruction is rewritten using the
+ * magic page. At this point the correct data is already in the paravirtual
+ * magic page so the result of "normal" emulation would be out of sync.
+ * This simplifies the code of non pv emulation but is only valid as long
+ * as the paravirtualized instructions have no side effects.
+ * valid example: "GPR[RT] <- value of sprn", this can be done normal + pv
+ * style afterwards and eventually has the correct (pv) value.
+ */
+int kvmppc_emulate_instruction_pv(struct kvm_vcpu *vcpu)
+{
+ u32 inst = vcpu->arch.last_inst;
+ int rs;
+ int rt;
+ int sprn;
+ enum emulation_result emulated = EMULATE_DONE;
+
+ switch (get_op(inst)) {
+ case 31:
+ switch (get_xop(inst)) {
+ case 339: /* mfspr */
+ sprn = get_sprn(inst);
+ rt = get_rt(inst);
+ switch (sprn) {
+ case SPRN_SPRG0:
+ vcpu->arch.gpr[rt] = kvmppc_get_pvreg(vcpu,
+ KVM_PPCPV_OFFSET_SPRG0);
+ break;
+ case SPRN_SPRG1:
+ vcpu->arch.gpr[rt] = kvmppc_get_pvreg(vcpu,
+ KVM_PPCPV_OFFSET_SPRG1);
+ break;
+ case SPRN_SPRG2:
+ vcpu->arch.gpr[rt] = kvmppc_get_pvreg(vcpu,
+ KVM_PPCPV_OFFSET_SPRG2);
+ break;
+ case SPRN_SPRG3:
+ vcpu->arch.gpr[rt] = kvmppc_get_pvreg(vcpu,
+ KVM_PPCPV_OFFSET_SPRG3);
+ break;
+ default:
+ printk(KERN_ERR"mfspr: unknown spr %x\n", sprn);
+ vcpu->arch.gpr[rt] = 0;
+ break;
+ }
+ break;
+
+ case 467: /* mtspr */
+ sprn = get_sprn(inst);
+ rs = get_rs(inst);
+ switch (sprn) {
+ case SPRN_SPRG0:
+ kvmppc_set_pvreg(vcpu, KVM_PPCPV_OFFSET_SPRG0,
+ vcpu->arch.gpr[rs]);
+ break;
+ case SPRN_SPRG1:
+ kvmppc_set_pvreg(vcpu, KVM_PPCPV_OFFSET_SPRG1,
+ vcpu->arch.gpr[rs]);
+ break;
+ case SPRN_SPRG2:
+ kvmppc_set_pvreg(vcpu, KVM_PPCPV_OFFSET_SPRG2,
+ vcpu->arch.gpr[rs]);
+ break;
+ case SPRN_SPRG3:
+ kvmppc_set_pvreg(vcpu, KVM_PPCPV_OFFSET_SPRG3,
+ vcpu->arch.gpr[rs]);
+ break;
+ default:
+ printk(KERN_ERR"mtspr: unknown spr %x\n", sprn);
+ emulated = EMULATE_FAIL;
+ break;
+ }
+ break;
+ }
+ break;
+ default:
+ printk(KERN_ERR"unknown op %d\n", get_op(inst));
+ emulated = EMULATE_FAIL;
+ break;
+ }
+
+ return emulated;
}
/* XXX to do:
@@ -255,6 +495,7 @@
int dcrn;
enum emulation_result emulated = EMULATE_DONE;
int advance = 1;
+ int rewritable = 0;
switch (get_op(inst)) {
case 3: /* trap */
@@ -432,13 +673,21 @@
vcpu->arch.gpr[rt] = mftbu(); break;
case SPRN_SPRG0:
- vcpu->arch.gpr[rt] = vcpu->arch.sprg0; break;
+ vcpu->arch.gpr[rt] = vcpu->arch.sprg0;
+ rewritable = 1;
+ break;
case SPRN_SPRG1:
- vcpu->arch.gpr[rt] = vcpu->arch.sprg1; break;
+ vcpu->arch.gpr[rt] = vcpu->arch.sprg1;
+ rewritable = 1;
+ break;
case SPRN_SPRG2:
- vcpu->arch.gpr[rt] = vcpu->arch.sprg2; break;
+ vcpu->arch.gpr[rt] = vcpu->arch.sprg2;
+ rewritable = 1;
+ break;
case SPRN_SPRG3:
- vcpu->arch.gpr[rt] = vcpu->arch.sprg3; break;
+ vcpu->arch.gpr[rt] = vcpu->arch.sprg3;
+ rewritable = 1;
+ break;
/* Note: SPRG4-7 are user-readable, so we don't get
* a trap. */
@@ -570,13 +819,21 @@
break;
case SPRN_SPRG0:
- vcpu->arch.sprg0 = vcpu->arch.gpr[rs]; break;
+ vcpu->arch.sprg0 = vcpu->arch.gpr[rs];
+ rewritable = 1;
+ break;
case SPRN_SPRG1:
- vcpu->arch.sprg1 = vcpu->arch.gpr[rs]; break;
+ vcpu->arch.sprg1 = vcpu->arch.gpr[rs];
+ rewritable = 1;
+ break;
case SPRN_SPRG2:
- vcpu->arch.sprg2 = vcpu->arch.gpr[rs]; break;
+ vcpu->arch.sprg2 = vcpu->arch.gpr[rs];
+ rewritable = 1;
+ break;
case SPRN_SPRG3:
- vcpu->arch.sprg3 = vcpu->arch.gpr[rs]; break;
+ vcpu->arch.sprg3 = vcpu->arch.gpr[rs];
+ rewritable = 1;
+ break;
/* Note: SPRG4-7 are user-readable. These values are
* loaded into the real SPRGs when resuming the
@@ -800,6 +1057,12 @@
KVMTRACE_3D(PPC_INSTR, vcpu, inst, vcpu->arch.pc, emulated, entryexit);
+ if (unlikely(rewritable && kvmppc_has_pvmem(vcpu))) {
+ /* last emul has to be done with the pv values */
+ emulated = kvmppc_emulate_instruction_pv(vcpu);
+ kvmppc_pvmem_rewrite_instruction(vcpu);
+ }
+
if (advance)
vcpu->arch.pc += 4; /* Advance past emulated instruction. */
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)
{
@@ -60,6 +69,22 @@
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__ */
#endif /* __POWERPC_KVM_PARA_H__ */
^ permalink raw reply [flat|nested] 11+ messages in thread