From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37095) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bpedt-0002jm-9B for qemu-devel@nongnu.org; Thu, 29 Sep 2016 12:57:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bpedn-0004pB-Ak for qemu-devel@nongnu.org; Thu, 29 Sep 2016 12:57:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:62870) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bpedn-0004p1-0l for qemu-devel@nongnu.org; Thu, 29 Sep 2016 12:56:59 -0400 References: <20160929112329.2408-1-rkrcmar@redhat.com> <20160929112329.2408-7-rkrcmar@redhat.com> <20160929180654.782b73c1@nial.brq.redhat.com> <20160929165626.GC12788@potion> From: Paolo Bonzini Message-ID: Date: Thu, 29 Sep 2016 18:56:53 +0200 MIME-Version: 1.0 In-Reply-To: <20160929165626.GC12788@potion> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 6/7] intel_iommu: reject broken EIM List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Igor Mammedov Cc: qemu-devel@nongnu.org, Peter Xu , Richard Henderson , Eduardo Habkost , "Michael S. Tsirkin" On 29/09/2016 18:56, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: > 2016-09-29 18:06+0200, Igor Mammedov: >> On Thu, 29 Sep 2016 15:18:36 +0200 >> Paolo Bonzini wrote: >>> On 29/09/2016 13:23, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: >>>> Cluster x2APIC cannot work without KVM's x2apic API when the maximal >>>> APIC ID is greater than 8 and only KVM's LAPIC can support x2APIC, s= o we >>>> forbid other APICs and also the old KVM case with less than 9, to >>>> simplify the code. >>>> >>>> There is no point in enabling EIM in forbidden APICs, so we keep it >>>> enabled only for the KVM APIC; unconditionally, because making the >>>> option depend on KVM version would be a maintanance burden. >>>> >>>> Signed-off-by: Radim Kr=C4=8Dm=C3=A1=C5=99 >>>> --- >>>> v2: >>>> * adapt to new intr_eim parameter >>>> * provide first linux version that has x2apic api >>>> * disable QEMU's LAPIC >>>> --- >>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>>> @@ -2481,7 +2482,20 @@ static void vtd_realize(DeviceState *dev, Err= or **errp) >>>> s->intr_eim =3D ON_OFF_AUTO_OFF; >>>> } >>>> if (s->intr_eim =3D=3D ON_OFF_AUTO_AUTO) { >>>> - s->intr_eim =3D ON_OFF_AUTO_ON; >>>> + s->intr_eim =3D kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON >>>> + : ON_OFF_AUTO_OFF; >>>> + } >>>> + if (s->intr_eim =3D=3D ON_OFF_AUTO_ON) { >>>> + if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) { >>>> + error_report("intel-iommu,eim=3Don requires support on = the KVM side " >>>> + "(X2APIC_API, first shipped in v4.7)."); >>>> + exit(1); =20 >>> >>> Please use error_setg and return instead (same in patches 4 and 5). >> Radim's version is consistent with error handling used throughout the = file. >> If we are to use preferred error_setg() here that preceding cleanup >> patch is in order to convert error_reports to error_setg. >=20 > There is one more error_report() in the file and it doesn't have > "Error **" -- I'll leave it be and change the rest. > It amounts to one extra patch that before [4/7] (could be squashed too)= . No problem then, keep using error_report I guess. Paolo >>>> + } >>>> + if (!kvm_irqchip_in_kernel()) { >>>> + error_report("intel-iommu,eim=3Don requires " >>>> + "accel=3Dkvm,kernel-irqchip=3Dsplit."); >> this prints: >> -device intel-iommu,intremap=3Don,eim=3Don: intel-iommu,eim=3Don requ= ires accel=3Dkvm,kernel-irqchip=3Dsplit >> so 'intel-iommu,' not really needed, the same would happen with error_= setg() >=20 > Yeah, really unreadable, thanks. >=20