Re: [PATCH 2/6] kvmppc: magic page hypercall - host part

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

Hollis Blanchard wrote:
> On Tue, 2008-08-19 at 12:36 +0200, ehrhardt@linux.vnet.ibm.com wrote:
>   
>> 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
>> @@ -21,6 +21,7 @@
>>  #include <linux/errno.h>
>>  #include <linux/err.h>
>>  #include <linux/kvm_host.h>
>> +#include <linux/kvm_para.h>
>>  #include <linux/module.h>
>>  #include <linux/vmalloc.h>
>>  #include <linux/fs.h>
>> @@ -43,6 +44,7 @@
>>  	{ "itlb_v",     VCPU_STAT(itlb_virt_miss_exits) },
>>  	{ "dtlb_r",     VCPU_STAT(dtlb_real_miss_exits) },
>>  	{ "dtlb_v",     VCPU_STAT(dtlb_virt_miss_exits) },
>> +	{ "dtlb_pv",    VCPU_STAT(dtlb_pvmem_miss_exits) },
>>  	{ "sysc",       VCPU_STAT(syscall_exits) },
>>  	{ "isi",        VCPU_STAT(isi_exits) },
>>  	{ "dsi",        VCPU_STAT(dsi_exits) },
>> @@ -337,6 +339,16 @@
>>  		unsigned long eaddr = vcpu->arch.fault_dear;
>>  		gfn_t gfn;
>>
>> +
>> +		if (vcpu->arch.pvmem && 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);
>> +			vcpu->stat.dtlb_pvmem_miss_exits++;
>> +			r = RESUME_GUEST;
>> +			break;
>> +		}
>> +
>>  		/* Check the guest TLB. */
>>  		gtlbe = kvmppc_44x_dtlb_search(vcpu, eaddr);
>>  		if (!gtlbe) {
>>     
>
> By the way, when this code is running, what's the rate of this new
> counter? How does it compare to the reduction in instruction emulation?
>
>   
>> @@ -488,6 +500,8 @@
>>
>>  	vcpu->arch.shadow_pid = 1;
>>
>> +	vcpu->arch.pvmem = NULL;
>>     
>
> Isn't the whole structure initialized to 0? I don't think this is
> needed.
>
>   
Even if not necessary I would like to keep it. We might change arch 
allocation or something else.
This initialization doesn't hurt anyone and is not performance critical.
But it's up to you - if you want me to remove it I'll do that. please 
drop me a mail if I should do that.
*Like win popup - "do you really want to delete this line?" - yes/no/maybe
> [...]
>>  static void kvmppc_emul_rfi(struct kvm_vcpu *vcpu)
>> @@ -207,8 +208,18 @@
>>  static int kvmppc_do_hypercall(struct kvm_vcpu *vcpu)
>>  {
>>  	int ret = 0;
>> +	struct page *pvmem_page;
>>
>>  	switch (vcpu->arch.gpr[0]) {
>> +	case KVM_HCALL_RESERVE_MAGICPAGE:
>> +		vcpu->arch.pvmem_gvaddr = vcpu->arch.gpr[3];
>> +		vcpu->arch.pvmem_gpaddr = vcpu->arch.gpr[4];
>> +		down_read(&current->mm->mmap_sem);
>> +		pvmem_page = gfn_to_page(vcpu->kvm, 
>> +			vcpu->arch.pvmem_gpaddr >> KVM_PPCPV_MAGIC_PAGE_SHIFT);
>> +		up_read(&current->mm->mmap_sem);
>> +		vcpu->arch.pvmem = kmap(pvmem_page);
>> +		break;
>>  	default:
>>  		printk(KERN_ERR "unknown hypercall %d\n", vcpu->arch.gpr[0]);
>>  		kvmppc_dump_vcpu(vcpu);
>>     
>
> Where is vcpu->arch.pvmem unmapped?
>   
atm nowhere - it is persistent once it is registered
> What happens if the guest makes repeated KVM_HCALL_RESERVE_MAGICPAGE
> hypercalls? Looks like a good way to leak host memory.
>
> Also, if we migrate a guest which has a page registered, the new host
> won't have vcpu->arch.pvmem set because the guest doesn't re-invoke the
> KVM_HCALL_RESERVE_MAGICPAGE hypercall.
>   
yeah, but I stored gvaddr/gpaddr. If the migration keeps those (and it 
should or a lot other things break) we can fix that.
This is bringing me back to our discussion that I wanted to have a 
"migration starts to run on new host" hook.
I need to look for that.

> I think we need to put a little more thought into all the corner cases
> here.
>   
yeah, lets do that in a brainstorm session next week if you have some time.
It's better to clean corners interactively ;-)


-- 

Grüsse / regards, 
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization