2016-10-08 15:21+0800, Peter Xu: > On Wed, Oct 05, 2016 at 03:06:55PM +0200, Radim Krčmář wrote: > > [...] > >> @@ -2472,10 +2473,22 @@ static bool vtd_decide_config(IntelIOMMUState *s, >> Error **errp) >> } >> >> if (s->intr_eim == ON_OFF_AUTO_AUTO) { >> - s->intr_eim = x86_iommu->intr_supported ? >> + s->intr_eim = x86_iommu->intr_supported && 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_setg(errp, "eim=on requires support on the KVM side" >> + "(X2APIC_API, first shipped in v4.7)"); >> + return false; >> + } >> + if (!kvm_irqchip_in_kernel()) { >> + error_setg(errp, "eim=on requires >> accel=kvm,kernel-irqchip=split"); >> + return false; >> + } > > I would prefer: > > if (kvm_irqchip_in_kernel()) { > if (!kvm_enable_x2apic()) { > error("enable x2apic failed"); > return false; > } > } else { > error("need split irqchip"); > return false; > }
Yeah, it looks better, thanks. > But that's really a matter of taste. So: I'll currently go for an implicit else: (because 4 levels of indentation are getting helper-function worthy and it has less curly braces) if (!kvm_irqchip_in_kernel()) { error("need split irqchip"); return false; } if (!kvm_enable_x2apic()) { error("enable x2apic failed"); return false; } > Reviewed-by: Peter Xu <pet...@redhat.com> I squashed [7/8] into this patch in v5 and the second one didn't have your r-b, so I made the change as I'd have to drop the r-b anyway.