[Xen-devel] [PATCH v3 0/4] x86: enable x2APIC mode regardless of interrupt remapping support

[Xen-devel] [PATCH v3 0/4] x86: enable x2APIC mode regardless of interrupt remapping support

[Roger Pau Monne]

Hello,

The following series aims to allow enabling x2APIC mode without
interrupt remapping support. The main usage of this would be in
virtualized environments, that usually provide x2APIC support but not
interrupt remapping.

See the last patch for some performance numbers of using x2APIC over
xAPIC when running Xen in pvshim mode.

Thanks, Roger.

Roger Pau Monne (4):
  x86/ioapic: only use dest32 with x2apic and interrupt remapping
    enabled
  x86/apic: force phys mode if interrupt remapping is disabled
  x86/smp: check APIC ID on AP bringup
  x86/apic: allow enabling x2APIC mode regardless of interrupt remapping

 docs/misc/xen-command-line.pandoc |  3 +-
 xen/arch/x86/apic.c               | 94 +++++++++++++++----------------
 xen/arch/x86/genapic/x2apic.c     | 18 +++++-
 xen/arch/x86/io_apic.c            | 14 ++---
 xen/arch/x86/smpboot.c            |  7 +++
 5 files changed, 78 insertions(+), 58 deletions(-)

-- 
2.24.0


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

[Xen-devel] [PATCH v3 1/4] x86/ioapic: only use dest32 with x2apic and interrupt remapping enabled

[Roger Pau Monne]

The IO-APIC code assumes that x2apic being enabled also implies
interrupt remapping being enabled, and hence will use the 32bit
destination field in the IO-APIC entry.

