OpenSolaris

<pre wrap="">On Wed, 2009-09-02 at 12:25 -0400, Tom Pothier wrote: </pre>

<pre wrap="">The webrev is now also available on opensolaris.org: http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/ </pre>

<pre wrap=""><!----> Tom, I've gotten through some of the code. Still have to review the x86pi enumerator itself and most of the "uts" changes. -scott ==usr/src/uts/common/sys/smbios.h== 1094 /* 1095 * SMBIOS OEM-specific (Type 144) Memory Array Extended Information. 1096 */ 1097 typedef struct smbios_memarray_ext { 1098 uint16_t smbmae_ma; /* memory array handle */ 1099 uint16_t smbmae_comp; /* component parent handle */ 1100 uint16_t smbmae_bdf; /* Bus/Dev/Funct (PCI) */ 1101 } smbios_memarray_ext_t; ==usr/src/common/smbios/smb_info.c== 896 int 897 smbios_info_extmemarray(smbios_hdl_t *shp, id_t id, smbios_memarray_ext_t *emap) 898 { ... 911 emap->smbmae_ma = exma.smbmarre_ma; 912 emap->smbmae_comp = exma.smbmarre_component; 913 emap->smbmae_bdf = exma.smbmarre_bdf; ==usr/src/cmd/smbios/smbios.c== 880 static void 881 print_extmemarray(smbios_hdl_t *shp, id_t id, FILE *fp) 882 { ... 888 (void) smbios_info_extmemarray(shp, id, &em); 889 890 oprintf(fp, " Physical Memory Array Handle: %u\n", em.smbmae_ma); 891 oprintf(fp, " Component Parent Handle: %u\n", em.smbmae_comp); 892 oprintf(fp, " BDF: 0x%x\n", em.smbmae_bdf); In the PRMS (BTW...is this available to external reviewers yet?) there's also a "Segment Group" field in the extended structure. This isn't being used in enumeration. But assuming it's important to someone, the field should be grabbed and printed via smbios. If it's vestigial, yank it from the PRMS. ==usr/src/lib/fm/topo/maps/i86pc/i86pc-legacy-hc-topology.xml== 28 <!-- 29 This map file is loaded by the generic enumerator (x86pi.so) when 30 an FMA-compliance SMBIOS can't be found. 31 --> Nit. "compliance" --> "compliant" ==usr/src/lib/fm/topo/modules/common/pcibus/did_props.c== 132 { NULL, &protocol_pgroup, TOPO_PROP_ASRU, ASRU_set }, 133 /* 134 * These props need to be put at the end of table. x86pi has its 135 * own way to set them. 136 */ 137 { NULL, &protocol_pgroup, TOPO_PROP_LABEL, label_set }, 138 { NULL, &protocol_pgroup, TOPO_PROP_FRU, FRU_set } The comment is intriguing. What makes label and FRU special that the ordering in the props[] arrays matters? Can the comment refer to functions/comments in the x86pi code that can clarify? ==usr/src/lib/fm/topo/modules/i86pc/chip/chip.h== 40 /* Below should match the definitions in x86pi_impl.h */ 41 #define X86PI_FULL 1 42 #define X86PI_PARTIAL 2 43 #define X86PI_NONE 3 44 45 /* 46 * FM_AWARE_SMBIOS means SMBIOS meets FMA needs 47 * X86PI_FULL is defined as 1 in x86pi.so 48 * X86PI_PARTIAL is defined as 2 in x86pi.so 49 * And passed from x86pi.so to chip.so as module 50 * private data 51 */ 52 #define FM_AWARE_SMBIOS(mod) \ 53 (topo_mod_getspecific(mod) != NULL && \ 54 (*(int *)topo_mod_getspecific(mod) == X86PI_FULL || \ 55 *(int *)topo_mod_getspecific(mod) == X86PI_PARTIAL)) I don't understand the usage of _PARTIAL here. In scanning the rest of the code (particularly x86pi_check_comp()), _PARTIAL is never a valid value for the x86pi enumerator. Makes logical sense too. Either the SMBIOS has what the enum needs to create the hierarchy or it doesn't. I understand that certain fields (serial numbers, labels) don't prevent construction of a valid hierarchy from SMBIOS records. But it does not look like _PARTIAL is used to convey this. ==usr/src/lib/fm/topo/modules/i86pc/chip/chip.c== 216 (void) topo_node_fru_set(strand, NULL, 0, &perr); 217 /* 218 * From the inherited FRU, extract the Serial 219 * number(if SMBIOS donates) and set it in the ASRU 220 */ 221 if (FM_AWARE_SMBIOS(mod)) { 222 char *val = NULL; 223 224 if (topo_prop_get_fmri(strand, TOPO_PGROUP_PROTOCOL, 225 TOPO_PROP_FRU, &fmri, &err) != 0) 226 whinge(mod, NULL, 227 "create_strand: topo_prop_get_fmri failed\n"); 228 if (nvlist_lookup_string(fmri, FM_FMRI_HC_SERIAL_ID, &val) != 0) 229 whinge(mod, NULL, 230 "create_strand: nvlist_lookup_string failed: \n"); 231 else 232 serial = topo_mod_strdup(mod, val); 233 nvlist_free(fmri); 234 } Is this correct? The inherited FRU of a strand may be a baseboard, in which case the FRU serial number does not make sense for an ASRU. It should be the serial number from the Type 4 record (if provided). And I think that's universal - whether or not the socket itself is a FRU. 271 if (FM_AWARE_SMBIOS(mod)) { 272 (void) topo_node_label_set(strand, NULL, &perr); 273 274 if (topo_node_resource(strand, &fmri, &perr) != 0) 275 whinge(mod, &nerr, "create_strand: " 276 "topo_node_resource failed\n"); 277 278 perr += nvlist_lookup_string(fmri, 279 FM_FMRI_HC_PART, &part); 280 perr += nvlist_lookup_string(fmri, 281 FM_FMRI_HC_REVISION, &rev); 282 283 if (perr != 0) 284 whinge(mod, NULL, 285 "create_strand: nvlist_add_string failed\n"); 286 287 perr += topo_prop_set_string(strand, PGNAME(STRAND), 288 FM_FMRI_HC_SERIAL_ID, TOPO_PROP_IMMUTABLE, serial, &perr); 289 perr += topo_prop_set_string(strand, PGNAME(STRAND), 290 FM_FMRI_HC_PART, TOPO_PROP_IMMUTABLE, part, &perr); 291 perr += topo_prop_set_string(strand, PGNAME(STRAND), 292 FM_FMRI_HC_REVISION, TOPO_PROP_IMMUTABLE, rev, &perr); 293 294 if (perr != 0) 295 whinge(mod, NULL, "create_strand: topo_prop_set_string" 296 "failed\n"); 297 298 nvlist_free(fmri); 299 topo_mod_strfree(mod, serial); 300 } Does any of this code belong? This is enumerating a strand, which from a label perspective will be the same as its parent core (which will be the same as its parent chip). Why isn't a single topo_node_label_set(strand, NULL, &perr) sufficient? If it does belong, line 285, copy/paste error? Debug message should reference nvlist_lookup_string(). 305 static int 306 create_core(topo_mod_t *mod, tnode_t *pnode, nvlist_t *cpu, 307 nvlist_t *auth, uint16_t chip_smbiosid) Same questions from the strand logic above (ASRU serial, label) apply to the core. 514 } else { 515 if (topo_node_resource(chip, &fmri, &perr) != 0) 516 whinge(mod, &nerr, "create_chip: " 517 "topo_node_resource failed\n"); ... 527 perr += nvlist_lookup_string(fmri, 528 FM_FMRI_HC_SERIAL_ID, &serial); 529 perr += nvlist_lookup_string(fmri, 530 FM_FMRI_HC_PART, &part); 531 perr += nvlist_lookup_string(fmri, 532 FM_FMRI_HC_REVISION, &rev); 533 534 if (perr != 0) 535 whinge(mod, NULL, 536 "create_chip: nvlist_add_string" 537 "failed\n"); First the nit. Line 536 I think should be nvlist_lookup_string. Now, wrt perr. At 514, perr may be set if topo_node_resource() fails. But the code keeps on marching. First, should it? Second, if it does and 514 has failed, you're guaranteed a misleading debug msg at 535. This same theme continues through this portion of the if/else body. 587 /* 588 * STATUS 589 * CPU Socket Populated 590 * CPU Socket Unpopulated 591 * Populated : Enabled 592 * Populated : Disabled by BIOS (Setup) 593 * Populated : Disabled by BIOS (Error) 594 * Populated : Idle In addition to adding in where these comes from (DMTF spec), I think this comment is better served colocated with topo_status_smbios_get(). Essentially, that routine is deciding whether or not to report the socket as somethin to be enumerated. ==usr/src/lib/fm/topo/modules/i86pc/chip/chip_smbios.c== General comment. The routines beginning with topo_ may cause confusion. When I saw these in other areas of the chip enum, I thought these were calls into libtopo. 280 /* 281 * This could be defined as topo_mod_strlen() 282 */ 283 size_t 284 topo_chip_strlen(const char *str) Then should it be? If other enum plugins would make use of this, then do so (note: likely additional ARC work) 294 static const char * 295 chip_cleanup_smbios_str(topo_mod_t *mod, const char *begin, int str_type) 296 { I think a comment here summarizing what string cleanup is being done is warranted. 383 if (buf != NULL) { 384 if (label != NULL) { 385 (void) strcpy(buf, label); 386 (void) strcat(buf, delim); 387 /* 388 * If we are working on a DIMM 389 * smbi_location has the Device Locator. 390 * add the Device Locator ex: CPU 0 391 * add the Bank Locator ex: DIMM 0 392 */ 393 (void) strcat(buf, c.smbi_location); 394 } else 395 (void) strcpy(buf, c.smbi_location); It looks like strcpy and strcat are OK here....generally get nervous when these are around. Any reason not to use their bounds-checking variants? ==usr/src/uts/common/os/fm.c== 1280 /* 1281 * same code as in fm_fmri_hc_set_common 1282 */ 1283 if (version != FM_HC_SCHEME_VERSION) { ... Then why not call fm_fmri_hc_set_common() vs. duplicating code?</pre>

