Skip to content

SCSI: Fix Persistent Reservation (PR) NVMe->SCSI conversion of HostID to Transport ID (#4383) - #4407

Merged
Alex Landau (alandau) merged 1 commit into
microsoft:release/1.8.2607from
alandau:cherrypick/release/1.8.2607/pr-4383
Sep 9, 2026
Merged

Alex Landau (alandau) merged 1 commit into
microsoft:release/1.8.2607from
alandau:cherrypick/release/1.8.2607/pr-4383

Conversation

@alandau

Copy link
Copy Markdown
Contributor

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:

# 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                             ........

… 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)
Copilot AI lite review requested due to automatic review settings September 9, 2026 21:02
@github-actions github-actions Bot added the release_1.8.2607 Targets the release/1.8.2607 branch. label Sep 9, 2026
@alandau
Alex Landau (alandau) enabled auto-merge (squash) September 9, 2026 21:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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
];
Comment thread vm/devices/storage/scsidisk/src/reservation.rs
@alandau
Alex Landau (alandau) merged commit 4c2938f into microsoft:release/1.8.2607 Sep 9, 2026
65 of 98 checks passed
@github-actions

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_1.8.2607 Targets the release/1.8.2607 branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants