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/SpiFvbServiceCommon
> .c | 7 +++++++
> >>
> Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf
> | 7 ++++---
> >> Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
> >> | 8
> ++++++++
> >> 3 files changed, 19 insertions(+), 3 deletions(-)
> >>
> >> diff --git
> >> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSer
> >> v
> >> iceCommon.c
> >> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSer
> >> v
> >> iceCommon.c
> >> index 942abf95a6..bcde98131d 100644
> >> ---
> >> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSer
> >> v
> >> iceCommon.c
> >> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFv
> >> +++ 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/SpiFvbSer
> >> v
> >> iceSmm.inf
> >> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSer
> >> v
> >> iceSmm.inf
> >> index 73049eceb2..f40067418a 100644
> >> ---
> >> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSer
> >> v
> >> iceSmm.inf
> >> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFv
> >> +++ 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 (#99922): https://edk2.groups.io/g/devel/message/99922
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]
-=-=-=-=-=-=-=-=-=-=-=-
