Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain; charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: [PATCH] KVM: lapic: stop advertising DIRECTED_EOI when in-kernel
 IOAPIC is in use
From:   Nikita Leshenko <nikita.leshchenko@oracle.com>
In-Reply-To: <20180209130133.28387-1-vkuznets@redhat.com>
Date:   Fri, 9 Feb 2018 17:20:48 +0200
Cc:     kvm <kvm@vger.kernel.org>, linux-kernel@vger.kernel.org,
        x86@kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        =?utf-8?B?UmFkaW0gS3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        Peter Xu <peterx@redhat.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <292414F1-6978-497E-9CE2-3847E171150B@oracle.com>
References: <20180209130133.28387-1-vkuznets@redhat.com>
To:     Vitaly Kuznetsov <vkuznets@redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

The patch looks correct, however I=E2=80=99m confused about why you =
consider
this to be a bug in the guest rather than a bug in KVM.

The spec for x2APIC states:
"The support for Directed EOI capability can be detected by means of
bit 24 in the Local APIC Version Register=E2=80=9D (Intel=E2=80=99s =
x2APIC spec, 2.5.1
Directed EOI)
It seems to me that Windows did the right thing by testing for the
presence of directed EOI feature rather than implying it exists by
testing a version number. KVM did the wrong thing by advertising a
feature it doesn=E2=80=99t support.

Therefore I think that you should change the comment to something like
=E2=80=9CKVM=E2=80=99s in-kernel IOAPIC doesn=E2=80=99t support Directed =
EOI register, so don=E2=80=99t
advertise this capability in the LAPIC Version Register.=E2=80=9D =
instead of
talking about buggy guests, as it may confuse future readers of this
code.

Thanks,
Nikita
> On 9 Feb 2018, at 15:01, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>=20
> Devices which use level-triggered interrupts under Windows 2016 with
> Hyper-V role enabled don't work: Windows disables EOI broadcast in =
SPIV
> unconditionally. Our in-kernel IOAPIC implementation emulates an old =
IOAPIC
> version which has no EOI register so EOI never happens.
>=20
> The issue was discovered and discussed a while ago:
> =
https://urldefense.proofpoint.com/v2/url?u=3Dhttps-3A__www.spinics.net_lis=
ts_kvm_msg148098.html&d=3DDwIBAg&c=3DRoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65ea=
pI_JnE&r=3DJD7W0KpKqI3xo5AglC-aIVDRz_ysy5CrQRnZ9Jb7je0&m=3DGWIw1X7PvyWESZa=
Iau591RwjCXYZTi6THVNSOEcdaxU&s=3D5QUI6ED5i6frC8BzcF_e7hp6Kd_OqAxkg0z73R-UI=
DI&e=3D
>=20
> While this is a guest OS bug (it should check that IOAPIC has the =
required
> capabilities before disabling EOI broadcast) we can workaround it in =
KVM:
> advertising DIRECTED_EOI with in-kernel IOAPIC makes little sense =
anyway.
>=20
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> - Radim's suggestion was to disable DIRECTED_EOI unconditionally but =
I'm not
>  that radical :-) In theory, we may have multiple IOAPICs in userspace =
in
>  future and DIRECTED_EOI can be leveraged.
> ---
> arch/x86/kvm/lapic.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>=20
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 924ac8ce9d50..5339287fee63 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -321,8 +321,16 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu)
> 	if (!lapic_in_kernel(vcpu))
> 		return;
>=20
> +	/*
> +	 * KVM emulates 82093AA datasheet (with in-kernel IOAPIC =
implementation)
> +	 * which doesn't have EOI register; Some buggy OSes (e.g. =
Windows with
> +	 * Hyper-V role) disable EOI broadcast in lapic not checking for =
IOAPIC
> +	 * version first and level-triggered interrupts never get EOIed =
in
> +	 * IOAPIC.
> +	 */
> 	feat =3D kvm_find_cpuid_entry(apic->vcpu, 0x1, 0);
> -	if (feat && (feat->ecx & (1 << (X86_FEATURE_X2APIC & 31))))
> +	if (feat && (feat->ecx & (1 << (X86_FEATURE_X2APIC & 31))) &&
> +	    !ioapic_in_kernel(vcpu->kvm))
> 		v |=3D APIC_LVR_DIRECTED_EOI;
> 	kvm_lapic_set_reg(apic, APIC_LVR, v);
> }
> --=20
> 2.14.3
>=20