Re: [PATCH v3 04/34] KVM: x86: hyper-v: Handle HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST{,EX} calls gently

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Maxim Levitsky <mlevitsk@redhat.com> writes:

> On Thu, 2022-04-14 at 15:19 +0200, Vitaly Kuznetsov wrote:
>> Currently, HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST{,EX} calls are handled
>> the exact same way as HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE{,EX}: by
>> flushing the whole VPID and this is sub-optimal. Switch to handling
>> these requests with 'flush_tlb_gva()' hooks instead. Use the newly
>> introduced TLB flush ring to queue the requests.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  arch/x86/kvm/hyperv.c | 132 ++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 115 insertions(+), 17 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index d66c27fd1e8a..759e1a16e5c3 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -1805,6 +1805,13 @@ static u64 kvm_get_sparse_vp_set(struct kvm *kvm, struct kvm_hv_hcall *hc,
>>  				  sparse_banks, consumed_xmm_halves, offset);
>>  }
>>  
>> +static int kvm_hv_get_tlb_flush_entries(struct kvm *kvm, struct kvm_hv_hcall *hc, u64 entries[],
>> +				       int consumed_xmm_halves, gpa_t offset)
>> +{
>> +	return kvm_hv_get_hc_data(kvm, hc, hc->rep_cnt, hc->rep_cnt,
>> +				  entries, consumed_xmm_halves, offset);
>> +}
>> +
>>  static inline int hv_tlb_flush_ring_free(struct kvm_vcpu_hv *hv_vcpu,
>>  					 int read_idx, int write_idx)
>>  {
>> @@ -1814,12 +1821,13 @@ static inline int hv_tlb_flush_ring_free(struct kvm_vcpu_hv *hv_vcpu,
>>  	return read_idx - write_idx - 1;
>>  }
>>  
>> -static void hv_tlb_flush_ring_enqueue(struct kvm_vcpu *vcpu)
>> +static void hv_tlb_flush_ring_enqueue(struct kvm_vcpu *vcpu, u64 *entries, int count)
>>  {
>>  	struct kvm_vcpu_hv_tlb_flush_ring *tlb_flush_ring;
>>  	struct kvm_vcpu_hv *hv_vcpu = to_hv_vcpu(vcpu);
>>  	int ring_free, write_idx, read_idx;
>>  	unsigned long flags;
>> +	int i;
>>  
>>  	if (!hv_vcpu)
>>  		return;
>> @@ -1845,14 +1853,34 @@ static void hv_tlb_flush_ring_enqueue(struct kvm_vcpu *vcpu)
>>  	if (!ring_free)
>>  		goto out_unlock;
>>  
>> -	tlb_flush_ring->entries[write_idx].addr = 0;
>> -	tlb_flush_ring->entries[write_idx].flush_all = 1;
>>  	/*
>> -	 * Advance write index only after filling in the entry to
>> -	 * synchronize with lockless reader.
>> +	 * All entries should fit on the ring leaving one free for 'flush all'
>> +	 * entry in case another request comes in. In case there's not enough
>> +	 * space, just put 'flush all' entry there.
>> +	 */
>> +	if (!count || count >= ring_free - 1 || !entries) {
>> +		tlb_flush_ring->entries[write_idx].addr = 0;
>> +		tlb_flush_ring->entries[write_idx].flush_all = 1;
>> +		/*
>> +		 * Advance write index only after filling in the entry to
>> +		 * synchronize with lockless reader.
>> +		 */
>> +		smp_wmb();
>> +		tlb_flush_ring->write_idx = (write_idx + 1) % KVM_HV_TLB_FLUSH_RING_SIZE;
>> +		goto out_unlock;
>> +	}
>> +
>> +	for (i = 0; i < count; i++) {
>> +		tlb_flush_ring->entries[write_idx].addr = entries[i];
>> +		tlb_flush_ring->entries[write_idx].flush_all = 0;
>> +		write_idx = (write_idx + 1) % KVM_HV_TLB_FLUSH_RING_SIZE;
>> +	}
>> +	/*
>> +	 * Advance write index only after filling in the entry to synchronize
>> +	 * with lockless reader.
>>  	 */
>>  	smp_wmb();
>> -	tlb_flush_ring->write_idx = (write_idx + 1) % KVM_HV_TLB_FLUSH_RING_SIZE;
>> +	tlb_flush_ring->write_idx = write_idx;
>>  
>>  out_unlock:
>>  	spin_unlock_irqrestore(&tlb_flush_ring->write_lock, flags);
>> @@ -1862,15 +1890,58 @@ void kvm_hv_vcpu_flush_tlb(struct kvm_vcpu *vcpu)
>>  {
>>  	struct kvm_vcpu_hv_tlb_flush_ring *tlb_flush_ring;
>>  	struct kvm_vcpu_hv *hv_vcpu = to_hv_vcpu(vcpu);
>> +	struct kvm_vcpu_hv_tlb_flush_entry *entry;
>> +	int read_idx, write_idx;
>> +	u64 address;
>> +	u32 count;
>> +	int i, j;
>>  
>> -	kvm_vcpu_flush_tlb_guest(vcpu);
>> -
>> -	if (!hv_vcpu)
>> +	if (!tdp_enabled || !hv_vcpu) {
>> +		kvm_vcpu_flush_tlb_guest(vcpu);
>>  		return;
>> +	}
>>  
>>  	tlb_flush_ring = &hv_vcpu->tlb_flush_ring;
>>  
>> -	tlb_flush_ring->read_idx = tlb_flush_ring->write_idx;
>> +	/*
>> +	 * TLB flush must be performed on the target vCPU so 'read_idx'
>> +	 * (AKA 'tail') cannot change underneath, the compiler is free
>> +	 * to re-read it.
>> +	 */
>> +	read_idx = tlb_flush_ring->read_idx;
>> +
>> +	/*
>> +	 * 'write_idx' (AKA 'head') can be concurently updated by a different
>> +	 * vCPU so we must be sure it's read once.
>> +	 */
>> +	write_idx = READ_ONCE(tlb_flush_ring->write_idx);
>> +
>> +	/* Pairs with smp_wmb() in hv_tlb_flush_ring_enqueue() */
>> +	smp_rmb();
>> +
>> +	for (i = read_idx; i != write_idx; i = (i + 1) % KVM_HV_TLB_FLUSH_RING_SIZE) {
>> +		entry = &tlb_flush_ring->entries[i];
>> +
>> +		if (entry->flush_all)
>> +			goto out_flush_all;
>
> I have an idea: instead of special 'flush all entry' in the ring,
> just have a boolean in parallel to the ring.
>
> Also the ring buffer entries will be 2x smaller since they won't need
> to have the 'flush all' boolean.
>
> This would allow to just flush the whole thing and discard the ring if that boolean is set,
> allow to not enqueue anything to the ring also if the boolean is already set,
> also we won't need to have extra space in the ring for that entry, etc, etc.
>
> Or if using kfifo, then it can contain plain u64 items, which is even more natural.
>

In the next version I switch to fifo and get rid of 'flush_all' entries
but instead of a boolean I use a 'magic' value of '-1' in GVA. This way
we don't need to synchronize with the reader and add any special
handling for the flag.

Note, in the future we may get back to having flags as part of entries
as it is now possible to analize guest's CR3. We'll likely add
'AddressSpace' to each entry. The 'flush all' entry, however, will
always remain 'special' to handle ring overflow case.

-- 
Vitaly