Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     Yuan Yao <yuan.yao@linux.intel.com>
Cc:     kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <seanjc@google.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Michael Kelley <mikelley@microsoft.com>,
        Siddharth Chandrasekaran <sidcha@amazon.de>,
        Maxim Levitsky <mlevitsk@redhat.com>,
        linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 05/38] KVM: x86: hyper-v: Handle
 HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST{,EX} calls gently
In-Reply-To: <20220608101847.63xavwsgfdprpaes@yy-desk-7060>
References: <20220606083655.2014609-1-vkuznets@redhat.com>
 <20220606083655.2014609-6-vkuznets@redhat.com>
 <20220608101847.63xavwsgfdprpaes@yy-desk-7060>
Date:   Thu, 09 Jun 2022 11:13:22 +0200
Message-ID: <87o7z2kxn1.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Yuan Yao <yuan.yao@linux.intel.com> writes:

> On Mon, Jun 06, 2022 at 10:36:22AM +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 fifo to queue the requests.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  arch/x86/kvm/hyperv.c | 100 +++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 88 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index 762b0b699fdf..956072592e2f 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -1806,32 +1806,82 @@ static u64 kvm_get_sparse_vp_set(struct kvm *kvm, struct kvm_hv_hcall *hc,
>>  				  sparse_banks, consumed_xmm_halves, offset);
>>  }
>>
>> -static void hv_tlb_flush_enqueue(struct kvm_vcpu *vcpu)
>> +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 void hv_tlb_flush_enqueue(struct kvm_vcpu *vcpu, u64 *entries, int count)
>>  {
>>  	struct kvm_vcpu_hv_tlb_flush_fifo *tlb_flush_fifo;
>>  	struct kvm_vcpu_hv *hv_vcpu = to_hv_vcpu(vcpu);
>>  	u64 entry = KVM_HV_TLB_FLUSHALL_ENTRY;
>> +	unsigned long flags;
>>
>>  	if (!hv_vcpu)
>>  		return;
>>
>>  	tlb_flush_fifo = &hv_vcpu->tlb_flush_fifo;
>>
>> -	kfifo_in_spinlocked(&tlb_flush_fifo->entries, &entry, 1, &tlb_flush_fifo->write_lock);
>> +	spin_lock_irqsave(&tlb_flush_fifo->write_lock, flags);
>> +
>> +	/*
>> +	 * All entries should fit on the fifo 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 && entries && count < kfifo_avail(&tlb_flush_fifo->entries)) {
>> +		WARN_ON(kfifo_in(&tlb_flush_fifo->entries, entries, count) != count);
>> +		goto out_unlock;
>> +	}
>> +
>> +	/*
>> +	 * Note: full fifo always contains 'flush all' entry, no need to check the
>> +	 * return value.
>> +	 */
>> +	kfifo_in(&tlb_flush_fifo->entries, &entry, 1);
>> +
>> +out_unlock:
>> +	spin_unlock_irqrestore(&tlb_flush_fifo->write_lock, flags);
>>  }
>>
>>  void kvm_hv_vcpu_flush_tlb(struct kvm_vcpu *vcpu)
>>  {
>>  	struct kvm_vcpu_hv_tlb_flush_fifo *tlb_flush_fifo;
>>  	struct kvm_vcpu_hv *hv_vcpu = to_hv_vcpu(vcpu);
>> +	u64 entries[KVM_HV_TLB_FLUSH_FIFO_SIZE];
>> +	int i, j, count;
>> +	gva_t gva;
>>
>> -	kvm_vcpu_flush_tlb_guest(vcpu);
>> -
>> -	if (!hv_vcpu)
>> +	if (!tdp_enabled || !hv_vcpu) {
>> +		kvm_vcpu_flush_tlb_guest(vcpu);
>>  		return;
>> +	}
>>
>>  	tlb_flush_fifo = &hv_vcpu->tlb_flush_fifo;
>>
>> +	count = kfifo_out(&tlb_flush_fifo->entries, entries, KVM_HV_TLB_FLUSH_FIFO_SIZE);
>
> Writers are protected by the fifo lock so only 1 writer VS 1 reader on
> this kfifo (at least so far), it shuold be safe but I'm not sure
> whether some unexpected cases there, e.g. KVM flushs another VCPU's
> kfifo while that VCPU is doing same thing for itself yet.
>

TLB is always flushed by the vCPU itself, here we just queue for it to
do so. Over-flushing is possible of course (e.g. the vCPU just flushed
and didn't even enter the guest but we're going to queue flush work for
it from other vCPUs), but that's nothing new even with the current
'dumb' implementation which always flushes everything.

The main concern should be that we never under-flush, i.e. return to the
caller while TLB on some target vCPUs was not flushed *and* target vCPUs
are running the guest.

-- 
Vitaly