Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

["Ni, Ray" <ray.ni@intel.com>]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, February 2, 2023 7:47 PM
> To: Gerd Hoffmann <kraxel@redhat.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Dong, Eric
> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>
> Subject: Re: [PATCH v3 5/5] OvmfPkg/SmmCpuFeaturesLib: Skip SMBASE
> configuration
> 
> I'm going to comment on this one email up-stream, because it showcases
> the community problem, as far as I'm concerned, and because Jiaxin made
> a reference to my initial request.
> 
> On 2/2/23 10:00, Gerd Hoffmann wrote:
> >   Hi,
> >
> >>> But the serialized SMBASE programming still happens, now in the PEI
> >>> module, and I don't see a good reason why the runtime the new PEI module
> >>> and the runtime of PiSmmCpuDxeSmm combined is faster than before.
> >>
> >> As I said, PEI module can also programs SMBASE in parallel, for
> >> example program the some register directly instead of depending the
> >> existing RSM instruction to reload the SMBASE register with the new
> >> allocated SMBASE each time when it exits SMM.
> >
> > Ok.  So new Intel processors apparently got new MSR(s) to set SMBASE
> > directly.  Any specific reason why you don't add support for that to
> > PiSmmCpuDxeSmm?  That would avoid needing the new HOB (and the related
> > problems with the 8190 cpu limit) in the first place.

The reason is the hardware interface to set SMBASE is in thread scope meaning
such firmware-hardware communication needs to be done for each CPU thread.

It's doable to program the hardware interface using DXE MP service protocol in
CpuSmm driver's entry point.
But, considering the standalone MM environment where the CpuMm driver runs
in a isolated environment and it cannot invoke any DXE or PEI MP service, you could
understand that why we choose to put the hardware interface programming in a separate
PEI module. This is the major reason.
I admit that a minor benefit of this design is we can isolate the private hardware interface
programming in a close-source module. Otherwise, the SmmCpuFeaturesLib might need to
expose a new API for the hardware interface programming.


> 
> See this is *exactly* my problem. The *whole work* on this should have
> started like this, with a new Feature Request Bugzilla:
> 
> "Intel are introducing a new processor register (MSR or other method)
> with their XY product line where firmware can program the SMBASE
> independently of the RSM instruction. The PEI code performing this logic
> will not be open sourced, similarly to other things that are kept
> binary-only in the FSP (firmware support packages), and perhaps
> similarly to how memory chips are initialized in the PEI phase too, by
> "MRC" (memory reference code). Because there is no intent to open source
> the initialization logic, possibly due to the new MSR not even being
> slated for documentation in the Intel SDM, we need a new *binary-only*
> interface."

Fair point. We will submit a Bugzilla for this.

> 
> *That* would have been honest, straight talk. Not this smoke-screen with
> "another vendor might have a different method". That's entirely
> speculative generality. Speculative generality has been an anti-pattern
> in software development for decades, even merely for technical means,
> but here the justification for it is not even technical: the language
> around the generality is just to hide the one actual purpose of the
> feature. Don't do that please. Describe your *specific* use case, list
> your arguments, and then explain your approach for making it
> regression-free for the existent cases.

Agree. Be specific and honest. Tell what we can tell and explain what we
cannot tell.

> 
> PI and UEFI are all about binary interfaces between proprietary vendors.
> As much as I disagree with the entire concept [*], I accept it as a fact
> of life. I just ask that, whenever that pattern (= introducing ABIs,
> rather than APIs) is exercised, at least the *actual goal* be documented.
> 
> ([*] I disagree with the concept for two reasons. One, ideological (no
> further explanation needed). Two, practical. If you "standardize" an
> interface when you have *one* application of it, that's always trouble.
> The second application, if there's ever going to be a second one, will
> almost surely not fit within the framework of the "standard". So it's
> best to either open source all the implementations, or at least openly
> document their *actual* interface needs. Clearly state that the initial
> interface implementation is "work in progress". If there are multiple
> (divergent) applications, *then* try to extract something common, either
> from the open source code bases, or the clearly documented data
> dependencies, and then codify that. UEFI is *hugely* harmed by the
> proliferation of protocols, where every new feature needs to be
> standardized, as soon as one implementation exists. These protocols then
> get ossified and linger around for absolutely forever. I feel that it's
> totally self-inflicted damage; it is the consequence of the proprietary
> software development model -- effectively the unwillingness to share.)

When we designed the flow for the new feature, we examined the design by
thinking about if current SMBASE relocation done in PiSmmCpu driver can fit
in the new design. Our conclusion is yes though we don’t have a plan to do it
meaning we don't want to separate today's SMBASE relocation logic in a separate
driver then produce a HOB.
I hate introducing new interfaces as well. A better design should not only cover
today's use case but also future's use case (5-10 years maybe, no guarantee of forever).
Sometimes the designer might invent some interfaces not flexible enough for satisfying
the future different instances of the similar use cases, that's a bad design in my opinion
but we cannot avoid that. I think that triggered the code-first process which requires
implementation first and standardization second. 
Though this new HOB is not in PI spec, you remind me that we might need to add more fields
to the HOB so a way to distinguish between different versions of the HOB should be considered.
The way could be to introduce a new GUID for new version of HOB, or add a field (version?) in the HOB.
I prefer the second.