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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.12.2019 11:56, Roger Pau Monné wrote:
> 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.

Oh indeed, you're right.

>>> @@ -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."

This or even more briefly "iommu_x2apic_enabled cannot be used here
in the error case". With this (which once again could be done while
committing)
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan

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