Re: [BUG] x2apic broken with current AMD hardware

[Elliott Mitchell <ehem+xen@xxxxxxx>]

On Mon, Mar 20, 2023 at 09:14:17AM +0100, Jan Beulich wrote:
> On 17.03.2023 18:26, Elliott Mitchell wrote:
> > On Fri, Mar 17, 2023 at 09:22:09AM +0100, Jan Beulich wrote:
> >> On 16.03.2023 23:03, Elliott Mitchell wrote:
> >>> On Mon, Mar 13, 2023 at 08:01:02AM +0100, Jan Beulich wrote:
> >>>> On 11.03.2023 01:09, Elliott Mitchell wrote:
> >>>>> On Thu, Mar 09, 2023 at 10:03:23AM +0100, Jan Beulich wrote:
> >>>>>>
> >>>>>> In any event you will want to collect a serial log at maximum 
> >>>>>> verbosity.
> >>>>>> It would also be of interest to know whether turning off the IOMMU 
> >>>>>> avoids
> >>>>>> the issue as well (on the assumption that your system has less than 255
> >>>>>> CPUs).
> >>>>>
> >>>>> I think I might have figured out the situation in a different fashion.
> >>>>>
> >>>>> I was taking a look at the BIOS manual for this motherboard and noticed
> >>>>> a mention of a "Local APIC Mode" setting.  Four values are listed
> >>>>> "Compatibility", "xAPIC", "x2APIC", and "Auto".
> >>>>>
> >>>>> That is the sort of setting I likely left at "Auto" and that may well
> >>>>> result in x2 functionality being disabled.  Perhaps the x2APIC
> >>>>> functionality on AMD is detecting whether the hardware is present, and
> >>>>> failing to test whether it has been enabled?  (could be useful to output
> >>>>> a message suggesting enabling the hardware feature)
> >>>>
> >>>> Can we please move to a little more technical terms here? What is 
> >>>> "present"
> >>>> and "enabled" in your view? I don't suppose you mean the CPUID bit (which
> >>>> we check) and the x2APIC-mode-enable one (which we drive as needed). It's
> >>>> also left unclear what the four modes of BIOS operation evaluate to. Even
> >>>> if we knew that, overriding e.g. "Compatibility" (which likely means some
> >>>> form of "disabled" / "hidden") isn't normally an appropriate thing to do.
> >>>> In "Auto" mode Xen likely should work - the only way I could interpret 
> >>>> the
> >>>> the other modes are "xAPIC" meaning no x2APIC ACPI tables entries (and
> >>>> presumably the CPUID bit also masked), "x2APIC" meaning x2APIC mode pre-
> >>>> enabled by firmware, and "Auto" leaving it to the OS to select. Yet 
> >>>> that's
> >>>> speculation on my part ...
> >>>
> >>> I provided the information I had discovered.  There is a setting for this
> >>> motherboard (likely present on some similar motherboards) which /may/
> >>> effect the issue.  I doubt I've tried "compatibility", but none of the
> >>> values I've tried have gotten the system to boot without "x2apic=false"
> >>> on Xen's command-line.
> >>>
> >>> When setting to "x2APIC" just after "(XEN) AMD-Vi: IOMMU Extended 
> >>> Features:"
> >>> I see the line "(XEN) - x2APIC".  Later is the line
> >>> "(XEN) x2APIC mode is already enabled by BIOS."  I'll guess "Auto"
> >>> leaves the x2APIC turned off since neither line is present.
> >>
> >> When "(XEN) - x2APIC" is absent the IOMMU can't be switched into x2APIC
> >> mode. Are you sure that's the case when using "Auto"?
> > 
> > grep -eAPIC\ driver -e-\ x2APIC:
> > 
> > "Auto":
> > (XEN) Using APIC driver default
> > (XEN) Overriding APIC driver with bigsmp
> > (XEN) Switched to APIC driver x2apic_cluster
> > 
> > "x2APIC":
> > (XEN) Using APIC driver x2apic_cluster
> > (XEN) - x2APIC
> > 
> > Yes, I'm sure.
> 
> Okay, this then means we're running in a mode we don't mean to run
> in: When the IOMMU claims to not support x2APIC mode (which is odd in
> the first place when at the same time the CPU reports x2APIC mode as
> supported), amd_iommu_prepare() is intended to switch interrupt
> remapping mode to "restricted" (which in turn would force x2APIC mode
> to "physical", not "clustered"). I notice though that there are a
> number of error paths in the function which bypass this setting. Could
> you add a couple of printk()s to understand which path is taken (each
> time; the function can be called more than once)?

If I'm reading the logs right, this could sugggest "Local APIC Mode"
setting was modifying the IOMMU side of the APIC, not the processor side
of the APIC setting.

There is also a "Compatibility" value which I haven't tried.  Perhaps
taking a look would be interesting.

> >>> Both cases the line "(XEN) Switched to APIC driver x2apic_cluster" is
> >>> present (so perhaps "Auto" merely doesn't activate it).
> >>
> >> Did you also try "x2apic_phys" on the Xen command line (just to be sure
> >> this isn't a clustered-mode only issue)?
> > 
> > No.  In fact x2apic_cluster is mentioned in all failure cases.
> 
> Could you give physical mode a try, please?

I had taken the previous message as an implicit request to do so.  I was
stating I had not previously done so.  While "x2apic=false" is functional
as a workaround, I really would like to get to the bottom of this.

> >>> Appears error_interrupt() needs locking or some concurrency handling
> >>> mechanism since the last error is jumbled.  With the setting "x2APIC"
> >>> I get a bunch of:
> >>> "(XEN) APIC error on CPU#: 00(08)(XEN) APIC error on CPU#: 00(08)"
> >>> (apparently one for each core)
> >>> Followed by "Receive accept error, Receive accept error," (again,
> >>> apparently one for each core).  Then a bunch of newlines (same pattern).
> >>
> >> This is a known issue, but since the messages shouldn't appear in the
> >> first place so far no-one has bothered to address this.
> > 
> > I won't claim it is the best solution, but see other message.
> > 
> > I'm tempted to propose allowing _Static_assert() since it is valuable
> > functionality for preventing issues.
> 
> How does _Static_assert() come into play here? Also note that we already
> use it when available ...

This is more in relation to the patch.  Appears GCC's C90 mode disables
_Static_assert(), so the _Static_assert(ARRAY_SIZE(args) == 8) had to be
dropped.


Come to think of it, I should have copied Neowutran on this thread as
everyone else has been running into this too.  Perhaps "x2apic=false" has
gotten less attention since it is a single-shot workaround.  Whereas
"tsc_mode = 1" needs to be repeated in every domain's configuration.


Neowutran, I'm unsure whether you remain subscribed to this list or not.
As such I'm attaching a copy of Jan Beulich's proposed patch, which may
(not yet tested) solve the "x2apic=false" issue.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@xxxxxxx  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

Attachment: x2apic.patch
Description: Text Data