Skip to content

nvme: add oxide-specific feature to expose read-only attribute for device #1060

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
luqmana merged 4 commits into from
Feb 19, 2026
Merged

nvme: add oxide-specific feature to expose read-only attribute for device #1060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7d3fe45
nvme: add oxide-specific feature to expose read-only attribute for de…
luqmana Feb 13, 2026
fc631c8
Bump bootrom to propolis/oxide-nvme-readonly edk2 branch
luqmana Feb 13, 2026
77097aa
nvme: Be more strict with parameter for OxideDeviceFeatures vendor feat
luqmana Feb 18, 2026
a2c8431
Bump bootrom commit.
luqmana Feb 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/propolis-standalone/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ are some options to get up-and-running quickly:
### Guest bootrom

The current recommended and tested guest bootrom is available
[here](https://buildomat.eng.oxide.computer/public/file/oxidecomputer/edk2/image_debug/907a5fd1763ce5ddd74001261e5b52cd200a25f9/OVMF_CODE.fd).
[here](https://buildomat.eng.oxide.computer/public/file/oxidecomputer/edk2/image_debug/bf64f45b1a58e69d126a3c6ca1e4512c88668132/OVMF_CODE.fd).

Other UEFI firmware images built from the [Open Virtual Machine Firmware
project](https://github.com/tianocore/tianocore.github.io/wiki/OVMF) may also
Expand Down
14 changes: 14 additions & 0 deletions lib/propolis/src/hw/nvme/admin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,19 @@ impl NvmeCtrl {
)
}

cmds::FeatureIdent::OxideDeviceFeatures => {
Copy link

@rmustacc rmustacc Feb 14, 2026

Choose a reason for hiding this comment

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

A few notes on validation here that I think we want to check:

  • cdw11 is used by features to control behavior on a per-feature basis. I would like to see us constrain this to be zero and drop an error if not so we can add additional behavior over time.
  • I don't see any logic checking the value of the CDW10 feature selector. Is that somewhere I missed? It looks like that's being dropped in AdminCmd::parse. Doesn't have to be fixed in this change, but probably should be a follow up for us there.
  • I think it's okay that we're not checking the data pointer since the spec says it's ignored when the feature doesn't use it, which means it can have garbage.
  • I feel less confident in cdw14 with feature by uuid, but I think it's probably okay in spirit.

Copy link

@rmustacc rmustacc Feb 14, 2026

Choose a reason for hiding this comment

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

One other thing now that I looked more closely here, it looks like we're going to commit to returning the same data regardless of namespace id. I think I'm fine with that, but just want to confirm that intent.

Copy link
Contributor Author

@luqmana luqmana Feb 18, 2026

Choose a reason for hiding this comment

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

  • cdw11 is used by features to control behavior on a per-feature basis. I would like to see us constrain this to be zero and drop an error if not so we can add additional behavior over time.

Added a check here specifically and opened #1061 to look at constraining the other features.

  • I don't see any logic checking the value of the CDW10 feature selector. Is that somewhere I missed? It looks like that's being dropped in AdminCmd::parse. Doesn't have to be fixed in this change, but probably should be a follow up for us there.

We're only advertising 1.0 support so that is there today.

One other thing now that I looked more closely here, it looks like we're going to commit to returning the same data regardless of namespace id. I think I'm fine with that, but just want to confirm that intent.

Yes, although now I'm wondering if it's worth constraining one way or the other in the same vein as rejecting non-zero cdw11. Not a huge difference either way since we only support a single namespace today anyways.

Copy link

@rmustacc rmustacc Feb 18, 2026

Choose a reason for hiding this comment

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

Yes, although now I'm wondering if it's worth constraining one way or the other in the same vein as rejecting non-zero cdw11. Not a huge difference either way since we only support a single namespace today anyways.

Well, it's the distinction between using namespace id 0 being that it isn't valid or in the future using the broadcast namespace. I think I agree that we can change this in the future. I ultimately convinced myself it was probably fine last time I was looking at it. Ultimately we're the only consumer right now so it's okay to change htis if need be.

if cmd.cdw11 != 0 {
// We don't currently accept any parameters for this feature
cmds::Completion::generic_err(STS_INVAL_FIELD)
} else {
cmds::Completion::success_val(
cmds::OxideDeviceFeatures(0)
Copy link

@rmustacc rmustacc Feb 18, 2026

Choose a reason for hiding this comment

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

I'll defer to someone else on the change in construction from using .into() to the current construction. For my own edification, why did that change?

Copy link
Contributor Author

@luqmana luqmana Feb 19, 2026

Choose a reason for hiding this comment

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

Just to match other similar bitstruct-defined structures (like FeatTemperatureThreshold) and better mirrors the definition on the edk2 side as well.

.with_read_only(self.read_only)
.0,
)
}
}

cmds::FeatureIdent::Reserved
| cmds::FeatureIdent::LbaRangeType
| cmds::FeatureIdent::SoftwareProgressMarker
Expand Down Expand Up @@ -457,6 +470,7 @@ impl NvmeCtrl {
| cmds::FeatureIdent::WriteAtomicity
| cmds::FeatureIdent::AsynchronousEventConfiguration
| cmds::FeatureIdent::SoftwareProgressMarker
| cmds::FeatureIdent::OxideDeviceFeatures
| cmds::FeatureIdent::Vendor(_) => {
cmds::Completion::generic_err(STS_INVAL_FIELD).dnr()
}
Expand Down
9 changes: 9 additions & 0 deletions lib/propolis/src/hw/nvme/bits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,15 @@ pub const FEAT_ID_WRITE_ATOMIC: u8 = 0x0A;
/// See NVMe 1.0e Section 5.12.1.11 Asynchronous Event Configuration (Feature Identifier 0Bh)
pub const FEAT_ID_ASYNC_EVENT_CFG: u8 = 0x0B;

/// Oxide-specific feature.
///
/// Provides device-specific features beyond the standard NVMe capabilities as
/// a single Dword result:
/// Bit 0 [ReadOnly] - If set, the device will complete all writes with
/// STS_WRITE_READ_ONLY_RANGE.
/// Bits 31-1 - Reserved.
pub const FEAT_ID_OXIDE_DEVICE_FEATURES: u8 = 0xF0;

// Identify CNS values

/// Identify - Namespace Structure
Expand Down
19 changes: 18 additions & 1 deletion lib/propolis/src/hw/nvme/cmds.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,12 @@ pub enum FeatureIdent {
/// This feature is persistent across power states.
/// See NVMe 1.0e Section 7.6.1.1 Software Progress Marker
SoftwareProgressMarker,
/// Vendor specific feature.

// Vendor specific features.
/// Oxide-specific feature - returns relevant device features.
OxideDeviceFeatures,

/// All other vendor specific features.
Vendor(#[allow(dead_code)] u8),
}

Expand All @@ -476,6 +481,7 @@ impl From<u8> for FeatureIdent {
0xC..=0x7F => Reserved,
0x80 => SoftwareProgressMarker,
0x81..=0xBF => Reserved,
FEAT_ID_OXIDE_DEVICE_FEATURES => OxideDeviceFeatures,
0xC0..=0xFF => Vendor(fid),
}
}
Expand Down Expand Up @@ -651,6 +657,17 @@ impl From<FeatInterruptVectorConfig> for u32 {
}
}

bitstruct! {
pub struct OxideDeviceFeatures(pub u32) {
/// Indicates the device is read-only and will complete all attempted
/// writes with `STS_WRITE_READ_ONLY_RANGE`.
pub read_only: bool = 0;

/// Reserved
reserved: u32 = 1..32;
}
}

/// A parsed NVM Command
#[allow(dead_code)]
#[derive(Debug)]
Expand Down
5 changes: 5 additions & 0 deletions lib/propolis/src/hw/nvme/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,8 @@ struct NvmeCtrl {

/// The Identify structure returned for Identify namespace commands
ns_ident: IdentifyNamespace,

read_only: bool,
}

impl NvmeCtrl {
Expand Down Expand Up @@ -566,6 +568,8 @@ impl NvmeCtrl {
.iter()
.filter_map(Option::as_ref)
.for_each(|sq| sq.update_params(params));

self.read_only = info.read_only;
}

/// Get Memory Page Size (MPS), expressed in bytes
Expand Down Expand Up @@ -899,6 +903,7 @@ impl PciNvme {
sqs: Default::default(),
ctrl_ident,
ns_ident,
read_only: false,
};

let pci_state = builder
Expand Down
4 changes: 2 additions & 2 deletions packaging/package-manifest.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,6 @@ output.type = "zone"
[[package.propolis-server.source.buildomat_blobs]]
repo = "edk2"
series = "image_debug"
commit = "907a5fd1763ce5ddd74001261e5b52cd200a25f9"
commit = "bf64f45b1a58e69d126a3c6ca1e4512c88668132"
artifact = "OVMF_CODE.fd"
sha256 = "ff12d5cb021e34447b44301f70434e861b07d2779c16abe2f2efef49ff02fffb"
sha256 = "740187046a7267d0de72d3455070333547dbc0ea023531471fb2b2a61effa448"
4 changes: 2 additions & 2 deletions phd-tests/artifacts.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@ kind = "bootrom"
[artifacts.ovmf.source.buildomat]
repo = "oxidecomputer/edk2"
series = "image_debug"
commit = "907a5fd1763ce5ddd74001261e5b52cd200a25f9"
sha256 = "ff12d5cb021e34447b44301f70434e861b07d2779c16abe2f2efef49ff02fffb"
commit = "bf64f45b1a58e69d126a3c6ca1e4512c88668132"
sha256 = "740187046a7267d0de72d3455070333547dbc0ea023531471fb2b2a61effa448"
Loading