On 29/09/2016 18:56, Radim Krčmář wrote: > 2016-09-29 18:06+0200, Igor Mammedov: >> On Thu, 29 Sep 2016 15:18:36 +0200 >> Paolo Bonzini <pbonz...@redhat.com> wrote: >>> On 29/09/2016 13:23, Radim Krčmář 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, so 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čmář <rkrc...@redhat.com> >>>> --- >>>> 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, Error >>>> **errp) >>>> s->intr_eim = ON_OFF_AUTO_OFF; >>>> } >>>> if (s->intr_eim == ON_OFF_AUTO_AUTO) { >>>> - s->intr_eim = ON_OFF_AUTO_ON; >>>> + s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON >>>> + : ON_OFF_AUTO_OFF; >>>> + } >>>> + if (s->intr_eim == ON_OFF_AUTO_ON) { >>>> + if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) { >>>> + error_report("intel-iommu,eim=on requires support on the KVM >>>> side " >>>> + "(X2APIC_API, first shipped in v4.7)."); >>>> + exit(1); >>> >>> 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. > > 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=on requires " >>>> + "accel=kvm,kernel-irqchip=split."); >> this prints: >> -device intel-iommu,intremap=on,eim=on: intel-iommu,eim=on requires >> accel=kvm,kernel-irqchip=split >> so 'intel-iommu,' not really needed, the same would happen with error_setg() > > Yeah, really unreadable, thanks. >