nvme: add oxide-specific feature to expose read-only attribute for device#1060
nvme: add oxide-specific feature to expose read-only attribute for device#1060
Conversation
luqmana
force-pushed
the
nvme-ro-feature
branch
from
9023ec7 to
fc631c8
Compare
| ) | ||
| } | ||
|
|
||
| cmds::FeatureIdent::OxideDeviceFeatures => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- 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.
There was a problem hiding this comment.
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.
| cmds::Completion::generic_err(STS_INVAL_FIELD) | ||
| } else { | ||
| cmds::Completion::success_val( | ||
| cmds::OxideDeviceFeatures(0) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Just to match other similar bitstruct-defined structures (like FeatTemperatureThreshold) and better mirrors the definition on the edk2 side as well.
packaging/package-manifest.toml
Outdated
| commit = "8c60ff4c271b0442b9a506a5cf9eab04cd5554aa" | ||
| artifact = "OVMF_CODE.fd" | ||
| sha256 = "ff12d5cb021e34447b44301f70434e861b07d2779c16abe2f2efef49ff02fffb" | ||
| sha256 = "9ce2266aaefb671a1977ddabb5155f8235be134f797e6c831abeb6c97c705569" |
There was a problem hiding this comment.
I assume this update is to pull in a bootrom with oxidecomputer/edk2#4? But when I looked at the SHAs for the buildomat jobs there they didn't match these. Am I looking in the wrong place?
There was a problem hiding this comment.
I was waiting for the buildomat job to finish and forgot to push up the latest update referencing the merged PR. Should all match now.
There was a problem hiding this comment.
Great, thanks! I'm eager to test this out :D
Pulls in fix for booting from read-only disk: - nvme: add oxide-specific feature to expose read-only attribute for device (oxidecomputer/propolis#1060).
Pulls in fix for booting from read-only disk: - oxidecomputer/propolis#1060
3 participants
Along with oxidecomputer/edk2#4 this fixes #1059.