[PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec

[PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec

[Gao, Zhichao]

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

Refer to the UEFI spec 2.8, Section 13.3.2:
A block device should be scanned as below order:
1. GPT
2. ISO 9660 (El Torito) (UDF should aslo be here)
4. MBR
5. no partition found

But the code implementation is:
1. GPT
2. MBR
3. ISO 9660 (El Torito) (UDF)
4. no partition found

Which would cause the ISO 9960 image with MBR info be treated as MBR
device. That would cause unexpect behavior. So fix it to follow the spec
description.

The fix of the PartitionInstallChildHandle would change the boot behavior
of Linux ISO image with MBR table. So add it after the order adjustment
to make no impact of the boot.

V2:
1. Add description of the different behavior between grub boot from MBR path and
from CD path
2. change patch #2 to revert "MdeModulePkg/PartitionDxe: Skip the MBR that add for CD-ROM"

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Gary Lin <glin@suse.com>
Cc: Andrew Fish <afish@apple.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>

Zhichao Gao (3):
  MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR
  MdeModulePkg/PartitionDxe: Revert changes for the special MBR
  MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander

 .../Universal/Disk/PartitionDxe/Mbr.c         | 37 +++----------------
 .../Universal/Disk/PartitionDxe/Partition.c   | 22 ++++++-----
 2 files changed, 18 insertions(+), 41 deletions(-)

-- 
2.21.0.windows.1

[PATCH V2 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR

[Gao, Zhichao]

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

Refer to UEFI spec 2.8, Section 13.3.2, a block device should
be scanned as below order:
1. GPT
2. ISO 9660 (El Torito) (UDF should aslo be here)
3. MBR
4. no partition found
Note: UDF is using the same boot method as CD, so put it in
the same priority with ISO 9660.

This would also solve the issue that ISO image with MBR would
be treat as MBR device instead of CD/DVD. That would make the
behavior of the image boot different:
If the CD/DVD's MBR be handled correctly, it would be enumerated
as a bootable device with MBR path and FAT filesystem. Some Linux
Distributions boot from such path (FAT with MBR path for ISO) would
come into the grub console instead of the installation selection.
With this change, the CD/DVD would always be enumerated with CD path.
And it would always boot to the installation selection.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Gary Lin <glin@suse.com>
Cc: Andrew Fish <afish@apple.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
index 6a43c3cafb..473e091320 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
@@ -35,11 +35,19 @@ EFI_DRIVER_BINDING_PROTOCOL gPartitionDriverBinding = {
 
 //
 // Prioritized function list to detect partition table.
+// Refer to UEFI Spec 13.3.2 Partition Discovery, the block device
+// should be scanned in below order:
+// 1. GPT
+// 2. ISO 9660 (El Torito) (or UDF)
+// 3. MBR
+// 4. no partiton found
+// Note: UDF is using a same method as booting from CD-ROM, so put it along
+//        with CD-ROM check.
 //
 PARTITION_DETECT_ROUTINE mPartitionDetectRoutineTable[] = {
   PartitionInstallGptChildHandles,
-  PartitionInstallMbrChildHandles,
   PartitionInstallUdfChildHandles,
+  PartitionInstallMbrChildHandles,
   NULL
 };
 
-- 
2.21.0.windows.1

Re: [edk2-devel] [PATCH V2 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR

[Ni, Ray]

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: Wednesday, August 12, 2020 9:21 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Gary Lin
> <glin@suse.com>; Andrew Fish <afish@apple.com>
> Subject: [edk2-devel] [PATCH V2 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> 
> Refer to UEFI spec 2.8, Section 13.3.2, a block device should
> be scanned as below order:
> 1. GPT
> 2. ISO 9660 (El Torito) (UDF should aslo be here)
> 3. MBR
> 4. no partition found
> Note: UDF is using the same boot method as CD, so put it in
> the same priority with ISO 9660.
> 
> This would also solve the issue that ISO image with MBR would
> be treat as MBR device instead of CD/DVD. That would make the
> behavior of the image boot different:
> If the CD/DVD's MBR be handled correctly, it would be enumerated
> as a bootable device with MBR path and FAT filesystem. Some Linux
> Distributions boot from such path (FAT with MBR path for ISO) would
> come into the grub console instead of the installation selection.
> With this change, the CD/DVD would always be enumerated with CD path.
> And it would always boot to the installation selection.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Andrew Fish <afish@apple.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> index 6a43c3cafb..473e091320 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> @@ -35,11 +35,19 @@ EFI_DRIVER_BINDING_PROTOCOL gPartitionDriverBinding = {
> 
>  //
>  // Prioritized function list to detect partition table.
> +// Refer to UEFI Spec 13.3.2 Partition Discovery, the block device
> +// should be scanned in below order:
> +// 1. GPT
> +// 2. ISO 9660 (El Torito) (or UDF)
> +// 3. MBR
> +// 4. no partiton found
> +// Note: UDF is using a same method as booting from CD-ROM, so put it along
> +//        with CD-ROM check.
>  //
>  PARTITION_DETECT_ROUTINE mPartitionDetectRoutineTable[] = {
>    PartitionInstallGptChildHandles,
> -  PartitionInstallMbrChildHandles,
>    PartitionInstallUdfChildHandles,
> +  PartitionInstallMbrChildHandles,
>    NULL
>  };
> 
> --
> 2.21.0.windows.1
> 
> 
> 

Re: [edk2-devel] [PATCH V2 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR

[Wu, Hao A]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao,
> Zhichao
> Sent: Wednesday, August 12, 2020 9:21 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Gary Lin
> <glin@suse.com>; Andrew Fish <afish@apple.com>
> Subject: [edk2-devel] [PATCH V2 1/3] MdeModulePkg/PartitionDxe: Put the
> UDF check ahead of MBR
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> 
> Refer to UEFI spec 2.8, Section 13.3.2, a block device should be scanned as
> below order:
> 1. GPT
> 2. ISO 9660 (El Torito) (UDF should aslo be here) 3. MBR 4. no partition found
> Note: UDF is using the same boot method as CD, so put it in the same priority
> with ISO 9660.
> 
> This would also solve the issue that ISO image with MBR would be treat as
> MBR device instead of CD/DVD. That would make the behavior of the image
> boot different:
> If the CD/DVD's MBR be handled correctly, it would be enumerated as a
> bootable device with MBR path and FAT filesystem. Some Linux Distributions
> boot from such path (FAT with MBR path for ISO) would come into the grub
> console instead of the installation selection.
> With this change, the CD/DVD would always be enumerated with CD path.
> And it would always boot to the installation selection.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Andrew Fish <afish@apple.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> index 6a43c3cafb..473e091320 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> @@ -35,11 +35,19 @@ EFI_DRIVER_BINDING_PROTOCOL
> gPartitionDriverBinding = {
> 
>  //
>  // Prioritized function list to detect partition table.
> +// Refer to UEFI Spec 13.3.2 Partition Discovery, the block device //
> +should be scanned in below order:
> +// 1. GPT
> +// 2. ISO 9660 (El Torito) (or UDF)
> +// 3. MBR
> +// 4. no partiton found
> +// Note: UDF is using a same method as booting from CD-ROM, so put it
> along
> +//        with CD-ROM check.
>  //
>  PARTITION_DETECT_ROUTINE mPartitionDetectRoutineTable[] = {
>    PartitionInstallGptChildHandles,
> -  PartitionInstallMbrChildHandles,
>    PartitionInstallUdfChildHandles,
> +  PartitionInstallMbrChildHandles,


Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


>    NULL
>  };
> 
> --
> 2.21.0.windows.1
> 
> 
> 

[PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert changes for the special MBR

[Gao, Zhichao]

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

Revert "MdeModulePkg/PartitionDxe: Skip the MBR that add for CD-ROM"

Follow the spec definition, the ISO 9660 (and UDF) would be
checked before the MBR. So it is not required to skip such
MBR talbe that contian the entire block device.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Gary Lin <glin@suse.com>
Cc: Andrew Fish <afish@apple.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 .../Universal/Disk/PartitionDxe/Mbr.c         | 37 +++----------------
 1 file changed, 6 insertions(+), 31 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
index 3830af1ea7..f0c92aa09a 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
@@ -39,7 +39,6 @@ PartitionValidMbr (
   UINT32  StartingLBA;
   UINT32  EndingLBA;
   UINT32  NewEndingLBA;
-  UINT32  SizeInLBA;
   INTN    Index1;
   INTN    Index2;
   BOOLEAN MbrValid;
@@ -52,34 +51,13 @@ PartitionValidMbr (
   //
   MbrValid = FALSE;
   for (Index1 = 0; Index1 < MAX_MBR_PARTITIONS; Index1++) {
-    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) {
+    if (Mbr->Partition[Index1].OSIndicator == 0x00 || UNPACK_UINT32 (Mbr->Partition[Index1].SizeInLBA) == 0) {
       continue;
     }
 
     MbrValid    = TRUE;
-    EndingLBA   = StartingLBA + SizeInLBA - 1;
+    StartingLBA = UNPACK_UINT32 (Mbr->Partition[Index1].StartingLBA);
+    EndingLBA   = StartingLBA + UNPACK_UINT32 (Mbr->Partition[Index1].SizeInLBA) - 1;
     if (EndingLBA > LastLba) {
       //
       // Compatibility Errata:
@@ -99,15 +77,12 @@ PartitionValidMbr (
     }
 
     for (Index2 = Index1 + 1; Index2 < MAX_MBR_PARTITIONS; Index2++) {
-      StartingLBA = UNPACK_UINT32 (Mbr->Partition[Index2].StartingLBA);
-      SizeInLBA   = UNPACK_UINT32 (Mbr->Partition[Index2].SizeInLBA);
-
-      if (Mbr->Partition[Index2].OSIndicator == 0x00 || SizeInLBA == 0) {
+      if (Mbr->Partition[Index2].OSIndicator == 0x00 || UNPACK_UINT32 (Mbr->Partition[Index2].SizeInLBA) == 0) {
         continue;
       }
 
-      NewEndingLBA = StartingLBA + SizeInLBA - 1;
-      if (NewEndingLBA >= StartingLBA && StartingLBA <= EndingLBA) {
+      NewEndingLBA = UNPACK_UINT32 (Mbr->Partition[Index2].StartingLBA) + UNPACK_UINT32 (Mbr->Partition[Index2].SizeInLBA) - 1;
+      if (NewEndingLBA >= StartingLBA && UNPACK_UINT32 (Mbr->Partition[Index2].StartingLBA) <= EndingLBA) {
         //
         // This region overlaps with the Index1'th region
         //
-- 
2.21.0.windows.1

Re: [edk2-devel] [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert changes for the special MBR

[Ni, Ray]

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: Wednesday, August 12, 2020 9:21 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Gary Lin
> <glin@suse.com>; Andrew Fish <afish@apple.com>
> Subject: [edk2-devel] [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert changes for the special MBR
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> 
> Revert "MdeModulePkg/PartitionDxe: Skip the MBR that add for CD-ROM"
> 
> Follow the spec definition, the ISO 9660 (and UDF) would be
> checked before the MBR. So it is not required to skip such
> MBR talbe that contian the entire block device.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Andrew Fish <afish@apple.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  .../Universal/Disk/PartitionDxe/Mbr.c         | 37 +++----------------
>  1 file changed, 6 insertions(+), 31 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> index 3830af1ea7..f0c92aa09a 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> @@ -39,7 +39,6 @@ PartitionValidMbr (
>    UINT32  StartingLBA;
>    UINT32  EndingLBA;
>    UINT32  NewEndingLBA;
> -  UINT32  SizeInLBA;
>    INTN    Index1;
>    INTN    Index2;
>    BOOLEAN MbrValid;
> @@ -52,34 +51,13 @@ PartitionValidMbr (
>    //
>    MbrValid = FALSE;
>    for (Index1 = 0; Index1 < MAX_MBR_PARTITIONS; Index1++) {
> -    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) {
> +    if (Mbr->Partition[Index1].OSIndicator == 0x00 || UNPACK_UINT32 (Mbr->Partition[Index1].SizeInLBA) == 0) {
>        continue;
>      }
> 
>      MbrValid    = TRUE;
> -    EndingLBA   = StartingLBA + SizeInLBA - 1;
> +    StartingLBA = UNPACK_UINT32 (Mbr->Partition[Index1].StartingLBA);
> +    EndingLBA   = StartingLBA + UNPACK_UINT32 (Mbr->Partition[Index1].SizeInLBA) - 1;
>      if (EndingLBA > LastLba) {
>        //
>        // Compatibility Errata:
> @@ -99,15 +77,12 @@ PartitionValidMbr (
>      }
> 
>      for (Index2 = Index1 + 1; Index2 < MAX_MBR_PARTITIONS; Index2++) {
> -      StartingLBA = UNPACK_UINT32 (Mbr->Partition[Index2].StartingLBA);
> -      SizeInLBA   = UNPACK_UINT32 (Mbr->Partition[Index2].SizeInLBA);
> -
> -      if (Mbr->Partition[Index2].OSIndicator == 0x00 || SizeInLBA == 0) {
> +      if (Mbr->Partition[Index2].OSIndicator == 0x00 || UNPACK_UINT32 (Mbr->Partition[Index2].SizeInLBA) == 0) {
>          continue;
>        }
> 
> -      NewEndingLBA = StartingLBA + SizeInLBA - 1;
> -      if (NewEndingLBA >= StartingLBA && StartingLBA <= EndingLBA) {
> +      NewEndingLBA = UNPACK_UINT32 (Mbr->Partition[Index2].StartingLBA) + UNPACK_UINT32 (Mbr-
> >Partition[Index2].SizeInLBA) - 1;
> +      if (NewEndingLBA >= StartingLBA && UNPACK_UINT32 (Mbr->Partition[Index2].StartingLBA) <= EndingLBA) {
>          //
>          // This region overlaps with the Index1'th region
>          //
> --
> 2.21.0.windows.1
> 
> 
> 

Re: [edk2-devel] [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert changes for the special MBR

[Wu, Hao A]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao,
> Zhichao
> Sent: Wednesday, August 12, 2020 9:21 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Gary Lin
> <glin@suse.com>; Andrew Fish <afish@apple.com>
> Subject: [edk2-devel] [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert
> changes for the special MBR
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> 
> Revert "MdeModulePkg/PartitionDxe: Skip the MBR that add for CD-ROM"
> 
> Follow the spec definition, the ISO 9660 (and UDF) would be checked before
> the MBR. So it is not required to skip such MBR talbe that contian the entire
> block device.


> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Andrew Fish <afish@apple.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  .../Universal/Disk/PartitionDxe/Mbr.c         | 37 +++----------------
>  1 file changed, 6 insertions(+), 31 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> index 3830af1ea7..f0c92aa09a 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> @@ -39,7 +39,6 @@ PartitionValidMbr (
>    UINT32  StartingLBA;
>    UINT32  EndingLBA;
>    UINT32  NewEndingLBA;
> -  UINT32  SizeInLBA;
>    INTN    Index1;
>    INTN    Index2;
>    BOOLEAN MbrValid;
> @@ -52,34 +51,13 @@ PartitionValidMbr (
>    //
>    MbrValid = FALSE;
>    for (Index1 = 0; Index1 < MAX_MBR_PARTITIONS; Index1++) {
> -    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) {
> +    if (Mbr->Partition[Index1].OSIndicator == 0x00 || UNPACK_UINT32
> + (Mbr->Partition[Index1].SizeInLBA) == 0) {
>        continue;
>      }
> 
>      MbrValid    = TRUE;
> -    EndingLBA   = StartingLBA + SizeInLBA - 1;
> +    StartingLBA = UNPACK_UINT32 (Mbr->Partition[Index1].StartingLBA);
> +    EndingLBA   = StartingLBA + UNPACK_UINT32 (Mbr-
> >Partition[Index1].SizeInLBA) - 1;
>      if (EndingLBA > LastLba) {
>        //
>        // Compatibility Errata:
> @@ -99,15 +77,12 @@ PartitionValidMbr (
>      }
> 
>      for (Index2 = Index1 + 1; Index2 < MAX_MBR_PARTITIONS; Index2++) {
> -      StartingLBA = UNPACK_UINT32 (Mbr->Partition[Index2].StartingLBA);
> -      SizeInLBA   = UNPACK_UINT32 (Mbr->Partition[Index2].SizeInLBA);
> -
> -      if (Mbr->Partition[Index2].OSIndicator == 0x00 || SizeInLBA == 0) {
> +      if (Mbr->Partition[Index2].OSIndicator == 0x00 || UNPACK_UINT32
> + (Mbr->Partition[Index2].SizeInLBA) == 0) {
>          continue;
>        }
> 
> -      NewEndingLBA = StartingLBA + SizeInLBA - 1;
> -      if (NewEndingLBA >= StartingLBA && StartingLBA <= EndingLBA) {
> +      NewEndingLBA = UNPACK_UINT32 (Mbr->Partition[Index2].StartingLBA)
> + UNPACK_UINT32 (Mbr->Partition[Index2].SizeInLBA) - 1;
> +      if (NewEndingLBA >= StartingLBA && UNPACK_UINT32
> + (Mbr->Partition[Index2].StartingLBA) <= EndingLBA) {
>          //
>          // This region overlaps with the Index1'th region
>          //
> --
> 2.21.0.windows.1
> 
> 
> 

Re: [edk2-devel] [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert changes for the special MBR

[Wu, Hao A]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao,
> Zhichao
> Sent: Wednesday, August 12, 2020 9:21 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Gary Lin
> <glin@suse.com>; Andrew Fish <afish@apple.com>
> Subject: [edk2-devel] [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert
> changes for the special MBR
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> 
> Revert "MdeModulePkg/PartitionDxe: Skip the MBR that add for CD-ROM"
> 
> Follow the spec definition, the ISO 9660 (and UDF) would be checked before
> the MBR. So it is not required to skip such MBR talbe that contian the entire
> block device.


Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Andrew Fish <afish@apple.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  .../Universal/Disk/PartitionDxe/Mbr.c         | 37 +++----------------
>  1 file changed, 6 insertions(+), 31 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> index 3830af1ea7..f0c92aa09a 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> @@ -39,7 +39,6 @@ PartitionValidMbr (
>    UINT32  StartingLBA;
>    UINT32  EndingLBA;
>    UINT32  NewEndingLBA;
> -  UINT32  SizeInLBA;
>    INTN    Index1;
>    INTN    Index2;
>    BOOLEAN MbrValid;
> @@ -52,34 +51,13 @@ PartitionValidMbr (
>    //
>    MbrValid = FALSE;
>    for (Index1 = 0; Index1 < MAX_MBR_PARTITIONS; Index1++) {
> -    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) {
> +    if (Mbr->Partition[Index1].OSIndicator == 0x00 || UNPACK_UINT32
> + (Mbr->Partition[Index1].SizeInLBA) == 0) {
>        continue;
>      }
> 
>      MbrValid    = TRUE;
> -    EndingLBA   = StartingLBA + SizeInLBA - 1;
> +    StartingLBA = UNPACK_UINT32 (Mbr->Partition[Index1].StartingLBA);
> +    EndingLBA   = StartingLBA + UNPACK_UINT32 (Mbr-
> >Partition[Index1].SizeInLBA) - 1;
>      if (EndingLBA > LastLba) {
>        //
>        // Compatibility Errata:
> @@ -99,15 +77,12 @@ PartitionValidMbr (
>      }
> 
>      for (Index2 = Index1 + 1; Index2 < MAX_MBR_PARTITIONS; Index2++) {
> -      StartingLBA = UNPACK_UINT32 (Mbr->Partition[Index2].StartingLBA);
> -      SizeInLBA   = UNPACK_UINT32 (Mbr->Partition[Index2].SizeInLBA);
> -
> -      if (Mbr->Partition[Index2].OSIndicator == 0x00 || SizeInLBA == 0) {
> +      if (Mbr->Partition[Index2].OSIndicator == 0x00 || UNPACK_UINT32
> + (Mbr->Partition[Index2].SizeInLBA) == 0) {
>          continue;
>        }
> 
> -      NewEndingLBA = StartingLBA + SizeInLBA - 1;
> -      if (NewEndingLBA >= StartingLBA && StartingLBA <= EndingLBA) {
> +      NewEndingLBA = UNPACK_UINT32 (Mbr->Partition[Index2].StartingLBA)
> + UNPACK_UINT32 (Mbr->Partition[Index2].SizeInLBA) - 1;
> +      if (NewEndingLBA >= StartingLBA && UNPACK_UINT32
> + (Mbr->Partition[Index2].StartingLBA) <= EndingLBA) {
>          //
>          // This region overlaps with the Index1'th region
>          //
> --
> 2.21.0.windows.1
> 
> 
> 

[PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander

[Gao, Zhichao]

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

PartitionInstallChildHandle's parameters Start and End is counted
by the BlockSize, but in the implementation it uses the parent
device's BlockSize to calculate the new Start, End and LastBlock.
It would cause the driver report incorrect block scope and the file
system would fail to be found with right block scope.
So correct it to the right value.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Gary Lin <glin@suse.com>
Cc: Andrew Fish <afish@apple.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
---
 MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
index 473e091320..f10ce7c65b 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
@@ -1149,8 +1149,8 @@ PartitionInstallChildHandle (
 
   Private->Signature        = PARTITION_PRIVATE_DATA_SIGNATURE;
 
-  Private->Start            = MultU64x32 (Start, ParentBlockIo->Media->BlockSize);
-  Private->End              = MultU64x32 (End + 1, ParentBlockIo->Media->BlockSize);
+  Private->Start            = MultU64x32 (Start, BlockSize);
+  Private->End              = MultU64x32 (End + 1, BlockSize);
 
   Private->BlockSize        = BlockSize;
   Private->ParentBlockIo    = ParentBlockIo;
@@ -1187,13 +1187,7 @@ PartitionInstallChildHandle (
 
   Private->Media.IoAlign   = 0;
   Private->Media.LogicalPartition = TRUE;
-  Private->Media.LastBlock = DivU64x32 (
-                               MultU64x32 (
-                                 End - Start + 1,
-                                 ParentBlockIo->Media->BlockSize
-                                 ),
-                                BlockSize
-                               ) - 1;
+  Private->Media.LastBlock = End - Start;
 
   Private->Media.BlockSize = (UINT32) BlockSize;
 
-- 
2.21.0.windows.1

Re: [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander

[Wu, Hao A]

> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Wednesday, August 12, 2020 9:21 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Gary Lin
> <glin@suse.com>; Andrew Fish <afish@apple.com>
> Subject: [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA
> size in child hander
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843
> 
> PartitionInstallChildHandle's parameters Start and End is counted by the
> BlockSize, but in the implementation it uses the parent device's BlockSize to
> calculate the new Start, End and LastBlock.
> It would cause the driver report incorrect block scope and the file system
> would fail to be found with right block scope.
> So correct it to the right value.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Andrew Fish <afish@apple.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> index 473e091320..f10ce7c65b 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle (
> 
>    Private->Signature        = PARTITION_PRIVATE_DATA_SIGNATURE;
> 
> -  Private->Start            = MultU64x32 (Start, ParentBlockIo->Media-
> >BlockSize);
> -  Private->End              = MultU64x32 (End + 1, ParentBlockIo->Media-
> >BlockSize);
> +  Private->Start            = MultU64x32 (Start, BlockSize);
> +  Private->End              = MultU64x32 (End + 1, BlockSize);


For function PartitionInstallChildHandle(), the input parameters 'Start' and
'End' (which denote the block index) should be of unit indicated by the input
parameter 'BlockSize'.

I checked all the callers of PartitionInstallChildHandle(), it looks to me that
GPT, MBR and UDF follow the above rule. While I am not able to get a straight-
forward match for the El Torito case. It seems fine with me, but I am not sure.
Anyway, if the CDROM content can be properly identified then I think the change
will be good.

Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> 
>    Private->BlockSize        = BlockSize;
>    Private->ParentBlockIo    = ParentBlockIo;
> @@ -1187,13 +1187,7 @@ PartitionInstallChildHandle (
> 
>    Private->Media.IoAlign   = 0;
>    Private->Media.LogicalPartition = TRUE;
> -  Private->Media.LastBlock = DivU64x32 (
> -                               MultU64x32 (
> -                                 End - Start + 1,
> -                                 ParentBlockIo->Media->BlockSize
> -                                 ),
> -                                BlockSize
> -                               ) - 1;
> +  Private->Media.LastBlock = End - Start;
> 
>    Private->Media.BlockSize = (UINT32) BlockSize;
> 
> --
> 2.21.0.windows.1

Re: [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander

[Gao, Zhichao]

For additional info,

I have test patch #3 without #1 and #2, and revert "MdeModulePkg/PartitionDxe: Skip the MBR that add for CD-ROM". The CD's FAT can be observed with MBR path and I can load the EFI application successfully. But the behavior is different as I mention in patch #1 commit message. Test with open SUSE 15.2, RedHat 8.2, Ubuntu 18.04 and Fedora 20.
Only Ubuntu can enter the installation selection but it output a "error: unknown filesystem".
Anyway, CDROM's MBR can be checked and handled correctly.

Thanks,
Zhichao

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Wednesday, August 12, 2020 2:35 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; Gary Lin
> <glin@suse.com>; Andrew Fish <afish@apple.com>
> Subject: RE: [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA
> size in child hander
> 
> > -----Original Message-----
> > From: Gao, Zhichao <zhichao.gao@intel.com>
> > Sent: Wednesday, August 12, 2020 9:21 AM
> > To: devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Gary Lin
> > <glin@suse.com>; Andrew Fish <afish@apple.com>
> > Subject: [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect
> > LBA size in child hander
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843
> >
> > PartitionInstallChildHandle's parameters Start and End is counted by
> > the BlockSize, but in the implementation it uses the parent device's
> > BlockSize to calculate the new Start, End and LastBlock.
> > It would cause the driver report incorrect block scope and the file
> > system would fail to be found with right block scope.
> > So correct it to the right value.
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Gary Lin <glin@suse.com>
> > Cc: Andrew Fish <afish@apple.com>
> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > Reviewed-by: Ray Ni <ray.ni@intel.com>
> > ---
> >  MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12
> > +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> >
> > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > index 473e091320..f10ce7c65b 100644
> > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> > @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle (
> >
> >    Private->Signature        = PARTITION_PRIVATE_DATA_SIGNATURE;
> >
> > -  Private->Start            = MultU64x32 (Start, ParentBlockIo->Media-
> > >BlockSize);
> > -  Private->End              = MultU64x32 (End + 1, ParentBlockIo->Media-
> > >BlockSize);
> > +  Private->Start            = MultU64x32 (Start, BlockSize);
> > +  Private->End              = MultU64x32 (End + 1, BlockSize);
> 
> 
> For function PartitionInstallChildHandle(), the input parameters 'Start' and 'End'
> (which denote the block index) should be of unit indicated by the input parameter
> 'BlockSize'.
> 
> I checked all the callers of PartitionInstallChildHandle(), it looks to me that GPT,
> MBR and UDF follow the above rule. While I am not able to get a straight-
> forward match for the El Torito case. It seems fine with me, but I am not sure.
> Anyway, if the CDROM content can be properly identified then I think the change
> will be good.
> 
> Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> >    Private->BlockSize        = BlockSize;
> >    Private->ParentBlockIo    = ParentBlockIo;
> > @@ -1187,13 +1187,7 @@ PartitionInstallChildHandle (
> >
> >    Private->Media.IoAlign   = 0;
> >    Private->Media.LogicalPartition = TRUE;
> > -  Private->Media.LastBlock = DivU64x32 (
> > -                               MultU64x32 (
> > -                                 End - Start + 1,
> > -                                 ParentBlockIo->Media->BlockSize
> > -                                 ),
> > -                                BlockSize
> > -                               ) - 1;
> > +  Private->Media.LastBlock = End - Start;
> >
> >    Private->Media.BlockSize = (UINT32) BlockSize;
> >
> > --
> > 2.21.0.windows.1

Re: [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec

[Gary Lin]

On Wed, Aug 12, 2020 at 09:21:21AM +0800, Zhichao Gao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843
> 
> Refer to the UEFI spec 2.8, Section 13.3.2:
> A block device should be scanned as below order:
> 1. GPT
> 2. ISO 9660 (El Torito) (UDF should aslo be here)
> 4. MBR
> 5. no partition found
> 
> But the code implementation is:
> 1. GPT
> 2. MBR
> 3. ISO 9660 (El Torito) (UDF)
> 4. no partition found
> 
> Which would cause the ISO 9960 image with MBR info be treated as MBR
> device. That would cause unexpect behavior. So fix it to follow the spec
> description.
> 
> The fix of the PartitionInstallChildHandle would change the boot behavior
> of Linux ISO image with MBR table. So add it after the order adjustment
> to make no impact of the boot.
> 
> V2:
> 1. Add description of the different behavior between grub boot from MBR path and
> from CD path
> 2. change patch #2 to revert "MdeModulePkg/PartitionDxe: Skip the MBR that add for CD-ROM"

I've tested the following ISO images and all booted as expected.

openSUSE Leap 15.2
SLES 15-SP2
Fedora 32
Ubuntu 20.04

Tested-by: Gary Lin <glin@suse.com>

> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Andrew Fish <afish@apple.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> 
> Zhichao Gao (3):
>   MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR
>   MdeModulePkg/PartitionDxe: Revert changes for the special MBR
>   MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander
> 
>  .../Universal/Disk/PartitionDxe/Mbr.c         | 37 +++----------------
>  .../Universal/Disk/PartitionDxe/Partition.c   | 22 ++++++-----
>  2 files changed, 18 insertions(+), 41 deletions(-)
> 
> -- 
> 2.21.0.windows.1
> 

Re: [edk2-devel] [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec

[Laszlo Ersek]

On 08/12/20 08:26, Gary Lin wrote:
> On Wed, Aug 12, 2020 at 09:21:21AM +0800, Zhichao Gao wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843
>>
>> Refer to the UEFI spec 2.8, Section 13.3.2:
>> A block device should be scanned as below order:
>> 1. GPT
>> 2. ISO 9660 (El Torito) (UDF should aslo be here)
>> 4. MBR
>> 5. no partition found
>>
>> But the code implementation is:
>> 1. GPT
>> 2. MBR
>> 3. ISO 9660 (El Torito) (UDF)
>> 4. no partition found
>>
>> Which would cause the ISO 9960 image with MBR info be treated as MBR
>> device. That would cause unexpect behavior. So fix it to follow the spec
>> description.
>>
>> The fix of the PartitionInstallChildHandle would change the boot behavior
>> of Linux ISO image with MBR table. So add it after the order adjustment
>> to make no impact of the boot.
>>
>> V2:
>> 1. Add description of the different behavior between grub boot from MBR path and
>> from CD path
>> 2. change patch #2 to revert "MdeModulePkg/PartitionDxe: Skip the MBR that add for CD-ROM"
> 
> I've tested the following ISO images and all booted as expected.
> 
> openSUSE Leap 15.2
> SLES 15-SP2
> Fedora 32
> Ubuntu 20.04
> 
> Tested-by: Gary Lin <glin@suse.com>

Thank you very much for the extensive testing!
Laszlo

> 
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Hao A Wu <hao.a.wu@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Gary Lin <glin@suse.com>
>> Cc: Andrew Fish <afish@apple.com>
>> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
>>
>> Zhichao Gao (3):
>>   MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR
>>   MdeModulePkg/PartitionDxe: Revert changes for the special MBR
>>   MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander
>>
>>  .../Universal/Disk/PartitionDxe/Mbr.c         | 37 +++----------------
>>  .../Universal/Disk/PartitionDxe/Partition.c   | 22 ++++++-----
>>  2 files changed, 18 insertions(+), 41 deletions(-)
>>
>> -- 
>> 2.21.0.windows.1
>>
> 
> 
> 
>