linux-kernel - Re: [PATCH] x86/kvm/hyper-v: Don't deactivate APICv unconditionally when Hyper-V SynIC enabled

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: <87sg9n3ilt.fsf@vitty.brq.redhat.com>
Date:   Thu, 05 Nov 2020 16:53:50 +0100
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     junjiehua0xff@...il.com
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Andrey Smetanin <asmetanin@...tuozzo.com>,
        Junjie Hua <junjiehua@...cent.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/kvm/hyper-v: Don't deactivate APICv unconditionally
 when Hyper-V SynIC enabled

junjiehua0xff@...il.com writes:

> From: Junjie Hua <junjiehua@...cent.com>
>
> The current implementation of Hyper-V SynIC[1] request to deactivate 
> APICv when SynIC is enabled, since the AutoEOI feature of SynIC is not 
> compatible with APICv[2].
>
> Actually, windows doesn't use AutoEOI if deprecating AutoEOI bit is set 
> (CPUID.40000004H:EAX[bit 9], HyperV-TLFS v6.0b section 2.4.5), we don't 
> need to disable APICv in this case.
>

Thank you for the patch, the fact that we disable APICv every time we
enable SynIC is nothing to be proud of. I'm, however, not sure we can
treat 'Recommend deprecating AutoEOI' as 'AutoEOI must not be
used.'. Could you please clarify which Windows versions you've tested
with with?

> [1] commit 5c919412fe61 ("kvm/x86: Hyper-V synthetic interrupt controller")
> [2] https://patchwork.kernel.org/patch/7486761/
>
> Signed-off-by: Junjie Hua <junjiehua@...cent.com>
> ---
>  arch/x86/kvm/hyperv.c | 18 +++++++++++++++++-
>  arch/x86/kvm/lapic.c  |  3 +++
>  2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 5c7c406..9eee2da 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -899,6 +899,19 @@ void kvm_hv_vcpu_postcreate(struct kvm_vcpu *vcpu)
>  	hv_vcpu->vp_index = kvm_vcpu_get_idx(vcpu);
>  }
>  
> +static bool kvm_hv_is_synic_autoeoi_deprecated(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_cpuid_entry2 *entry;
> +
> +	entry = kvm_find_cpuid_entry(vcpu,
> +				HYPERV_CPUID_ENLIGHTMENT_INFO,
> +				0);
> +	if (!entry)
> +		return false;
> +
> +	return entry->eax & HV_DEPRECATING_AEOI_RECOMMENDED;
> +}

I think we should complement (replace?) this with checking that no SINTx
was configured with AutoEOI (and immeditely inhibit APICv if the
situation changes).

> +
>  int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages)
>  {
>  	struct kvm_vcpu_hv_synic *synic = vcpu_to_synic(vcpu);
> @@ -908,7 +921,10 @@ int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages)
>  	 * not compatible with APICV, so request
>  	 * to deactivate APICV permanently.
>  	 */
> -	kvm_request_apicv_update(vcpu->kvm, false, APICV_INHIBIT_REASON_HYPERV);
> +	if (!kvm_hv_is_synic_autoeoi_deprecated(vcpu))
> +		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;
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 105e785..0bb431f 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1263,6 +1263,9 @@ void kvm_apic_set_eoi_accelerated(struct kvm_vcpu *vcpu, int vector)
>  
>  	trace_kvm_eoi(apic, vector);
>  
> +	if (test_bit(vector, vcpu_to_synic(apic->vcpu)->vec_bitmap))
> +		kvm_hv_synic_send_eoi(apic->vcpu, vector);
> +
>  	kvm_ioapic_send_eoi(apic, vector);
>  	kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
>  }

-- 
Vitaly