[v3,2/4] x86/apic: force phys mode if interrupt remapping is disabled
diff mbox series

Message ID 20191204162025.37510-3-roger.pau@citrix.com
State New
Headers show
Series
  • x86: enable x2APIC mode regardless of interrupt remapping support
Related show

Commit Message

Roger Pau Monne Dec. 4, 2019, 4:20 p.m. UTC
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(-)

Comments

Jan Beulich Dec. 5, 2019, 9:32 a.m. UTC | #1
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
Roger Pau Monne Dec. 9, 2019, 10:25 a.m. UTC | #2
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.
Jan Beulich Dec. 9, 2019, 2:30 p.m. UTC | #3
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
Roger Pau Monne Dec. 9, 2019, 2:50 p.m. UTC | #4
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.

Patch
diff mbox series

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;