Skip to content
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.

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

discuss firmware_revision #191

Open
glimchb opened this issue Nov 7, 2022 · 4 comments
Open

discuss firmware_revision #191

glimchb opened this issue Nov 7, 2022 · 4 comments
Assignees
Labels
Storage APIs or code related to storage area

Comments

@glimchb
Copy link
Member

glimchb commented Nov 7, 2022

from slack:

Vipin Jain
1 hour ago
can we also close on the firmware version?

Ben Walker
1 hour ago
sure - can you clarify what information you're trying to convey through the NVMe firmware version?

Vipin Jain
1 hour ago
the version of the control/datapath being run for NVMe, which is different from that of the device's firmware. In our case customer changes this information based on their own P4 and so they want us to specify what version is being run

Ben Walker
1 hour ago
does this need to be discovered via NVMe commands by the host?

Vipin Jain
1 hour ago
well nvme driver does report the firmware revision, so that's what needs to be shown there

Ben Walker
1 hour ago
but the customers need to see it through the NVMe firmware version?

Ben Walker
1 hour ago
they can't query the device config some other way?

Vipin Jain
1 hour ago
but firmware version is exactly for that reason, why use some other config or sideband way?

Ben Walker
1 hour ago
because firmware version is not really clearly defined for the case where parts of the firmware are being dynamically loaded/modified

Ben Walker
1 hour ago
for instance, if you present a subsystem with two namespaces, can each namespace have different P4? Certainly in our product they can be programmed very differently.

Ben Walker
1 hour ago
and what firmware version would we report in that case?

Vipin Jain
1 hour ago
to me, the dynamic notion is more flexible and allows modularity of individual personas of an xPU

Vipin Jain
1 hour ago
a subsystem having a firmware_version is what seems usefule for now, I am not sure about the granularity beyond that

Ben Walker
1 hour ago
but if you have two namespaces that use different data paths, what firmware version do you report?

Ben Walker
1 hour ago
if they're in the same subsystem

Vipin Jain
1 hour ago
a subsystem uses one logic of P4 code for everything on the xPU - at least knowing how P4 modularity works, it is perhaps too far for anyone to have a different P4 program for different namespaces

Ben Walker
1 hour ago
your subsystem does

Ben Walker
1 hour ago
let's say ours, or someones, doesn't

Ben Walker
1 hour ago
certainly ours is much more programmable than that

Vipin Jain
1 hour ago
ok, I will put this field in vendor extensions then

Vipin Jain
1 hour ago
and remove it from the PR

Vipin Jain
1 hour ago
I think if we have it this way, everyone can report the generic/same version and I can do it differently

Ben Walker
1 hour ago
we'll support things like an iSCSI-backed namespace and an NVMe-oF/RDMA backed namespace in one subsystem

Ben Walker
1 hour ago
we'll support custom data services written by the user that we don't even know about or can enumerate

Ben Walker
1 hour ago
it's too complex to possibly capture in a single firmware version

Ben Walker
1 hour ago
so I agree with you - OPI should not have this and you can do it in the vendor extension

Vipin Jain
1 hour ago
ok

Vipin Jain
1 hour ago
pushed the change, thanks!

Ben Walker
1 hour ago
lgtm, approved

@glimchb glimchb added the Storage APIs or code related to storage area label Nov 7, 2022
@jainvipin
Copy link
Contributor

just for the context, above discussion is to specify the firmware_revision in the status of the frontend controller object.

@glimchb
Copy link
Member Author

glimchb commented Nov 16, 2022

@jainvipin I see firmware_revision move to the status, if this is enough , we can close this issue

@jainvipin
Copy link
Contributor

sorry @glimchb - I meant it be in the Spec and not status, because the firmware (aka P4 code dictating the revision of P4 code) is owned by the user (e.g. a cloud provider) dynamically.

@glimchb
Copy link
Member Author

glimchb commented Nov 18, 2022

sure, please open a PR and move it to spec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Storage APIs or code related to storage area
Projects
None yet
Development

No branches or pull requests

2 participants