On 04/07/2026 18:17, Keith Monahan wrote:
Can I get some eyes on this approach please?
Here's my proposal to break up this large rewrite in reviewable sized chunks:
1. lasi_ncr710: remove dead and shadowing defines
2. lasi_ncr710: compute hw identifiers instead of caching them
3. ncr53c710: remove the redundant standalone SysBus device
4. ncr53c710: prepare state, register names and tracepoints
5. ncr53c710: rewrite the status and message phase handlers
6. ncr53c710: rewrite reselection, queueing and the data completion path
7. ncr53c710: rewrite the register read path
8. ncr53c710: rewrite the register write path and soft reset
9. ncr53c710: rewrite the SCRIPTS execution engine
10. ncr53c710: complete cached-backend transfers asynchronously
11. ncr53c710: support HP-UX overlapped tagged commands
12. ncr53c710: extend MESSAGE REJECT handling for HP-UX
To be clear, I will be modifying ncr53c710.c in place. And, of course, modifying the
other files as necessary. Those changes are minimal.
Every patch will buildable. There is a non-booting window between patches 5-8 as the
new helpers must land before the new loop that matches them, so the device compiles
but can't drive I/O until they reconverge.
How does this sound?
Keith
Hi Keith,
Thank you for your contribution to QEMU. I found myself in a similar situation a
couple of years back when I ended up reworking a large part of the ESP SCSI emulation
to fix some fairly obscure edge cases booting Classic MacOS, so I understand how
difficult a task this can be.
If you would like to take a look at my series and cover letter then you can find it
at https://patchew.org/QEMU/[email protected]/,
but I've summarised some thoughts below based upon my experience:
- I focused on using a small set of reference images and made sure they would
boot upon every commit. This was really helpful as sometimes even a subtle
code
movement would cause something to break unexpectedly, and helped me to find
extra edge cases I had missed.
- Keep changes to individual devices in separate commits where possible, and
also
the same goes for refactoring/renaming commits and changes to the device
logic/functionality. Often these commits can be merged earlier than later
patches in the series where the reviewer may have more questions.
- Not all reviewers are familiar with SCSI, so writing a good cover letter
explaining each part of the series will help reviewers understand the
changes
during review.
- Run your local tests in both a normal and --enable-debug build, and also
with
asan (--enable-asan) to catch any memory errors
- Adding a functional test if possible would help prevent future regressions.
Similarly for reviewers I should note that the LSI SCRIPTS engine is quite complex:
in effect it is almost like a separate processor executing its own programs (SCRIPTS)
to perform I/O.
As I remarked in my ESP series cover letter, it is almost impossible for someone to
perform a comprehensive review without being very familiar with the controller and
its datasheet, so when attempting to review the series, I would probably focus on
looking how it compares with the existing lsi53c895a code which is mature and working
well. For me this would mean that adding notes explaining any unusual differences
between the two devices on the commit message would be extremely helpful during review.
You mentioned above the problem with implementing new helpers causing boot to break
between patches 5 and 8: is it possible to bring in the new logic/helpers a phase at
a time (like I did with ESP) to help keep things working? Perhaps a temporary lock to
enable the async helpers to work in a sync manner temporarily could also help?
ATB,
Mark.
