Code Review: x86 Generic FMA Topology Enumerator

The webrev is now also available on opensolaris.org:

http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/

thx,
-t

Post by Tom Pothier
Hi,
I'd like to invite folks to participate in the code review for the x86
Generic FMA Topology Enumerator project. I'd like to have comments
back no later than COB on October 5, 2009.
https://ssg-east.east.sun.com/~pothier/webrevs/onnv-x86gentopo-pb/
I've also placed the code review 'onnv-x86gentopo-pb.pdf' file on the
OpenSolaris project page under the "Documents" heading.
changeset: 10254:27683d5d640a
tag: tip
date: Tue Sep 01 15:15:19 2009 -0400
PSARC/2009/XXX x86 Generic FMA Topology Enumerator
6841286 Need x86 generic FMA topo enumerator
6853537 x86gentopo needs OEM-Specific SMBIOS structures
6785310 Implement SMBIOS contained elements/handles
6865771 Topology relationships should be derived from contained
handles & elements of SMBIOS
6865814 Chip enumerator should derive serials & labels using
libsmbios, if SMBIOS is FM aware
6865845 /dev/fm should export the Initial APICID, SMBIOS based
ID/instance to the chip enumerator
6866456 Generic Topology FMRI ereport
usr/src/cmd/fm/eversholt/files/i386/i86pc/intel.esc
usr/src/cmd/mdb/intel/modules/generic_cpu/gcpu.c
usr/src/cmd/smbios/smbios.c
usr/src/common/smbios/smb_info.c
usr/src/common/smbios/smb_open.c
usr/src/lib/fm/topo/libtopo/common/mapfile-vers
usr/src/lib/fm/topo/libtopo/common/topo_mod.map
usr/src/lib/fm/topo/maps/i86pc/Makefile
usr/src/lib/fm/topo/maps/i86pc/i86pc-hc-topology.xml
usr/src/lib/fm/topo/modules/common/pcibus/did_props.c
usr/src/lib/fm/topo/modules/i86pc/Makefile
usr/src/lib/fm/topo/modules/i86pc/chip/Makefile
usr/src/lib/fm/topo/modules/i86pc/chip/chip.c
usr/src/lib/fm/topo/modules/i86pc/chip/chip.h
usr/src/lib/fm/topo/modules/i86pc/chip/chip_amd.c
usr/src/lib/fm/topo/modules/i86pc/chip/chip_intel.c
usr/src/lib/libsmbios/common/mapfile-vers
usr/src/pkgdefs/SUNWfmd/prototype_i386
usr/src/uts/common/io/devfm.c
usr/src/uts/common/os/fm.c
usr/src/uts/common/sys/devfm.h
usr/src/uts/common/sys/fm/protocol.h
usr/src/uts/common/sys/smbios.h
usr/src/uts/common/sys/smbios_impl.h
usr/src/uts/i86pc/cpu/amd_opteron/ao_mca.c
usr/src/uts/i86pc/cpu/authenticamd/authamd_main.c
usr/src/uts/i86pc/cpu/generic_cpu/gcpu_mca.c
usr/src/uts/i86pc/cpu/genuineintel/gintel_main.c
usr/src/uts/i86pc/os/cmi.c
usr/src/uts/i86pc/os/cmi_hw.c
usr/src/uts/i86pc/os/startup.c
usr/src/uts/i86xpv/os/xen_machdep.c
usr/src/uts/intel/Makefile.files
usr/src/uts/intel/io/devfm_machdep.c
usr/src/uts/intel/io/mc-amd/mcamd.h
usr/src/uts/intel/io/mc-amd/mcamd_drv.c
usr/src/uts/intel/io/mc-amd/mcamd_subr.c
usr/src/uts/intel/sys/cpu_module.h
usr/src/uts/intel/sys/hypervisor.h
usr/src/lib/fm/topo/maps/i86pc/i86pc-legacy-hc-topology.xml
usr/src/lib/fm/topo/modules/i86pc/chip/chip_smbios.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/Makefile
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_bboard.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_chassis.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_generic.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_hostbridge.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_impl.h
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_subr.c
usr/src/uts/intel/os/fmsmb.c
usr/src/uts/intel/sys/fm/smb/fmsmb.h
thx,
-t

Tom Pothier

2009-09-03 12:50:25 UTC

Hi,

If you plan on reviewing this code could you please reply to me as I
need a list of reviewers for the C-Team checklist.

thx,
-t

Post by Tom Pothier
http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/
thx,
-t

