Re: [RFC PATCH 0/11] Rework gfn_to_pfn_cache

From: Paolo Bonzini <bonzini@gnu.org>
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 19:46:03 +0100	[thread overview]
Message-ID: <cfa1a13d-5ad2-3ca4-147e-84273ffa2f38@gnu.org> (raw)
In-Reply-To: <7bcb9dafa55c283f9f9d0b841f4a53f0b6b3286d.camel@infradead.org>

On 11/16/21 18:57, David Woodhouse wrote:
>>> +		read_lock(&gpc->lock);
>>> +		if (!kvm_gfn_to_pfn_cache_check(vcpu->kvm, gpc, gpc->gpa, PAGE_SIZE)) {
>>> +			read_unlock(&gpc->lock);
>>>    			goto mmio_needed;
>>> +		}
>>> +
>>> +		vapic_page = gpc->khva;
>> If we know this gpc is of the synchronous kind, I think we can skip the
>> read_lock/read_unlock here?!?
> Er... this one was OUTSIDE_GUEST_MODE and is the atomic kind, which
> means it needs to hold the lock for the duration of the access in order
> to prevent (preemption and) racing with the invalidate?
> 
> It's the IN_GUEST_MODE one (in my check_guest_maps()) where we might
> get away without the lock, perhaps?

Ah, this is check_nested_events which is mostly IN_GUEST_MODE but not 
always (and that sucks for other reasons).  I'll think a bit more about 
it when I actually do the work.

>>>    		__kvm_apic_update_irr(vmx->nested.pi_desc->pir,
>>>    			vapic_page, &max_irr);
>>> @@ -3749,6 +3783,7 @@ static int vmx_complete_nested_posted_interrupt(struct kvm_vcpu *vcpu)
>>>    			status |= (u8)max_irr;
>>>    			vmcs_write16(GUEST_INTR_STATUS, status);
>>>    		}
>>> +		read_unlock(&gpc->lock);
>>>    	}
>>>    
> I just realised that the mark_page_dirty() on invalidation and when the
> the irqfd workqueue refreshes the gpc might fall foul of the same
> dirty_ring problem that I belatedly just spotted with the Xen shinfo
> clock write. I'll fix it up to*always*  require a vcpu (to be
> associated with the writes), and reinstate the guest_uses_pa flag since
> that can no longer in implicit in (vcpu!=NULL).

Okay.

> I may leave the actual task of fixing nesting to you, if that's OK, as
> long as we consider the new gfn_to_pfn_cache sufficient to address the
> problem? I think it's mostly down to how we*use*  it now, rather than
> the fundamental design of cache itself?

Yes, I agree.  Just stick whatever you have for nesting as an extra 
patch at the end, and I'll take it from there.

Paolo