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 <[email protected]> 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ář <[email protected]>
>>>> ---
>>>> 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.
>
