attributeOverride / bios section questions.

[williamspatrick]

1. How does the attribute code handle if an attribute changes size between two firmware levels? If we had an attribute that was 1 byte and was a BIOS setting, what happens if it needs to change to 2 bytes? Will the ATTR_PERM parsing code automatically handle this?
2. How does the attribute code handle if an attribute has been deleted? If we have an attribute as a BIOS setting in one firmware level and then we deleted that attribute entirely, what is the behavior? Does the attribute code crash or create an error log or silently ignore the extra attribute?

[scprek]

1. Seems like the attribute override tool needs to inject that attributes override again with the updated attribute info, so it knows it's now 2 vs 1 bytes. The parsing code will adjust automatically, because it puts 100% of its trust into the ATTR_TMP/PERM sections.There's no "update or check" attribute changes in the parsing code, so if it's not re-written the parsing code would not get the desired result.

2..When talking about BIOS settings we are just talking about ATTR_PERM correct? If that's the case then an errlog is created for missing attributes. If we are also talking about ATTR_TMP, I need some help figuring that out. The attributeoverride code itself does not fail if an attribute doesn't exists for ATTR_TMP, but is an errl created somewhere down the path? Future details below.

ATTR_PERM attributeTank.C @writePermAttributes

• This one's easy to follow, it creates an errorlog if _trySetAttr fails

errlHndl_t AttributeTank::writePermAttributes() if (l_found) { TRACFCOMP(g_trac_targeting, "writePermAttributes: Value " "NOT applied to target, override failed for Attr " "ID:0x%X Value:0x%llX on target 0x%X - current value " "0x%llX", l_attrHdr.iv_attrId, _reinterpret_cast<uint64_t *>(l_attr->iv_pVal), (_l_permTargetList)->getAttr<ATTR_HUID>(), reinterpret_cast<uint64_t *>(io_pAttrData) ); /@ * @errortype * @moduleid TARG_WRITE_PERM_ATTR * @reasoncode TARG_RC_WRITE_PERM_ATTR_FAIL * @userdata1 Target specified * @userdata2 Attribute specified * @devdesc Failure applying given attribute override * on given target _/ UTIL::createTracingError( TARG_WRITE_PERM_ATTR, TARG_RC_WRITE_PERM_ATTR_FAIL, (_l_permTargetList)->getAttr<ATTR_HUID>(), l_attrHdr.iv_attrId, 0,0, l_err); break; } else { TRACFCOMP(g_trac_targeting, "writePermAttributes: Target " "does not have attribute, override NOT applied for " "Attr ID:0x%X on target 0x%X", l_attrHdr.iv_attrId, (l_permTargetList)->getAttr<ATTR_HUID>() ); /@ * @errortype * @moduleid TARG_WRITE_PERM_ATTR * @reasoncode TARG_RC_WRITE_PERM_ATTR_TARGET_FAIL * @userdata1 Target specified * @userdata2 Attribute specified * @devdesc Given target does not have given attribute * to apply override _/ UTIL::createTracingError( TARG_WRITE_PERM_ATTR, TARG_RC_WRITE_PERM_ATTR_TARGET_FAIL, (_l_permTargetList)->getAttr<ATTR_HUID>(), l_attrHdr.iv_attrId, 0,0, l_err); break; }

ATTR_TMP (This may be out of the scope of the question)

• Currently there is no check for attribute existence, the attribute override (if well formatted in pnor) is added to the corresponding attributeTank (FAPI or Targeting)
• looks like in the FAPI case it calls AttributeTank::getAttribute() and gets a bool, not an errl then with targeting it calls _tryGetAttr which also returns a bool
• FAPI works through macros it returns a FAPI_RC, I'm not sure how that's handled #define ATTR_VPD_POWER_CONTROL_CAPABLE_GETMACRO(ID, PTARGET, VAL)
  fapi::AttrOverrideSync::getAttrOverrideFunc(fapi::ID, PTARGET, &VAL) ?
  fapi::FAPI_RC_SUCCESS : fapi::platAttrSvc::fapiPlatGetControlCapable(PTARGET,VAL)
• For targeting how are _tryGetAttr() fails handled?
• It seems that somewhere in the code it's possible that an errl is created if a non-existent attribute is accessed, but if they aren't then silently ignored?

MAINLINE PATH CALL in istepdispatcher.C // Get Attribute overrides from PNOR else { PNOR::SectionInfo_t l_sectionInfo; // Get temporary attribute overrides from pnor err = PNOR::getSectionInfo(PNOR::ATTR_TMP, l_sectionInfo); // Attr override sections are optional so just delete error if (err) { delete err; err = NULL; } else { TRACFCOMP(g_trac_initsvc,"init: processing temporary overrides"); err = TARGETING::getAttrOverrides(l_sectionInfo); if (err) { TRACFCOMP(g_trac_initsvc,"Failed getAttrOverrides: getting temporary overrides"); break; } } // Get permanent attribute overrides from pnor err = PNOR::getSectionInfo(PNOR::ATTR_PERM, l_sectionInfo); // Attr override sections are optional so just delete error if (err) { delete err; err = NULL; } else { TRACFCOMP(g_trac_initsvc,"init: processing permanent overrides"); err = TARGETING::getAttrOverrides(l_sectionInfo); if (err) { TRACFCOMP(g_trac_initsvc,"Failed getAttrOverrides: getting permanent overrides"); break; } } }

[williamspatrick]

1. We can't "trust" the ATTR_PERM section because it could be coming from an older version of hostboot code. We need to come up with a sane mechanism to either erase a poorly sized attribute, ignore it, or handle it.
2. The ATTR_PERM is the only one I'm concerned with. The ATTR_TMP is only meant to be used in MFG and so the images should be well controlled. Again, we could be having to use an ATTR_PERM section that was created with older firmware so we need to come up with something better than throwing an error or crashing.

[scprek]

I feel we shouldn't just erase it, because people may be confused what happened to their override. Rather if we just ignore it, should we have some console trace that says bad override or something?

If we try to handle it, wouldn't we have to do the checks that the attrOverride tool does now to ensure things are correct and then actually write the correct override to pnor?

[dcrowell77]

I like the idea of a visible console trace as well.

[dcrowell77]

We had a discussion among the team today and one thing we're going to do is investigate what it would take to move the attribute processing into Hostboot itself instead of having an external tool. This would go a long ways toward future-proofing overrides since simple data type expansions (uint8->uint16) would keep working, and even things like enumeration remapping would work. The major argument against this approach is that it adds a pile of string data to our image so we'll need to weigh whether or not we can handle the load.