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

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

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(&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.
>>
>>     
>
> 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