OS-6731 expose microcode level as property on strand topo nodes

[joyent-automation]

OS-6731 expose microcode level as property on strand topo nodes

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/6046/.
The raw archive of this CR is here.
See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@rmustacc commented at 2019-04-04T20:20:56

Patch Set 2:

(2 comments)

Patch Set 2 code comments

usr/src/uts/i86pc/os/cmi_hw.c#1651 @rmustacc

I'm not sure using the CMI_HDL_OPFUNC is safe on xpv, as that will basically cause a NULL dereference as the macro doesn't check to see if the member is present or not.

usr/src/uts/i86pc/os/cmi_hw.c#1651 @rejohnst

Not sure if I'm doing the right thing here, but I changed it to manually define the function and placed it inside a preprocessor guard so it doesn't get defined for xpv.

I did test it and it seems to work.

usr/src/uts/i86pc/os/cmi_hw.c#1651 @rmustacc

I went through and double checked the cpuid/ucode code and it actually should be valid on XPV. So I think we should just define the same op entry point on both XPV and not and we can use a handle function. That way we don't need to ifdef it.

usr/src/uts/i86pc/os/cmi_hw.c#1651 @rejohnst

I looked at this and the problem is that in the XPV context, we don't appear to have access to the cpu_t, so - assuming I'm reading the code correctly, the only way to support the function for XPV would be to modify the xen-mca code to plumb the ucode rev up through the xen_mc_logical_cpu_t struct. I don't think it's worth the effort and I'm not certain I could even test it that easily.

usr/src/uts/i86pc/os/cpuid.c#5019 @rmustacc

Is a pass 1 assert correct here? I thought these were basically orthogonal concepts.

usr/src/uts/i86pc/os/cpuid.c#5019 @rejohnst

I think I just copied this from one of the other functions. Removed.

@rejohnst commented at 2019-04-19T17:18:43

Patch Set 3:

(2 comments)

@rmustacc commented at 2019-04-22T14:25:23

Patch Set 2:

(1 comment)

@rmustacc commented at 2019-04-22T14:31:15

Patch Set 3:

(1 comment)

Patch Set 3 code comments

usr/src/uts/intel/io/devfm_machdep.c#208 @rmustacc

Is this cast necessary? Doesn't this return a uint32_t?

usr/src/uts/intel/io/devfm_machdep.c#208 @rejohnst

Done

@rejohnst commented at 2019-06-24T21:11:39

Patch Set 4:

(2 comments)

I also reworked the changes to the chip topo module to leverage the UFM stuff.

@rmustacc commented at 2019-06-24T21:44:27

Patch Set 4:

(5 comments)

So I think the xpv handling of the microcode rev is still problematic. What I'd suggest doing is instead change the signature of the cmi_get_ucode_rev to something like:

cmi_errno_t cmi_get_ucode_rev(cmi_hdl_t, uint32_t *);

And have cmi_get_ucode_rev return CMIERR_NOTSUP or similar if there is no implementation function like is the case today for xpv. This would get you out of the ifdef games and would make the cmio_ucode_rev be useful.

Patch Set 4 code comments

usr/src/lib/fm/topo/modules/i86pc/chip/chip.c#337 @rmustacc

I suspect we may have already hashed it out, but at least for Intel, the microcode exists on a per-core basis and is shared in the case of strands. Does it make sense to create the UFM on the strand and not the core still?

usr/src/lib/fm/topo/modules/i86pc/chip/chip.c#339 @rmustacc

This appears to never be freed after creating the UFM. I'd suggest initializing ucode_rev_str to NULL and unconditionally freeing it before the return at +354.

usr/src/uts/i86pc/os/cmi_hw.c#128 @rmustacc

Why do we have the cmio_ucode_rev and functions if nothing actually calls and uses it now?

usr/src/uts/intel/io/devfm_machdep.c#208 @rmustacc

I'm confused about the most recent version. Right now this function (cmi_hdl_ucode_rev) is only defined under xpv. But there are no ifdefs here at all. How does that work?

usr/src/uts/intel/sys/cpu_module.h#169 @rmustacc

Why isn't this under ifdef !xpv?