OK. I didn't trust it blindly (though it's tempting). I set it up to test
this overnight, so probably I'll have test failures in the morning.
agape
brent
On Mon, Feb 23, 2026, 10:59 PM Damien Zammit <[email protected]> wrote:
> Hi, no, your AI assistant is hallucinating:
>
> spl_t is of type int which is 4 bytes.
> Therefore I did not forget to patch spl.S, it does not need updating.
> Also you cannot have a CX16 macro that fits in one instruction easily
> because 8 is the maximum stride that instructions support, so I didn't
> bother.
>
> Thanks,
> Damien
>
> Sent from Proton Mail Android
>
>
> -------- Original Message --------
> On 24/2/26 2:27 pm, <[email protected]> wrote:
>
> > Damien Zammit <[email protected]> writes:
> >
> > > curr_spl is actually 4 bytes, but the CX() macro
> > > was expanding to 8 byte stride on x86_64.
> > > Add a new macro specifically for 8 byte widths and
> > > use the correct stride macro for every asm instruction.
> >
> > Hi Damien, Samuel,
> >
> > I'm Brent Baccala's AI assistant. I did a systematic review of every CX
> > macro usage in the gnumach tree and cross-referenced it against this v3
> > patch. The overall approach is good — the CX/CX8 split is clean and
> > the conversions in locore.S and cswitch.S are all correct. The CX(CX())
> > trick for the 16-byte kernel_timer stride is clever, and it also fixes
> > a pre-existing bug on both architectures where the second lea used the
> > wrong register (%edx/%rdx instead of %ecx/%rcx), producing cpu*4 or
> > cpu*8 instead of cpu*16.
> >
> > However, there's a critical issue: x86_64/spl.S is not modified by v3.
> >
> > The commit message says "curr_spl is actually 4 bytes", but on x86_64,
> > spl_t is typedef'd as unsigned long (i386/i386/spl.h line 34), making
> > curr_ipl an array of 8-byte elements. The SPL *values* fit in 32 bits
> > (and the movl/cmpl/xchgl instructions are fine for accessing them), but
> > the array *stride* must be 8 on x86_64 because that's how big each
> > element is.
> >
> > After v3, CX() uses 4-byte stride unconditionally. All 10 curr_ipl
> > accesses in x86_64/spl.S still use CX(), so on SMP, CPU N would read
> > from offset N*4 instead of N*8 — straddling two array elements.
> >
> > The fix is either:
> > (a) Convert all CX(EXT(curr_ipl),...) in x86_64/spl.S to CX8(),
> > keeping the movl/cmpl/xchgl instructions as-is (32-bit access
> > to the low half of an 8-byte element works on little-endian), or
> > (b) Change spl_t from unsigned long to int on x86_64, if there's no
> > reason it needs to be 64-bit. SPL values are small integers.
> >
> > Two additional suggestions from Brent and myself:
> >
> > 1. (Brent's suggestion) Rename CX to CX4. Having CX mean "4-byte stride"
> > is non-obvious, especially since it used to mean "pointer-sized
> stride."
> > CX4/CX8 makes the stride explicit in both macro names and would
> prevent
> > this kind of mistake in the future.
> >
> > 2. (My suggestion) Consider a CX16 macro for the kernel_timer case.
> > The CX(CX()) double-nesting works but is fragile — it relies on
> > 4*4=16, which is a coincidence of the current timer structure size.
> > A CX16(addr, reg) macro that expands to a two-instruction lea
> sequence
> > would be more self-documenting and harder to get wrong.
> >
> > Here's the complete list of CX usages that need CX8 (or would need it
> > if x86_64/spl.S were converted):
> >
> > x86_64/spl.S (NOT in v3 — needs fixing):
> > Line 51: movl CX(EXT(curr_ipl),%rdx),%eax
> > Line 80: cmpl $(SPL0),CX(EXT(curr_ipl),%rdx)
> > Line 82: movl $(SPL0),CX(EXT(curr_ipl),%rdx)
> > Line 127: xchgl CX(EXT(curr_ipl),%rdx),%eax
> > Line 135: cmpl $SPL7,CX(EXT(curr_ipl),%rax)
> > Line 148: cmpl CX(EXT(curr_ipl),%rax),%edx
> > Line 197: cmpl CX(EXT(curr_ipl),%rax),%edx
> > Line 199: movl %edx,CX(EXT(curr_ipl),%rax)
> > Line 219: cmpl $SPL7,CX(EXT(curr_ipl),%rax)
> > Line 238: xchgl CX(EXT(curr_ipl),%rax),%edx
> >
> > x86_64/locore.S (correctly converted in v3):
> > kernel_stack, int_stack_base, int_stack_top, need_ast,
> > current_timer, in_interrupt — all use CX8
> >
> > x86_64/cswitch.S (correctly converted in v3):
> > kernel_stack, int_stack_base — all use CX8
> >
> > x86_64/cpuboot.S:
> > int_stack_top, cpu_id_lut — use hardcoded addressing
> > (correct scale factors, can't easily use CX macros in boot context)
> >
> > The full review report is available if anyone wants to see the complete
> > table of every CX/CX8 usage across both architectures.
> >
> > Best regards,
> > Claude (AI assistant for Brent Baccala)
> >
>
