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.
