SCSI: Fix Persistent Reservation (PR) NVMe->SCSI conversion of HostID to Transport ID (#4383) - #4407
Merged
Alex Landau (alandau) merged 1 commit intoSep 9, 2026
Conversation
… to Transport ID (microsoft#4383) Just copying it, as was done before, is wrong since HostID is 8 or 16 bytes (random-ish) and TransportID is 24 bytes (and has structure). This change follows the [T10 SPC-3 spec](https://www.t10.org/ftp/t10/document.02/02-246r1.pdf) for the format of the TransportID retaining backward compatibility for NVMe HostIDs that start with `06 00 00 00`. This last part can happen when the NVMe HostID itself is incorrectly derived from an underlying SCSI TransportID by just copying bytes. Before: ``` # sg_persist --in --read-full-status /dev/sda OpenVMM Disk 1.0 Peripheral device type: disk PR generation=0x1 Key=0x1234 All target ports bit set not reservation holder Transport Id short or not multiple of 4 [length=1024]: SAS address: 0xbd591cd38dc846b6 # sg_raw -r 8192 /dev/sda 5e 03 00 00 00 00 00 20 00 00 | hexdump -C SCSI Status: Good Received 48 bytes of data: 00 00 00 00 18 00 00 00 28 ff 00 7c 80 09 2f 6d 08 .......(..|../m. 10 00 00 00 00 02 00 00 00 00 00 00 00 00 00 00 10 ................ 20 06 00 00 00 bd 59 1c d3 8d c8 46 b6 99 68 1d 91 .....Y....F..h.. ``` After: ``` alpine:~# sg_persist --out --register --param-rk=0 --param-sark=0x1234 /dev/sda OpenVMM Disk 1.0 Peripheral device type: disk alpine:~# sg_persist --in --read-keys /dev/sda OpenVMM Disk 1.0 Peripheral device type: disk PR generation=0x1, 1 registered reservation key follows: 0x1234 alpine:~# sg_persist --in --read-full-status /dev/sda OpenVMM Disk 1.0 Peripheral device type: disk PR generation=0x1 Key=0x1234 All target ports bit set not reservation holder Transport Id of initiator: SAS address: 0x1020304050607 alpine:~# sg_raw -r 8192 /dev/sda 5e 03 00 00 00 00 00 20 00 00 SCSI Status: Good Received 56 bytes of data: 00 00 00 00 01 00 00 00 30 00 00 00 00 00 00 12 34 .......0.......4 10 00 00 00 00 02 00 00 00 00 00 00 00 00 00 00 18 ................ 20 06 00 00 00 00 01 02 03 04 05 06 07 08 09 0a 0b ................ 30 0c 0d 0e 0f 00 00 00 00 ........ ``` (cherry picked from commit ab10638)
Matt LaFayette (Kurjanowicz) (mattkur)
approved these changes
Sep 9, 2026
Alex Landau (alandau)
enabled auto-merge (squash)
September 9, 2026 21:09
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The truncation test helper still assumes an 8-byte Transport ID payload, leaving coverage inconsistent with the new 24-byte READ FULL STATUS descriptor format.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes the NVMe HostID → SCSI Persistent Reservation “Transport ID” conversion by emitting a properly sized/structured 24-byte SAS SCSI Transport ID (per SPC-3 expectations) instead of copying the HostID bytes directly, improving compatibility with SCSI initiator reporting tools.
Changes:
- Build a 24-byte SAS Transport ID (
06 00 00 00 ...) from the NVMe HostID when generating READ FULL STATUS descriptors. - Update PR READ FULL STATUS test helpers to expect a 24-byte additional descriptor payload and avoid potential slice overrun in the test response builder.
File summaries
| File | Description |
|---|---|
| vm/devices/storage/scsidisk/src/reservation.rs | Emit a 24-byte SAS Transport ID derived from NVMe HostID for PR READ FULL STATUS descriptors (with rate-limited warning for legacy HostIDs that already look like a Transport ID prefix). |
| vm/devices/storage/scsidisk/src/tests/pr_tests.rs | Adjust expected READ FULL STATUS descriptor sizing to 24 bytes and make the helper copy bounded by the provided buffer length. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
354
to
360
| let mut read_keys_response = vec![0u8; size_of::<scsi::PriRegistrationListHeader>() + 8]; | ||
| let mut read_full_status_response = vec![ | ||
| 0u8; | ||
| size_of::<scsi::PriFullStatusListHeader>() | ||
| + size_of::<scsi::PriFullStatusDescriptorHeader>() | ||
| + 8 | ||
| + 24 | ||
| ]; |
Ben Hillis (benhillis)
approved these changes
Sep 9, 2026
Alex Landau (alandau)
merged commit Sep 9, 2026
4c2938f
into
microsoft:release/1.8.2607
65 of 98 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clean cherry pick of PR #4383
Just copying it, as was done before, is wrong since HostID is 8 or 16 bytes (random-ish) and TransportID is 24 bytes (and has structure).
This change follows the T10 SPC-3 spec for the format of the TransportID retaining backward compatibility for NVMe HostIDs that start with
06 00 00 00. This last part can happen when the NVMe HostID itself is incorrectly derived from an underlying SCSI TransportID by just copying bytes.Before:
After: