linux-kernel - Re: [PATCH v2 5/5] KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in use

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <572a0dbf8f656f49dfcf49596f2b6e10236ba7a9.camel@redhat.com>
Date:   Wed, 26 May 2021 13:02:23 +0300
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>, kvm@...r.kernel.org
Cc:     Sean Christopherson <seanjc@...gle.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Kechen Lu <kechenl@...dia.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/5] KVM: x86: hyper-v: Deactivate APICv only when
 AutoEOI feature is in use

On Mon, 2021-05-24 at 18:21 +0200, Paolo Bonzini wrote:
> On 18/05/21 16:43, Vitaly Kuznetsov wrote:
> > APICV_INHIBIT_REASON_HYPERV is currently unconditionally forced upon
> > SynIC activation as SynIC's AutoEOI is incompatible with APICv/AVIC. It is,
> > however, possible to track whether the feature was actually used by the
> > guest and only inhibit APICv/AVIC when needed.
> > 
> > TLFS suggests a dedicated 'HV_DEPRECATING_AEOI_RECOMMENDED' flag to let
> > Windows know that AutoEOI feature should be avoided. While it's up to
> > KVM userspace to set the flag, KVM can help a bit by exposing global
> > APICv/AVIC enablement: in case APICv/AVIC usage is impossible, AutoEOI
> > is still preferred.
> > 
> > Signed-off-by: Vitaly Kuznetsov <vkuznets@...hat.com>
> 
> Should it also disable APICv unconditionally if 
> HV_DEPRECATING_AEOI_RECOMMENDED is not in the guest CPUID?  That should 
> avoid ping-pong between enabled and disabled APICv even in pathological 
> cases that we cannot think about.

Probably not worth it, as the guest might still not use it.
We already disable/enable AVIC at the rate of a few iterations
per second when PIC sends its interrupts via ExtINT,
and we need an interrupt window.

This is sadly something that windows still does use and
it seems to work.

Best regards,
	Maxim Levitsky

> 
> Paolo
> 
> > ---
> >   arch/x86/include/asm/kvm_host.h |  3 +++
> >   arch/x86/kvm/hyperv.c           | 27 +++++++++++++++++++++------
> >   2 files changed, 24 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index bf5807d35339..5e03ab4c0e4f 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -936,6 +936,9 @@ struct kvm_hv {
> >   	/* How many vCPUs have VP index != vCPU index */
> >   	atomic_t num_mismatched_vp_indexes;
> >   
> > +	/* How many SynICs use 'AutoEOI' feature */
> > +	atomic_t synic_auto_eoi_used;
> > +
> >   	struct hv_partition_assist_pg *hv_pa_pg;
> >   	struct kvm_hv_syndbg hv_syndbg;
> >   };
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index f98370a39936..89e7d5b99279 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -87,6 +87,10 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic,
> >   static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> >   				int vector)
> >   {
> > +	struct kvm_vcpu *vcpu = hv_synic_to_vcpu(synic);
> > +	struct kvm_hv *hv = to_kvm_hv(vcpu->kvm);
> > +	int auto_eoi_old, auto_eoi_new;
> > +
> >   	if (vector < HV_SYNIC_FIRST_VALID_VECTOR)
> >   		return;
> >   
> > @@ -95,10 +99,25 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> >   	else
> >   		__clear_bit(vector, synic->vec_bitmap);
> >   
> > +	auto_eoi_old = bitmap_weight(synic->auto_eoi_bitmap, 256);
> > +
> >   	if (synic_has_vector_auto_eoi(synic, vector))
> >   		__set_bit(vector, synic->auto_eoi_bitmap);
> >   	else
> >   		__clear_bit(vector, synic->auto_eoi_bitmap);
> > +
> > +	auto_eoi_new = bitmap_weight(synic->auto_eoi_bitmap, 256);
> > +
> > +	/* Hyper-V SynIC auto EOI SINTs are not compatible with APICV */
> > +	if (!auto_eoi_old && auto_eoi_new) {
> > +		if (atomic_inc_return(&hv->synic_auto_eoi_used) == 1)
> > +			kvm_request_apicv_update(vcpu->kvm, false,
> > +						 APICV_INHIBIT_REASON_HYPERV);
> > +	} else if (!auto_eoi_new && auto_eoi_old) {
> > +		if (atomic_dec_return(&hv->synic_auto_eoi_used) == 0)
> > +			kvm_request_apicv_update(vcpu->kvm, true,
> > +						 APICV_INHIBIT_REASON_HYPERV);
> > +	}
> >   }
> >   
> >   static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
> > @@ -931,12 +950,6 @@ int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages)
> >   
> >   	synic = to_hv_synic(vcpu);
> >   
> > -	/*
> > -	 * Hyper-V SynIC auto EOI SINT's are
> > -	 * not compatible with APICV, so request
> > -	 * to deactivate APICV permanently.
> > -	 */
> > -	kvm_request_apicv_update(vcpu->kvm, false, APICV_INHIBIT_REASON_HYPERV);
> >   	synic->active = true;
> >   	synic->dont_zero_synic_pages = dont_zero_synic_pages;
> >   	synic->control = HV_SYNIC_CONTROL_ENABLE;
> > @@ -2198,6 +2211,8 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
> >   				ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED;
> >   			if (!cpu_smt_possible())
> >   				ent->eax |= HV_X64_NO_NONARCH_CORESHARING;
> > +			if (enable_apicv)
> > +				ent->eax |= HV_DEPRECATING_AEOI_RECOMMENDED;
> >   			/*
> >   			 * Default number of spinlock retry attempts, matches
> >   			 * HyperV 2016.
> >