This is safe now, but there's no reason to not enable x2APIC even
without interrupt remapping, and hence the IO-APIC code needs to use
the 32 bit destination field only when both interrupt remapping and
x2APIC are enabled.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Fix set_ioapic_affinity_irq.
---
 xen/arch/x86/io_apic.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 97cb2d154a..6238df494b 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -562,7 +562,7 @@ set_ioapic_affinity_irq(struct irq_desc *desc, const cpumask_t *mask)
 
     dest = set_desc_affinity(desc, mask);
     if (dest != BAD_APICID) {
-        if ( !x2apic_enabled )
+        if ( !iommu_intremap || !x2apic_enabled )
             dest = SET_APIC_LOGICAL_ID(dest);
         entry = irq_2_pin + irq;
         for (;;) {
@@ -964,7 +964,7 @@ static hw_irq_controller ioapic_edge_type;
 #define IOAPIC_LEVEL	1
 
 #define SET_DEST(ent, mode, val) do { \
-    if (x2apic_enabled) \
+    if (x2apic_enabled && iommu_intremap) \
         (ent).dest.dest32 = (val); \
     else \
         (ent).dest.mode.mode##_dest = (val); \
@@ -1194,14 +1194,14 @@ static void /*__init*/ __print_IO_APIC(bool boot)
 	printk(KERN_DEBUG ".... IRQ redirection table:\n");
 
 	printk(KERN_DEBUG " NR %s Msk Trg IRR Pol Stat DstM DelM Vec\n",
-               x2apic_enabled ? " DestID" : "Dst");
+               (x2apic_enabled && iommu_intremap) ? " DestID" : "Dst");
 
 	for (i = 0; i <= reg_01.bits.entries; i++) {
             struct IO_APIC_route_entry entry;
 
             entry = ioapic_read_entry(apic, i, 0);
 
-            if ( x2apic_enabled )
+            if ( x2apic_enabled && iommu_intremap )
                 printk(KERN_DEBUG " %02x %08x", i, entry.dest.dest32);
             else
                 printk(KERN_DEBUG " %02x  %02x ", i,
@@ -2504,9 +2504,9 @@ void dump_ioapic_irq_info(void)
                    rte.dest_mode ? 'L' : 'P',
                    rte.delivery_status, rte.polarity, rte.irr,
                    rte.trigger ? 'L' : 'E', rte.mask,
-                   x2apic_enabled ? 8 : 2,
-                   x2apic_enabled ? rte.dest.dest32
-                                  : rte.dest.logical.logical_dest);
+                   (x2apic_enabled && iommu_intremap) ? 8 : 2,
+                   (x2apic_enabled && iommu_intremap) ?
+                       rte.dest.dest32 : rte.dest.logical.logical_dest);
 
             if ( entry->next == 0 )
                 break;
-- 
2.24.0


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

[Xen-devel] [PATCH v3 2/4] x86/apic: force phys mode if interrupt remapping is disabled

[Roger Pau Monne]

Cluster mode can only be used with interrupt remapping support, since
the top 16bits of the APIC ID are filled with the cluster ID, and
hence on systems where the physical ID is still smaller than 255 the
cluster ID is not. Force x2APIC to use physical mode if there's no
interrupt remapping support.

Note that this requires a further patch in order to enable x2APIC
without interrupt remapping support.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Update command line doc.
 - Fix logic to set x2apic_phys if user has specified a value.
 - Force phys mode if no interrupt remapping support.

Changes since v1:
 - New in this version.
---
 docs/misc/xen-command-line.pandoc |  3 ++-
 xen/arch/x86/genapic/x2apic.c     | 18 +++++++++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 891d2d439f..d9495ef6b9 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2303,7 +2303,8 @@ Permit use of x2apic setup for SMP environments.
 ### x2apic_phys (x86)
 > `= <boolean>`
 
-> Default: `true` if **FADT** mandates physical mode, `false` otherwise.
+> Default: `true` if **FADT** mandates physical mode or if interrupt remapping
+>          is not available, `false` otherwise.
 
 In the case that x2apic is in use, this option switches between physical and
 clustered mode.  The default, given no hint from the **FADT**, is cluster
diff --git a/xen/arch/x86/genapic/x2apic.c b/xen/arch/x86/genapic/x2apic.c
index d5a17f10d5..79b6c07329 100644
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -226,7 +226,23 @@ boolean_param("x2apic_phys", x2apic_phys);
 const struct genapic *__init apic_x2apic_probe(void)
 {
     if ( x2apic_phys < 0 )
-        x2apic_phys = !!(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
+    {
+        if ( !iommu_intremap )
+            /*
+             * Force physical mode if there's no interrupt remapping support:
+             * the ID in clustered mode requires a 32 bit destination field due
+             * to the usage of the high 16 bits to store the cluster ID.
+             */
+            x2apic_phys = true;
+        else
+            x2apic_phys = !!(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
+    }
+    else if ( !x2apic_phys && !iommu_intremap )
+    {
+        printk("WARNING: x2APIC cluster mode is not supported without interrupt remapping\n"
+               "x2APIC: forcing phys mode\n");
+        x2apic_phys = true;
+    }
 
     if ( x2apic_phys )
         return &apic_x2apic_phys;
-- 
2.24.0


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

[Xen-devel] [PATCH v3 3/4] x86/smp: check APIC ID on AP bringup

[Roger Pau Monne]

Check that the processor to be woken up APIC ID is addressable in the
current APIC mode.

Note that in practice systems with APIC IDs > 255 should already have
x2APIC enabled by the firmware, and hence this is mostly a safety
belt.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Reword error message.
---
 xen/arch/x86/smpboot.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index fa691b6ba0..8cbb7173a4 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -1317,6 +1317,13 @@ int __cpu_up(unsigned int cpu)
     if ( (apicid = x86_cpu_to_apicid[cpu]) == BAD_APICID )
         return -ENODEV;
 
+    if ( (!x2apic_enabled || !iommu_intremap) && (apicid >> 8) )
+    {
+        printk("Unsupported: APIC ID %#x in xAPIC mode w/o interrupt remapping",
+               apicid);
+        return -EINVAL;
+    }
+
     if ( (ret = do_boot_cpu(apicid, cpu)) != 0 )
         return ret;
 
-- 
2.24.0


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

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

[Roger Pau Monne]

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 v2:
 - Cache the result of iommu_enable_x2apic so it can be used in the
   lapic suspend/resume paths.

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 | 94 ++++++++++++++++++++++-----------------------
 1 file changed, 45 insertions(+), 49 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index a8ee18636f..333115bc88 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -44,6 +44,8 @@ static bool __read_mostly tdt_enabled;
 static bool __initdata tdt_enable = true;
 boolean_param("tdt", tdt_enable);
 
+static bool __read_mostly iommu_x2apic_enabled;
+
 static struct {
     int active;
     /* r/w apic fields */
@@ -492,7 +494,8 @@ static void __enable_x2apic(void)
 
 static void resume_x2apic(void)
 {
-    iommu_enable_x2apic();
+    if ( iommu_x2apic_enabled )
+        iommu_enable_x2apic();
     __enable_x2apic();
 }
 
@@ -695,7 +698,8 @@ int lapic_suspend(void)
 
     local_irq_save(flags);
     disable_local_APIC();
-    iommu_disable_x2apic();
+    if ( iommu_x2apic_enabled )
+        iommu_disable_x2apic();
     local_irq_restore(flags);
     return 0;
 }
@@ -860,7 +864,6 @@ void __init x2apic_bsp_setup(void)
 {
     struct IO_APIC_route_entry **ioapic_entries = NULL;
     const char *orig_name;
-    bool intremap_enabled;
 
     if ( !cpu_has_x2apic )
         return;
@@ -875,56 +878,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:
+            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;
         }
-        /* 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 ( iommu_x2apic_enabled )
+            force_iommu = 1;
+    }
 
     if ( !x2apic_enabled )
     {
@@ -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() )
+    {
+        /*
+         * 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, !iommu_x2apic_enabled);
+        unmask_8259A();
+    }
 
 out:
     if ( ioapic_entries )
-- 
2.24.0


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

Re: [Xen-devel] [PATCH v3 1/4] x86/ioapic: only use dest32 with x2apic and interrupt remapping enabled

[Jan Beulich]

On 04.12.2019 17:20, Roger Pau Monne wrote:
> The IO-APIC code assumes that x2apic being enabled also implies
> interrupt remapping being enabled, and hence will use the 32bit
> destination field in the IO-APIC entry.
> 
> This is safe now, but there's no reason to not enable x2APIC even
> without interrupt remapping, and hence the IO-APIC code needs to use
> the 32 bit destination field only when both interrupt remapping and
> x2APIC are enabled.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [Xen-devel] [PATCH v3 2/4] x86/apic: force phys mode if interrupt remapping is disabled

[Jan Beulich]

On 04.12.2019 17:20, Roger Pau Monne wrote:
> Cluster mode can only be used with interrupt remapping support, since
> the top 16bits of the APIC ID are filled with the cluster ID, and
> hence on systems where the physical ID is still smaller than 255 the
> cluster ID is not. Force x2APIC to use physical mode if there's no
> interrupt remapping support.
> 
> Note that this requires a further patch in order to enable x2APIC
> without interrupt remapping support.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit ...

> --- a/xen/arch/x86/genapic/x2apic.c
> +++ b/xen/arch/x86/genapic/x2apic.c
> @@ -226,7 +226,23 @@ boolean_param("x2apic_phys", x2apic_phys);
>  const struct genapic *__init apic_x2apic_probe(void)
>  {
>      if ( x2apic_phys < 0 )
> -        x2apic_phys = !!(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
> +    {
> +        if ( !iommu_intremap )
> +            /*
> +             * Force physical mode if there's no interrupt remapping support:
> +             * the ID in clustered mode requires a 32 bit destination field due
> +             * to the usage of the high 16 bits to store the cluster ID.
> +             */
> +            x2apic_phys = true;
> +        else
> +            x2apic_phys = !!(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);

... I wonder why you didn't make this

        x2apic_phys = !iommu_intremap || (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);

(not the least because of allowing to drop the somewhat ugly !!).

Jan

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

Re: [Xen-devel] [PATCH v3 3/4] x86/smp: check APIC ID on AP bringup

[Jan Beulich]

On 04.12.2019 17:20, Roger Pau Monne wrote:
> Check that the processor to be woken up APIC ID is addressable in the
> current APIC mode.
> 
> Note that in practice systems with APIC IDs > 255 should already have
> x2APIC enabled by the firmware, and hence this is mostly a safety
> belt.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with ...

> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -1317,6 +1317,13 @@ int __cpu_up(unsigned int cpu)
>      if ( (apicid = x86_cpu_to_apicid[cpu]) == BAD_APICID )
>          return -ENODEV;
>  
> +    if ( (!x2apic_enabled || !iommu_intremap) && (apicid >> 8) )
> +    {
> +        printk("Unsupported: APIC ID %#x in xAPIC mode w/o interrupt remapping",
> +               apicid);

... the lost newline added back (can be done while commiting).

Jan

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

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

[Jan Beulich]

On 04.12.2019 17:20, Roger Pau Monne wrote:
> 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 v2:
>  - Cache the result of iommu_enable_x2apic so it can be used in the
>    lapic suspend/resume paths.
> 
> 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 | 94 ++++++++++++++++++++++-----------------------
>  1 file changed, 45 insertions(+), 49 deletions(-)
> 
> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
> index a8ee18636f..333115bc88 100644
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -44,6 +44,8 @@ static bool __read_mostly tdt_enabled;
>  static bool __initdata tdt_enable = true;
>  boolean_param("tdt", tdt_enable);
>  
> +static bool __read_mostly iommu_x2apic_enabled;
> +
>  static struct {
>      int active;
>      /* r/w apic fields */
> @@ -492,7 +494,8 @@ static void __enable_x2apic(void)
>  
>  static void resume_x2apic(void)
>  {
> -    iommu_enable_x2apic();
> +    if ( iommu_x2apic_enabled )
> +        iommu_enable_x2apic();
>      __enable_x2apic();
>  }
>  
> @@ -695,7 +698,8 @@ int lapic_suspend(void)
>  
>      local_irq_save(flags);
>      disable_local_APIC();
> -    iommu_disable_x2apic();
> +    if ( iommu_x2apic_enabled )
> +        iommu_disable_x2apic();
>      local_irq_restore(flags);
>      return 0;
>  }
> @@ -860,7 +864,6 @@ void __init x2apic_bsp_setup(void)
>  {
>      struct IO_APIC_route_entry **ioapic_entries = NULL;
>      const char *orig_name;
> -    bool intremap_enabled;
>  
>      if ( !cpu_has_x2apic )
>          return;
> @@ -875,56 +878,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:
> +            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.)

> @@ -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?

Jan

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

Re: [Xen-devel] [PATCH v3 2/4] x86/apic: force phys mode if interrupt remapping is disabled

[Roger Pau Monné]

On Thu, Dec 05, 2019 at 10:32:34AM +0100, Jan Beulich wrote:
> On 04.12.2019 17:20, Roger Pau Monne wrote:
> > Cluster mode can only be used with interrupt remapping support, since
> > the top 16bits of the APIC ID are filled with the cluster ID, and
> > hence on systems where the physical ID is still smaller than 255 the
> > cluster ID is not. Force x2APIC to use physical mode if there's no
> > interrupt remapping support.
> > 
> > Note that this requires a further patch in order to enable x2APIC
> > without interrupt remapping support.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> albeit ...
> 
> > --- a/xen/arch/x86/genapic/x2apic.c
> > +++ b/xen/arch/x86/genapic/x2apic.c
> > @@ -226,7 +226,23 @@ boolean_param("x2apic_phys", x2apic_phys);
> >  const struct genapic *__init apic_x2apic_probe(void)
> >  {
> >      if ( x2apic_phys < 0 )
> > -        x2apic_phys = !!(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
> > +    {
> > +        if ( !iommu_intremap )
> > +            /*
> > +             * Force physical mode if there's no interrupt remapping support:
> > +             * the ID in clustered mode requires a 32 bit destination field due
> > +             * to the usage of the high 16 bits to store the cluster ID.
> > +             */
> > +            x2apic_phys = true;
> > +        else
> > +            x2apic_phys = !!(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
> 
> ... I wonder why you didn't make this
> 
>         x2apic_phys = !iommu_intremap || (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
> 
> (not the least because of allowing to drop the somewhat ugly !!).

Feel free to do it at commit (and reindent the comment), or else I can
resend a new version if that's too intrusive.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v3 3/4] x86/smp: check APIC ID on AP bringup

[Roger Pau Monné]

On Thu, Dec 05, 2019 at 10:33:44AM +0100, Jan Beulich wrote:
> On 04.12.2019 17:20, Roger Pau Monne wrote:
> > Check that the processor to be woken up APIC ID is addressable in the
> > current APIC mode.
> > 
> > Note that in practice systems with APIC IDs > 255 should already have
> > x2APIC enabled by the firmware, and hence this is mostly a safety
> > belt.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with ...
> 
> > --- a/xen/arch/x86/smpboot.c
> > +++ b/xen/arch/x86/smpboot.c
> > @@ -1317,6 +1317,13 @@ int __cpu_up(unsigned int cpu)
> >      if ( (apicid = x86_cpu_to_apicid[cpu]) == BAD_APICID )
> >          return -ENODEV;
> >  
> > +    if ( (!x2apic_enabled || !iommu_intremap) && (apicid >> 8) )
> > +    {
> > +        printk("Unsupported: APIC ID %#x in xAPIC mode w/o interrupt remapping",
> > +               apicid);
> 
> ... the lost newline added back (can be done while commiting).

Sorry for dropping it, please add it at commit.

Thanks, Roger.

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

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

[Roger Pau Monné]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/4] x86/apic: force phys mode if interrupt remapping is disabled

[Jan Beulich]

On 09.12.2019 11:25, Roger Pau Monné wrote:
> On Thu, Dec 05, 2019 at 10:32:34AM +0100, Jan Beulich wrote:
>> On 04.12.2019 17:20, Roger Pau Monne wrote:
>>> Cluster mode can only be used with interrupt remapping support, since
>>> the top 16bits of the APIC ID are filled with the cluster ID, and
>>> hence on systems where the physical ID is still smaller than 255 the
>>> cluster ID is not. Force x2APIC to use physical mode if there's no
>>> interrupt remapping support.
>>>
>>> Note that this requires a further patch in order to enable x2APIC
>>> without interrupt remapping support.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> albeit ...
>>
>>> --- a/xen/arch/x86/genapic/x2apic.c
>>> +++ b/xen/arch/x86/genapic/x2apic.c
>>> @@ -226,7 +226,23 @@ boolean_param("x2apic_phys", x2apic_phys);
>>>  const struct genapic *__init apic_x2apic_probe(void)
>>>  {
>>>      if ( x2apic_phys < 0 )
>>> -        x2apic_phys = !!(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
>>> +    {
>>> +        if ( !iommu_intremap )
>>> +            /*
>>> +             * Force physical mode if there's no interrupt remapping support:
>>> +             * the ID in clustered mode requires a 32 bit destination field due
>>> +             * to the usage of the high 16 bits to store the cluster ID.
>>> +             */
>>> +            x2apic_phys = true;
>>> +        else
>>> +            x2apic_phys = !!(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
>>
>> ... I wonder why you didn't make this
>>
>>         x2apic_phys = !iommu_intremap || (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
>>
>> (not the least because of allowing to drop the somewhat ugly !!).
> 
> Feel free to do it at commit (and reindent the comment), or else I can
> resend a new version if that's too intrusive.

Doing these adjustments at commit time ought to be fine. It's
just that I'd prefer to wait with committing this series until
4.13 is fully finished.

Jan

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

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

[Jan Beulich]

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@suse.com>

Jan

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

Re: [Xen-devel] [PATCH v3 2/4] x86/apic: force phys mode if interrupt remapping is disabled

[Roger Pau Monné]

On Mon, Dec 09, 2019 at 03:30:27PM +0100, Jan Beulich wrote:
> On 09.12.2019 11:25, Roger Pau Monné wrote:
> > On Thu, Dec 05, 2019 at 10:32:34AM +0100, Jan Beulich wrote:
> >> On 04.12.2019 17:20, Roger Pau Monne wrote:
> >>> Cluster mode can only be used with interrupt remapping support, since
> >>> the top 16bits of the APIC ID are filled with the cluster ID, and
> >>> hence on systems where the physical ID is still smaller than 255 the
> >>> cluster ID is not. Force x2APIC to use physical mode if there's no
> >>> interrupt remapping support.
> >>>
> >>> Note that this requires a further patch in order to enable x2APIC
> >>> without interrupt remapping support.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >> albeit ...
> >>
> >>> --- a/xen/arch/x86/genapic/x2apic.c
> >>> +++ b/xen/arch/x86/genapic/x2apic.c
> >>> @@ -226,7 +226,23 @@ boolean_param("x2apic_phys", x2apic_phys);
> >>>  const struct genapic *__init apic_x2apic_probe(void)
> >>>  {
> >>>      if ( x2apic_phys < 0 )
> >>> -        x2apic_phys = !!(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
> >>> +    {
> >>> +        if ( !iommu_intremap )
> >>> +            /*
> >>> +             * Force physical mode if there's no interrupt remapping support:
> >>> +             * the ID in clustered mode requires a 32 bit destination field due
> >>> +             * to the usage of the high 16 bits to store the cluster ID.
> >>> +             */
> >>> +            x2apic_phys = true;
> >>> +        else
> >>> +            x2apic_phys = !!(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
> >>
> >> ... I wonder why you didn't make this
> >>
> >>         x2apic_phys = !iommu_intremap || (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
> >>
> >> (not the least because of allowing to drop the somewhat ugly !!).
> > 
> > Feel free to do it at commit (and reindent the comment), or else I can
> > resend a new version if that's too intrusive.
> 
> Doing these adjustments at commit time ought to be fine. It's
> just that I'd prefer to wait with committing this series until
> 4.13 is fully finished.

That's fine, I don't have any hurry. All patches are Acked or RB now,
so I will hold off sending a new version. Let me know if the patches
don't apply cleanly when committing and I can send an updated version
with the minor comments from this last round.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v3 3/4] x86/smp: check APIC ID on AP bringup

[Jan Beulich]

On 04.12.2019 17:20, Roger Pau Monne wrote:
> Check that the processor to be woken up APIC ID is addressable in the
> current APIC mode.
> 
> Note that in practice systems with APIC IDs > 255 should already have
> x2APIC enabled by the firmware, and hence this is mostly a safety
> belt.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v2:
>  - Reword error message.
> ---
>  xen/arch/x86/smpboot.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index fa691b6ba0..8cbb7173a4 100644
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -1317,6 +1317,13 @@ int __cpu_up(unsigned int cpu)
>      if ( (apicid = x86_cpu_to_apicid[cpu]) == BAD_APICID )
>          return -ENODEV;
>  
> +    if ( (!x2apic_enabled || !iommu_intremap) && (apicid >> 8) )

Btw, I'll change the right side to "apicid >= 0xff", as that ID is
special. Or perhaps this should really be

    if ( (!x2apic_enabled && apicid >= APIC_ALL_CPUS) ||
         (!iommu_intremap && (apicid >> 8)) )

Thoughts?

Jan

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

Re: [Xen-devel] [PATCH v3 3/4] x86/smp: check APIC ID on AP bringup

[Andrew Cooper]

On 20/12/2019 15:17, Jan Beulich wrote:
> On 04.12.2019 17:20, Roger Pau Monne wrote:
>> Check that the processor to be woken up APIC ID is addressable in the
>> current APIC mode.
>>
>> Note that in practice systems with APIC IDs > 255 should already have
>> x2APIC enabled by the firmware, and hence this is mostly a safety
>> belt.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> Changes since v2:
>>  - Reword error message.
>> ---
>>  xen/arch/x86/smpboot.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
>> index fa691b6ba0..8cbb7173a4 100644
>> --- a/xen/arch/x86/smpboot.c
>> +++ b/xen/arch/x86/smpboot.c
>> @@ -1317,6 +1317,13 @@ int __cpu_up(unsigned int cpu)
>>      if ( (apicid = x86_cpu_to_apicid[cpu]) == BAD_APICID )
>>          return -ENODEV;
>>  
>> +    if ( (!x2apic_enabled || !iommu_intremap) && (apicid >> 8) )
> Btw, I'll change the right side to "apicid >= 0xff", as that ID is
> special. Or perhaps this should really be
>
>     if ( (!x2apic_enabled && apicid >= APIC_ALL_CPUS) ||
>          (!iommu_intremap && (apicid >> 8)) )
>
> Thoughts?

LGTM.

~Andrew

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