[PATCH V3 0/3] MdeModulePkg/PartitionDxe: Fix the partition check issue

[PATCH V3 0/3] MdeModulePkg/PartitionDxe: Fix the partition check issue

[Gao, Zhichao]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823

V1:
Separate the UDF from the partition rountine array and do the check
for every media.

V2:
Drop V1 because it is a bug: there should not be two partition types in one media.
1. Correct the LastBlock value in MBR handler. It should be the number of
sectors (512 bytes).
2. Skip the MBR check if the MBR is added for the Windows comaptiblity. We treat such
media as ElTorito and do the check.
3. Fix the partition check bug: One partition type returns already started should
be treated as success to avoid multi partition type be installed into same media.

V3:
patch #1: Remove the useless variable and add the reason and impact of the fix.
patch #2: Remove useless condition and make the comments more clear.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>

Zhichao Gao (3):
  MdeModulePkg/PartitionDxe: Correct the MBR last block value
  MdeModulePkg/PartitionDxe: Skip the MBR that add for CD-ROM
  MdeModulePkg/PartitionDxe: Add already start check for child hanldes

 .../Universal/Disk/PartitionDxe/Mbr.c         | 50 +++++++++++++++----
 .../Universal/Disk/PartitionDxe/Partition.c   |  9 ++++
 2 files changed, 48 insertions(+), 11 deletions(-)

-- 
2.21.0.windows.1

[PATCH V3 1/3] MdeModulePkg/PartitionDxe: Correct the MBR last block value

[Gao, Zhichao]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823

