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

Roger Pau Monne posted 4 patches 12 weeks ago

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

Posted by Roger Pau Monne 12 weeks ago
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 v1:
 - New in this version.
---
 xen/arch/x86/genapic/x2apic.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/arch/x86/genapic/x2apic.c b/xen/arch/x86/genapic/x2apic.c
index d5a17f10d5..7e32ee22ff 100644
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -228,6 +228,14 @@ const struct genapic *__init apic_x2apic_probe(void)
     if ( x2apic_phys < 0 )
         x2apic_phys = !!(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
 
+    if ( !x2apic_phys && !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;
+
     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

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

Posted by Roger Pau Monné 12 weeks ago
On Fri, Nov 29, 2019 at 12:28:49PM +0100, 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>

This is missing a command line doc update and the logic below ignores
a user-set x2apic_phys value. Will wait for comments on other patches
before resending, sorry.

Roger.

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

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

Posted by Jan Beulich 11 weeks ago
On 29.11.2019 12:38, Roger Pau Monné  wrote:
> On Fri, Nov 29, 2019 at 12:28:49PM +0100, 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>
> 
> This is missing a command line doc update and the logic below ignores
> a user-set x2apic_phys value.

So what would the behavior be in your opinion when the user
has requested cluster mode? I can't see you do much other
than panic()-ing, perhaps it's better to override the request
(as you already do)?

Jan

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

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

Posted by Roger Pau Monné 11 weeks ago
On Tue, Dec 03, 2019 at 04:14:46PM +0100, Jan Beulich wrote:
> On 29.11.2019 12:38, Roger Pau Monné  wrote:
> > On Fri, Nov 29, 2019 at 12:28:49PM +0100, 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>
> > 
> > This is missing a command line doc update and the logic below ignores
> > a user-set x2apic_phys value.
> 
> So what would the behavior be in your opinion when the user
> has requested cluster mode? I can't see you do much other
> than panic()-ing, perhaps it's better to override the request
> (as you already do)?

I think panic'ing is fine, a user shouldn't be setting x2apic_phys
unless they know what are doing, and then Xen changing it on the back
of the user also doesn't seem fine.

A panic explaining that x2apic_phys=false is not supported and that
the box can only be booted with x2apic phys mode should be fine IMO.

Thanks, Roger.

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

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

Posted by Jan Beulich 11 weeks ago
On 04.12.2019 10:17, Roger Pau Monné wrote:
> On Tue, Dec 03, 2019 at 04:14:46PM +0100, Jan Beulich wrote:
>> On 29.11.2019 12:38, Roger Pau Monné  wrote:
>>> On Fri, Nov 29, 2019 at 12:28:49PM +0100, 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>
>>>
>>> This is missing a command line doc update and the logic below ignores
>>> a user-set x2apic_phys value.
>>
>> So what would the behavior be in your opinion when the user
>> has requested cluster mode? I can't see you do much other
>> than panic()-ing, perhaps it's better to override the request
>> (as you already do)?
> 
> I think panic'ing is fine, a user shouldn't be setting x2apic_phys
> unless they know what are doing, and then Xen changing it on the back
> of the user also doesn't seem fine.
> 
> A panic explaining that x2apic_phys=false is not supported and that
> the box can only be booted with x2apic phys mode should be fine IMO.

I can see this as a valid position to take. Personally, however, I
do think we should avoid failing to boot if we easily can. (Yes, we
should log the fact that we ignore a command line option in such a
case.)

Jan

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

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

Posted by Roger Pau Monné 11 weeks ago
On Wed, Dec 04, 2019 at 10:34:14AM +0100, Jan Beulich wrote:
> On 04.12.2019 10:17, Roger Pau Monné wrote:
> > On Tue, Dec 03, 2019 at 04:14:46PM +0100, Jan Beulich wrote:
> >> On 29.11.2019 12:38, Roger Pau Monné  wrote:
> >>> On Fri, Nov 29, 2019 at 12:28:49PM +0100, 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>
> >>>
> >>> This is missing a command line doc update and the logic below ignores
> >>> a user-set x2apic_phys value.
> >>
> >> So what would the behavior be in your opinion when the user
> >> has requested cluster mode? I can't see you do much other
> >> than panic()-ing, perhaps it's better to override the request
> >> (as you already do)?
> > 
> > I think panic'ing is fine, a user shouldn't be setting x2apic_phys
> > unless they know what are doing, and then Xen changing it on the back
> > of the user also doesn't seem fine.
> > 
> > A panic explaining that x2apic_phys=false is not supported and that
> > the box can only be booted with x2apic phys mode should be fine IMO.
> 
> I can see this as a valid position to take. Personally, however, I
> do think we should avoid failing to boot if we easily can. (Yes, we
> should log the fact that we ignore a command line option in such a
> case.)

Ack, I don't have a strong opinion, so I would go this route.

Thanks, Roger.

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