From: "Chiu, Chasel" <chasel.chiu@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
"Chiu, Chasel" <chasel.chiu@intel.com>,
"mikuback@linux.microsoft.com" <mikuback@linux.microsoft.com>,
"Oram, Isaac W" <isaac.w.oram@intel.com>
Cc: "S, Ashraf Ali" <ashraf.ali.s@intel.com>,
"Chaganty, Rangasai V" <rangasai.v.chaganty@intel.com>,
"Ni, Ray" <ray.ni@intel.com>,
"Kubacki, Michael" <michael.kubacki@microsoft.com>
Subject: Re: [edk2-devel] [edk2-platforms: PATCH] IntelSiliconPkg/SpiFvbServiceSmm: Support Other NVS variable region.
Date: Thu, 9 Feb 2023 18:28:32 +0000 [thread overview]
Message-ID: <BN9PR11MB548314C8A83E7B270BA04D34E6D99@BN9PR11MB5483.namprd11.prod.outlook.com> (raw)
In-Reply-To: <174237FCE67B704B.15261@groups.io>
Hello,
Another V4 patch sent for applying more comment/commit message feedbacks form Isaac.
Please help to review again.
Thanks,
Chasel
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chiu, Chasel
> Sent: Thursday, February 9, 2023 9:26 AM
> To: devel@edk2.groups.io; mikuback@linux.microsoft.com; Oram, Isaac W
> <isaac.w.oram@intel.com>
> Cc: S, Ashraf Ali <ashraf.ali.s@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>; Ni, Ray <ray.ni@intel.com>; Kubacki,
> Michael <michael.kubacki@microsoft.com>
> Subject: Re: [edk2-devel] [edk2-platforms: PATCH]
> IntelSiliconPkg/SpiFvbServiceSmm: Support Other NVS variable region.
>
>
> Thanks for good suggestions Isaac and Michael!
> I have sent V3 patch to apply all the suggestions, please help to review again.
>
> Thanks,
> Chasel
>
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
> > Kubacki
> > Sent: Thursday, February 9, 2023 8:47 AM
> > To: devel@edk2.groups.io; Oram, Isaac W <isaac.w.oram@intel.com>;
> > Chiu, Chasel <chasel.chiu@intel.com>
> > Cc: S, Ashraf Ali <ashraf.ali.s@intel.com>; Chaganty, Rangasai V
> > <rangasai.v.chaganty@intel.com>; Ni, Ray <ray.ni@intel.com>; Kubacki,
> > Michael <michael.kubacki@microsoft.com>
> > Subject: Re: [edk2-devel] [edk2-platforms: PATCH]
> > IntelSiliconPkg/SpiFvbServiceSmm: Support Other NVS variable region.
> >
> > Thanks, that's useful background. @chasel, you should probably put
> > this info in the commit message so it is captured in source history.
> >
> > Given the default value is zero, it seems reasonable. I was also
> > initially confused by the name of the PCD.
> >
> > Another idea would be something like "PcdFlashNvStorageAdditionalSize".
> >
> > Please do at least update the commit message to include additional context.
> >
> > Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>
> >
> > On 2/9/2023 11:04 AM, Isaac Oram wrote:
> > > It is a legacy that exists in current and past implementations.
> > > There is a
> > complex arbitrary relationship between the runtime updateable regions
> > in existing platform designs.
> > > There is something like:
> > > - Variable store (large)
> > > - Error log (small)
> > > - Fault tolerant working area (>= size of prior 2 regions)
> > > - Fault tolerant metadata (small).
> > > And there are assumptions about ordering and packing built into
> > > board flash
> > layouts.
> > >
> > > I don't think that we should introduce "other variable" as a
> > > concept, because
> > variable solutions don't support two regions, so it isn't a UEFI variable region.
> > OtherUpdatable might be ok, but still seems confusing to me.
> > > I think that we should add the support for the *ErrorLog* region so
> > > that the
> > open FvbServices can be used by current implementations. Then we
> > should eliminate the "ErrorLog" use completely. My thought is that
> > this makes the connection to legacy clear. And also motivates us to
> > eliminate all the vestigial references to the ErrorLog in edk2 and edk2-
> platforms.
> > >
> > > New updateable regions should not be hard-coded into this area and
> > > should
> > have a cleaner solution, as Michael suggests.
> > >
> > > I understand if we don't want to support legacy or workarounds, but
> > > I think
> > that currently adoption and use of the open content is higher
> > priority. Which is why we are requesting this workaround to match
> > "proprietary" FVB services behavior.
> > >
> > > Regards,
> > > Isaac
> > >
> > > -----Original Message-----
> > > From: Michael Kubacki <mikuback@linux.microsoft.com>
> > > Sent: Thursday, February 9, 2023 7:40 AM
> > > To: devel@edk2.groups.io; Chiu, Chasel <chasel.chiu@intel.com>
> > > Cc: S, Ashraf Ali <ashraf.ali.s@intel.com>; Oram, Isaac W
> > > <isaac.w.oram@intel.com>; Chaganty, Rangasai V
> > > <rangasai.v.chaganty@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > > Kubacki, Michael <michael.kubacki@microsoft.com>
> > > Subject: Re: [edk2-devel] [edk2-platforms: PATCH]
> > IntelSiliconPkg/SpiFvbServiceSmm: Support Other NVS variable region.
> > >
> > > Is there a reason this other content can't go into it's own FV?
> > >
> > > On 2/9/2023 12:14 AM, Chiu, Chasel wrote:
> > >> Platform may implement Other NVS variable region following Regular
> > >> variable region and in this case SpiFvbService should include both
> > >> region size when calculating the total NVS region size.
> > >>
> > >> One usage model is EventLog NVS region and there could be others.
> > >>
> > >> Cc: Ashraf Ali S <ashraf.ali.s@intel.com>
> > >> Cc: Isaac Oram <isaac.w.oram@intel.com>
> > >> Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> > >> Cc: Ray Ni <ray.ni@intel.com>
> > >> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
> > >> Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
> > >> ---
> > >>
> > Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServic
> > eCommon
> > .c | 7 +++++++
> > >>
> > Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServic
> > eSmm.inf
> > | 7 ++++---
> > >> Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec | 8
> > ++++++++
> > >> 3 files changed, 19 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git
> > >> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbS
> > >> er
> > >> v
> > >> iceCommon.c
> > >> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbS
> > >> er
> > >> v
> > >> iceCommon.c
> > >> index 942abf95a6..bcde98131d 100644
> > >> ---
> > >> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbS
> > >> er
> > >> v
> > >> iceCommon.c
> > >> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/Spi
> > >> +++ Fv
> > >> +++ b
> > >> +++ ServiceCommon.c
> > >> @@ -568,6 +568,13 @@ GetVariableFvInfo (
> > >> return;
> > >>
> > >> }
> > >>
> > >>
> > >>
> > >> + //
> > >>
> > >> + // GetVariableFlashNvStorageInfo () only reports regular
> > >> + variable region information,
> > >>
> > >> + // if platform implemented a separate Other variable region
> > >> + following the regular variable region,
> > >>
> > >> + // the size should be included as overall NVS variable region size.
> > >>
> > >> + //
> > >>
> > >> + NvStoreLength += PcdGet32 (PcdFlashNvStorageOtherVariableSize);
> > >>
> > >> +
> > >>
> > >> Status = GetVariableFlashFtwSpareInfo (&NvBaseAddress,
> > >> &Length64);
> > >>
> > >> if (!EFI_ERROR (Status)) {
> > >>
> > >> // Stay within the current UINT32 size assumptions in the variable stack.
> > >>
> > >> diff --git
> > >> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbS
> > >> er
> > >> v
> > >> iceSmm.inf
> > >> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbS
> > >> er
> > >> v
> > >> iceSmm.inf
> > >> index 73049eceb2..f40067418a 100644
> > >> ---
> > >> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbS
> > >> er
> > >> v
> > >> iceSmm.inf
> > >> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/Spi
> > >> +++ Fv
> > >> +++ b
> > >> +++ ServiceSmm.inf
> > >> @@ -43,9 +43,10 @@
> > >> IntelSiliconPkg/IntelSiliconPkg.dec
> > >>
> > >>
> > >>
> > >> [Pcd]
> > >>
> > >> - gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase ##
> > CONSUMES
> > >>
> > >> - gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize ##
> > CONSUMES
> > >>
> > >> - gIntelSiliconPkgTokenSpaceGuid.PcdFlashVariableStoreType ##
> > SOMETIMES_CONSUMES
> > >>
> > >> + gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase ##
> > CONSUMES
> > >>
> > >> + gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize ##
> > CONSUMES
> > >>
> > >> + gIntelSiliconPkgTokenSpaceGuid.PcdFlashVariableStoreType ##
> > SOMETIMES_CONSUMES
> > >>
> > >> +
> > >> + gIntelSiliconPkgTokenSpaceGuid.PcdFlashNvStorageOtherVariableSize
> > >> + ## CONSUMES
> > >>
> > >>
> > >>
> > >> [Sources]
> > >>
> > >> FvbInfo.c
> > >>
> > >> diff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
> > >> b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
> > >> index 63dae756ad..7034ab93b0 100644
> > >> --- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
> > >> +++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
> > >> @@ -194,3 +194,11 @@
> > >> # Other value: reserved for future use.<BR>
> > >>
> > >> # @Prompt Flash Variable Store type.
> > >>
> > >>
> > >> gIntelSiliconPkgTokenSpaceGuid.PcdFlashVariableStoreType|0x00|UINT8
> > >> |0
> > >> x
> > >> 0000000E
> > >>
> > >> +
> > >>
> > >> + ## Declares Separate NVS Variable Region Size.<BR><BR>
> > >>
> > >> + # Platform may implement a Regular variable region and an Other
> > >> + variable region, which will require this PCD
> > >>
> > >> + # to tell SpiFvbService to include both regions.<BR>
> > >>
> > >> + # 0: No separate Other variable region.<BR>
> > >>
> > >> + # non-zero: The size of a separate Other variable region
> > >> + following the Regular variable region.<BR>
> > >>
> > >> + # @Prompt Separate NVS Variable Region Size.
> > >>
> > >> +
> > >> + gIntelSiliconPkgTokenSpaceGuid.PcdFlashNvStorageOtherVariableSize
> > >> + |0
> > >> + x
> > >> + 00000000|UINT32|0x0000000F
> > >>
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
>
>
>
>
>
prev parent reply other threads:[~2023-02-09 18:28 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-09 5:14 [edk2-platforms: PATCH] IntelSiliconPkg/SpiFvbServiceSmm: Support Other NVS variable region Chiu, Chasel
2023-02-09 15:39 ` [edk2-devel] " Michael Kubacki
2023-02-09 16:04 ` Isaac Oram
2023-02-09 16:46 ` Michael Kubacki
2023-02-09 17:25 ` Chiu, Chasel
[not found] ` <174237FCE67B704B.15261@groups.io>
2023-02-09 18:28 ` Chiu, Chasel [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=BN9PR11MB548314C8A83E7B270BA04D34E6D99@BN9PR11MB5483.namprd11.prod.outlook.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