search

oxidecomputer / propolis

VMM userspace for illumos bhyve
Mozilla Public License 2.0
172 stars 22 forks source link

Accurate versions in SMBIOS tables #701

Open luqmana opened 2 months ago

luqmana commented 2 months ago

The version information in the BIOS (type 0) and System (type 1) tables are currently hardcoded. We could instead set them to the OVMF and propolis versions (/commit hash?), respectively.

jclulow commented 2 months ago

If you're going to include the commit hash it might also help to include the commit depth (as close to a monotonic revision number as you can get with git) like we do for Helios components. And, I guess, also the branch name, etc.

hawkw commented 1 month ago

Since Propolis itself is not published to crates.io, we don't really increment the crate version number very often --- it will almost always be 0.1.0. I think the Git metadata is much likelier to be useful.

We may also want to include the bhyve API version in a system version string?

pfmooney commented 1 month ago

One point worth noting: If we put version data about propolis and/or bhyve in the SMBIOS tables, it will potentially become out of date when the instance is live-migrated.

hawkw commented 1 month ago

Oh, hmm, that's a good point @pfmooney. It would sure be a shame to break guests just because we migrated to a VMM built from a different Git commit.

I do have a branch implementing the Propolis version in the type 1 table (https://github.com/oxidecomputer/propolis/pull/702), but I'm happy to drop it if we think this is actually a bad idea.

hawkw commented 1 month ago

Regarding the OVMF version, since Propolis just gets the bootrom from a path to an arbitrary file in the TOML config, it doesn't have an easy way to know what the OVMF version is. Probably, we want to make the reported BIOS version configurable in the config file, and ensure that it's set to whatever OVMF version Propolis gets?

pfmooney commented 1 month ago

I'm happy to drop it if we think this is actually a bad idea.

I don't think it's a bad idea, but rather one of limited utility, at least for now. I'd rather not clutter up the build until we have a clear reason.

Probably, we want to make the reported BIOS version configurable in the config file, and ensure that it's set to whatever OVMF version Propolis gets?

At some point, I imagine we'll have some sort of selectable bootrom profiles (complete with version metadata), but until then, setting it via the config is probably the right path. (And I wouldn't even bother with wiring it up in standalone)

pfmooney commented 1 month ago

It would sure be a shame to break guests just because we migrated to a VMM built from a different Git commit.

I don't believe this would actually be breaking. Once fw_cfg is fixed (a WIP) in propolis, we'll simply migrate the generated contents, so they remain fixed after the instance initialization code sets them the first time.

rmustacc commented 1 month ago

I missed the discussion here when I wrote up this comment in #702.

It would sure be a shame to break guests just because we migrated to a VMM built from a different Git commit.

I don't believe this would actually be breaking. Once fw_cfg is fixed (a WIP) in propolis, we'll simply migrate the generated contents, so they remain fixed after the instance initialization code sets them the first time.

While not breaking, this feels like it'd still be misleading to guests. Putting it in here suggests that customers should use it and key off of it semantically. So the fact that it can't be updated at run-time across a migration to reflect a change is a strong reason for me that this shouldn't be reflecting something that's non-static.

In the case of OVMF, I assume that the OVMF version migrates with the guest so that would be consistent? But I don't see how that can be the case at all for Propolis. So I think the question we should be asking is what is how does a customer use this usefully and how do we ensure it reflects the system as a whole that they are using versus a point in time snapshot of the virutalization impl.