Re: [RFC PATCH 2/2] kvm, x86: use ro page and don't copy shared page

[Avi Kivity <avi@redhat.com>]

On 06/09/2010 10:53 AM, Lai Jiangshan wrote:
> When page fault, we always call get_user_pages(write=1).
>
> Actually, we don't need to do this when it is not write fault.
> get_user_pages(write=1) will cause shared page(ksm) copied.
> If this page is not modified in future, this copying and the copied page
> are just wasted. Ksm may scan and merge them and may cause thrash.
>
> This patch is not for inclusion, because I know nothing about mmio
> and this patch includes a "workaround" which ensures mmio pfns
> are always writable in tdp_page_fault().
> The guest can't even boot up without this workaround.
>    

mmio pfns are used for device assignment.  These are host pfns that 
don't have a struct page, instead they belong to a device BAR.

I don't understand why you see a failure since they aren't even present 
on guests without assigned devices.

> @@ -2357,7 +2359,12 @@ static int tdp_page_fault(struct kvm_vcpu *vcpu, gva_t gpa,
>
>   	mmu_seq = vcpu->kvm->mmu_notifier_seq;
>   	smp_rmb();
> -	pfn = kvm_get_pfn_for_gfn(vcpu->kvm, gfn, 1);
> +	pfn = kvm_get_pfn_for_gfn(vcpu->kvm, gfn, error_code&  PFERR_WRITE_MASK);
>    

This is a pessimization, since now we may need two faults per page, one 
to page it in read-only and another to establish write access.

May not be so bad for tdp, but will surely reduce performance on shadow.

The way I think it should be improved, is to extend 
get_user_pages_fast() to also return the pte.  So now, if we get a page 
for read, but it happens to have a writeable/dirty pte, we can still 
allow write access in the spte.

> +	if (!(error_code&  PFERR_WRITE_MASK)&&  kvm_is_mmio_pfn(pfn)) {
> +		kvm_release_pfn_clean(pfn);
> +		/* I don't know why we have to ensure mmio pfns are always writable. */
> +		pfn = kvm_get_pfn_for_gfn(vcpu->kvm, gfn, 1);
> +	}
>    

Wierd.  For what gfn/pfns does this trigger?

In general this is a great optimization.

-- 
error compiling committee.c: too many arguments to function