The MmCoreFfsFindMmDriver only checks for encapsulated compressed FVs.
When an inner FV is uncompressed, StandaloneMmCore will miss the FV and
all the MM drivers in the FV will not be dispatched.
Add checks for uncompressed inner FV to fix this issue.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Wei6 Xu <wei6.xu@intel.com>
---
 StandaloneMmPkg/Core/FwVol.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c
index fa335d62c252..783dbaf9b048 100644
--- a/StandaloneMmPkg/Core/FwVol.c
+++ b/StandaloneMmPkg/Core/FwVol.c
@@ -116,6 +116,25 @@ MmCoreFfsFindMmDriver (
       break;
     }
 
+    //
+    // Check uncompressed firmware volumes
+    //
+    Status = FfsFindSectionData (
+               EFI_SECTION_FIRMWARE_VOLUME_IMAGE,
+               FileHeader,
+               &SectionData,
+               &SectionDataSize
+               );
+    if (!EFI_ERROR (Status)) {
+      if (SectionDataSize > sizeof (EFI_FIRMWARE_VOLUME_HEADER)) {
+        InnerFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)SectionData;
+        MmCoreFfsFindMmDriver (InnerFvHeader, Depth + 1);
+      }
+    }
+
+    //
+    // Check compressed firmware volumes
+    //
     Status = FfsFindSection (
                EFI_SECTION_GUID_DEFINED,
                FileHeader,
-- 
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110300): https://edk2.groups.io/g/devel/message/110300
Mute This Topic: https://groups.io/mt/102270549/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 10/30/23 08:49, Wei6 Xu wrote:
> The MmCoreFfsFindMmDriver only checks for encapsulated compressed FVs.
> When an inner FV is uncompressed, StandaloneMmCore will miss the FV and
> all the MM drivers in the FV will not be dispatched.
> Add checks for uncompressed inner FV to fix this issue.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Wei6 Xu <wei6.xu@intel.com>
> ---
>  StandaloneMmPkg/Core/FwVol.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c
> index fa335d62c252..783dbaf9b048 100644
> --- a/StandaloneMmPkg/Core/FwVol.c
> +++ b/StandaloneMmPkg/Core/FwVol.c
> @@ -116,6 +116,25 @@ MmCoreFfsFindMmDriver (
>        break;
>      }
>  
> +    //
> +    // Check uncompressed firmware volumes
> +    //
> +    Status = FfsFindSectionData (
> +               EFI_SECTION_FIRMWARE_VOLUME_IMAGE,
> +               FileHeader,
> +               &SectionData,
> +               &SectionDataSize
> +               );
> +    if (!EFI_ERROR (Status)) {
> +      if (SectionDataSize > sizeof (EFI_FIRMWARE_VOLUME_HEADER)) {
> +        InnerFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)SectionData;
> +        MmCoreFfsFindMmDriver (InnerFvHeader, Depth + 1);
> +      }
> +    }
> +
> +    //
> +    // Check compressed firmware volumes
> +    //
>      Status = FfsFindSection (
>                 EFI_SECTION_GUID_DEFINED,
>                 FileHeader,

(1) In case we find an EFI_SECTION_FIRMWARE_VOLUME_IMAGE, do we still want to look for an EFI_SECTION_GUID_DEFINED?

I think that's quite unlikely. If you agree, can you add a "continue" right after the new MmCoreFfsFindMmDriver() call?

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

(2) We have a separate "InnerFvHeader" assignment, near the bottom of this (first) loop in the function:

     Status = FindFfsSectionInSections (
                DstBuffer,
                DstBufferSize,
                EFI_SECTION_FIRMWARE_VOLUME_IMAGE,
                &Section
                );
     if (EFI_ERROR (Status)) {
       goto FreeDstBuffer;
     }
 
     InnerFvHeader = (VOID *)(Section + 1);
     Status        = MmCoreFfsFindMmDriver (InnerFvHeader, Depth + 1);

That doesn't look right to me; what if Section in fact points to an EFI_COMMON_SECTION_HEADER2?

FfsFindSectionData() handles this internally; it checks IS_SECTION2(), and then adjusts SectionData accordingly:

      if (IS_SECTION2 (Section)) {
        *SectionData     = (VOID *)((EFI_COMMON_SECTION_HEADER2 *)Section + 1);
        ...
      } else {
        *SectionData     = (VOID *)(Section + 1);
        ...
      }

I think we need to set InnerFvHeader similarly, here. Can you please append a separate patch for fixing that?

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110316): https://edk2.groups.io/g/devel/message/110316
Mute This Topic: https://groups.io/mt/102270549/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-