| Message ID | 20191129112851.19273-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: enable x2APIC mode regardless of interrupt remapping support | expand |
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.
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
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.
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
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.
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;
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(+)