Re: [edk2-devel] [PATCH v2 1/1] Silicon/FitGen: Enhance Slot mode support force mode for multiple FV.

["Aaron Li" <aaron.li@intel.com>]

Hi Liming,

I had replied under your comments.
I'll send the v3 version of this patch.

Best,
Aaron

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of gaoliming
Sent: Tuesday, December 22, 2020 9:37 AM
To: devel@edk2.groups.io; Li, Aaron <aaron.li@intel.com>
Cc: Feng, Bob C <bob.c.feng@intel.com>
Subject: 回复: [edk2-devel] [PATCH v2 1/1] Silicon/FitGen: Enhance Slot mode support force mode for multiple FV.

Aaron:
  I add my comment. 

Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+69301+4905953+8761045@groups.io
> <bounce+27952+69301+4905953+8761045@groups.io> 代表 Aaron Li
> 发送时间: 2020年12月21日 14:26
> 收件人: devel@edk2.groups.io
> 抄送: Bob Feng <bob.c.feng@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>
> 主题: [edk2-devel] [PATCH v2 1/1] Silicon/FitGen: Enhance Slot mode
support
> force mode for multiple FV.
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3122
> 
> Adding "-LF"/"-lf" option to support slot mode without FFS GUID check.
> It will support the scenario that multiple Microcode FV with different FFS
> GUID enable slot mode.
> The size of slot should be 16 byte-aligned, and larger than every
> microcode.
> 
> Signed-off-by: Aaron Li <aaron.li@intel.com>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> ---
>  Silicon/Intel/Tools/FitGen/FitGen.c | 39 +++++++++++++-------
>  Silicon/Intel/Tools/FitGen/FitGen.h |  2 +-
>  2 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/Silicon/Intel/Tools/FitGen/FitGen.c
> b/Silicon/Intel/Tools/FitGen/FitGen.c
> index e9541c1e95cb..6f7ddedf7e5f 100644
> --- a/Silicon/Intel/Tools/FitGen/FitGen.c
> +++ b/Silicon/Intel/Tools/FitGen/FitGen.c
> @@ -333,9 +333,10 @@ Returns:
>            "\t[-F <FitTablePointerOffset>] [-F <FitTablePointerOffset>]
[-V
> <FitHeaderVersion>]\n"
> 
>            "\t[-NA]\n"
> 
>            "\t[-A <MicrocodeAlignment>]\n"
> 
> -           "\t[-REMAP <TopFlashAddress>\n"
> 
> +          "\t[-REMAP <TopFlashAddress>\n"
> 
>            "\t[-CLEAR]\n"
> 
>            "\t[-L <MicrocodeSlotSize> <MicrocodeFfsGuid>]\n"
> 
> +          "\t[-LF <MicrocodeSlotSize>]\n"
> 
>            "\t[-I <BiosInfoGuid>]\n"
> 
>            "\t[-S <StartupAcmAddress
> StartupAcmSize>|<StartupAcmGuid>] [-V <StartupAcmVersion>]\n"
> 
>            "\t[-U <DiagnstAcmAddress>|<DiagnstAcmGuid>]\n"
> 
> @@ -366,6 +367,7 @@ Returns:
>    printf ("\tMicrocodeGuid          - Guid of Microcode Module.\n");
> 
>    printf ("\tMicrocodeSlotSize      - Occupied region size of each
> Microcode binary.\n");
> 
>    printf ("\tMicrocodeFfsGuid       - Guid of FFS which is used to save
> Microcode binary");
> 
> +  printf ("\t-LF                    - Microcode Slot mode without FFS
> check, treat all Microcode FV as slot mode. In this case the Microcode FV
> should only contain one FFS.\n");
> 
>    printf ("\t-NA                    - No 0x800 aligned Microcode
> requirement. No -NA means Microcode is aligned with option
> MicrocodeAlignment value.\n");
> 
>    printf ("\tMicrocodeAlignment     - HEX value of Microcode alignment.
> Ignored if \"-NA\" is specified. Default value is 0x800. The Microcode
update
> data must start at a 16-byte aligned linear address.\n");
> 
>    printf ("\tRecordType             - FIT entry record type. User should
> ensure it is ordered.\n");
> 
> @@ -882,11 +884,13 @@ Returns:
>    UINTN                       FitEntryNumber;
> 
>    BOOLEAN                     BiosInfoExist;
> 
>    BOOLEAN                     SlotMode;
> 
> +  BOOLEAN                     SlotModeForce;
> 
>    BIOS_INFO_HEADER            *BiosInfo;
> 
>    BIOS_INFO_STRUCT            *BiosInfoStruct;
> 
>    UINTN                       BiosInfoIndex;
> 
> 
> 
> -  SlotMode = FALSE;
> 
> +  SlotMode      = FALSE;
> 
> +  SlotModeForce = FALSE;
> 
> 
> 
>    //
> 
>    // Init index
> 
> @@ -1031,7 +1035,9 @@ Returns:
>    //
> 
>    if ((Index + 1 >= argc) ||
> 
>        ((strcmp (argv[Index], "-L") != 0) &&
> 
> -       (strcmp (argv[Index], "-l") != 0)) ) {
> 
> +       (strcmp (argv[Index], "-l") != 0) &&
> 
> +       (strcmp (argv[Index], "-LF") != 0) &&
> 
> +       (strcmp (argv[Index], "-lf") != 0))) {
> 
>      //
> 
>      // Bypass
> 
>      //
> 
> @@ -1039,18 +1045,21 @@ Returns:
>    } else {
> 
>      SlotSize = xtoi (argv[Index + 1]);
> 
> 
> 
> -    if (SlotSize == 0) {
> 
> -      printf ("Invalid slotsize = %d\n", SlotSize);
> 
> +    if (SlotSize & 0xF) {
> 
> +      printf ("Invalid slotsize = 0x%x, Microcode data must start at a
> 16-byte aligned linear address!\n", SlotSize);
> 
>        return 0;
> 
>      }
> 
If SlotSize is zero, is it valid or not?
-- Slot size should not be zero, I'll change the code.

> -
> 
> -    SlotMode = IsGuidData(argv[Index + 2], &MicrocodeFfsGuid);
> 
> -    if (!SlotMode) {
> 
> -      printf ("Need a ffs GUID for search uCode ffs\n");
> 
> -      return 0;
> 
> +    if (strcmp (argv[Index], "-LF") == 0 || strcmp (argv[Index], "-lf")
== 0) {
> 
> +      SlotModeForce = TRUE;
> 
> +      Index += 2;
> 
> +    } else {
> 
> +      SlotMode = IsGuidData(argv[Index + 2], &MicrocodeFfsGuid);
> 
> +      if (!SlotMode) {
> 
> +        printf ("Need a ffs GUID for search uCode ffs\n");
> 
> +        return 0;
> 
> +      }
> 
> +      Index += 3;
> 
>      }
> 
> -
> 
> -    Index += 3;
> 
>    }
> 
> 
> 
>    //
> 
> @@ -1219,6 +1228,10 @@ Returns:
>                gFitTableContext.FitEntryNumber++;
> 
> 
> 
>                if (SlotSize != 0) {
> 
> +                if (SlotSize < MicrocodeSize) {
> 
> +                  printf ("Parameter incorrect, Slot size: %x is too
small
> for Microcode size: %x!\n", SlotSize, MicrocodeSize);
> 
> +                  return 0;
> 
> +                }

What purpose is for this new check?
-- The slot size should be larger than microcode size, since the microcode is contained by slot.

Thanks
Liming
> 
>                  MicrocodeBuffer += SlotSize;
> 
>                } else {
> 
>                  MicrocodeBuffer += MicrocodeSize;
> 
> @@ -1228,7 +1241,7 @@ Returns:
>              ///
> 
>              /// Check the remaining buffer
> 
>              ///
> 
> -            if (((UINT32)(MicrocodeBuffer - MicrocodeFileBuffer) <
> MicrocodeFileSize) && SlotMode != 0) {
> 
> +            if (((UINT32)(MicrocodeBuffer - MicrocodeFileBuffer) <
> MicrocodeFileSize) && (SlotMode || SlotModeForce)) {
> 
>                for (Walker = MicrocodeBuffer; Walker <
> MicrocodeFileBuffer + MicrocodeFileSize; Walker++) {
> 
>                  if (*Walker != 0xFF) {
> 
>                    printf ("Error: detect non-spare space after uCode
> array, please check uCode array!\n");
> 
> diff --git a/Silicon/Intel/Tools/FitGen/FitGen.h
> b/Silicon/Intel/Tools/FitGen/FitGen.h
> index 435fc26209da..5add6a8870e9 100644
> --- a/Silicon/Intel/Tools/FitGen/FitGen.h
> +++ b/Silicon/Intel/Tools/FitGen/FitGen.h
> @@ -31,7 +31,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  // Utility version information
> 
>  //
> 
>  #define UTILITY_MAJOR_VERSION 0
> 
> -#define UTILITY_MINOR_VERSION 63
> 
> +#define UTILITY_MINOR_VERSION 64
> 
>  #define UTILITY_DATE          __DATE__
> 
> 
> 
>  //
> 
> --
> 2.29.2.windows.2
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#69301): https://edk2.groups.io/g/devel/message/69301
> Mute This Topic: https://groups.io/mt/79120990/4905953
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [gaoliming@byosoft.com.cn]
> -=-=-=-=-=-=
>