Re: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3

[Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>]

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(&current->mm->mmap_sem);
>> @@ -219,6 +253,18 @@
>>  			vcpu->arch.pvmem_gpaddr >> 
>> KVM_PPCPV_MAGIC_PAGE_SHIFT);
>>  		up_read(&current->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(&current->mm->mmap_sem);
>> +		pcpage = gfn_to_page(vcpu->kvm,
>> +				guest_pcaddr >> 
>> KVM_PPCPV_MAGIC_PAGE_SHIFT);
>> +		up_read(&current->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