On Thu, 2021-12-16 at 16:47 +0800, Peter Xu wrote: > Hi, David, > > On Thu, Dec 09, 2021 at 10:08:40PM +0000, David Woodhouse wrote: > > We don't need to check kvm_enable_x2apic(). It's perfectly OK to support > > interrupt remapping even if we can't address CPUs above 254. Kind of > > pointless, but still functional. > > We only checks kvm_enable_x2apic() if eim=on is set, right? I mean, we can > still enable IR without x2apic even with current code? > > Could you elaborate what's the use scenario for this patch? Thanks in > advance.
You can have IR, EIM *and* X2APIC if kvm_enable_x2apic() fails. You just can't have any CPUs with an APIC ID > 254. But qemu is going to bail out *anyway* if you attempt to have CPUs with APIC IDs above 254 without the corresponding kernel-side support, so there's no need to check it here. > > The check on kvm_enable_x2apic() needs to happen *anyway* in order to > > allow CPUs above 254 even without an IOMMU, so allow that to happen > > elsewhere. > > > > However, we do require the *split* irqchip in order to rewrite I/OAPIC > > destinations. So fix that check while we're here. > > > > Signed-off-by: David Woodhouse <dw...@infradead.org> > > Reviewed-by: Peter Xu <pet...@redhat.com> > > Acked-by: Jason Wang <jasow...@redhat.com> > > I think the r-b and a-b should be for patch 2 not this one? :) > Yes, I think I must have swapped those round. Thanks. > > --- > > hw/i386/intel_iommu.c | 7 +------ > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > index bd288d45bb..0d1c72f08e 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -3760,15 +3760,10 @@ static bool vtd_decide_config(IntelIOMMUState *s, > > Error **errp) > > ON_OFF_AUTO_ON : > > ON_OFF_AUTO_OFF; > > } > > if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) { > > - if (!kvm_irqchip_in_kernel()) { > > + if (!kvm_irqchip_is_split()) { > > I think this is okay, but note that we'll already fail if !split in > x86_iommu_realize(): > > bool irq_all_kernel = kvm_irqchip_in_kernel() && !kvm_irqchip_is_split(); > > /* Both Intel and AMD IOMMU IR only support "kernel-irqchip={off|split}" > */ > if (x86_iommu_ir_supported(x86_iommu) && irq_all_kernel) { > error_setg(errp, "Interrupt Remapping cannot work with " > "kernel-irqchip=on, please use 'split|off'."); > return; > } OK, then perhaps the entire check is redundant?
smime.p7s
Description: S/MIME cryptographic signature