Post by Tom Pothier
Hi,
I'd like to invite folks to participate in the code review for the
x86 Generic FMA Topology Enumerator project. I'd like to have
comments back no later than COB on October 5, 2009.
https://ssg-east.east.sun.com/~pothier/webrevs/onnv-x86gentopo-pb/
I've also placed the code review 'onnv-x86gentopo-pb.pdf' file on the
OpenSolaris project page under the "Documents" heading.
changeset: 10254:27683d5d640a
tag: tip
date: Tue Sep 01 15:15:19 2009 -0400
PSARC/2009/XXX x86 Generic FMA Topology Enumerator
6841286 Need x86 generic FMA topo enumerator
6853537 x86gentopo needs OEM-Specific SMBIOS structures
6785310 Implement SMBIOS contained elements/handles
6865771 Topology relationships should be derived from
contained handles & elements of SMBIOS
6865814 Chip enumerator should derive serials & labels using
libsmbios, if SMBIOS is FM aware
6865845 /dev/fm should export the Initial APICID, SMBIOS based
ID/instance to the chip enumerator
6866456 Generic Topology FMRI ereport
usr/src/cmd/fm/eversholt/files/i386/i86pc/intel.esc
usr/src/cmd/mdb/intel/modules/generic_cpu/gcpu.c
usr/src/cmd/smbios/smbios.c
usr/src/common/smbios/smb_info.c
usr/src/common/smbios/smb_open.c
usr/src/lib/fm/topo/libtopo/common/mapfile-vers
usr/src/lib/fm/topo/libtopo/common/topo_mod.map
usr/src/lib/fm/topo/maps/i86pc/Makefile
usr/src/lib/fm/topo/maps/i86pc/i86pc-hc-topology.xml
usr/src/lib/fm/topo/modules/common/pcibus/did_props.c
usr/src/lib/fm/topo/modules/i86pc/Makefile
usr/src/lib/fm/topo/modules/i86pc/chip/Makefile
usr/src/lib/fm/topo/modules/i86pc/chip/chip.c
usr/src/lib/fm/topo/modules/i86pc/chip/chip.h
usr/src/lib/fm/topo/modules/i86pc/chip/chip_amd.c
usr/src/lib/fm/topo/modules/i86pc/chip/chip_intel.c
usr/src/lib/libsmbios/common/mapfile-vers
usr/src/pkgdefs/SUNWfmd/prototype_i386
usr/src/uts/common/io/devfm.c
usr/src/uts/common/os/fm.c
usr/src/uts/common/sys/devfm.h
usr/src/uts/common/sys/fm/protocol.h
usr/src/uts/common/sys/smbios.h
usr/src/uts/common/sys/smbios_impl.h
usr/src/uts/i86pc/cpu/amd_opteron/ao_mca.c
usr/src/uts/i86pc/cpu/authenticamd/authamd_main.c
usr/src/uts/i86pc/cpu/generic_cpu/gcpu_mca.c
usr/src/uts/i86pc/cpu/genuineintel/gintel_main.c
usr/src/uts/i86pc/os/cmi.c
usr/src/uts/i86pc/os/cmi_hw.c
usr/src/uts/i86pc/os/startup.c
usr/src/uts/i86xpv/os/xen_machdep.c
usr/src/uts/intel/Makefile.files
usr/src/uts/intel/io/devfm_machdep.c
usr/src/uts/intel/io/mc-amd/mcamd.h
usr/src/uts/intel/io/mc-amd/mcamd_drv.c
usr/src/uts/intel/io/mc-amd/mcamd_subr.c
usr/src/uts/intel/sys/cpu_module.h
usr/src/uts/intel/sys/hypervisor.h
usr/src/lib/fm/topo/maps/i86pc/i86pc-legacy-hc-topology.xml
usr/src/lib/fm/topo/modules/i86pc/chip/chip_smbios.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/Makefile
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_bboard.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_chassis.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_generic.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_hostbridge.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_impl.h
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_subr.c
usr/src/uts/intel/os/fmsmb.c
usr/src/uts/intel/sys/fm/smb/fmsmb.h
thx,
-t

Scott Davenport

2009-09-04 21:40:36 UTC

Post by Tom Pothier
http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/

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 

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?

Srihari Venkatesan

2009-09-04 23:40:30 UTC

Hi Scott,

Thanks for the review, please see some responses inline..
(responded only to the parts that i implemented)

Post by Tom Pothier
http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/

X86PI_PARTIAL needs to be removed in x86pi_impl.h & chip.h
Was in the pending list, but got missed..
I will file a pre-rti CR to fix this.

Post by Scott Davenport
==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.

Agree, this needs to be fixed, ASRU's serial number should not be
dependent on the FRUness of the Socket.
Will raise a pre-rti CR and fix it.

Post by Scott Davenport
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?

After setting the Label for the Strand node, we also add Serial,
Part & Revision in the "strand-properties" ( - this was based upon
a comment from the portfolio-review that - chip/core/strand should
all carry the serial, part, revision properties separately.. )

Should this be reconsidered ?

Post by Scott Davenport
If it does belong, line 285, copy/paste error? Debug message
should reference nvlist_lookup_string().

Yes, will fix it.

Post by Scott Davenport
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.

Yes, will fix it.

Post by Scott Davenport
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.

Will fix.

Post by Scott Davenport
Now, wrt perr. At 514, perr may be set if topo_node_resource()
fails. But the code keeps on marching. First, should it?

Yes, thats the way most of the Serial, Part, Revision related failures
(failures
related to SMBIOS derivations or other) is treated.. we complain
via debug messages, but we march, so that even without
Serials, Part, Revisions, Labels etc, a bare topology can still get
enumerated.

I can add a debug message for each nvlist_lookup_string() failure...

Post by Scott Davenport
Second,
if it does and 514 has failed, you're guaranteed a misleading
debug msg at 535.

True, will fix.

Post by Scott Davenport
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.

ok.

Post by Scott Davenport
==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.

ok. will pick function names that will start with chip_ instead of topo_

Post by Scott Davenport
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)

I had raised a need for topo_mod_strlen() earlier, did not get feedback
and there was no other consumer in the x86gentopo project, apart
from chip_smbios.c, so confined it here.
Will take this change outside the scope of this project.

Post by Scott Davenport
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.

Will add a comment.

Post by Scott Davenport
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?

Yes, will use strlcpy, strlcat & check for buffer overruns..

also noted now that,
lines
377 buf = topo_mod_alloc(mod, topo_chip_strlen(label) +
378 topo_chip_strlen(delim) +
379 topo_chip_strlen(c.smbi_location) +
380 topo_chip_strlen(blank) +
381 topo_chip_strlen(dimm_bank));

should be
377 buf = topo_mod_alloc(mod, topo_chip_strlen(label) +
...
...
381 topo_chip_strlen(dimm_bank) + 1);

will fix it.

(will fix as review-feedback continues and will keep updating the
changes to Tom)

Thank you
Srihari

Post by Scott Davenport
==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?
_______________________________________________
fm-discuss mailing list

Scott Davenport

2009-09-08 20:13:59 UTC

Thanks for the other replies, Srihari. A followup on the
strand enum code below.

-scott

