Re: [RFC PATCH 0/11] Rework gfn_to_pfn_cache

[Paolo Bonzini <bonzini@gnu.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