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

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

Hi Laszlo,

See below my feedback.

> 
> 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."
> 

This is about the edk2 open source working process, I agree Bugzilla is one of good approach for new feature info. I though Bugzilla REF is optional item in the patch if we can document the details in the Commit message.

If Bugzilla for each patch series is the mandatory item, let me know, I apologize for that missing.

> *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.
> 

I don't agree it's the "smoke-screen" or " speculative generality" with "...another vendor might have a different method...". below is my *original words*, it's not only that one in short words:

"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. Different vender might has different implementation.  Another benefit with this series will make the smbase relocation more independent & more simple compared with existing process in smm cpu driver. 
As you can see, this patch doesn't break existing code functionality& work flow, which means it's the compatible changes, which will bring the new approach for the pre-allocated smbase solution."

For pre-allocated smbase solution ---> Different vender might has different implementation --> It's flexible for the producer to implement the pre-allocated smbase and make sure it has taken effect in the system. PEI module (to do the pre-allocated smbase) code is not in this series.

I agree Laszlo show me the more precise words than me in above (appreciate that), it's more easier for anyone in community to understand why I don't want to open source the hob producer. I'm willing adopt the Laszlo's comments to continue refine the patch series. But my attitude to the open source --  it's not the words like "smoke-screen" or "speculative generality".  You can say I'm unprofessional to handle such case, I agree I'm not aware so much design detail need exposed. But it's not intended to do that.


I apologize for the first version patch not clearly document the hob usage. But from the patch series v2 (https://edk2.groups.io/g/devel/message/98484), most of info are included for the hob, no matter in the commit message or in the hob .h file (https://edk2.groups.io/g/devel/message/98485) and I even explained the benefit of pismmcpu driver about the smm init (https://edk2.groups.io/g/devel/message/98486). I agree the explaining might be not enough for all of us understanding the design, but I flatter myself I have a positive attitude towards any opinion for anyone. With all your comments, I can make it more better. Thanks all comments from Laszlo & Gerd & Ray.  


> 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.)
> 

This is really good comments impress me.


> >>> Do you intent submitting code for OVMF producing such a HOB?
> >>> There isn't any in this series.
> >>
> >> No, we won't do that.
> >
> > Then there is no point in changing the OVMF code,
> > other than maybe adding an ASSERT that there is no such HOB.
> 
> I agree.
> 
> My initial request was for *considering* OVMF's library instance.
> "Considering" means evaluating, and modifying *if* necessary, and *as*
> necessary. I could not perform this evaluation myself: initially, the
> purpose of the new interface was obscure, so I couldn't tell if OVMF was
> affected or not.
> 
> Now that we *know* that OVMF is unaffected, we should just use library
> instances for what library instances exist for: customization (platform
> or otherwise). Because OVMF will not contain a PEI module initializing
> the SMBASE regs as described, there is no need for adding (dead) code to
> OVMF's lib instance. Adding the ASSERT() *definitely* makes sense
> however: it's a statement that we have *considered* the applicability of
> the feature. That is *precisely* what I missed from the initial
> announcement.
> 
> Look, when you see a summary diffstat for a patch set that modifies N
> instances of a library class, but not the one in OVMF -- or in any one
> particular subsystem --, you ask yourself: "is this an oversight, or is
> this intentional? did they just miss or ignore that platform or
> subsystem, or did they evaluate it as unaffected"?
> 
> *That* is exactly what should have been in the original BZ or cover
> letter. "I've grepped the edk2 and edk2-platforms tree for all instances
> of this library class; I need to modify instances X, Y, Z; I *know* I
> *need not* modify P, Q; and I'm *unsure* about U, V and W. Please advise".
> 
> The ASSERT() will give us a good opportunity to write a comment and/or
> commit message around this.
> 

ASSERT is fine to me by replacing the "if condition" to handle that. we can discuss the patch for correct changes in OVMF. 



> This whole ordeal is just another piece of evidence for why I cannot
> remain a member of this community. Hiding information, as a *basic modus
> operandi*, is incompatible with open source development.
> 
> It's *fine* if a project or a contributor adopts a "my way or the high
> way" attitude. Throwing open source code over the wall is still open
> source. It's not open development any more, but still open source -- it
> is still valuable (though less so). There may be many *valid* reasons
> for doing open source, but not open development. What pains me is the
> dishonest or at least mixed / sloppy messaging about *what edk2 is*. Is
> it open source, or is it open development?
> 
> https://github.com/tianocore/tianocore.github.io/wiki/TianoCore-Who-we-are
> 

Thanks,
Jiaxin