mirror of
https://github.com/ARMSX2/ARMSX3.git
synced 2026-08-24 16:58:52 -07:00
RSX: second hardening round for the semaphore wait, from re-review
A blind re-review of the previous commit (six lenses, fresh reviewers)
found real gaps in the hardening itself. Addressed here:
- The EVTSTRM gate failed open to the spin: with the event stream absent
on a core whose armed WFE does not park, disabling the fallback
reinstated the original full-rate spin. The paced tier now degrades to
a 100 us scheduler sleep instead, which also keeps the timeout and
service polls running at a bounded cadence.
- Gate the FIFO-idle wait_for_event() the same way (three reviewers
independently flagged the contradiction between asm.hpp's new
precondition and this ungated sibling). Without the stream it yields,
which is that path's pre-WFE behavior.
- Non-Linux ARM64 now defaults to the previous commit's behavior instead
of silently disabling the fallback: the false default was a regression
against 002a9b274 on the Apple Silicon and Windows-on-ARM targets, and
no HWCAP equivalent exists there to probe.
- The loop's snapshot is now read through the existing atomic reference
(relaxed observe()) instead of a plain reference: the previous form was
a formal data race whose correct codegen depended on an unrelated
virtual call staying opaque to the optimizer.
- Guard unaligned semaphore addresses on the acquire path: exclusive
loads fault on unaligned addresses, semaphore_release already rejects
them, and acquire did not. Unaligned waits now use the paced tier only,
with a warning.
- Surface the probe in the startup capability string (EVTSTRM-on/off) so
every log records which wait shape was selected; previously the three
possible states were indistinguishable in any output.
- Log the first-observed semaphore value in the recovery-timeout message
as well; the previous message could not distinguish a value that
changed during the wait from one that never moved.
- Comment corrections: the post-budget wake-on-write claim now states the
pacing-period bound honestly; the x86 note names the yield fallback on
CPUs without waitpkg/mwaitx; the event-stream period is stated as a
kernel-dependent range. Note the previous commit's claim that x86 was
unaffected was wrong: the snapshot change lets the x86 early-out fire
where it previously compared a value against itself; the direction is
an earlier return when the semaphore changed during the prologue.
Device check (Odin 3, ME menu, 30 s): 168.0G instructions, and the new
capability line reads EVTSTRM-on, proving the paced branch was live in
the measured run. Known residuals (ledgered, out of scope): HWCAP is a
boot-time global while the stream enable is per-CPU (migration edge);
no parking-core device has been measured; no automated test covers the
path.
This commit is contained in:
@@ -6,6 +6,9 @@
|
||||
#include "Emu/RSX/rsx_profiler.h"
|
||||
#include "util/sysinfo.hpp"
|
||||
|
||||
#include <chrono>
|
||||
#include <thread>
|
||||
|
||||
#include "context_accessors.define.h"
|
||||
|
||||
namespace rsx
|
||||
@@ -52,17 +55,31 @@ namespace rsx
|
||||
u64 start = get_system_time();
|
||||
u64 last_check_val = start;
|
||||
|
||||
// Kept for the timeout log: lets a report distinguish a semaphore that
|
||||
// changed after we started waiting from one that never moved at all.
|
||||
const u32 first_observed = static_cast<u32>(sema);
|
||||
|
||||
// The exclusive-load wait below faults on an unaligned address; the
|
||||
// release side ignores unaligned semaphores, so mirror that here and
|
||||
// fall back to a plain paced wait instead of the armed one.
|
||||
const bool aligned = (addr % 4) == 0;
|
||||
|
||||
if (!aligned)
|
||||
{
|
||||
rsx_log.warning("NV406E semaphore acquire is using an unaligned semaphore; using unmonitored waits. (address=0x%x)", addr);
|
||||
}
|
||||
|
||||
#if defined(ARCH_ARM64)
|
||||
u64 spin_budget = 0;
|
||||
// Without the kernel's architected timer event stream a monitor-less
|
||||
// WFE has no bounded wake, so the event-stream fallback below must
|
||||
// stay disabled and the wait keeps the armed-spin shape.
|
||||
// Whether wait_for_event() below has a bounded wake on this kernel.
|
||||
// Without it the tiered wait degrades to a timed sleep rather than
|
||||
// re-exposing the unpaced spin (or an unbounded park).
|
||||
const bool has_event_stream = utils::has_wfe_event_stream();
|
||||
#endif
|
||||
|
||||
while (true)
|
||||
{
|
||||
const RsxSemaphore observed = sema;
|
||||
const RsxSemaphore observed = atomic_sema.observe();
|
||||
|
||||
if (observed == arg)
|
||||
{
|
||||
@@ -91,11 +108,12 @@ namespace rsx
|
||||
|
||||
if ((current - start) > tdr)
|
||||
{
|
||||
// If longer than driver timeout force exit. The awaited and
|
||||
// last-observed values are logged so a timeout caused by a
|
||||
// transient value (stored, then overwritten before the waiter
|
||||
// observed it) is distinguishable from a never-signaled one.
|
||||
rsx_log.error("nv406e::semaphore_acquire has timed out. semaphore_address=0x%X, awaited=0x%X, observed=0x%X", addr, arg, static_cast<u32>(observed));
|
||||
// If longer than driver timeout force exit. first/last observed
|
||||
// let a report distinguish a value that changed during the wait
|
||||
// (first != last, or last != first-known-stuck) from one that
|
||||
// never moved; a transient hit of the awaited value between
|
||||
// observations remains invisible by nature.
|
||||
rsx_log.error("nv406e::semaphore_acquire has timed out. semaphore_address=0x%X, awaited=0x%X, first_observed=0x%X, last_observed=0x%X", addr, arg, first_observed, static_cast<u32>(observed));
|
||||
break;
|
||||
}
|
||||
}
|
||||
@@ -118,21 +136,44 @@ namespace rsx
|
||||
// second instead of waiting. Give the armed form a short window first - on
|
||||
// cores where it parks it keeps its instant wake-on-write, and where it
|
||||
// does not it acts as a brief spin that still catches short waits - then
|
||||
// interleave the event-stream park, which is what actually paces the loop
|
||||
// on a core whose armed WFE spins. The armed one-shot still runs on every
|
||||
// iteration (it costs ~nothing where it is broken), so on cores where it
|
||||
// parks, wake-on-write is kept even after the budget is spent.
|
||||
if (has_event_stream && ++spin_budget > 500)
|
||||
// interleave a paced wait: the event-stream park when the kernel provides
|
||||
// the stream, a timed sleep when it does not. The armed one-shot still
|
||||
// runs each iteration afterwards, so on cores where it parks, a store
|
||||
// wakes the second half of every post-budget iteration promptly; during
|
||||
// the paced half a store is only seen at the next tick, so post-budget
|
||||
// wake latency on such cores is bounded by (not free of) the pacing
|
||||
// period.
|
||||
if (++spin_budget > 500)
|
||||
{
|
||||
utils::wait_for_event();
|
||||
if (has_event_stream)
|
||||
{
|
||||
utils::wait_for_event();
|
||||
}
|
||||
else
|
||||
{
|
||||
// No event stream: neither WFE form has a wake this code can
|
||||
// bound, so pace with the scheduler instead of spinning or
|
||||
// parking blind. Also keeps the timeout and service polls
|
||||
// above running at a bounded cadence.
|
||||
std::this_thread::sleep_for(std::chrono::microseconds(100));
|
||||
}
|
||||
}
|
||||
|
||||
if (!aligned)
|
||||
{
|
||||
// Exclusive loads fault on unaligned addresses; rely on the
|
||||
// pacing above plus the plain re-read at the loop top.
|
||||
utils::pause();
|
||||
continue;
|
||||
}
|
||||
#endif
|
||||
// On x86 this waits on the address (umwait/mwaitx), bounded by the
|
||||
// timeout. On ARM the timeout is ignored: the wait ends on a write
|
||||
// to the armed cache line or an event-stream tick on cores where
|
||||
// the armed WFE parks, and returns immediately on cores where it
|
||||
// does not (measured on Oryon), where wait_for_event() above
|
||||
// provides the pacing instead.
|
||||
// On x86 with waitpkg/mwaitx this waits on the address, bounded by
|
||||
// the timeout; without those extensions it degrades to a yield. On
|
||||
// ARM the timeout is ignored: the wait ends on a write to the armed
|
||||
// cache line or an event-stream tick on cores where the armed WFE
|
||||
// parks, and returns immediately on cores where it does not
|
||||
// (measured on Oryon), where the paced wait above sets the loop's
|
||||
// cadence instead.
|
||||
utils::spin_on_cacheline_once(atomic_sema, observed, 100);
|
||||
}
|
||||
|
||||
|
||||
@@ -7,6 +7,7 @@
|
||||
#include "Core/RSXReservationLock.hpp"
|
||||
#include "Emu/Memory/vm_reservation.h"
|
||||
#include "Emu/Cell/lv2/sys_rsx.h"
|
||||
#include "util/sysinfo.hpp"
|
||||
#include "NV47/HW/context.h"
|
||||
#include "rsx_profiler.h"
|
||||
|
||||
@@ -804,10 +805,18 @@ namespace rsx
|
||||
s_fifo_idle_spins++;
|
||||
utils::pause();
|
||||
}
|
||||
else
|
||||
else if (utils::has_wfe_event_stream())
|
||||
{
|
||||
utils::wait_for_event();
|
||||
}
|
||||
else
|
||||
{
|
||||
// Without the event stream a monitor-less WFE has no bounded
|
||||
// wake, so a park here could sleep through the guest's PUT
|
||||
// advance until an unrelated interrupt. Yield instead - the
|
||||
// pre-WFE behavior of this path.
|
||||
std::this_thread::yield();
|
||||
}
|
||||
#else
|
||||
std::this_thread::yield();
|
||||
#endif
|
||||
|
||||
+3
-2
@@ -189,8 +189,9 @@ namespace utils
|
||||
// Park the core until an event arrives, with no syscall.
|
||||
//
|
||||
// On arm64 WFE drops the core into a low-power state. When the kernel enables the
|
||||
// architected event stream a wakeup arrives on a fixed short period (tens of
|
||||
// microseconds), and timer interrupts wake it regardless -- it cannot stall
|
||||
// architected event stream a wakeup arrives on a fixed short period (kernel- and
|
||||
// timer-frequency dependent: measured ~30-60 us on one device, kernel defaults
|
||||
// target ~100 us), and timer interrupts wake it regardless -- it cannot stall
|
||||
// indefinitely. The event stream is a kernel property, not a guarantee: check
|
||||
// utils::has_wfe_event_stream() (HWCAP_EVTSTRM) before making this wait
|
||||
// load-bearing; without the stream the park lasts until the next unrelated
|
||||
|
||||
@@ -404,9 +404,11 @@ bool utils::has_wfe_event_stream()
|
||||
// next unrelated interrupt.
|
||||
return (getauxval(AT_HWCAP) & HWCAP_EVTSTRM) != 0;
|
||||
#else
|
||||
// Unknown platforms: report false so callers stay on wait shapes that
|
||||
// do not depend on the event stream.
|
||||
return false;
|
||||
// Non-Linux ARM64 (Apple, Windows-on-ARM): no HWCAP equivalent exists.
|
||||
// Report true so callers keep the same wait shapes they used before this
|
||||
// probe existed; a platform where the stream-paced wait misbehaves needs
|
||||
// a measured probe here, not a capability bit.
|
||||
return true;
|
||||
#endif
|
||||
}();
|
||||
return g_value;
|
||||
@@ -597,6 +599,10 @@ std::string utils::get_system_info()
|
||||
{
|
||||
result += " | Neon";
|
||||
}
|
||||
|
||||
// Surfaced so every log records whether monitor-less WFE waits have a
|
||||
// bounded wake on this kernel (drives the RSX wait-shape selection).
|
||||
fmt::append(result, " | EVTSTRM-%s", has_wfe_event_stream() ? "on" : "off");
|
||||
#else
|
||||
|
||||
if (has_avx())
|
||||
|
||||
Reference in New Issue
Block a user