[Suggestion/Bug] Extend the usage of 'shared' modules

[Suggestion/Bug] Extend the usage of 'shared' modules

[Marvin H?user]

Dear developers,

Dear KabylakeSiliconPkg devs: If you are not interested in the suggestion of generic, shared modules, please read my comment on 2) nevertheless, as it contains a potential bug report. Sorry, but I could not verify this yet.

I have been exploring the code of most of the Intel 'open platforms' lately and noticed they shared a bunch of slightly different modules, which are diverse to fit the platform's requirements.
One of them is PchSmmDispatcher/QNCSmmDispatcher. I'm not sure about non-Intel platforms, though in my opinion, there should be a generic module in MdeModulePkg, if it can be shared across brands, or one in In IntelSiliconPkg, if it's useful for Intel platforms only. To see why I think a generic, shared module that consumes a platform library would be hugely beneficial, please take a look at these lines:


  1.  https://github.com/tianocore/edk2-platforms/blob/devel-MinPlatform/Silicon/Intel/KabylakeSiliconPkg/Pch/PchSmiDispatcher/Smm/PchSmmCore.c#L361
  2.  https://github.com/tianocore/edk2-platforms/blob/devel-MinPlatform/Silicon/Intel/KabylakeSiliconPkg/Pch/PchSmiDispatcher/Smm/PchSmmCore.c#L890
  3.  https://github.com/tianocore/edk2/blob/master/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmmCore.c#L753

1) shows a hook to SmmReadyToLock, which I first and only saw in KabylakeSiliconPkg. This addition has not been brought to the Quark platform, for example. With a shared module, all platforms that embed it would have profited.

2) on the other hand shows KabylakeSiliconPkg's PchSmmDispatcher accessing RecordInDb after the callback function has been executed, which QuarkSocPkg's QNCSmmDispatcher explicitely forbids in 3), as it might have been freed by the call. I am not sure whether it was a bug in the Quark firmware that has been fixed by the time the Kabylake code was written, though I don't believe so, because the changes to the loop to process multiple SMI sources in one go is not present in the Kabylake code either. If there was a shared module, the fix to Quark would have profited all other platforms.

It's obvious to me that KabylakeSiliconPkg has a bunch of things that were previously only distributed privately as part of the private Reference Code. I think that keeping the RC as small as possible (the very best case would be only platform library classes consumed by generic, open source modules) is a goal worth persuing, as more code can be shared and community-verified as part of EDK2.

Thanks for your time!

Regards,
Marvin.