move smbios table creation to propolis (out of server+standalone) #759
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cpuid_ident is non-optional, any defaulting happens in applications
iximeow
commented
Sep 5, 2024
Comment on lines
+1159
to
+1160
| if cpuid::host_query(cpuid::Ident(0x8000_0000, None)).eax | ||
| >= 0x8000_0004 |
There was a problem hiding this comment.
boring but funny note: the oldest CPUs that don't support cpuid leaves 0x8000_000[1..4] are, i think, Pentium II and K5. so as long as Propolis is running on a CPU newer than those, this arm is always taken?
not really sure how i feel about this, but it seems interesting this is the only state in MachineInitializerState right now..
iximeow
commented
Sep 6, 2024
Comment on lines
957
to
+976
| // With "only" types 0, 1, 4, 16, 17, and 32, we are technically missing | ||
| // some (types 3, 7, 9, 19) of the data required by the 2.7 spec. The | ||
| // data provided here were what we determined was a reasonable | ||
| // collection to start with. Should further requirements arise, we may | ||
| // expand on it. | ||
| let mut smb_tables = smbios::Tables::new(0x7f00.into()); | ||
| smb_tables.add(0x0000.into(), &smb_type0).unwrap(); | ||
| smb_tables.add(0x0100.into(), &smb_type1).unwrap(); | ||
| smb_tables.add(0x0300.into(), &smb_type4).unwrap(); | ||
| smb_tables.add(phys_mem_array_handle, &smb_type16).unwrap(); | ||
| smb_tables.add(0x1700.into(), &smb_type17).unwrap(); | ||
| smb_tables.add(0x3200.into(), &smb_type32).unwrap(); | ||
| smb_tables.add(0x0000.into(), &smbios_params.table_type0()).unwrap(); | ||
| smb_tables.add(0x0100.into(), &smbios_params.table_type1()).unwrap(); | ||
| smb_tables.add(0x0300.into(), &smbios_params.table_type4()).unwrap(); | ||
| let phys_mem_array_handle = 0x1600.into(); | ||
| smb_tables | ||
| .add(phys_mem_array_handle, &smbios_params.table_type16()) | ||
| .unwrap(); | ||
| smb_tables | ||
| .add( | ||
| 0x1700.into(), | ||
| &smbios_params.table_type17(phys_mem_array_handle), | ||
| ) | ||
| .unwrap(); | ||
| smb_tables.add(0x3200.into(), &smbios_params.table_type32()).unwrap(); |
There was a problem hiding this comment.
@pfmooney how's this seem? my thinking is that the SmbiosParams::table_* are "this is probably a reasonable way to put the tables together", and if we find value in changing any of the defaults we'd hoist them into fields of SmbiosParameters.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
tiny cleanup i noticed alongside #756: the two
generate_smbios()were similar but not quite the same, and it wasn't entirely clear what external inputs were to the tables either would generate. so, take theSmbiosParametersidea frompropolis-standaloneand makepropolis-serverput its parameters in a similar struct.nice benefit: anything we make configurable for
propolis-standaloneis at least progress towards configurability forpropolis- server.in the process i've added a few more fields to
SmbiosParametersfor values that either varied or had different defaulting behavior depending on the application:rom_release_date,cpuid_vendor, and the value as we use table 1's serial number and uuid, which i'm callingsystem_id- it's the instance's ID and just left as defaults inpropolis-standalone.edit: i did go a bit beyond just code motion in acb6da1. as i was moving this around i realized that's the only place we need a scratchpad in
MachineInitializer, so i wanted to see how things look withrom_sizepassed around on its own. seems fine? do we expectMachineInitializerStatewould eventually have more state in it?