Post by Scott Davenport
271 if (FM_AWARE_SMBIOS(mod)) {
272 (void) topo_node_label_set(strand, NULL,

&perr);

Post by Scott Davenport
273
274 if (topo_node_resource(strand, &fmri, &perr) !=

Post by Scott Davenport
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");

Post by Scott Davenport
286
287 perr += topo_prop_set_string(strand,

PGNAME(STRAND),

Post by Scott Davenport
288 FM_FMRI_HC_SERIAL_ID, TOPO_PROP_IMMUTABLE,

serial, &perr);

Post by Scott Davenport
289 perr += topo_prop_set_string(strand,

PGNAME(STRAND),

Post by Scott Davenport
290 FM_FMRI_HC_PART, TOPO_PROP_IMMUTABLE, part,

&perr);

Post by Scott Davenport
291 perr += topo_prop_set_string(strand,

PGNAME(STRAND),

Post by Scott Davenport
292 FM_FMRI_HC_REVISION, TOPO_PROP_IMMUTABLE,

rev, &perr);

Post by Scott Davenport
293
294 if (perr != 0)

topo_prop_set_string"

Post by Scott Davenport
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?

Alright....I don't recall that comment. Or there's a variance on
the interpretation of "separately". If Serviceability has a
need for this (perhaps ASR?), then OK.

My understanding is that the FRU FMRI certainly needs to have the
part/serial/rev of the FRU in question. For a strand, this will be
the same as the parent chip. I'd think that topo_node_fru_set() with
the 2nd parameter as NULL would cover this. I'm less clear that
strand-properties itself needs the same info.

Thx,
-scott

Tom Pothier

2009-09-10 15:05:41 UTC

Thanks Scott, I'll fill in my part below.

thx,
-t

Post by Srihari Venkatesan
Hi Scott,
Thanks for the review, please see some responses inline..
(responded only to the parts that i implemented)

Post by Tom Pothier
http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/

x86gentopo spec'd it and is the only consumer; the PRMS needs to change
(I sent the email).

Post by Scott Davenport
==usr/src/lib/fm/topo/maps/i86pc/i86pc-legacy-hc-topology.xml==
28 
Nit. "compliance" --> "compliant"

Done.

Post by Scott Davenport
==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?

I've sent an email to the author asking for clarification.

Post by Scott Davenport
==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.

X86PI_PARTIAL needs to be removed in x86pi_impl.h & chip.h
Was in the pending list, but got missed..
I will file a pre-rti CR to fix this.

Yes, sorry. Thanks for taking care of this Srihari :-) .

Post by Scott Davenport
==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.

Agree, this needs to be fixed, ASRU's serial number should not be
dependent on the FRUness of the Socket.
Will raise a pre-rti CR and fix it.

Post by Scott Davenport
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?

Post by Scott Davenport
If it does belong, line 285, copy/paste error? Debug message
should reference nvlist_lookup_string().

Yes, will fix it.

Post by Scott Davenport
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.

Will fix.

Post by Scott Davenport
Now, wrt perr. At 514, perr may be set if topo_node_resource()
fails. But the code keeps on marching. First, should it?

Yes, thats the way most of the Serial, Part, Revision related failures
(failures related to SMBIOS derivations or other) is treated.. we
complain
via debug messages, but we march, so that even without
Serials, Part, Revisions, Labels etc, a bare topology can still get
enumerated.
I can add a debug message for each nvlist_lookup_string() failure...

Post by Scott Davenport
Second,
if it does and 514 has failed, you're guaranteed a misleading
debug msg at 535.

True, will fix.

ok.

ok. will pick function names that will start with chip_ instead of topo_

Will add a comment.

Yes, will use strlcpy, strlcat & check for buffer overruns..
also noted now that,
lines
377 buf = topo_mod_alloc(mod, topo_chip_strlen(label) +
378 topo_chip_strlen(delim) +
379 topo_chip_strlen(c.smbi_location) +
380 topo_chip_strlen(blank) +
381 topo_chip_strlen(dimm_bank));
should be
377 buf = topo_mod_alloc(mod, topo_chip_strlen(label) +
...
...
381 topo_chip_strlen(dimm_bank) + 1);
will fix it.
(will fix as review-feedback continues and will keep updating the
changes to Tom)
Thank you
Srihari

Trang Do

2009-09-09 18:37:09 UTC

Hi Scott,
Thank you for reviewing. Please see inline (fm.c)

Post by Tom Pothier
http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/

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 
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?

Agreed. Will call fm_fmri_hc_set_common(). I don't remember why I
duplicated the code

Thanks
Trang

Post by Scott Davenport
_______________________________________________
fm-discuss mailing list

Mike Shapiro

2009-09-06 00:59:02 UTC

Post by Tom Pothier
http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/
thx,
-t

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.

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.
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.

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

--
Mike Shapiro, Sun Microsystems Open Storage / Fishworks. blogs.sun.com/mws/

Dale Ghent

2009-09-06 02:26:57 UTC

FWIW, I have a contribution being reviewed by Seth G that updates the
smbios components tables that are in 2.6

http://cr.opensolaris.org/~daleg/smbios2.6/

dunno if it has any effect on this, but figured I'd let you all know
since this touches smbios too

/dale

Post by Tom Pothier
http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/
thx,
-t

usr/src/common/smbios/smb_info.c
if (isp->is_type == SMB_TYPE_EOT)
return (smb_set_errno(shp, ESMB_TYPE));
otherwise you return bogus data.
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.
I just want it to return an array of contained id_t's of other records.
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.
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
--
Mike Shapiro, Sun Microsystems Open Storage / Fishworks. blogs.sun.com/mws/
_______________________________________________
x86gentopo-discuss mailing list
http://mail.opensolaris.org/mailman/listinfo/x86gentopo-discuss

Tom Pothier

2009-09-08 13:19:51 UTC

Thank you; these changes don't seem to impact our code, but the heads up
is always appreciated :-) .

thx,
-t

Post by Dale Ghent
FWIW, I have a contribution being reviewed by Seth G that updates the
smbios components tables that are in 2.6
http://cr.opensolaris.org/~daleg/smbios2.6/
dunno if it has any effect on this, but figured I'd let you all know
since this touches smbios too
/dale

Post by Tom Pothier
http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/
thx,
-t

