Page MenuHomeFreeBSD
Differential D24107 Diff 69945 usr.sbin/bhyve/smbiostbl.c

Changeset View

Standalone View

View Options

usr.sbin/bhyve/smbiostbl.c

Show First 20 Lines • Show All 249 Lines • ▼ Show 20 Lines
#define SMBIOS_MDF_UNKNOWN 0x0004 /* unknown */ #define SMBIOS_MDF_UNKNOWN 0x0004 /* unknown */
struct smbios_table_type17 { struct smbios_table_type17 {
struct smbios_structure header; struct smbios_structure header;
uint16_t arrayhand; /* handle of physl mem array */ uint16_t arrayhand; /* handle of physl mem array */
uint16_t errhand; /* handle of mem error data */ uint16_t errhand; /* handle of mem error data */
uint16_t twidth; /* total width in bits */ uint16_t twidth; /* total width in bits */
uint16_t dwidth; /* data width in bits */ uint16_t dwidth; /* data width in bits */
uint16_t size; /* size in bytes */ uint16_t size; /* size in kb or mb */
rgrimesUnsubmitted
Done
Inline Actions

Comment is miss leading, this is size in kilobytes or megabytes depending on MSB.

rgrimes: Comment is miss leading, this is size in kilobytes or megabytes depending on MSB.
uint8_t form; /* form factor */ uint8_t form; /* form factor */
uint8_t set; /* set */ uint8_t set; /* set */
uint8_t dloc; /* device locator string */ uint8_t dloc; /* device locator string */
uint8_t bloc; /* phys bank locator string */ uint8_t bloc; /* phys bank locator string */
uint8_t type; /* memory type */ uint8_t type; /* memory type */
uint16_t flags; /* memory characteristics */ uint16_t flags; /* memory characteristics */
uint16_t maxspeed; /* maximum speed in mhz */ uint16_t maxspeed; /* maximum speed in mhz */
uint8_t manufacturer; /* manufacturer string */ uint8_t manufacturer; /* manufacturer string */
uint8_t serial; /* serial number string */ uint8_t serial; /* serial number string */
uint8_t asset; /* asset tag string */ uint8_t asset; /* asset tag string */
uint8_t part; /* part number string */ uint8_t part; /* part number string */
uint8_t attributes; /* attributes */ uint8_t attributes; /* attributes */
uint32_t xsize; /* extended size in mbs */ uint32_t xsize; /* extended size in mb */
uint16_t curspeed; /* current speed in mhz */ uint16_t curspeed; /* current speed in mhz */
uint16_t minvoltage; /* minimum voltage */ uint16_t minvoltage; /* minimum voltage */
uint16_t maxvoltage; /* maximum voltage */ uint16_t maxvoltage; /* maximum voltage */
uint16_t curvoltage; /* configured voltage */ uint16_t curvoltage; /* configured voltage */
} __packed; } __packed;
/* /*
* Memory Array Mapped Address * Memory Array Mapped Address
▲ Show 20 Lines • Show All 159 Lines • ▼ Show 20 Linesstatic int smbios_type16_initializer(struct smbios_structure *template_entry,
uint16_t *n, uint16_t *size); uint16_t *n, uint16_t *size);
struct smbios_table_type17 smbios_type17_template = { struct smbios_table_type17 smbios_type17_template = {
{ SMBIOS_TYPE_MEMDEVICE, sizeof (struct smbios_table_type17), 0 }, { SMBIOS_TYPE_MEMDEVICE, sizeof (struct smbios_table_type17), 0 },
-1, /* handle of physical memory array */ -1, /* handle of physical memory array */
-1, /* handle of memory error data */ -1, /* handle of memory error data */
64, /* total width in bits including ecc */ 64, /* total width in bits including ecc */
64, /* data width in bits */ 64, /* data width in bits */
0x7fff, /* size in bytes (0x7fff=use extended)*/ 0, /* size in kb or mb (0x7fff=use extended)*/
SMBIOS_MDFF_UNKNOWN, SMBIOS_MDFF_UNKNOWN,
0, /* set (0x00=none, 0xff=unknown) */ 0, /* set (0x00=none, 0xff=unknown) */
1, /* device locator string */ 1, /* device locator string */
2, /* physical bank locator string */ 2, /* physical bank locator string */
SMBIOS_MDT_UNKNOWN, SMBIOS_MDT_UNKNOWN,
SMBIOS_MDF_UNKNOWN, SMBIOS_MDF_UNKNOWN,
0, /* maximum memory speed in mhz (0=unknown) */ 0, /* maximum memory speed in mhz (0=unknown) */
3, /* manufacturer string */ 3, /* manufacturer string */
▲ Show 20 Lines • Show All 234 Lines • ▼ Show 20 Lines
} }
static int static int
smbios_type17_initializer(struct smbios_structure *template_entry, smbios_type17_initializer(struct smbios_structure *template_entry,
const char **template_strings, char *curaddr, char **endaddr, const char **template_strings, char *curaddr, char **endaddr,
uint16_t *n, uint16_t *size) uint16_t *n, uint16_t *size)
{ {
struct smbios_table_type17 *type17; struct smbios_table_type17 *type17;
uint64_t memsize, size_MB;
rgrimesUnsubmitted
Done
Inline Actions

I just noticed this, its possible for these types to be overflowed as uint64_t/1024 can be 54 bits long and uint64_t/MB can be 44 bits.

rgrimes: I just noticed this, its possible for these types to be overflowed as uint64_t/1024 can be 54…
bcranAuthorUnsubmitted
Done
Inline Actions

I'll change them back to uint64_t before committing.

bcran: I'll change them back to uint64_t before committing.
smbios_generic_initializer(template_entry, template_strings, smbios_generic_initializer(template_entry, template_strings,
curaddr, endaddr, n, size); curaddr, endaddr, n, size);
type17 = (struct smbios_table_type17 *)curaddr; type17 = (struct smbios_table_type17 *)curaddr;
type17->arrayhand = type16_handle; type17->arrayhand = type16_handle;
type17->xsize = guest_lomem;
if (guest_himem > 0) { memsize = guest_lomem + guest_himem;
curaddr = *endaddr; size_MB = memsize / MB;
smbios_generic_initializer(template_entry, template_strings,
rgrimesUnsubmitted
Done
Inline Actions

This calculation seems odd, I see later it is overwritten for large memory, cant we just continue to use extended all the time and leave this set at 0x7fff? Never mind, spec says not. Ok how about doing:
if (memsize <= (32GB-1MB)) {

  type17->size = size_MB; } else {
 type17->size = 0x7FFF;
 if (guest_himem <= 0 {
  #error should never happen
 } else {
fall into the code below, remove the if, we just did that with an error check

This would make it follow how the spec is written

rgrimes: This calculation seems odd, I see later it is overwritten for large memory, cant we just…
jhbUnsubmitted
Done
Inline Actions

It is not overwritten as we allocate a separate type 17 entry for guest_himem, however, it does seem odd that we set the entry size for lomem to the full size instead of just the lomem size as we did before. That is, it seems we now create two entries for the same logical "DIMM". One holds the truncated size and one holds the extended size if we truncated, otherwise it holds zero, but we only create this second DIMM if guest_himem is > 0? I would expect us to do 1 of two things:

  1. 1 DIMM for both guest_lomem + guest_himem. (This closer to real hardware in practice)
  2. 1 DIMM for guest_lomem and 1 DIMM for guest_himem (what the older code does).
jhb: It is not overwritten as we allocate a separate type 17 entry for guest_himem, however, it does…
rgrimesUnsubmitted
Done
Inline Actions

There is no check for truncation here

rgrimes: There is no check for truncation here
bcranAuthorUnsubmitted
Done
Inline Actions

Thanks, I'll correct those issues.

bcran: Thanks, I'll correct those issues.
bcranAuthorUnsubmitted
Done
Inline Actions

Sorry, do you mean it should check if the guest has less than 32MB, and if so store the size with the high bit set?

bcran: Sorry, do you mean it should check if the guest has less than 32MB, and if so store the size…
rgrimesUnsubmitted
Done
Inline Actions

Well phab has moved this to point at the wrong line, but no what I meant was you do a & 0x7fff or 0x7fffffff on variables that may lead to you truncated a value with no error or warning, which may lead to unexpected results.

Though I believe the &0x7fff is now fixed due to the logic changes, the &0x7ffffff could lead to a truncation and should probably just be a if > 0x7ffffff return error;

rgrimes: Well phab has moved this to point at the wrong line, but no what I meant was you do a & 0x7fff…
curaddr, endaddr, n, size); // Can represent up to ~32GB.
type17 = (struct smbios_table_type17 *)curaddr; // The top bit is 0 for MB, 1 for KB.
jhbUnsubmitted
Done
Inline Actions

Please stick to C style comments to be consistent with the rest of the code.

jhb: Please stick to C style comments to be consistent with the rest of the code.
bcranAuthorUnsubmitted
Done
Inline Actions

Good point. Fixed.

bcran: Good point. Fixed.
type17->arrayhand = type16_handle; type17->size = size_MB & 0x7FFF;
type17->xsize = guest_himem;
jhbUnsubmitted
Done
Inline Actions

I concur with only a warning and not error'ing here. DMI/SMBIOS tables are generally only used for cosmetics (FreeBSD uses it to be more tidy for dmesg, but all the "real" stuff uses SMAP / EFI memory map).

jhb: I concur with only a warning and not error'ing here. DMI/SMBIOS tables are generally only used…
if (size_MB >= 0x7FFF) {
type17->size = 0x7FFF;
// Can represent up to ~2PB.
// The top bit is reserved.
type17->xsize = size_MB & 0x7FFFFFFF;
rgrimesUnsubmitted
Done
Inline Actions

Should this be >= to, as if it is exactly 0x7fff, aka 32GB-1mb you'll end up with a memory size of 0.

rgrimes: Should this be >= to, as if it is exactly 0x7fff, aka 32GB-1mb you'll end up with a memory size…
} }
return (0); return (0);
} }
static int static int
smbios_type19_initializer(struct smbios_structure *template_entry, smbios_type19_initializer(struct smbios_structure *template_entry,
const char **template_strings, char *curaddr, char **endaddr, const char **template_strings, char *curaddr, char **endaddr,
Show All 30 Linessmbios_ep_initializer(struct smbios_entry_point *smbios_ep, uint32_t staddr)
smbios_ep->eplen = 0x1F; smbios_ep->eplen = 0x1F;
assert(sizeof (struct smbios_entry_point) == smbios_ep->eplen); assert(sizeof (struct smbios_entry_point) == smbios_ep->eplen);
smbios_ep->major = 2; smbios_ep->major = 2;
smbios_ep->minor = 6; smbios_ep->minor = 6;
smbios_ep->revision = 0; smbios_ep->revision = 0;
memcpy(smbios_ep->ianchor, SMBIOS_ENTRY_IANCHOR, memcpy(smbios_ep->ianchor, SMBIOS_ENTRY_IANCHOR,
SMBIOS_ENTRY_IANCHORLEN); SMBIOS_ENTRY_IANCHORLEN);
smbios_ep->staddr = staddr; smbios_ep->staddr = staddr;
smbios_ep->bcdrev = 0x24; smbios_ep->bcdrev = 0x24;
rgrimesUnsubmitted
Done
Inline Actions

We need to be very careful about just bumping this because we added *some* feature from a new version, we must be sure the whole of the SMBios implementation in bhyve is conformat with that version. Luckly the difference between 2.6 and 2.7 is minor, but before this bumps we should look at all the changes to make sure they are implemented.

rgrimes: We need to be very careful about just bumping this because we added *some* feature from a new…
jhbUnsubmitted
Done
Inline Actions

If anything, it seems to be the other way around. We already define 2.7 fields in other types already (e.g. type 3 and types 16+19 for which we hardcode 2.7+ only encodings). I suspect we chose a conservative version number here to appease older guests previously.

jhb: If anything, it seems to be the other way around. We already define 2.7 fields in other types…
bcranAuthorUnsubmitted
Done
Inline Actions

Ah, okay. That makes sense actually: we can define fields that are only defined by a newer version of the spec because systems which only know about the version we specify won't know to look at the newer fields, and shouldn't get confused.

bcran: Ah, okay. That makes sense actually: we can define fields that are only defined by a newer…
bcranAuthorUnsubmitted
Done
Inline Actions

From reading the spec, it looks like bcdrev should match the major/minor fields. Do you think I should bump bcdrev to 0x26 here, or leave it as it is?

bcran: From reading the spec, it looks like bcdrev should match the major/minor fields. Do you think I…
rgrimesUnsubmitted
Done
Inline Actions

From digging in svn blame it seems in r272007 Peter Grehan bumped smbios->minor without bumping the BCD value. I would suggest we start another review to fix, and eliminate the possibility of this happening again in the future with:

smbios_ep->bcdrev = ((smbios_ep->major & 0xF) << 4) | smbios_ep->minor & 0xF));

rgrimes: From digging in svn blame it seems in r272007 Peter Grehan bumped smbios->minor without bumping…
rgrimesUnsubmitted
Done
Inline Actions

Why are these 2 lines showing up as what look like reverse diffs now?

rgrimes: Why are these 2 lines showing up as what look like reverse diffs now?
bcranAuthorUnsubmitted
Done
Inline Actions

Oops, I gave the wrong revision to "arc patch" - "HEAD~3" instead of "HEAD~5" so it didn't have the full patch. Should be fixed now, sorry.

bcran: Oops, I gave the wrong revision to "arc patch" - "HEAD~3" instead of "HEAD~5" so it didn't have…
} }
static void static void
smbios_ep_finalizer(struct smbios_entry_point *smbios_ep, uint16_t len, smbios_ep_finalizer(struct smbios_entry_point *smbios_ep, uint16_t len,
uint16_t num, uint16_t maxssize) uint16_t num, uint16_t maxssize)
{ {
uint8_t checksum; uint8_t checksum;
int i; int i;
▲ Show 20 Lines • Show All 74 Lines • Show Last 20 Lines