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 <dwmw2@xxxxxxxxxxxxx>
> > Reviewed-by: Peter Xu <peterx@xxxxxxxxxx>
> > Acked-by: Jason Wang <jasowang@xxxxxxxxxx>
>
> 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?
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