usr/src/common/smbios/smb_info.c
if (isp->is_type == SMB_TYPE_EOT)
return (smb_set_errno(shp, ESMB_TYPE));
otherwise you return bogus data.
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.
I just want it to return an array of contained id_t's of other records.
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.
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
--
Mike Shapiro, Sun Microsystems Open Storage / Fishworks.
blogs.sun.com/mws/
_______________________________________________
x86gentopo-discuss mailing list
http://mail.opensolaris.org/mailman/listinfo/x86gentopo-discuss

Tom Pothier

2009-09-10 19:32:40 UTC

Hi Mike,

Thank you, comments below.

Post by Tom Pothier
http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/
thx,
-t

usr/src/common/smbios/smb_info.c
if (isp->is_type == SMB_TYPE_EOT)
return (smb_set_errno(shp, ESMB_TYPE));
otherwise you return bogus data.

Added. Should this also be added to smbios_info_common() at line 207?

Post by Tom Pothier
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.

Are you referencing the Type-3 elements min/max here?

Post by Tom Pothier
I just want it to return an array of contained id_t's of other records.
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.

So I think what you want is in the case of Type-2 "Contained Object
Handles", return an array of these handles; in the case of Type-4
contained elements return an array of just the "Contained Element
Type"(s) - not the entire element.

If this is the case, I'll start making the changes and expect them in
the next webrev.

thx,
-t

Post by Tom Pothier
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

Mike Shapiro

2009-09-24 19:05:11 UTC

Post by Tom Pothier
Hi Mike,
Thank you, comments below.

Post by Tom Pothier
http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/
thx,
-t

usr/src/common/smbios/smb_info.c
if (isp->is_type == SMB_TYPE_EOT)
return (smb_set_errno(shp, ESMB_TYPE));
otherwise you return bogus data.

Added. Should this also be added to smbios_info_common() at line 207?

Sorry for delay in replying: I've been out past week and a half.
No, it doesn't belong there because the code is written to return
a set of empty strings for types that have no common info.

Post by Tom Pothier
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.

Are you referencing the Type-3 elements min/max here?

Yes.

Post by Tom Pothier
I just want it to return an array of contained id_t's of other records.
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.

So I think what you want is in the case of Type-2 "Contained Object
Handles", return an array of these handles; in the case of Type-4
contained elements return an array of just the "Contained Element
Type"(s) - not the entire element.
If this is the case, I'll start making the changes and expect them
in the next webrev.
thx,
-t

Yes. Send back an updated webrev when you have it and I'll
try to respin very quickly for you. Appreciate your patience.

-Mike

---
Mike Shapiro, Sun Microsystems Open Storage / Fishworks. blogs.sun.com/
mws/

Scott Davenport

2009-09-08 23:40:34 UTC

Post by Tom Pothier
http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/

Tom,

Finished the review of the enumerator proper. Comments below.
Expect to finish the rest tomorrow. Sorry this is taking
several iterations, lots of code here.

-scott

==usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi.c==
231 * If there are no contained elements/handles,
232 * build a best guess topo:
233 *
234 * Main Chassis
235 * Motherboard
236 * CMP Chip/Core/Strands
237 * Memory Controllers/Memory Devices (DIMMs)
238 * PCIE HostBrige
239 * PCIE Root Complex

Hmm...the above looks fine (not a guess) when
there's no contained handles and only a single
Type 2 record. A monolithic motherboard config.
And memory controller/chip relationship comes
from the OEM extensions.

And if there's multiple Type 2s *without*
contained handles, that should be a violation
indicating an out-of-spec SMBIOS, right? I
don't see a check for this in fm_smb_check().

I suppose I'm taking exception to the "best
guess" phrase on 232.

==usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_bboard.c==
108 /*
109 * Set the FRU lable to "MB" for base board type 'motherboard';
110 * otherwise use the SMBIOS location string.
111 */
112 if (bb.smbb_type == SMB_BBT_MOTHER) {
113 bb_hcfmri.location = topo_mod_strdup(mod, "MB");
114 } else {
115 bb_hcfmri.location = topo_mod_strdup(mod, ip.smbi_location);
116 }

This looks incorrect. Just because a structure type is
SMB_BBT_MOTHER doesn't mean it's label is unilaterally "MB".
I believe that the SMBIOS location string should always be
taken for the label. Case in point - a system with multiple
boards all with SMB_BBT_MOTHER Type 2s. I wouldn't even
advocate using "MB" as a fallback if the SMBIOS location
field is unpopulated. The generic code should not make any
assumptions on what labels are.

122 switch (bbnp->type) {
123 case SMB_BBT_SBLADE :
124 case SMB_BBT_DAUGHTER :
125 case SMB_BBT_PROCMEM :
126 case SMB_BBT_PROCIO :
127 case SMB_BBT_INTER :
128 instance = systemboard++;
129 break;
130 case SMB_BBT_PROC :
131 instance = cpuboard++;
132 break;
133 case SMB_BBT_IO :
134 instance = ioboard++;
135 break;
136 case SMB_BBT_MEM :
137 instance = memboard++;
138 break;
139 case SMB_BBT_MOTHER :
140 instance = motherboard++;
141 break;
142 /*
143 * Enumerate other baseboard types
144 * as systemboard
145 */
146 default :
147 instance = systemboard++;
148 }

Nit. You can refactor here and combine lines 123-127 and line
146 into a single case.

338 /* allocate space for and get contained hanldes */

Nit. "hanldes" --> "handles".

==usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_hostbridge.c==
135 (void) did_props_set(tn_hbr, did, ExHB_common_props, ExHB_propcnt - 2);
...
161 if (did_props_set(tn_rc, did, RC_common_props, RC_propcnt - 2) < 0) {
...
192 (void) did_props_set(tn_hbr, did, HB_common_props, HB_propcnt - 2);

Can the magic '2's be explained?

==usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_subr.c==
475 * All the checks for compatibility are done within the kenrel where the

Nit. "kenrel" --> "kernel".

==usr/src/uts/intel/os/fmsmb.c==
342 /*
343 * Verify SMBIOS structures for x86 generic topology.
344 *
345 * Return (0) on success.
346 */
347 static int
348 fm_smb_check(smbios_hdl_t *shp)

This routine needs a check to return -1 for
# Type 2 > 1 AND contained elements/handles
*not* present. See notes above on x86pi.c.

Tom Pothier

2009-09-10 17:10:24 UTC

Thanks Scott (again :-) ). my comments below.

