Re: [RFC PATCH 0/11] Rework gfn_to_pfn_cache

From: Paolo Bonzini <pbonzini@redhat.com>
To: David Woodhouse <dwmw2@infradead.org>,
	Sean Christopherson <seanjc@google.com>
Cc: kvm <kvm@vger.kernel.org>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	Joao Martins <joao.m.martins@oracle.com>,
	"jmattson@google.com" <jmattson@google.com>,
	"wanpengli@tencent.com" <wanpengli@tencent.com>,
	"vkuznets@redhat.com" <vkuznets@redhat.com>,
	"mtosatti@redhat.com" <mtosatti@redhat.com>,
	"joro@8bytes.org" <joro@8bytes.org>,
	karahmed@amazon.com
Subject: Re: [RFC PATCH 0/11] Rework gfn_to_pfn_cache
Date: Tue, 16 Nov 2021 16:49:27 +0100	[thread overview]
Message-ID: <02cdb0b0-c7b0-34c5-63c1-aec0e0b14cf7@redhat.com> (raw)
In-Reply-To: <126b7fcbfa78988b0fceb35f86588bd3d5aae837.camel@infradead.org>

On 11/16/21 16:09, David Woodhouse wrote:
> On Tue, 2021-11-16 at 15:57 +0100, Paolo Bonzini wrote:
>> This should not be needed, should it?  As long as the gfn-to-pfn
>> cache's vcpu field is handled properly, the request will just cause
>> the vCPU not to enter.
> 
> If the MMU mappings never change, the request never happens. But the
> memslots *can* change, so it does need to be revalidated each time
> through I think?

That needs to be done on KVM_SET_USER_MEMORY_REGION, using the same 
request (or even the same list walking code) as the MMU notifiers.

>> It would have to take the gpc->lock around
>> changes to gpc->vcpu though (meaning: it's probably best to add a
>> function gfn_to_pfn_cache_set_vcpu).
> 
> Hm, in my head that was never going to *change* for a given gpc; it
> *belongs* to that vCPU for ever (and was even part of vmx->nested. for
> that vCPU, to replace e.g. vmx->nested.pi_desc_map).

Ah okay, I thought it would be set in nested vmentry and cleared in 
nested vmexit.

> +static void nested_vmx_check_guest_maps(struct kvm_vcpu *vcpu)
> +{
> +	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +	struct gfn_to_pfn_cache *gpc;
> +
> +	bool valid;
> +
> +	if (nested_cpu_has_posted_intr(vmcs12)) {
> +		gpc = &vmx->nested.pi_desc_cache;
> +
> +		read_lock(&gpc->lock);
> +		valid = kvm_gfn_to_pfn_cache_check(vcpu->kvm, gpc,
> +						   vmcs12->posted_intr_desc_addr,
> +						   PAGE_SIZE);
> +		read_unlock(&gpc->lock);
> +		if (!valid) {
> +			/* XX: This isn't idempotent. Make it so, or use a different
> +			 * req for the 'refresh'. */
> +			kvm_make_request(KVM_REQ_GET_NESTED_STATE_PAGES, vcpu);
> +			return;
> +		}
> +	}
> +}

That's really slow to do on every vmentry.

> So nested_get_vmcs12_pages() certainly isn't idempotent right now
> because of all the kvm_vcpu_map() calls, which would just end up
> leaking — but I suppose the point is to kill all those, and then maybe
> it will be?

Yes, exactly.  That might be a larger than normal patch, but it should 
not be one too hard to review.  Once there's something that works, we 
can think of how to split (if it's worth it).

Paolo

> I quite liked the idea of *not* refreshing the caches immediately,m
> because we can wait until the vCPU is in L2 mode again and actually
> *needs* them.
>   
> 