On 2015-07-30 10:09:34, Laszlo Ersek wrote:
> (Sigh, I left off the list address. This should be discussed publicly.
> Resending.)
>
> Clearly, the SMBIOS patches I posted and got committed last time are not
> good enough. That's because the SMBIOS 3.0 entry point is structurally
> different from the prior versions (because why not). Therefore, now that
> Wei has QEMU generate SMBIOS 3.0 payload for the firmware, the checks we
> currently have in place, fail.
>
> (First and foremost, the *size* of the 3.0 entry point is different from
> that of the 2.8 one. We correctly catch this, but we incorrectly don't
> recognize 3.0.)
>
> I can write the code for telling apart 2.8 from 3.0; that's not the
> problem. The problem is that this code would have to be triplicated, as
> things currently stand:
>
> (1) "ArmVirtPkg/QemuFwCfgToPcdDxe/QemuFwCfgToPcd.c" sets
> PcdSmbiosVersion for "MdeModulePkg/Universal/SmbiosDxe" in the
> ArmVirtPkg build, early in the DXE phase,
>
> (2) "OvmfPkg/PlatformPei/Platform.c" sets the same PCD for the same
> generic driver in the OvmfPkg build, in the PEI phase,
>
> (3) "OvmfPkg/SmbiosPlatformDxe/Qemu.c" verifies the entry point again,
> and fetches the actual tables. (Note that
> (TablesSize != QemuAnchor.TableLength) is checked too, so there is a
> cross-dependency between the two blobs, the entry point and the
> actual tables.)
>
> Now, the current triplication we have is simplistic, so it shouldn't be
> a large problem. But, if I need to add smarts for detecting v2 versus
> v3, I would hate to triplicate that logic too. This is what code sites
> (1) and (2) both should do, for determining PcdSmbiosVersion:
>
> - Find "etc/smbios/smbios-anchor". If it is missing, bail.
> - Compare the size of that fw-cfg blob against SMBIOS_TABLE_ENTRY_POINT.
> - If it is a match, perform the current _SM_ and _DMI_ checks.
> - If they fail, bail.
> - If they match, check that QemuAnchor.MajorVersion is actually 2.
> - If that fails, bail.
> - If it matches, set PcdSmbiosVersion to 0x02__.
> - Otherwise, compare the size of the anchor blob against
> SMBIOS_TABLE_3_0_ENTRY_POINT.
> - If it is a match, perform the (new) _SM3_ check.
> - If it fails, bail.
> - Check that MajorVersion (which now lives at a different offset) is
> 3.
> - If that fails, bail.
> - If it matches, set PcdSmbiosVersion to 0x03__.
> - Otherwise, bail.
>
> In (3):
> - repeat all of the above, except do not *set* the PCD -- enforce it
> instead. ASSERT() that the PCD already has the right value (because
> at this point MdeModulePkg/Universal/SmbiosDxe is expected to have
> been dispatched, and consumed the PCD).
> - If the version is 2, then compare TableLength == tables blob size.
> If it fails, return NULL; otherwise return the tables.
> - If the version is 3, then compare TableMaximumSize >= tables blob
> size. If it fails, return NULL; otherwise return the tables.
> - If the version is something different, return NULL.
>
> As I said, this can be coded as-written, but I strongly dislike the code
> triplication, and I expect you guys will too. So what do you propose? I
> could extract a tiny BASE library (a single function) that:
> - depends on QemuFwCfgLib
> - takes a BOOLEAN parameter that tells it whether to set the PCD, or
> check and enforce the PCD
> - return the detected SMBIOS version (2, 3, or "missing/unknown")
>
> Then I could use this function in sites (1) and (2) for setting the PCD,
> and then use it in (3) for enforcing the PCD, and based on the returned
> SMBIOS version, perform the version-specific tables blob size check too.
Would it help if you used a NULL library with a constructor to set
gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion?
MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
<LibraryClasses>
NULL|OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf
}
I'm not sure about the enforce part, but maybe this could allow you to
also share some code between detect & enforce:
OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf {
<LibraryClasses>
NULL|OvmfPkg/Library/SmbiosVersionLib/ValidateSmbiosVersionLib.inf
}
But, I'm a little skeptical of the necessity of this second one...
-Jordan
> Small problem: you are going to hate me for introducing a
> single-function library *class*.
>
> So how do I share *one function* between different *phase* modules of
> different top-level packages? Should we introduce BaseCrapLib ^W
> BaseUtilityLib, and just keep throwing such functions in there?
>
> If so, what names (pathnames, basenames, library class and instance
> names etc) do you recommend?
>
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