PartitionValidMbr function's second parameter should be the
last sector of the device. For MBR partition, the block size is
sector size, i.e. 512 bytes. The original value is media block
last LBA which is counted by the media block size. And media
block size is not always 512 bytes, it may be larger which would
cause the MBR boundary check incorrect. The boundary check is
based on the partition entry start LBA and size of LBA which
are both counted by the sector number (512 bytes).

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
index dac451a144..f0c92aa09a 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
@@ -135,14 +135,17 @@ PartitionInstallMbrChildHandles (
   EFI_DEVICE_PATH_PROTOCOL     *LastDevicePathNode;
   UINT32                       BlockSize;
   UINT32                       MediaId;
-  EFI_LBA                      LastBlock;
+  EFI_LBA                      LastSector;
   EFI_PARTITION_INFO_PROTOCOL  PartitionInfo;
 
   Found           = EFI_NOT_FOUND;
 
-  BlockSize = BlockIo->Media->BlockSize;
-  MediaId   = BlockIo->Media->MediaId;
-  LastBlock = BlockIo->Media->LastBlock;
+  BlockSize   = BlockIo->Media->BlockSize;
+  MediaId     = BlockIo->Media->MediaId;
+  LastSector  = DivU64x32 (
+                  MultU64x32 (BlockIo->Media->LastBlock + 1, BlockSize),
+                  MBR_SIZE
+                  ) - 1;
 
   //
   // Ensure the block size can hold the MBR
@@ -167,7 +170,7 @@ PartitionInstallMbrChildHandles (
     Found = Status;
     goto Done;
   }
-  if (!PartitionValidMbr (Mbr, LastBlock)) {
+  if (!PartitionValidMbr (Mbr, LastSector)) {
     goto Done;
   }
   //
-- 
2.21.0.windows.1

Re: [PATCH V3 1/3] MdeModulePkg/PartitionDxe: Correct the MBR last block value

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Tuesday, July 14, 2020 9:23 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH V3 1/3] MdeModulePkg/PartitionDxe: Correct the MBR last
> block value
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> 
> PartitionValidMbr function's second parameter should be the
> last sector of the device. For MBR partition, the block size is
> sector size, i.e. 512 bytes. The original value is media block
> last LBA which is counted by the media block size. And media
> block size is not always 512 bytes, it may be larger which would
> cause the MBR boundary check incorrect. The boundary check is
> based on the partition entry start LBA and size of LBA which
> are both counted by the sector number (512 bytes).
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> index dac451a144..f0c92aa09a 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> @@ -135,14 +135,17 @@ PartitionInstallMbrChildHandles (
>    EFI_DEVICE_PATH_PROTOCOL     *LastDevicePathNode;
>    UINT32                       BlockSize;
>    UINT32                       MediaId;
> -  EFI_LBA                      LastBlock;
> +  EFI_LBA                      LastSector;
>    EFI_PARTITION_INFO_PROTOCOL  PartitionInfo;
> 
>    Found           = EFI_NOT_FOUND;
> 
> -  BlockSize = BlockIo->Media->BlockSize;
> -  MediaId   = BlockIo->Media->MediaId;
> -  LastBlock = BlockIo->Media->LastBlock;
> +  BlockSize   = BlockIo->Media->BlockSize;
> +  MediaId     = BlockIo->Media->MediaId;
> +  LastSector  = DivU64x32 (
> +                  MultU64x32 (BlockIo->Media->LastBlock + 1, BlockSize),
> +                  MBR_SIZE
> +                  ) - 1;
> 
>    //
>    // Ensure the block size can hold the MBR
> @@ -167,7 +170,7 @@ PartitionInstallMbrChildHandles (
>      Found = Status;
>      goto Done;
>    }
> -  if (!PartitionValidMbr (Mbr, LastBlock)) {
> +  if (!PartitionValidMbr (Mbr, LastSector)) {
>      goto Done;
>    }
>    //
> --
> 2.21.0.windows.1

[PATCH V3 2/3] MdeModulePkg/PartitionDxe: Skip the MBR that add for CD-ROM

[Gao, Zhichao]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823

Refer to
http://manpages.ubuntu.com/manpages/bionic/man8/mkudffs.8.html.
Some Linux ISOs may have the MBR table for compatibility reasons
for Windows. The MBR tale would contain the partition entry with
start LBA0 and whole media size. There are two methods to check
the filesystem in the CD-ROM:
1. MBR partition check (Windows)
2. Whole disk check (MAC OS)

UEFI doesn't have the MBR check for UDF and Eltorito. But it may
pass the MBR check for such table and fail to detect the filesystem
of UDF. Skip the MBR check if the MBR is added for Windows
compatiblity so that the partition driver can continue UDF and
ElTorito check.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 .../Universal/Disk/PartitionDxe/Mbr.c         | 37 ++++++++++++++++---
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
index f0c92aa09a..3830af1ea7 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
@@ -39,6 +39,7 @@ PartitionValidMbr (
   UINT32  StartingLBA;
   UINT32  EndingLBA;
   UINT32  NewEndingLBA;
+  UINT32  SizeInLBA;
   INTN    Index1;
   INTN    Index2;
   BOOLEAN MbrValid;
@@ -51,13 +52,34 @@ PartitionValidMbr (
   //
   MbrValid = FALSE;
   for (Index1 = 0; Index1 < MAX_MBR_PARTITIONS; Index1++) {
-    if (Mbr->Partition[Index1].OSIndicator == 0x00 || UNPACK_UINT32 (Mbr->Partition[Index1].SizeInLBA) == 0) {
+    StartingLBA = UNPACK_UINT32 (Mbr->Partition[Index1].StartingLBA);
+    SizeInLBA   = UNPACK_UINT32 (Mbr->Partition[Index1].SizeInLBA);
+
+    //
+    // If the MBR with partition entry covering the ENTIRE disk, i.e. start at LBA0
+    // with whole disk size, we treat it as an invalid MBR partition.
+    //
+    if ((StartingLBA == 0) &&
+        (SizeInLBA == (LastLba + 1))) {
+      //
+      // Refer to the http://manpages.ubuntu.com/manpages/bionic/man8/mkudffs.8.html
+      // "WHOLE DISK VS PARTITION"
+      // Some linux ISOs may put the MBR table in the first 512 bytes for compatibility reasons with Windows.
+      // Linux  kernel  ignores MBR table if contains partition which starts at sector 0.
+      // Skip it because we don't have the partition check for UDF(El Torito compatible).
+      // It would continue to do the whole disk check in the UDF routine.
+      //
+      DEBUG ((DEBUG_INFO, "PartitionValidMbr: MBR table has partition entry covering the ENTIRE disk. Don't treat it as a valid MBR.\n"));
+
+      return FALSE;
+    }
+
+    if (Mbr->Partition[Index1].OSIndicator == 0x00 || SizeInLBA == 0) {
       continue;
     }
 
     MbrValid    = TRUE;
-    StartingLBA = UNPACK_UINT32 (Mbr->Partition[Index1].StartingLBA);
-    EndingLBA   = StartingLBA + UNPACK_UINT32 (Mbr->Partition[Index1].SizeInLBA) - 1;
+    EndingLBA   = StartingLBA + SizeInLBA - 1;
     if (EndingLBA > LastLba) {
       //
       // Compatibility Errata:
@@ -77,12 +99,15 @@ PartitionValidMbr (
     }
 
     for (Index2 = Index1 + 1; Index2 < MAX_MBR_PARTITIONS; Index2++) {
-      if (Mbr->Partition[Index2].OSIndicator == 0x00 || UNPACK_UINT32 (Mbr->Partition[Index2].SizeInLBA) == 0) {
+      StartingLBA = UNPACK_UINT32 (Mbr->Partition[Index2].StartingLBA);
+      SizeInLBA   = UNPACK_UINT32 (Mbr->Partition[Index2].SizeInLBA);
+
+      if (Mbr->Partition[Index2].OSIndicator == 0x00 || SizeInLBA == 0) {
         continue;
       }
 
-      NewEndingLBA = UNPACK_UINT32 (Mbr->Partition[Index2].StartingLBA) + UNPACK_UINT32 (Mbr->Partition[Index2].SizeInLBA) - 1;
-      if (NewEndingLBA >= StartingLBA && UNPACK_UINT32 (Mbr->Partition[Index2].StartingLBA) <= EndingLBA) {
+      NewEndingLBA = StartingLBA + SizeInLBA - 1;
+      if (NewEndingLBA >= StartingLBA && StartingLBA <= EndingLBA) {
         //
         // This region overlaps with the Index1'th region
         //
-- 
2.21.0.windows.1

Re: [PATCH V3 2/3] MdeModulePkg/PartitionDxe: Skip the MBR that add for CD-ROM

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Tuesday, July 14, 2020 9:23 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH V3 2/3] MdeModulePkg/PartitionDxe: Skip the MBR that add
> for CD-ROM
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> 
> Refer to
> http://manpages.ubuntu.com/manpages/bionic/man8/mkudffs.8.html.
> Some Linux ISOs may have the MBR table for compatibility reasons
> for Windows. The MBR tale would contain the partition entry with
> start LBA0 and whole media size. There are two methods to check
> the filesystem in the CD-ROM:
> 1. MBR partition check (Windows)
> 2. Whole disk check (MAC OS)
> 
> UEFI doesn't have the MBR check for UDF and Eltorito. But it may
> pass the MBR check for such table and fail to detect the filesystem
> of UDF. Skip the MBR check if the MBR is added for Windows
> compatiblity so that the partition driver can continue UDF and
> ElTorito check.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  .../Universal/Disk/PartitionDxe/Mbr.c         | 37 ++++++++++++++++---
>  1 file changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> index f0c92aa09a..3830af1ea7 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> @@ -39,6 +39,7 @@ PartitionValidMbr (
>    UINT32  StartingLBA;
>    UINT32  EndingLBA;
>    UINT32  NewEndingLBA;
> +  UINT32  SizeInLBA;
>    INTN    Index1;
>    INTN    Index2;
>    BOOLEAN MbrValid;
> @@ -51,13 +52,34 @@ PartitionValidMbr (
>    //
>    MbrValid = FALSE;
>    for (Index1 = 0; Index1 < MAX_MBR_PARTITIONS; Index1++) {
> -    if (Mbr->Partition[Index1].OSIndicator == 0x00 || UNPACK_UINT32 (Mbr-
> >Partition[Index1].SizeInLBA) == 0) {
> +    StartingLBA = UNPACK_UINT32 (Mbr->Partition[Index1].StartingLBA);
> +    SizeInLBA   = UNPACK_UINT32 (Mbr->Partition[Index1].SizeInLBA);
> +
> +    //
> +    // If the MBR with partition entry covering the ENTIRE disk, i.e. start at
> LBA0
> +    // with whole disk size, we treat it as an invalid MBR partition.
> +    //
> +    if ((StartingLBA == 0) &&
> +        (SizeInLBA == (LastLba + 1))) {
> +      //
> +      // Refer to the
> http://manpages.ubuntu.com/manpages/bionic/man8/mkudffs.8.html
> +      // "WHOLE DISK VS PARTITION"
> +      // Some linux ISOs may put the MBR table in the first 512 bytes for
> compatibility reasons with Windows.
> +      // Linux  kernel  ignores MBR table if contains partition which starts at
> sector 0.
> +      // Skip it because we don't have the partition check for UDF(El Torito
> compatible).
> +      // It would continue to do the whole disk check in the UDF routine.
> +      //
> +      DEBUG ((DEBUG_INFO, "PartitionValidMbr: MBR table has partition entry
> covering the ENTIRE disk. Don't treat it as a valid MBR.\n"));
> +
> +      return FALSE;
> +    }
> +
> +    if (Mbr->Partition[Index1].OSIndicator == 0x00 || SizeInLBA == 0) {
>        continue;
>      }
> 
>      MbrValid    = TRUE;
> -    StartingLBA = UNPACK_UINT32 (Mbr->Partition[Index1].StartingLBA);
> -    EndingLBA   = StartingLBA + UNPACK_UINT32 (Mbr-
> >Partition[Index1].SizeInLBA) - 1;
> +    EndingLBA   = StartingLBA + SizeInLBA - 1;
>      if (EndingLBA > LastLba) {
>        //
>        // Compatibility Errata:
> @@ -77,12 +99,15 @@ PartitionValidMbr (
>      }
> 
>      for (Index2 = Index1 + 1; Index2 < MAX_MBR_PARTITIONS; Index2++) {
> -      if (Mbr->Partition[Index2].OSIndicator == 0x00 || UNPACK_UINT32 (Mbr-
> >Partition[Index2].SizeInLBA) == 0) {
> +      StartingLBA = UNPACK_UINT32 (Mbr->Partition[Index2].StartingLBA);
> +      SizeInLBA   = UNPACK_UINT32 (Mbr->Partition[Index2].SizeInLBA);
> +
> +      if (Mbr->Partition[Index2].OSIndicator == 0x00 || SizeInLBA == 0) {
>          continue;
>        }
> 
> -      NewEndingLBA = UNPACK_UINT32 (Mbr->Partition[Index2].StartingLBA) +
> UNPACK_UINT32 (Mbr->Partition[Index2].SizeInLBA) - 1;
> -      if (NewEndingLBA >= StartingLBA && UNPACK_UINT32 (Mbr-
> >Partition[Index2].StartingLBA) <= EndingLBA) {
> +      NewEndingLBA = StartingLBA + SizeInLBA - 1;
> +      if (NewEndingLBA >= StartingLBA && StartingLBA <= EndingLBA) {
>          //
>          // This region overlaps with the Index1'th region
>          //
> --
> 2.21.0.windows.1

[PATCH V3 3/3] MdeModulePkg/PartitionDxe: Add already start check for child hanldes

[Gao, Zhichao]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823

Treat the EFI_ALREADY_STARTED as EFI_SUCCESS to avoid the partition driver
continuely check next routine function.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
index d1c878ad2e..6a43c3cafb 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
@@ -1276,6 +1276,15 @@ PartitionInstallChildHandle (
   } else {
     FreePool (Private->DevicePath);
     FreePool (Private);
+
+    //
+    // if the Status == EFI_ALREADY_STARTED, it means the child handles
+    // are already installed. So return EFI_SUCCESS to avoid do the next
+    // partition type check.
+    //
+    if (Status == EFI_ALREADY_STARTED) {
+      Status = EFI_SUCCESS;
+    }
   }
 
   return Status;
-- 
2.21.0.windows.1

Re: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/PartitionDxe: Add already start check for child hanldes

[Ni, Ray]

Zhichao,
Can you add more information in the commit message on what bug the patch can fix?
With that, Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao,
> Zhichao
> Sent: Tuesday, July 14, 2020 9:23 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/PartitionDxe: Add
> already start check for child hanldes
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> 
> Treat the EFI_ALREADY_STARTED as EFI_SUCCESS to avoid the partition driver
> continuely check next routine function.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> index d1c878ad2e..6a43c3cafb 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> @@ -1276,6 +1276,15 @@ PartitionInstallChildHandle (
>    } else {
>      FreePool (Private->DevicePath);
>      FreePool (Private);
> +
> +    //
> +    // if the Status == EFI_ALREADY_STARTED, it means the child handles
> +    // are already installed. So return EFI_SUCCESS to avoid do the next
> +    // partition type check.
> +    //
> +    if (Status == EFI_ALREADY_STARTED) {
> +      Status = EFI_SUCCESS;
> +    }
>    }
> 
>    return Status;
> --
> 2.21.0.windows.1
> 
> 
> 

Re: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/PartitionDxe: Add already start check for child hanldes

[Gao, Zhichao]

Sure.

Update the commit message here:

The partition binding driver would run serval times during BDS. If the partition supports MBR, it would pass the first connect in the MBR routine function. The second connect would return already started which would be treated as not found and continue to run next routine. That is incorrect behavior. The device should only support one partition format. Treat the already started as success to avoid incorrect next partition routine check.

Thanks,
Zhichao

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Tuesday, July 14, 2020 10:04 AM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/PartitionDxe: Add
> already start check for child hanldes
> 
> Zhichao,
> Can you add more information in the commit message on what bug the patch
> can fix?
> With that, Reviewed-by: Ray Ni <ray.ni@intel.com>
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao,
> > Zhichao
> > Sent: Tuesday, July 14, 2020 9:23 AM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> > Subject: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/PartitionDxe: Add
> > already start check for child hanldes
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> >
> > Treat the EFI_ALREADY_STARTED as EFI_SUCCESS to avoid the partition
> > driver continuely check next routine function.
> >
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> > ---
> >  MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > index d1c878ad2e..6a43c3cafb 100644
> > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > @@ -1276,6 +1276,15 @@ PartitionInstallChildHandle (
> >    } else {
> >      FreePool (Private->DevicePath);
> >      FreePool (Private);
> > +
> > +    //
> > +    // if the Status == EFI_ALREADY_STARTED, it means the child handles
> > +    // are already installed. So return EFI_SUCCESS to avoid do the next
> > +    // partition type check.
> > +    //
> > +    if (Status == EFI_ALREADY_STARTED) {
> > +      Status = EFI_SUCCESS;
> > +    }
> >    }
> >
> >    return Status;
> > --
> > 2.21.0.windows.1
> >
> >
> > 
> 

Re: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/PartitionDxe: Add already start check for child hanldes

[Ni, Ray]

Looks good to me! Thanks!!

> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Tuesday, July 14, 2020 1:56 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/PartitionDxe: Add
> already start check for child hanldes
> 
> Sure.
> 
> Update the commit message here:
> 
> The partition binding driver would run serval times during BDS. If the partition
> supports MBR, it would pass the first connect in the MBR routine function. The
> second connect would return already started which would be treated as not
> found and continue to run next routine. That is incorrect behavior. The device
> should only support one partition format. Treat the already started as success
> to avoid incorrect next partition routine check.
> 
> Thanks,
> Zhichao
> 
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Tuesday, July 14, 2020 10:04 AM
> > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> > Cc: Wu, Hao A <hao.a.wu@intel.com>
> > Subject: RE: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/PartitionDxe: Add
> > already start check for child hanldes
> >
> > Zhichao,
> > Can you add more information in the commit message on what bug the patch
> > can fix?
> > With that, Reviewed-by: Ray Ni <ray.ni@intel.com>
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao,
> > > Zhichao
> > > Sent: Tuesday, July 14, 2020 9:23 AM
> > > To: devel@edk2.groups.io
> > > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> > > Subject: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/PartitionDxe: Add
> > > already start check for child hanldes
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> > >
> > > Treat the EFI_ALREADY_STARTED as EFI_SUCCESS to avoid the partition
> > > driver continuely check next routine function.
> > >
> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > > Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> > > ---
> > >  MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > > b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > > index d1c878ad2e..6a43c3cafb 100644
> > > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > > @@ -1276,6 +1276,15 @@ PartitionInstallChildHandle (
> > >    } else {
> > >      FreePool (Private->DevicePath);
> > >      FreePool (Private);
> > > +
> > > +    //
> > > +    // if the Status == EFI_ALREADY_STARTED, it means the child handles
> > > +    // are already installed. So return EFI_SUCCESS to avoid do the next
> > > +    // partition type check.
> > > +    //
> > > +    if (Status == EFI_ALREADY_STARTED) {
> > > +      Status = EFI_SUCCESS;
> > > +    }
> > >    }
> > >
> > >    return Status;
> > > --
> > > 2.21.0.windows.1
> > >
> > >
> > > 
> >
> 

Re: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/PartitionDxe: Add already start check for child hanldes

[Gary Lin]

On Tue, Jul 14, 2020 at 09:22:59AM +0800, Gao, Zhichao via groups.io wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> 
> Treat the EFI_ALREADY_STARTED as EFI_SUCCESS to avoid the partition driver
> continuely check next routine function.
Hi Zhichao,

I just found that this patch breaks the loading of openSUSE iso images[*].
Would you mind to take a look at it?

Thanks,

Gary Lin

[*] http://download.opensuse.org/distribution/leap/15.2/iso/openSUSE-Leap-15.2-NET-x86_64.iso

> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> index d1c878ad2e..6a43c3cafb 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> @@ -1276,6 +1276,15 @@ PartitionInstallChildHandle (
>    } else {
>      FreePool (Private->DevicePath);
>      FreePool (Private);
> +
> +    //
> +    // if the Status == EFI_ALREADY_STARTED, it means the child handles
> +    // are already installed. So return EFI_SUCCESS to avoid do the next
> +    // partition type check.
> +    //
> +    if (Status == EFI_ALREADY_STARTED) {
> +      Status = EFI_SUCCESS;
> +    }
>    }
>  
>    return Status;
> -- 
> 2.21.0.windows.1
> 
> 
> 
> 

Re: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/PartitionDxe: Add already start check for child hanldes

[Gao, Zhichao]

Yes. I have checked the image with OVMF. The SUSE image has the MBR table but not like others, such as Ubuntu, RedHat and Fedora which would contain a partition entry with the entire disk.

That means it would pass the MBR child handle check and would never go to the UDF(or ElTorito) check.

I think we should update the invalid MBR check logic.

Thanks,
Zhichao

> -----Original Message-----
> From: Gary Lin <glin@suse.com>
> Sent: Thursday, July 16, 2020 11:33 AM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/PartitionDxe: Add
> already start check for child hanldes
> 
> On Tue, Jul 14, 2020 at 09:22:59AM +0800, Gao, Zhichao via groups.io wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> >
> > Treat the EFI_ALREADY_STARTED as EFI_SUCCESS to avoid the partition
> > driver continuely check next routine function.
> Hi Zhichao,
> 
> I just found that this patch breaks the loading of openSUSE iso images[*].
> Would you mind to take a look at it?
> 
> Thanks,
> 
> Gary Lin
> 
> [*] http://download.opensuse.org/distribution/leap/15.2/iso/openSUSE-Leap-
> 15.2-NET-x86_64.iso
> 
> >
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> > ---
> >  MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > index d1c878ad2e..6a43c3cafb 100644
> > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > @@ -1276,6 +1276,15 @@ PartitionInstallChildHandle (
> >    } else {
> >      FreePool (Private->DevicePath);
> >      FreePool (Private);
> > +
> > +    //
> > +    // if the Status == EFI_ALREADY_STARTED, it means the child handles
> > +    // are already installed. So return EFI_SUCCESS to avoid do the next
> > +    // partition type check.
> > +    //
> > +    if (Status == EFI_ALREADY_STARTED) {
> > +      Status = EFI_SUCCESS;
> > +    }
> >    }
> >
> >    return Status;
> > --
> > 2.21.0.windows.1
> >
> >
> > 
> >

Re: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/PartitionDxe: Add already start check for child hanldes

[Ni, Ray]

Zhichao,
What does the SUSE MBR table look like?

Thanks,
Ray

> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Thursday, July 16, 2020 1:09 PM
> To: Gary Lin <glin@suse.com>; devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: RE: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/PartitionDxe: Add already start check for child hanldes
> 
> Yes. I have checked the image with OVMF. The SUSE image has the MBR table but not like others, such as Ubuntu, RedHat
> and Fedora which would contain a partition entry with the entire disk.
> 
> That means it would pass the MBR child handle check and would never go to the UDF(or ElTorito) check.
> 
> I think we should update the invalid MBR check logic.
> 
> Thanks,
> Zhichao
> 
> > -----Original Message-----
> > From: Gary Lin <glin@suse.com>
> > Sent: Thursday, July 16, 2020 11:33 AM
> > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> > Subject: Re: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/PartitionDxe: Add
> > already start check for child hanldes
> >
> > On Tue, Jul 14, 2020 at 09:22:59AM +0800, Gao, Zhichao via groups.io wrote:
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> > >
> > > Treat the EFI_ALREADY_STARTED as EFI_SUCCESS to avoid the partition
> > > driver continuely check next routine function.
> > Hi Zhichao,
> >
> > I just found that this patch breaks the loading of openSUSE iso images[*].
> > Would you mind to take a look at it?
> >
> > Thanks,
> >
> > Gary Lin
> >
> > [*] http://download.opensuse.org/distribution/leap/15.2/iso/openSUSE-Leap-
> > 15.2-NET-x86_64.iso
> >
> > >
> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > > Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> > > ---
> > >  MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > > b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > > index d1c878ad2e..6a43c3cafb 100644
> > > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > > @@ -1276,6 +1276,15 @@ PartitionInstallChildHandle (
> > >    } else {
> > >      FreePool (Private->DevicePath);
> > >      FreePool (Private);
> > > +
> > > +    //
> > > +    // if the Status == EFI_ALREADY_STARTED, it means the child handles
> > > +    // are already installed. So return EFI_SUCCESS to avoid do the next
> > > +    // partition type check.
> > > +    //
> > > +    if (Status == EFI_ALREADY_STARTED) {
> > > +      Status = EFI_SUCCESS;
> > > +    }
> > >    }
> > >
> > >    return Status;
> > > --
> > > 2.21.0.windows.1
> > >
> > >
> > > 
> > >
>