Hi Scott,
Thanks for the review, please see some responses inline..
(responded only to the parts that i implemented)
Scott Davenport wrote:
On Wed, 2009-09-02 at 12:25 -0400, Tom Pothier wrote:
The webrev is now also available on opensolaris.org:
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/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.
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 ?
If it does belong, line 285, copy/paste error? Debug message
should reference nvlist_lookup_string().
Yes, will fix it.
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.
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.
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...
Second,
if it does and 514 has failed, you're guaranteed a misleading
debug msg at 535.
True, will fix.
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.
==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_
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.
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.
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
==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
fm-discuss@opensolaris.org
