[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 6/7] intel_iommu: reject broken EIM
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH v2 6/7] intel_iommu: reject broken EIM |
Date: |
Thu, 29 Sep 2016 18:56:53 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 |
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 <address@hidden> 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ář <address@hidden>
>>>> ---
>>>> 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.
>