Re: [PATCH 0/6] PiDxeS3BootScriptLib: Support multiple PCI segment

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Laszlo,

You are correct.  No functional change.

I think I was more concerned about source changes to the functions that 
were already well tested, and wanted to focus the source code changes on 
the functions that were being enabled for multiple segments.

If we are all confident from the code review and testing that we have not
introduced any regression, then it is a good change.

Reviewed-by: Michael Kinney <michael.d.kinney@intel.com>

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Tuesday, August 23, 2016 7:26 PM
> To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: edk2-devel@ml01.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Chan Amy
> <chan.amy@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Wei, David
> <david.wei@intel.com>
> Subject: Re: [edk2] [PATCH 0/6] PiDxeS3BootScriptLib: Support multiple PCI segment
> 
> On 08/19/16 03:35, Star Zeng wrote:
> > Support multiple PCI segment for PCI_CONFIG2 opcodes.
> >
> > PiDxeS3BootScriptLib needs to be updated to consume PciSegmentLib
> > instead of PciLib. That means platforms need to add PciSegmentLib
> > declaration like below in platform dsc if the PciSegmentLib was
> > not declared in platform dsc before.
> >
> > PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
> >
> > For platforms only have one segment,
> > MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf is recommended
> > to be used and declared in platform dsc for PiDxeS3BootScriptLib to have
> > equivalent functionality with before.
> >
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Chan Amy <chan.amy@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Kelly Steele <kelly.steele@intel.com>
> > Cc: David Wei <david.wei@intel.com>
> > Cc: Chao Zhang <chao.b.zhang@intel.com>
> >
> > Star Zeng (6):
> >   MdeModulePkg PiDxeS3BootScriptLib: Remove the trailing white spaces
> >   MdeModulePkg PiDxeS3BootScriptLib: Support multiple PCI segment
> >   Vlv2TbltDevicePkg: Declare PciSegmentLib in platform dsc
> >   QuarkPlatformPkg: Declare PciSegmentLib in platform dsc
> >   QuarkSocPkg/QuarkSocPkg.dsc: Declare PciSegmentLib
> >   SecurityPkg/SecurityPkg.dsc: Declare PciSegmentLib
> >
> >  .../PiDxeS3BootScriptLib/BootScriptExecute.c       | 411 +++++++++----------
> >  .../BootScriptInternalFormat.h                     |   2 +-
> >  .../Library/PiDxeS3BootScriptLib/BootScriptSave.c  | 451 ++++++++++-----------
> >  .../PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf    |   4 +-
> >  .../PiDxeS3BootScriptLib/DxeS3BootScriptLib.uni    |   2 +-
> >  .../PiDxeS3BootScriptLib/InternalBootScriptLib.h   |  26 +-
> >  QuarkPlatformPkg/Quark.dsc                         |   1 +
> >  QuarkPlatformPkg/QuarkMin.dsc                      |   1 +
> >  QuarkSocPkg/QuarkSocPkg.dsc                        |   1 +
> >  SecurityPkg/SecurityPkg.dsc                        |   1 +
> >  Vlv2TbltDevicePkg/PlatformPkgGccX64.dsc            |   1 +
> >  Vlv2TbltDevicePkg/PlatformPkgIA32.dsc              |   1 +
> >  Vlv2TbltDevicePkg/PlatformPkgX64.dsc               |   1 +
> >  13 files changed, 450 insertions(+), 453 deletions(-)
> >
> 
> For patches #1 and #2:
> 
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> 
> (Also compared some logs.)
> 
> I read the sub-thread under #2, but I don't understand Mike's concern. I
> can be wrong of course, but in my understanding, the boot script's
> internal representation does not change. The "saver" side only relaxes
> the Segment=0 requirement. And, the "executor side" accommodates nonzero
> segments in the Pci2 opcodes, and rebases the Pci[1] opcode functions on
> top of Pci2 opcode functions (with hardcoded Segment=0).
> 
> I don't understand why calling PciSegmentLib functions with a UINT64
> parameter where the segment bit-field is hardcoded to 0 is worse than
> calling PciLib (uncapable of nonzero segments) with an UINTN parameter.
> 
> Is this about the cost of a function call on Ia32? That is, assuming a
> very long S3 boot script, the patch might noticeably slow down S3 resume
> on Ia32?
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel