public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Kinney, Michael D" <michael.d.kinney@intel.com>
To: Laszlo Ersek <lersek@redhat.com>,
	"Zeng, Star" <star.zeng@intel.com>,
	"Kinney, Michael D" <michael.d.kinney@intel.com>
Cc: "edk2-devel@ml01.01.org" <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: [PATCH 0/6] PiDxeS3BootScriptLib: Support multiple PCI segment
Date: Thu, 1 Sep 2016 15:57:44 +0000	[thread overview]
Message-ID: <E92EE9817A31E24EB0585FDF735412F564805B1B@ORSMSX113.amr.corp.intel.com> (raw)
In-Reply-To: <7ec1654a-55c8-26bc-5340-85d10907b0e1@redhat.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


      reply	other threads:[~2016-09-01 15:57 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-08-19  7:35 [PATCH 0/6] PiDxeS3BootScriptLib: Support multiple PCI segment Star Zeng
2016-08-19  7:35 ` [PATCH 1/6] MdeModulePkg PiDxeS3BootScriptLib: Remove the trailing white spaces Star Zeng
2016-08-19  7:35 ` [PATCH 2/6] MdeModulePkg PiDxeS3BootScriptLib: Support multiple PCI segment Star Zeng
2016-08-23  1:58   ` Kinney, Michael D
2016-08-23  2:09     ` Zeng, Star
2016-08-23  3:44       ` Kinney, Michael D
2016-08-23  8:57         ` Zeng, Star
2016-08-19  7:35 ` [PATCH 3/6] Vlv2TbltDevicePkg: Declare PciSegmentLib in platform dsc Star Zeng
2016-08-22  6:54   ` Wei, David
2016-08-19  7:35 ` [PATCH 4/6] QuarkPlatformPkg: " Star Zeng
2016-08-19  7:35 ` [PATCH 5/6] QuarkSocPkg/QuarkSocPkg.dsc: Declare PciSegmentLib Star Zeng
2016-08-19  7:35 ` [PATCH 6/6] SecurityPkg/SecurityPkg.dsc: " Star Zeng
2016-08-19 13:35 ` [PATCH 0/6] PiDxeS3BootScriptLib: Support multiple PCI segment Yao, Jiewen
2016-08-24  2:26 ` Laszlo Ersek
2016-09-01 15:57   ` Kinney, Michael D [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=E92EE9817A31E24EB0585FDF735412F564805B1B@ORSMSX113.amr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox