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; mikub...@linux.microsoft.com; Oram, Isaac W > <isaac.w.o...@intel.com> > Cc: S, Ashraf Ali <ashraf.al...@intel.com>; Chaganty, Rangasai V > <rangasai.v.chaga...@intel.com>; Ni, Ray <ray...@intel.com>; Kubacki, > Michael <michael.kuba...@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.o...@intel.com>; > > Chiu, Chasel <chasel.c...@intel.com> > > Cc: S, Ashraf Ali <ashraf.al...@intel.com>; Chaganty, Rangasai V > > <rangasai.v.chaga...@intel.com>; Ni, Ray <ray...@intel.com>; Kubacki, > > Michael <michael.kuba...@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.kuba...@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 <mikub...@linux.microsoft.com> > > > Sent: Thursday, February 9, 2023 7:40 AM > > > To: devel@edk2.groups.io; Chiu, Chasel <chasel.c...@intel.com> > > > Cc: S, Ashraf Ali <ashraf.al...@intel.com>; Oram, Isaac W > > > <isaac.w.o...@intel.com>; Chaganty, Rangasai V > > > <rangasai.v.chaga...@intel.com>; Ni, Ray <ray...@intel.com>; > > > Kubacki, Michael <michael.kuba...@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.al...@intel.com> > > >> Cc: Isaac Oram <isaac.w.o...@intel.com> > > >> Cc: Rangasai V Chaganty <rangasai.v.chaga...@intel.com> > > >> Cc: Ray Ni <ray...@intel.com> > > >> Cc: Michael Kubacki <michael.kuba...@microsoft.com> > > >> Signed-off-by: Chasel Chiu <chasel.c...@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 > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#99926): https://edk2.groups.io/g/devel/message/99926 Mute This Topic: https://groups.io/mt/96847771/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-