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

[PATCH V2 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.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.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         | 42 +++++++++++++++----
 .../Universal/Disk/PartitionDxe/Partition.c   |  9 ++++
 2 files changed, 44 insertions(+), 7 deletions(-)

-- 
2.21.0.windows.1

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

[Gao, Zhichao]

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

The MBR last block value should be sector (512 bytes) numbers.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
index dac451a144..aa0b6cadcc 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
@@ -137,12 +137,14 @@ PartitionInstallMbrChildHandles (
   UINT32                       MediaId;
   EFI_LBA                      LastBlock;
   EFI_PARTITION_INFO_PROTOCOL  PartitionInfo;
+  UINT64                       MediaSize;
 
   Found           = EFI_NOT_FOUND;
 
   BlockSize = BlockIo->Media->BlockSize;
   MediaId   = BlockIo->Media->MediaId;
-  LastBlock = BlockIo->Media->LastBlock;
+  MediaSize = MultU64x32 (BlockIo->Media->LastBlock + 1, BlockSize);
+  LastBlock = DivU64x32 (MediaSize, 512) - 1;
 
   //
   // Ensure the block size can hold the MBR
-- 
2.21.0.windows.1

[PATCH V2 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>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 .../Universal/Disk/PartitionDxe/Mbr.c         | 38 ++++++++++++++++---
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
index aa0b6cadcc..b5cb56842a 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,35 @@ 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 contains itself, i.e. start with LBA0,
+    // and have the same size with the media, we treat it as a El Torito partition.
+    //
+    if ((StartingLBA == 0) &&
+        (SizeInLBA != 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: The MBR table has partition entry start at sector 0.\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 +100,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 V2 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>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Zhichao Gao <zhichao.gao@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: [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Fix the partition check issue

[Laszlo Ersek]

Hi Zhichao,

On 07/08/20 04:27, Zhichao Gao wrote:
> 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.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.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         | 42 +++++++++++++++----
>  .../Universal/Disk/PartitionDxe/Partition.c   |  9 ++++
>  2 files changed, 44 insertions(+), 7 deletions(-)
> 

I've skimmed the patches briefly -- I've realized that I don't know
enough to usefully review them. I'll defer to Hao and Ray.

Thanks
Laszlo

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

[Wu, Hao A]

> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Wednesday, July 8, 2020 10:27 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> Ersek <lersek@redhat.com>
> Subject: [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Fix the partition check
> issue
> 
> 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.


Hello Zhichao,

The changes look good to me.
May I know what tests have been done for the series? Thanks.

Best Regards,
Hao Wu


> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.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         | 42 +++++++++++++++----
>  .../Universal/Disk/PartitionDxe/Partition.c   |  9 ++++
>  2 files changed, 44 insertions(+), 7 deletions(-)
> 
> --
> 2.21.0.windows.1

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

[Gao, Zhichao]

I have tested on the OVMF with linux iso image like Ubuntu, Fedora and RedHat.

It can enumerate under the shell and load the grub efi application.

Thanks,
Zhichao

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Thursday, July 9, 2020 1:04 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Fix the partition check
> issue
> 
> > -----Original Message-----
> > From: Gao, Zhichao <zhichao.gao@intel.com>
> > Sent: Wednesday, July 8, 2020 10:27 AM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> > Ersek <lersek@redhat.com>
> > Subject: [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Fix the partition
> > check issue
> >
> > 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.
> 
> 
> Hello Zhichao,
> 
> The changes look good to me.
> May I know what tests have been done for the series? Thanks.
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.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         | 42 +++++++++++++++----
> >  .../Universal/Disk/PartitionDxe/Partition.c   |  9 ++++
> >  2 files changed, 44 insertions(+), 7 deletions(-)
> >
> > --
> > 2.21.0.windows.1

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

[Wu, Hao A]

> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Friday, July 10, 2020 8:41 AM
> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Fix the partition
> check issue
> 
> I have tested on the OVMF with linux iso image like Ubuntu, Fedora and
> RedHat.
> 
> It can enumerate under the shell and load the grub efi application.


Thanks Zhichao,

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

Best Regards,
Hao Wu


> 
> Thanks,
> Zhichao
> 
> > -----Original Message-----
> > From: Wu, Hao A <hao.a.wu@intel.com>
> > Sent: Thursday, July 9, 2020 1:04 PM
> > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > Subject: RE: [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Fix the
> > partition check issue
> >
> > > -----Original Message-----
> > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > Sent: Wednesday, July 8, 2020 10:27 AM
> > > To: devel@edk2.groups.io
> > > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > > Laszlo Ersek <lersek@redhat.com>
> > > Subject: [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Fix the partition
> > > check issue
> > >
> > > 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.
> >
> >
> > Hello Zhichao,
> >
> > The changes look good to me.
> > May I know what tests have been done for the series? Thanks.
> >
> > Best Regards,
> > Hao Wu
> >
> >
> > >
> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.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         | 42 +++++++++++++++----
> > >  .../Universal/Disk/PartitionDxe/Partition.c   |  9 ++++
> > >  2 files changed, 44 insertions(+), 7 deletions(-)
> > >
> > > --
> > > 2.21.0.windows.1

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

[Ni, Ray]

1. Can you rename LastBlock to LastSector and remove the MediaSize local variable?
2. Can you add comments to describe that sector size is 512?
3. Can you explain why this fix is needed in the commit message?

Thanks,
Ray



> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Wednesday, July 8, 2020 10:27 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: [PATCH V2 1/3] MdeModulePkg/PartitionDxe: Correct the MBR last
> block value
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> 
> The MBR last block value should be sector (512 bytes) numbers.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> index dac451a144..aa0b6cadcc 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> @@ -137,12 +137,14 @@ PartitionInstallMbrChildHandles (
>    UINT32                       MediaId;
>    EFI_LBA                      LastBlock;
>    EFI_PARTITION_INFO_PROTOCOL  PartitionInfo;
> +  UINT64                       MediaSize;
> 
>    Found           = EFI_NOT_FOUND;
> 
>    BlockSize = BlockIo->Media->BlockSize;
>    MediaId   = BlockIo->Media->MediaId;
> -  LastBlock = BlockIo->Media->LastBlock;
> +  MediaSize = MultU64x32 (BlockIo->Media->LastBlock + 1, BlockSize);
> +  LastBlock = DivU64x32 (MediaSize, 512) - 1;
> 
>    //
>    // Ensure the block size can hold the MBR
> --
> 2.21.0.windows.1

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

[Ni, Ray]

> +    //
> +    // If the MBR with partition entry contains itself, i.e. start with LBA0,
> +    // and have the same size with the media, we treat it as a El Torito partition.
> +    //
> +    if ((StartingLBA == 0) &&
> +        (SizeInLBA != 0) &&
1. Can this check "SizeInLBA != 0" be removed?

> +      DEBUG ((DEBUG_INFO, "PartitionValidMbr: The MBR table has partition
> entry start at sector 0.\n"));
2. Can the debug message be refined as below?
" PartitionValidMbr: MBR table has partition entry covering the ENTIRE disk. Don't treat it as a valid MBR."

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

[Gao, Zhichao]

Sure. I would follow the 3 suggestion in next version patch set.

Thanks,
Zhichao

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Monday, July 13, 2020 1:59 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [PATCH V2 1/3] MdeModulePkg/PartitionDxe: Correct the MBR last
> block value
> 
> 1. Can you rename LastBlock to LastSector and remove the MediaSize local
> variable?
> 2. Can you add comments to describe that sector size is 512?
> 3. Can you explain why this fix is needed in the commit message?
> 
> Thanks,
> Ray
> 
> 
> 
> > -----Original Message-----
> > From: Gao, Zhichao <zhichao.gao@intel.com>
> > Sent: Wednesday, July 8, 2020 10:27 AM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> > Ersek <lersek@redhat.com>
> > Subject: [PATCH V2 1/3] MdeModulePkg/PartitionDxe: Correct the MBR
> > last block value
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> >
> > The MBR last block value should be sector (512 bytes) numbers.
> >
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > ---
> >  MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> > b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> > index dac451a144..aa0b6cadcc 100644
> > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> > @@ -137,12 +137,14 @@ PartitionInstallMbrChildHandles (
> >    UINT32                       MediaId;
> >    EFI_LBA                      LastBlock;
> >    EFI_PARTITION_INFO_PROTOCOL  PartitionInfo;
> > +  UINT64                       MediaSize;
> >
> >    Found           = EFI_NOT_FOUND;
> >
> >    BlockSize = BlockIo->Media->BlockSize;
> >    MediaId   = BlockIo->Media->MediaId;
> > -  LastBlock = BlockIo->Media->LastBlock;
> > +  MediaSize = MultU64x32 (BlockIo->Media->LastBlock + 1, BlockSize);
> > + LastBlock = DivU64x32 (MediaSize, 512) - 1;
> >
> >    //
> >    // Ensure the block size can hold the MBR
> > --
> > 2.21.0.windows.1
> 

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

[Gao, Zhichao]

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Monday, July 13, 2020 2:45 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Skip the MBR that add
> for CD-ROM
> 
> > +    //
> > +    // If the MBR with partition entry contains itself, i.e. start with LBA0,
> > +    // and have the same size with the media, we treat it as a El Torito partition.
> > +    //
> > +    if ((StartingLBA == 0) &&
> > +        (SizeInLBA != 0) &&
> 1. Can this check "SizeInLBA != 0" be removed?

Yes. I don't think the LastLBA would be the max UINT64 value. So we can remove this condition.

> 
> > +      DEBUG ((DEBUG_INFO, "PartitionValidMbr: The MBR table has partition
> > entry start at sector 0.\n"));
> 2. Can the debug message be refined as below?
> " PartitionValidMbr: MBR table has partition entry covering the ENTIRE disk.
> Don't treat it as a valid MBR."

Sure. Your suggestion make the change more clear. I would change this in the comment of the section as well.

Thanks,
Zhichao