Laszlo, Thanks for the all git hints and advice.
I have split out the DSC update to the UefiCpuPkg, so it comes after the PiSmmCpuiDxeSmm module is buildable. Thanks, Mike >-----Original Message----- >From: Laszlo Ersek [mailto:ler...@redhat.com] >Sent: Monday, October 19, 2015 7:51 AM >To: Kinney, Michael D; edk2-de...@ml01.01.org >Cc: Paolo Bonzini >Subject: Re: [PATCH v4 17/19] UefiCpuPkg: Add PiSmmCpuDxeSmm module no >IA32/X64 files > >On 10/19/15 16:20, Laszlo Ersek wrote: >> On 10/19/15 09:44, Michael Kinney wrote: >>> Add module that initializes a CPU for the SMM environment and >>> installs the first level SMI handler. This module along with the >>> SMM IPL and SMM Core provide the services required for >>> DXE_SMM_DRIVERS to register hardware and software SMI handlers. >>> >>> CPU specific features are abstracted through the SmmCpuFeaturesLib >>> >>> Platform specific features are abstracted through the >>> SmmCpuPlatformHookLib >>> >>> Several PCDs are added to enable/disable features and configure >>> settings for the PiSmmCpuDxeSmm module >>> >>> Changes between [PATCH v1] and [PATCH v2]: >>> 1) Swap PTE init order for QEMU compatibility. >>> Current PTE initialization algorithm works on HW but breaks QEMU >>> emulator. Update the PTE initialization order to be compatible >>> with both. >>> 2) Update comment block that describes 32KB SMBASE alignment >requirement >>> to match contents of Intel(R) 64 and IA-32 Architectures Software >>> Developer's Manual >>> 3) Remove BUGBUG comment and call to ClearSmi() that is not required. >>> SMI should be cleared by root SMI handler. >>> >>> [jeff....@intel.com: Fix code style issues reported by ECC] >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Michael Kinney <michael.d.kin...@intel.com> >>> Cc: Laszlo Ersek <ler...@redhat.com> >>> Cc: Paolo Bonzini <pbonz...@redhat.com> >>> >>> [pbonz...@redhat.com: InitPaging: prepare PT before filling in PDE] >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com> >>> --- >> >> Acked-by: Laszlo Ersek <ler...@redhat.com> > >Hm, actually, I have a few comments for this patch; sorry that I haven't >noticed these things earlier. > >( >First, I can see that the patch removes some whitespace that was >introduced in this same patch series earlier. I think it would be nicer >not to add that extra whitespace in the first place, but I don't want to >obsess about such banalities. > >... FWIW, if you find some whitespace at the end of the series that >you'd like to remove, you can use "git blame" on the source file to >locate the commit that introduced that line, then use "git rebase -i" >with the "edit" action to remove the whitespace right from the patch >that adds it. >) > >Second -- this is again a "general git hint" -- please try to ensure >that the tree builds at every stage of the patch series. Someone could >be bisecting the tree later on, in search for the commit that introduced >an unrelated bug, and that bisection can take him or her to semi-random >patches in the history. The tree should build at each individual commit. > >I think this patch probably breaks the build: >- it adds the "UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf" file with > references to Ia32 and X64 source files >- those files themselves are delayed to the next two patches, >- the PiSmmCpuDxeSmm.inf file is referenced in > "UefiCpuPkg/UefiCpuPkg.dsc". > >It certainly makes sense to construct a complex driver in several >stages, many of which might not compile in separation. (I do that all >the time.) However, the driver's INF file should be added to the DSC >(and FDF, if any) file(s) only when the driver *does* build. > >A person bisecting the history for an unrelated bug won't care that >PiSmmCpuDxeSmm is not "available" midway in this series, but he or she >will definitely care that "UefiCpuPkg/UefiCpuPkg.dsc" *build* midway in >this series, so he or she can test it for the unrelated bug. > >I don't want to obsess about this (we're at v4 and this driver is huge >and also very important, plus I'm *getting* a favor here, not *doing* >one -- I shouldn't be a pain to work with), but in general this is >something to keep in mind when working with git. Once the subject >feature is complete, several more local rebases are usually needed to >rework the series such that each patch is logically minimal and >indivisible (to help reviewers focus) and that the tree builds at each >single stage. > >Unfortunately, for validating the latter (= builds at each patch), >there's really no better tool than writing a script that checks out the >tree at each patch and builds it. (For bonus points, the script can >pause after each build and regression-test the build automatically, or >ask the developer to do that. But I agree that at v4 that's not really >practical.) > >Third, to obsess a bit more about logical minimality and focus, patches >that remove whitespace (introduced *before* the subject series) should >be kept separate from any other patches. This way the commit message can >just say "remove whitespace", and the patch is trivial to verify for >reviewers. Reviewing whitespace removal and functional changes in the >same patch causes double vision. :) > >Summary: please consider the above practices in the future; for now, the >only thing I think should be fixed is the build breakage. The easiest >way to do it should be splitting out the INF file's addition to the DSC >file, and moving it to the end of the series as a standalone patch. But >I don't insist. :) > >Thanks! >Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