<pre wrap=""> _______________________________________________ fm-discuss mailing list fm-discuss at opensolaris dot org </pre>

<pre wrap="">On Wed, Sep 02, 2009 at 12:25:36PM -0400, Tom Pothier wrote: </pre>

<pre wrap="">The webrev is now also available on opensolaris.org: http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/ thx, -t </pre>

<pre wrap=""><!----> I reviewed just the SMBIOS part. Here are the changes I'd like to see: usr/src/common/smbios/smb_info.c Line 275 needs to have this code: if (isp->is_type == SMB_TYPE_EOT) return (smb_set_errno(shp, ESMB_TYPE)); otherwise you return bogus data. </pre>

<pre wrap=""> usr/src/common/smbios/smb_info.c The interface to smb_info_contains() is not what I want. I don't want to expose bcopy'ing out the implementation gunk with min/max. </pre>

<pre wrap=""> I just want it to return an array of contained id_t's of other records. The interface should be this: int smbios_info_contains(smbios_hdl_t *shp, id_t id, uint_t idc, id_t *idv); The caller passes an array of uninitialized id_t's 'idv', length 'idc' The function decodes everything about record 'id', copies out the internal record handles from whatever their encoding is (uint8/16) to a set of id_t's with a for loop (not bcopy, since the size varies). The function returns the actual count of contained things. If 'idc' is smaller than that count, then idv only contains 'idc' elements. i.e. you do not copy past the end of the caller's buffer. The smb_infospecs table should contain fields to indicate (a) offset of member with count of elements, (b) offset of member with contained elements, (c) something to indicate format of those contained records. For (c) either introduce flags or have a function pointer to handle the different cases. </pre>

<pre wrap=""> The rest of smbios stuff looks good: thanks for working on this. Please be sure to get a lot of help testing-- this wad is really important but has a rather huge test matrix. -Mike </pre>