thx,
-t

Post by Tom Pothier
http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/

Yeap.

Post by Scott Davenport
And if there's multiple Type 2s *without*
contained handles, that should be a violation
indicating an out-of-spec SMBIOS, right? I
don't see a check for this in fm_smb_check().

Check added.

Post by Scott Davenport
I suppose I'm taking exception to the "best
guess" phrase on 232.

Re-phrased.

Post by Scott Davenport
==usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_bboard.c==
108 /*
109 * Set the FRU lable to "MB" for base board type 'motherboard';
110 * otherwise use the SMBIOS location string.
111 */
112 if (bb.smbb_type == SMB_BBT_MOTHER) {
113 bb_hcfmri.location = topo_mod_strdup(mod, "MB");
114 } else {
115 bb_hcfmri.location = topo_mod_strdup(mod, ip.smbi_location);
116 }
This looks incorrect. Just because a structure type is
SMB_BBT_MOTHER doesn't mean it's label is unilaterally "MB".
I believe that the SMBIOS location string should always be
taken for the label. Case in point - a system with multiple
boards all with SMB_BBT_MOTHER Type 2s. I wouldn't even
advocate using "MB" as a fallback if the SMBIOS location
field is unpopulated. The generic code should not make any
assumptions on what labels are.

Yes, I agree. Changed to use the "Location in Chassis" string.

Post by Scott Davenport
122 switch (bbnp->type) {
128 instance = systemboard++;
129 break;
131 instance = cpuboard++;
132 break;
134 instance = ioboard++;
135 break;
137 instance = memboard++;
138 break;
140 instance = motherboard++;
141 break;
142 /*
143 * Enumerate other baseboard types
144 * as systemboard
145 */
147 instance = systemboard++;
148 }
Nit. You can refactor here and combine lines 123-127 and line
146 into a single case.

Yes, I meant to do this :-( . I changed this to only the default case
for systemboard and put all other board "types" in the comment.

Post by Scott Davenport
338 /* allocate space for and get contained hanldes */
Nit. "hanldes" --> "handles".

Done.

Post by Scott Davenport
==usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_hostbridge.c==
135 (void) did_props_set(tn_hbr, did, ExHB_common_props, ExHB_propcnt - 2);
...
161 if (did_props_set(tn_rc, did, RC_common_props, RC_propcnt - 2) < 0) {
...
192 (void) did_props_set(tn_hbr, did, HB_common_props, HB_propcnt - 2);
Can the magic '2's be explained?

Magically delicious ?? :-) . Ah, this is because ExHB_propcnt is set
to the total number of properties and x86gentopo will define FRU and
Label itself (-2). This also goes back to your comment why the label and
fru properties are moved in
usr/src/lib/fm/topo/modules/common/pcibus/did_props.c; it's because
x86gentopo takes care of them and doesn't rely on the pcibus enumerator
to: ddi_props_set(..,..,.., propcnt -2)

Post by Scott Davenport
==usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_subr.c==
475 * All the checks for compatibility are done within the kenrel where the
Nit. "kenrel" --> "kernel".

Done.

Post by Scott Davenport
==usr/src/uts/intel/os/fmsmb.c==
342 /*
343 * Verify SMBIOS structures for x86 generic topology.
344 *
345 * Return (0) on success.
346 */
347 static int
348 fm_smb_check(smbios_hdl_t *shp)
This routine needs a check to return -1 for
# Type 2 > 1 AND contained elements/handles
*not* present. See notes above on x86pi.c.

Yup, added.

thx,
-t

Scott Davenport

2009-09-09 18:42:36 UTC

Post by Tom Pothier
http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/
thx,
-t

Tom,

And now been through the remainder of the code. Comments below.

Thanks,
-scott

==usr/src/uts/common/os/fm.c==
1269 fm_fmri_hc_create(nvlist_t *fmri, int version, const nvlist_t *auth,
...
1322 pairs[i] = fm_nvlist_create(nva);
1323 if (nvlist_add_string(pairs[i], FM_FMRI_HC_NAME, hcname) != 0 ||
1324 nvlist_add_string(pairs[i], FM_FMRI_HC_ID, hcid) != 0) {
1325 atomic_add_64(
1326 &erpt_kstat_data.fmri_set_failed.value.ui64, 1);
1327 return;
1328 }
...
1359 if (nvlist_add_nvlist_array(fmri, FM_FMRI_HC_LIST, pairs,
1360 npairs + n) != 0) {
1361 atomic_add_64(&erpt_kstat_data.fmri_set_failed.value.ui64, 1);
1362 return;
1363 }
1364
1365 for (i = 0; i < npairs + n; i++) {
1366 if (pairs[i] != NULL)
1367 fm_nvlist_destroy(pairs[i], FM_NVA_RETAIN);
1368 }

Possible memory leak here? You're walking the list of bboards,
creating an nvlist pair for each. Suppose you're half way into the
walk and the fm_nvlist_create() at 1322 fails. This'll cause failures
for the nvlist_add_string() calls at 1323 & 1324. Then a stat is
bumped and return. Any successful fm_nvlist_create()s for pairs[i]
are left dangling.

I think the check for NULL at 1366 is unnecessary. If you've gotten
that far, all of the pairs[i] pointers are valid.

You may want to look at fm_fmri_hc_set(), which is doing something
similar to this function.

==usr/src/uts/i86pc/cpu/amd_opteron/ao_mca.c==

Nit. The 'fma_compat' variable seems unnecessary as it's
set as !x86gentopo_legacy, an externally available variable.
FWIW, using the extern is the route taken in authamd_main.c,
gcpu_mca.c, and gintel_main.c.

==usr/src/uts/i86pc/cpu/generic_cpu/gcpu_mca.c==
505 gcpu_fmri_create(cmi_hdl_t hdl, nv_alloc_t *nva)
...
512 if (!x86gentopo_legacy) {
513 fmri = cmi_hdl_smb_bboard(hdl);
514
515 if (fmri == NULL) {
516 if (panicstr) {
517 fm_nvlist_destroy(nvl, FM_NVA_RETAIN);
518 nv_alloc_reset(nva);
519 } else {
520 fm_nvlist_destroy(nvl, FM_NVA_FREE);
521 }
522 return (NULL);
523 }
524
525 fm_fmri_hc_create(nvl, FM_HC_SCHEME_VERSION,
526 NULL, NULL, fmri, 3,
527 "chip", cmi_hdl_smb_chipid(hdl),
528 "core", cmi_hdl_coreid(hdl),
529 "strand", cmi_hdl_strandid(hdl));
530 } else {
531 fm_fmri_hc_set(nvl, FM_HC_SCHEME_VERSION, NULL, NULL, 4,
532 "motherboard", 0,
533 "chip", cmi_hdl_chipid(hdl),
534 "core", cmi_hdl_coreid(hdl),
535 "strand", cmi_hdl_strandid(hdl));
536 }

Curious...why do the checks at 515 and 516 only apply for
generic enumeration? Why not legacy mode too?

==usr/src/uts/i86pc/cpu/genuineintel/gintel_main.c==
297 gintel_gentopo_ereport_create_resource_elem(cmi_hdl_t hdl, nv_alloc_t *nva,
298 mc_unum_t *unump)
299 {
300 nvlist_t *nvl, *snvl;
301 nvlist_t *board_list = NULL;
302 board_list = cmi_hdl_smb_bboard(hdl);
303 /*
304 * do we need to duplicate the nvlist here?
305 */
306 if (board_list == NULL) {
307 return (NULL);
308 }

Has the question at 304 been answered?

==usr/src/uts/i86pc/os/cmi_hw.c==
1287 hdl->cmih_smb_bboard = fm_smb_bboard(strand_apicid);
1288 if (hdl->cmih_smb_bboard == NULL)
1289 cmn_err(CE_WARN, "Read smbios boards failed");

Please add a comment that if 1288 is true, topo reverts to
legacy mode (via the calls to fm_smb_bboard() and smb_bboard()
in fmsmb.c)

==usr/src/uts/intel/os/fmsmb.c==

In one of my prior emails, I had comments on this file.
Noticed a few more things the 2nd time around.

52 typedef enum baseb {
53 BB_BAD = 0, /* There is no bb value 0 */
54 BB_UNKOWN, /* Unknown */
55 BB_OTHER, /* Other */
...

Comment referencing where enum values come from (DMTF) would
be nice. "BB_UNKOWN" --> "BB_UNKNOWN".

69 static struct bboard_type {
70 bbd_t baseb;
71 const char *name;
72 } bbd_type[] = {
73 {BB_BAD, NULL},
74 {BB_UNKOWN, "unknown"},
75 {BB_OTHER, "other"},
76 {BB_BLADE, "blade"},
77 {BB_CONNSW, "connswitch"},
78 {BB_SMM, "smmodule"},
79 {BB_PROCMOD, "cpuboard"},
80 {BB_IOMOD, "ioboard"},
81 {BB_MEMMOD, "memboard"},
82 {BB_DBOARD, "systemboard"},
83 {BB_MBOARD, "motherboard"},
84 {BB_PROCMMOD, "systemboard"},
85 {BB_PROCIOMOD, "systemboard"},
86 {BB_ICONNBD, "iconnboard"}
87 };
...

So this maps a baseboard type to an FMA hc scheme canonical
name. Note that several of the strings are no in the hc
names list (other, smmodule, iconnboard, etc). While they're
not used in the code today, it may be better not to map
these to bogus names. "BB_UNKOWN" --> "BB_UNKNOWN".

Also, in x86pi_bboard, SMB_BBT_BLADE is equated to "systemboard",
but here "blade". Disconnect?

108 static smbs_cnt_t *
109 smb_create_strcnt(int count)
110 {
111 smbs_cnt_t *types = NULL;
112 int i, j;
113
114 types = kmem_zalloc(sizeof (smbs_cnt_t), KM_SLEEP);
115 if (types == NULL) {
116 cmn_err(CE_WARN, "Can't allocate smb strcnt memory");
117 return (NULL);
118 }

You're using the KM_SLEEP flag here, so the memory alloc
is guaranteed to succeed (see kmem_alloc(9F)). The
checks from 115-118 are unnecessary. Ripple this through the
rest of the smb_create_strcnt() function.

931 if (find_matching_proc(shp, strand_apicid,
932 bb_smbid, proc_hdl, is_proc)) {
933 fmri = fm_nvlist_create(NULL);
934 if (fmri == NULL) {
935 cmn_err(CE_WARN, "Can't allocate fmri mem");
936 goto bad;
937 }
938 /*
939 * find parent by walking the cont_by_id
940 */
941 rc = smb_get_bb_fmri(shp, fmri, bb_smbid, bbstypes);
942 smb_free_strcnt(bbstypes, bb_strcnt);
943 if (rc == 0) {
944 return (fmri);
945 } else
946 goto bad;
947 }
948
949 }
950
951 cmn_err(CE_NOTE, "Can't get bboards");
952 smb_free_strcnt(bbstypes, bb_strcnt);
953 bad:

If the check at 934 is true, do you need to free bbstypes
before line 936?

Trang Do

2009-09-09 21:13:12 UTC

Scott,
Thanks for the review. Please see inline

Post by Tom Pothier
http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/
thx,
-t

Agreed. I need to free memory if the nvlist_add_string() fails.
I just checked the fm_fmri_hc_set() code. It does not free memory for
the similiar cases. I don't know if there is a reason for it, or the code
is missing.

Post by Scott Davenport
I think the check for NULL at 1366 is unnecessary. If you've gotten
that far, all of the pairs[i] pointers are valid.

Will remove the check

Post by Scott Davenport
You may want to look at fm_fmri_hc_set(), which is doing something
similar to this function.
==usr/src/uts/i86pc/cpu/amd_opteron/ao_mca.c==
Nit. The 'fma_compat' variable seems unnecessary as it's
set as !x86gentopo_legacy, an externally available variable.
FWIW, using the extern is the route taken in authamd_main.c,
gcpu_mca.c, and gintel_main.c.

will remove

Post by Scott Davenport
==usr/src/uts/i86pc/cpu/generic_cpu/gcpu_mca.c==
505 gcpu_fmri_create(cmi_hdl_t hdl, nv_alloc_t *nva)
...
512 if (!x86gentopo_legacy) {
513 fmri = cmi_hdl_smb_bboard(hdl);
514
515 if (fmri == NULL) {
516 if (panicstr) {
517 fm_nvlist_destroy(nvl, FM_NVA_RETAIN);
518 nv_alloc_reset(nva);
519 } else {
520 fm_nvlist_destroy(nvl, FM_NVA_FREE);
521 }
522 return (NULL);
523 }
524
525 fm_fmri_hc_create(nvl, FM_HC_SCHEME_VERSION,
526 NULL, NULL, fmri, 3,
527 "chip", cmi_hdl_smb_chipid(hdl),
528 "core", cmi_hdl_coreid(hdl),
529 "strand", cmi_hdl_strandid(hdl));
530 } else {
531 fm_fmri_hc_set(nvl, FM_HC_SCHEME_VERSION, NULL, NULL, 4,
532 "motherboard", 0,
533 "chip", cmi_hdl_chipid(hdl),
534 "core", cmi_hdl_coreid(hdl),
535 "strand", cmi_hdl_strandid(hdl));
536 }
Curious...why do the checks at 515 and 516 only apply for
generic enumeration? Why not legacy mode too?

The legacy mode does not need the check at 515 because it uses fixed
basedboard instead
of nvlist fmri.
The check at 516 does not need for both cases because the free memory is
done by
the caller, so I will remove the code from 516-521

Post by Scott Davenport
==usr/src/uts/i86pc/cpu/genuineintel/gintel_main.c==
297 gintel_gentopo_ereport_create_resource_elem(cmi_hdl_t hdl, nv_alloc_t *nva,
298 mc_unum_t *unump)
299 {
300 nvlist_t *nvl, *snvl;
301 nvlist_t *board_list = NULL;
302 board_list = cmi_hdl_smb_bboard(hdl);
303 /*
304 * do we need to duplicate the nvlist here?
305 */
306 if (board_list == NULL) {
307 return (NULL);
308 }
Has the question at 304 been answered?

Yes. I will remove the comments

Post by Scott Davenport
==usr/src/uts/i86pc/os/cmi_hw.c==
1287 hdl->cmih_smb_bboard = fm_smb_bboard(strand_apicid);
1288 if (hdl->cmih_smb_bboard == NULL)
1289 cmn_err(CE_WARN, "Read smbios boards failed");
Please add a comment that if 1288 is true, topo reverts to
legacy mode (via the calls to fm_smb_bboard() and smb_bboard()
in fmsmb.c)

Will do

Post by Scott Davenport
==usr/src/uts/intel/os/fmsmb.c==
In one of my prior emails, I had comments on this file.
Noticed a few more things the 2nd time around.
52 typedef enum baseb {
53 BB_BAD = 0, /* There is no bb value 0 */
54 BB_UNKOWN, /* Unknown */
55 BB_OTHER, /* Other */
...
Comment referencing where enum values come from (DMTF) would
be nice. "BB_UNKOWN" --> "BB_UNKNOWN".

Will change

Post by Scott Davenport
69 static struct bboard_type {
70 bbd_t baseb;
71 const char *name;
72 } bbd_type[] = {
73 {BB_BAD, NULL},
74 {BB_UNKOWN, "unknown"},
75 {BB_OTHER, "other"},
76 {BB_BLADE, "blade"},
77 {BB_CONNSW, "connswitch"},
78 {BB_SMM, "smmodule"},
79 {BB_PROCMOD, "cpuboard"},
80 {BB_IOMOD, "ioboard"},
81 {BB_MEMMOD, "memboard"},
82 {BB_DBOARD, "systemboard"},
83 {BB_MBOARD, "motherboard"},
84 {BB_PROCMMOD, "systemboard"},
85 {BB_PROCIOMOD, "systemboard"},
86 {BB_ICONNBD, "iconnboard"}
87 };
...
So this maps a baseboard type to an FMA hc scheme canonical
name. Note that several of the strings are no in the hc
names list (other, smmodule, iconnboard, etc). While they're
not used in the code today, it may be better not to map
these to bogus names. "BB_UNKOWN" --> "BB_UNKNOWN".

Even they are not used we still need to map them because the
based board type numbers in the smbios structure are numbered in
that order

Post by Scott Davenport
Also, in x86pi_bboard, SMB_BBT_BLADE is equated to "systemboard",
but here "blade". Disconnect?

I will check the x86pi_bboard and make sure they are the same.

Post by Scott Davenport
108 static smbs_cnt_t *
109 smb_create_strcnt(int count)
110 {
111 smbs_cnt_t *types = NULL;
112 int i, j;
113
114 types = kmem_zalloc(sizeof (smbs_cnt_t), KM_SLEEP);
115 if (types == NULL) {
116 cmn_err(CE_WARN, "Can't allocate smb strcnt memory");
117 return (NULL);
118 }
You're using the KM_SLEEP flag here, so the memory alloc
is guaranteed to succeed (see kmem_alloc(9F)). The
checks from 115-118 are unnecessary. Ripple this through the
rest of the smb_create_strcnt() function.

Will do

Post by Scott Davenport
931 if (find_matching_proc(shp, strand_apicid,
932 bb_smbid, proc_hdl, is_proc)) {
933 fmri = fm_nvlist_create(NULL);
934 if (fmri == NULL) {
935 cmn_err(CE_WARN, "Can't allocate fmri mem");
936 goto bad;
937 }
938 /*
939 * find parent by walking the cont_by_id
940 */
941 rc = smb_get_bb_fmri(shp, fmri, bb_smbid, bbstypes);
942 smb_free_strcnt(bbstypes, bb_strcnt);
943 if (rc == 0) {
944 return (fmri);
945 } else
946 goto bad;
947 }
948
949 }
950
951 cmn_err(CE_NOTE, "Can't get bboards");
952 smb_free_strcnt(bbstypes, bb_strcnt);
If the check at 934 is true, do you need to free bbstypes
before line 936?

Will do

Thanks
Trang

Tom Pothier

2009-09-24 20:46:59 UTC

Hello,

The updated webrev, with I believe includes all code review comments to
date, is now available:

http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb-124/

If you do check the webrev and explicitly approve it, please let me know
so I can "truthfully" place a "yes" next to your name on the C-Team
checklist. :-)

Also, mentioned before, we'd like to wrap up the code review cycle by
COB Oct 5, 2009.

thx,
-t

Post by Tom Pothier
http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb/
thx,
-t

Post by Tom Pothier
Hi,
I'd like to invite folks to participate in the code review for the
x86 Generic FMA Topology Enumerator project. I'd like to have
comments back no later than COB on October 5, 2009.
https://ssg-east.east.sun.com/~pothier/webrevs/onnv-x86gentopo-pb/
I've also placed the code review 'onnv-x86gentopo-pb.pdf' file on the
OpenSolaris project page under the "Documents" heading.
changeset: 10254:27683d5d640a
tag: tip
date: Tue Sep 01 15:15:19 2009 -0400
PSARC/2009/XXX x86 Generic FMA Topology Enumerator
6841286 Need x86 generic FMA topo enumerator
6853537 x86gentopo needs OEM-Specific SMBIOS structures
6785310 Implement SMBIOS contained elements/handles
6865771 Topology relationships should be derived from
contained handles & elements of SMBIOS
6865814 Chip enumerator should derive serials & labels using
libsmbios, if SMBIOS is FM aware
6865845 /dev/fm should export the Initial APICID, SMBIOS based
ID/instance to the chip enumerator
6866456 Generic Topology FMRI ereport
usr/src/cmd/fm/eversholt/files/i386/i86pc/intel.esc
usr/src/cmd/mdb/intel/modules/generic_cpu/gcpu.c
usr/src/cmd/smbios/smbios.c
usr/src/common/smbios/smb_info.c
usr/src/common/smbios/smb_open.c
usr/src/lib/fm/topo/libtopo/common/mapfile-vers
usr/src/lib/fm/topo/libtopo/common/topo_mod.map
usr/src/lib/fm/topo/maps/i86pc/Makefile
usr/src/lib/fm/topo/maps/i86pc/i86pc-hc-topology.xml
usr/src/lib/fm/topo/modules/common/pcibus/did_props.c
usr/src/lib/fm/topo/modules/i86pc/Makefile
usr/src/lib/fm/topo/modules/i86pc/chip/Makefile
usr/src/lib/fm/topo/modules/i86pc/chip/chip.c
usr/src/lib/fm/topo/modules/i86pc/chip/chip.h
usr/src/lib/fm/topo/modules/i86pc/chip/chip_amd.c
usr/src/lib/fm/topo/modules/i86pc/chip/chip_intel.c
usr/src/lib/libsmbios/common/mapfile-vers
usr/src/pkgdefs/SUNWfmd/prototype_i386
usr/src/uts/common/io/devfm.c
usr/src/uts/common/os/fm.c
usr/src/uts/common/sys/devfm.h
usr/src/uts/common/sys/fm/protocol.h
usr/src/uts/common/sys/smbios.h
usr/src/uts/common/sys/smbios_impl.h
usr/src/uts/i86pc/cpu/amd_opteron/ao_mca.c
usr/src/uts/i86pc/cpu/authenticamd/authamd_main.c
usr/src/uts/i86pc/cpu/generic_cpu/gcpu_mca.c
usr/src/uts/i86pc/cpu/genuineintel/gintel_main.c
usr/src/uts/i86pc/os/cmi.c
usr/src/uts/i86pc/os/cmi_hw.c
usr/src/uts/i86pc/os/startup.c
usr/src/uts/i86xpv/os/xen_machdep.c
usr/src/uts/intel/Makefile.files
usr/src/uts/intel/io/devfm_machdep.c
usr/src/uts/intel/io/mc-amd/mcamd.h
usr/src/uts/intel/io/mc-amd/mcamd_drv.c
usr/src/uts/intel/io/mc-amd/mcamd_subr.c
usr/src/uts/intel/sys/cpu_module.h
usr/src/uts/intel/sys/hypervisor.h
usr/src/lib/fm/topo/maps/i86pc/i86pc-legacy-hc-topology.xml
usr/src/lib/fm/topo/modules/i86pc/chip/chip_smbios.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/Makefile
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_bboard.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_chassis.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_generic.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_hostbridge.c
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_impl.h
usr/src/lib/fm/topo/modules/i86pc/x86pi/x86pi_subr.c
usr/src/uts/intel/os/fmsmb.c
usr/src/uts/intel/sys/fm/smb/fmsmb.h
thx,
-t

_______________________________________________
fm-discuss mailing list

Mike Shapiro

2009-09-25 06:04:13 UTC

Post by Tom Pothier
Hello,
The updated webrev, with I believe includes all code review comments
http://cr.opensolaris.org/~pothier/onnv-x86gentopo-pb-124/
If you do check the webrev and explicitly approve it, please let me
know so I can "truthfully" place a "yes" next to your name on the C-
Team checklist. :-)
Also, mentioned before, we'd like to wrap up the code review cycle
by COB Oct 5, 2009.
thx,
-t

The SMBIOS stuff looks good to me now. I'll leave it to the
others on this list to review the rest.

-Mike

---
Mike Shapiro, Sun Microsystems Open Storage / Fishworks. blogs.sun.com/
mws/

Tom Pothier

2009-10-02 16:09:35 UTC