Re: [Xen-devel] [PATCH v3 4/4] x86/apic: allow enabling x2APIC mode regardless of interrupt remapping

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Dec 05, 2019 at 10:45:59AM +0100, Jan Beulich wrote:
> On 04.12.2019 17:20, Roger Pau Monne wrote:
> > +        switch ( iommu_enable_x2apic() )
> >          {
> > +        case 0:
> > +            iommu_x2apic_enabled = true;
> > +            break;
> > +
> > +        case -ENXIO: /* ACPI_DMAR_X2APIC_OPT_OUT set */
> > +            if ( x2apic_enabled )
> > +                panic("IOMMU requests xAPIC mode, but x2APIC already 
> > enabled by firmware\n");
> > +
> >              printk("Not enabling x2APIC (upon firmware request)\n");
> > -            intremap_enabled = false;
> > +            iommu_x2apic_enabled = false;
> >              goto restore_out;
> > +
> > +        default:
> > +            printk(XENLOG_ERR "Failed to enable Interrupt Remapping\n");
> > +            iommu_x2apic_enabled = false;
> > +            break;
> 
> I guess you still need to panic() in the failure cases if x2apic_phys
> is false. Unless you can still properly switch from cluster to
> physical mode at this point in time. (If you go the panic() route,
> I'd appreciate if you could avoid making x2apic_phys non-static.)

I don't think Xen needs to check x2apic_phys or panic here, the x2apic
probe that selects phys or cluster mode is done afterwards in
apic_x2apic_probe, which is called after the attempt to enable
interrupt remapping and hence will take this result into account.

> > @@ -938,13 +931,16 @@ void __init x2apic_bsp_setup(void)
> >          printk("Switched to APIC driver %s\n", genapic.name);
> >  
> >  restore_out:
> > -    /*
> > -     * NB: do not use raw mode when restoring entries if the iommu has been
> > -     * enabled during the process, because the entries need to be 
> > translated
> > -     * and added to the remapping table in that case.
> > -     */
> > -    restore_IO_APIC_setup(ioapic_entries, !intremap_enabled);
> > -    unmask_8259A();
> > +    if ( iommu_supports_x2apic() )
> 
> Hmm, I first wanted to suggest to use iommu_x2apic_enabled here, but
> I realize the error cases above would then be wrong. Perhaps better
> to leave a brief comment to this effect?

Ack, would you be fine with:

"Note that iommu_x2apic_enabled cannot be used here because if the
IOMMU supports x2APIC but enabling failed Xen wouldn't restore the
IO-APIC and the 8259A state correctly."

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel