diff mbox series

[v2,4/4] x86/apic: allow enabling x2APIC mode regardless of interrupt remapping

Message ID 20191129112851.19273-5-roger.pau@citrix.com (mailing list archive)
State Superseded
Headers show
Series x86: enable x2APIC mode regardless of interrupt remapping support | expand

Commit Message

Roger Pau Monne Nov. 29, 2019, 11:28 a.m. UTC
x2APIC mode doesn't mandate interrupt remapping, and hence can be
enabled independently. This patch enables x2APIC when available,
regardless of whether there's interrupt remapping support.

This is beneficial specially when running on virtualized environments,
since it reduces the amount of vmexits. For example when sending an
IPI in xAPIC mode Xen performs at least 3 different accesses to the
APIC MMIO region, while when using x2APIC mode a single wrmsr is used.

The following numbers are from a lock profiling of a Xen PV shim
running a Linux PV kernel with 32 vCPUs and xAPIC mode:

(XEN) Global lock flush_lock: addr=ffff82d0804af1c0, lockval=03190319, not locked
(XEN)   lock:656153(892606463454), block:602183(9495067321843)

Average lock time:   1360363ns
Average block time: 15767743ns

While the following are from the same configuration but with the shim
using x2APIC mode:

(XEN) Global lock flush_lock: addr=ffff82d0804b01c0, lockval=1adb1adb, not locked
(XEN)   lock:1841883(1375128998543), block:1658716(10193054890781)

Average lock time:   746588ns
Average block time: 6145147ns

Enabling x2APIC has halved the average lock time, thus reducing
contention.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Fix error paths of iommu_enable_x2apic call in x2apic_bsp_setup.
---
NB: should enabling x2APIC without interrupt remapping be limited to
running on virtualized environments?

The bigger performance benefit is indeed achieved when using x2APIC on
virt environments, but I also don't see why we wouldn't want to try
using it everywhere where it's supported.
---
 xen/arch/x86/apic.c | 89 +++++++++++++++++++++------------------------
 1 file changed, 42 insertions(+), 47 deletions(-)

Comments

Jan Beulich Dec. 3, 2019, 3:33 p.m. UTC | #1
On 29.11.2019 12:28, Roger Pau Monne wrote:
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -492,7 +492,8 @@ static void __enable_x2apic(void)
>  
>  static void resume_x2apic(void)
>  {
> -    iommu_enable_x2apic();
> +    if ( iommu_supports_x2apic() )
> +        iommu_enable_x2apic();

The hooks called by this function are __init, and at least the AMD
one also isn't (I think) prepared to be called more than once.

Jan
Roger Pau Monne Dec. 4, 2019, 1:51 p.m. UTC | #2
On Tue, Dec 03, 2019 at 04:33:09PM +0100, Jan Beulich wrote:
> On 29.11.2019 12:28, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/apic.c
> > +++ b/xen/arch/x86/apic.c
> > @@ -492,7 +492,8 @@ static void __enable_x2apic(void)
> >  
> >  static void resume_x2apic(void)
> >  {
> > -    iommu_enable_x2apic();
> > +    if ( iommu_supports_x2apic() )
> > +        iommu_enable_x2apic();
> 
> The hooks called by this function are __init, and at least the AMD
> one also isn't (I think) prepared to be called more than once.

It's already called twice, there's one call to iommu_supports_x2apic
in x2apic_bsp_setup and another one in iommu_enable_x2apic, so I think
calling iommu_supports_x2apic multiple times is fine (or we would
already have issues).

I will cache the value of iommu_enable_x2apic to be used in the
suspend/resume paths of the local apic, so there's no need to call
iommu_supports_x2apic after init.

Thanks, Roger.
Jan Beulich Dec. 4, 2019, 2:28 p.m. UTC | #3
On 04.12.2019 14:51, Roger Pau Monné wrote:
> On Tue, Dec 03, 2019 at 04:33:09PM +0100, Jan Beulich wrote:
>> On 29.11.2019 12:28, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/apic.c
>>> +++ b/xen/arch/x86/apic.c
>>> @@ -492,7 +492,8 @@ static void __enable_x2apic(void)
>>>  
>>>  static void resume_x2apic(void)
>>>  {
>>> -    iommu_enable_x2apic();
>>> +    if ( iommu_supports_x2apic() )
>>> +        iommu_enable_x2apic();
>>
>> The hooks called by this function are __init, and at least the AMD
>> one also isn't (I think) prepared to be called more than once.
> 
> It's already called twice, there's one call to iommu_supports_x2apic
> in x2apic_bsp_setup and another one in iommu_enable_x2apic, so I think
> calling iommu_supports_x2apic multiple times is fine (or we would
> already have issues).

Ah, right - amd_iommu_prepare() bails (relatively) early when
noticing it had been called before.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index a8ee18636f..eaf8924585 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -492,7 +492,8 @@  static void __enable_x2apic(void)
 
 static void resume_x2apic(void)
 {
-    iommu_enable_x2apic();
+    if ( iommu_supports_x2apic() )
+        iommu_enable_x2apic();
     __enable_x2apic();
 }
 
@@ -695,7 +696,8 @@  int lapic_suspend(void)
 
     local_irq_save(flags);
     disable_local_APIC();
-    iommu_disable_x2apic();
+    if ( iommu_supports_x2apic() )
+        iommu_disable_x2apic();
     local_irq_restore(flags);
     return 0;
 }
@@ -875,56 +877,46 @@  void __init x2apic_bsp_setup(void)
         printk("x2APIC: Already enabled by BIOS: Ignoring cmdline disable.\n");
     }
 
-    if ( !iommu_supports_x2apic() )
+    if ( iommu_supports_x2apic() )
     {
-        if ( !x2apic_enabled )
+        if ( (ioapic_entries = alloc_ioapic_entries()) == NULL )
         {
-            printk("Not enabling x2APIC: depends on IOMMU support\n");
-            return;
+            printk("Allocate ioapic_entries failed\n");
+            goto out;
         }
-        panic("x2APIC: already enabled by BIOS, but no IOMMU support\n");
-    }
 
-    if ( (ioapic_entries = alloc_ioapic_entries()) == NULL )
-    {
-        printk("Allocate ioapic_entries failed\n");
-        goto out;
-    }
-
-    if ( save_IO_APIC_setup(ioapic_entries) )
-    {
-        printk("Saving IO-APIC state failed\n");
-        goto out;
-    }
+        if ( save_IO_APIC_setup(ioapic_entries) )
+        {
+            printk("Saving IO-APIC state failed\n");
+            goto out;
+        }
 
-    mask_8259A();
-    mask_IO_APIC_setup(ioapic_entries);
+        mask_8259A();
+        mask_IO_APIC_setup(ioapic_entries);
 
-    switch ( iommu_enable_x2apic() )
-    {
-    case 0:
-        intremap_enabled = true;
-        break;
-    case -ENXIO: /* ACPI_DMAR_X2APIC_OPT_OUT set */
-        if ( !x2apic_enabled )
+        switch ( iommu_enable_x2apic() )
         {
+        case 0:
+            intremap_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;
             goto restore_out;
+
+        default:
+            printk(XENLOG_ERR "Failed to enable Interrupt Remapping\n");
+            intremap_enabled = false;
+            break;
         }
-        /* fall through */
-    default:
-        if ( x2apic_enabled )
-            panic("Interrupt remapping could not be enabled while "
-                  "x2APIC is already enabled by BIOS\n");
-
-        printk(XENLOG_ERR
-               "Failed to enable Interrupt Remapping: Will not enable x2APIC.\n");
-        intremap_enabled = false;
-        goto restore_out;
-    }
 
-    force_iommu = 1;
+        if ( intremap_enabled )
+            force_iommu = 1;
+    }
 
     if ( !x2apic_enabled )
     {
@@ -938,13 +930,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() )
+    {
+        /*
+         * 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();
+    }
 
 out:
     if ( ioapic_entries )