Execute the ARMv8-M atomics yaxpeax 0.5.0 now decodes - #15
Conversation
`execute` routes the ARMv8-M load-acquire and store-release family to `try_v8m`. That routine reads the encoding directly and models the acquire, release, and exclusive semantics as plain accesses on a single core. yaxpeax 0.5.0 names these opcodes where 0.4.0 rejected them. The decode-failure path that used to carry them no longer runs. Both entry paths remain. The routine serves either decoder version. RRX rotates right by one through carry. Its encoding carries a shift amount of zero. `shift_c` returned early on a zero amount, so an RRX shifting a data-processing operand left the value untouched. The RRX case now runs before that early return. `do_shift` takes a carry-in, so the operand and addressing paths pass the core's `C`. Four tests cover the new paths: a byte store-exclusive read back through a load-acquire, the word-size LDA and STL that carry no size suffix, an RRX-shifted operand through `shift_c`, and an ADD whose second operand is RRX-shifted.
|
I had approved and merged previous changes by iximeow based on code inspection, not testing. Testing uncovered problems fixed here. |
There was a problem hiding this comment.
I will be more explicit here than I was in #14: I was pretty disappointed to learn that sp-emu used yaxpeax-arm without anyone bothering to mention it, and that disappointment grew into mild offense as I found commits in the (now squashed) history of @sion42x committing "fixes" for bugs that amount to "a user of the library includes some bit patterns to avoid the library entirely". again, without anyone bothering to mention the issues, and without really keeping track of what you might want to mention upstream, or be aware of when upstream fixes happen.
obviously I'm not expecting attribution, the library is 0BSD for a reason. I would hope that we are excited to build on the work of our peers - I often am! - and would at the very least say to "hmm, is this how it's supposed to work". I don't know what people did or didn't even know, since I could easily imagine src/cpu.rs came to exist with no one reading the source themselves to see the comments Claude left! most cynically, maybe the fact that Claude produced all the workarounds is a sign that my hobby projects don't matter, since a few dollars of tokens is a suitable substitute?
as I have tried to impress on @sion42x in previous conversations, I don't particularly care how my colleagues choose to do their work. I do care that regardless of the tools we use, we do so with the intention to collaborate with peers as we would typically expect in the course of doing good engineering. here, I'd have hoped that means "reporting issues upstream to the colleague you can find trivially on Matrix", because that certainly would be a nudge towards me that I should flush out remaining ARM bugs and then there is a reusable software library that others can benefit from as well.
given that this is software we intend to emulate the working product, and given who we think might want to run this on their systems, I was pretty surprised with how quickly #14 merged given the curiosity and seemingly incorrect extant behavior. obviously the failure mode here is not "damaged hardware", but it seems like a very quick way for potential customers to get a bad impression of the quality of our engineering if Voxel may be subtly wrong; I was surprised to see how minimal the testing is here given how it's built.
do what you want here; I've clearly got work to do around Thumb, SVE, etc, and there is more to do with A32 generally. I don't plan to review the CPU emulation here and I won't break things further by bumping versions here.
| /// RRX, which rotates through carry by one bit whatever amount arrives. | ||
| fn shift_c(v: u32, style: ShiftStyle, amt: u32, cin: bool) -> (u32, bool) { | ||
| // RRX rotates through carry by one bit (E2.1.348 RRX_C). DecodeImmShift | ||
| // (E2.1.92) pairs it with an amount of 1, and yaxpeax reports 0, so key on |
There was a problem hiding this comment.
this is also a yaxpeax-arm bug, it should report the correct shift amount. i'll fix this at some point upstream.
There was a problem hiding this comment.
For today, putting in comments: TODO: report upstream/remove work-around when fixed in upstream.
There was a problem hiding this comment.
I will follow up later in the week and file issues if you have not already done that.
| /// decode-failure path, for a decoder that rejects them. | ||
| /// hw1 = 1110_1000_110L_Rnnn (0xE8C0 store, 0xE8D0 load); hw2[11:8]=1111, | ||
| /// hw2[7:4] = size/exclusive selector. | ||
| /// hw2[7:4] = size/exclusive selector. Encodings: DDI0553B.s C2.4.68-73 |
There was a problem hiding this comment.
Have you verified these citations? If we're gonna have them it's worth a good check against the PDF.
There was a problem hiding this comment.
Yes, looking these up in the PDF was necessary.
|
|
||
| /// ARM Shift_C: shift with carry-out (the bit last shifted out). `amt == 0` | ||
| /// leaves the value and carry unchanged. | ||
| /// ARM Shift_C (DDI0553B.s E2.1.372): shift with carry-out (the bit last |
There was a problem hiding this comment.
Another new citation, just checking if reviewed/correct.
| /// The word-size acquire/release forms carry no size suffix, so they decode | ||
| /// as their own opcodes rather than as members of the B/H family. A `u32` | ||
| /// atomic uses these forms, so the RoT reaches them first. |
There was a problem hiding this comment.
Is this true? The RoT image only has two lda word forms, and bootleby only uses the byte forms. It sounds like you're describing the ISA instead of the images. If you want to say the word forms are their own opcodes I'd say that instead.
There was a problem hiding this comment.
Yes, the comment describes the ISA The second sentence was not checked and needs to be deleted.
|
@iximeow Thank you for the contribution, and for calling out areas where the code/comments were irresponsible wrt upstream yaxpeax. I'm working on correcting that starting with #16 and would still appreciate input, bumps, and guidance (as well as to contribute issues/fixes upstream) going forward when you're able. |
3 participants
executeroutes the ARMv8-M load-acquire and store-release family totry_v8m. That routine reads the encoding directly and models the acquire, release, and exclusive semantics as plain accesses on a single core. yaxpeax 0.5.0 names these opcodes where 0.4.0 rejected them. The decode-failure path that used to carry them no longer runs. Both entry paths remain. The routine serves either decoder version.RRX rotates right by one through carry. Its encoding carries a shift amount of zero.
shift_creturned early on a zero amount, so an RRX shifting a data-processing operand left the value untouched. The RRX case now runs before that early return.do_shifttakes a carry-in, so the operand and addressing paths pass the core'sC.Four tests cover the new paths: a byte store-exclusive read back through a load-acquire, the word-size LDA and STL that carry no size suffix, an RRX-shifted operand through
shift_c, and an ADD whose second operand is RRX-shifted.