| Message ID | 20191204162025.37510-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: enable x2APIC mode regardless of interrupt remapping support | expand |
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
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.
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
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 )
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(-)