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

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

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: Remove the check for special MBR
  MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander

 .../Universal/Disk/PartitionDxe/Mbr.c         | 19 ----------------
 .../Universal/Disk/PartitionDxe/Partition.c   | 22 ++++++++++---------
 2 files changed, 12 insertions(+), 29 deletions(-)

-- 
2.21.0.windows.1

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

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: [PATCH 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR

[Ni, Ray]

Zhichao,
Can you also add notes in the commit message describing that for some ISOs (better with more specific ISO info), the MBR information is not correct?

Thanks,
Ray


> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Tuesday, August 11, 2020 2:43 PM
> 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 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.
> 
> 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: [PATCH 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR

[Gao, Zhichao]

Ray,

The MBR info is correct. The order change is to avoid the MBR being checked before UDF/ISO 9660 check.
That is why I make the patch #3 in the last of the patch set.

Thanks,
Zhichao

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Tuesday, August 11, 2020 4:04 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Gary Lin <glin@suse.com>; Andrew Fish <afish@apple.com>
> Subject: RE: [PATCH 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead
> of MBR
> 
> Zhichao,
> Can you also add notes in the commit message describing that for some ISOs
> (better with more specific ISO info), the MBR information is not correct?
> 
> Thanks,
> Ray
> 
> 
> > -----Original Message-----
> > From: Gao, Zhichao <zhichao.gao@intel.com>
> > Sent: Tuesday, August 11, 2020 2:43 PM
> > 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 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.
> >
> > 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: [PATCH 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR

[Ni, Ray]

> > 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.

Can you please explain this in detail?
It's ok to not provide the "root" cause of why the image boot behavior is different.
Saying the specific issue can help people to understand the issue in future.

> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Tuesday, August 11, 2020 4:34 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Gary Lin <glin@suse.com>; Andrew Fish <afish@apple.com>
> Subject: RE: [PATCH 1/3] MdeModulePkg/PartitionDxe: Put the UDF check
> ahead of MBR
> 
> Ray,
> 
> The MBR info is correct. The order change is to avoid the MBR being checked
> before UDF/ISO 9660 check.
> That is why I make the patch #3 in the last of the patch set.
> 
> Thanks,
> Zhichao
> 
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Tuesday, August 11, 2020 4:04 PM
> > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> > Gary Lin <glin@suse.com>; Andrew Fish <afish@apple.com>
> > Subject: RE: [PATCH 1/3] MdeModulePkg/PartitionDxe: Put the UDF check
> ahead
> > of MBR
> >
> > Zhichao,
> > Can you also add notes in the commit message describing that for some ISOs
> > (better with more specific ISO info), the MBR information is not correct?
> >
> > Thanks,
> > Ray
> >
> >
> > > -----Original Message-----
> > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > Sent: Tuesday, August 11, 2020 2:43 PM
> > > 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 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.
> > >
> > > 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: [PATCH 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR

[Gao, Zhichao]

OK. I would put the detail in next version patch. Let me put a sample here:

With patch #3 but without patch #1, the MBR table of ISO 9660 image can be handled correctly, i.e. it would be treat as MBR block device. We can find the bootable image thru MBR path FAT filesystem. When boot Linux Distribution, it comes into the grub console instead of the installation selection.

Thanks,
Zhichao

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Tuesday, August 11, 2020 9:49 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Gary Lin <glin@suse.com>; Andrew Fish <afish@apple.com>
> Subject: RE: [PATCH 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead
> of MBR
> 
> > > 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.
> 
> Can you please explain this in detail?
> It's ok to not provide the "root" cause of why the image boot behavior is different.
> Saying the specific issue can help people to understand the issue in future.
> 
> > -----Original Message-----
> > From: Gao, Zhichao <zhichao.gao@intel.com>
> > Sent: Tuesday, August 11, 2020 4:34 PM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Gary Lin <glin@suse.com>; Andrew Fish
> > <afish@apple.com>
> > Subject: RE: [PATCH 1/3] MdeModulePkg/PartitionDxe: Put the UDF check
> > ahead of MBR
> >
> > Ray,
> >
> > The MBR info is correct. The order change is to avoid the MBR being
> > checked before UDF/ISO 9660 check.
> > That is why I make the patch #3 in the last of the patch set.
> >
> > Thanks,
> > Zhichao
> >
> > > -----Original Message-----
> > > From: Ni, Ray <ray.ni@intel.com>
> > > Sent: Tuesday, August 11, 2020 4:04 PM
> > > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>; Gary Lin <glin@suse.com>; Andrew Fish
> > > <afish@apple.com>
> > > Subject: RE: [PATCH 1/3] MdeModulePkg/PartitionDxe: Put the UDF
> > > check
> > ahead
> > > of MBR
> > >
> > > Zhichao,
> > > Can you also add notes in the commit message describing that for
> > > some ISOs (better with more specific ISO info), the MBR information is not
> correct?
> > >
> > > Thanks,
> > > Ray
> > >
> > >
> > > > -----Original Message-----
> > > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > > Sent: Tuesday, August 11, 2020 2:43 PM
> > > > 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 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.
> > > >
> > > > 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 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR

[Ni, Ray]

Thanks. Good enough!

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao, Zhichao
> Sent: Wednesday, August 12, 2020 8:45 AM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Gary Lin <glin@suse.com>; Andrew Fish
> <afish@apple.com>
> Subject: Re: [edk2-devel] [PATCH 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR
> 
> OK. I would put the detail in next version patch. Let me put a sample here:
> 
> With patch #3 but without patch #1, the MBR table of ISO 9660 image can be handled correctly, i.e. it would be treat as
> MBR block device. We can find the bootable image thru MBR path FAT filesystem. When boot Linux Distribution, it comes
> into the grub console instead of the installation selection.
> 
> Thanks,
> Zhichao
> 
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Tuesday, August 11, 2020 9:49 PM
> > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> > Gary Lin <glin@suse.com>; Andrew Fish <afish@apple.com>
> > Subject: RE: [PATCH 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead
> > of MBR
> >
> > > > 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.
> >
> > Can you please explain this in detail?
> > It's ok to not provide the "root" cause of why the image boot behavior is different.
> > Saying the specific issue can help people to understand the issue in future.
> >
> > > -----Original Message-----
> > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > Sent: Tuesday, August 11, 2020 4:34 PM
> > > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>; Gary Lin <glin@suse.com>; Andrew Fish
> > > <afish@apple.com>
> > > Subject: RE: [PATCH 1/3] MdeModulePkg/PartitionDxe: Put the UDF check
> > > ahead of MBR
> > >
> > > Ray,
> > >
> > > The MBR info is correct. The order change is to avoid the MBR being
> > > checked before UDF/ISO 9660 check.
> > > That is why I make the patch #3 in the last of the patch set.
> > >
> > > Thanks,
> > > Zhichao
> > >
> > > > -----Original Message-----
> > > > From: Ni, Ray <ray.ni@intel.com>
> > > > Sent: Tuesday, August 11, 2020 4:04 PM
> > > > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > > <hao.a.wu@intel.com>; Gary Lin <glin@suse.com>; Andrew Fish
> > > > <afish@apple.com>
> > > > Subject: RE: [PATCH 1/3] MdeModulePkg/PartitionDxe: Put the UDF
> > > > check
> > > ahead
> > > > of MBR
> > > >
> > > > Zhichao,
> > > > Can you also add notes in the commit message describing that for
> > > > some ISOs (better with more specific ISO info), the MBR information is not
> > correct?
> > > >
> > > > Thanks,
> > > > Ray
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > > > Sent: Tuesday, August 11, 2020 2:43 PM
> > > > > 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 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.
> > > > >
> > > > > 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
> > > >
> > >
> >
> 
> 
> 

[PATCH 2/3] MdeModulePkg/PartitionDxe: Remove the check for special MBR

[Gao, Zhichao]

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

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         | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
index 3830af1ea7..822bf03e92 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
@@ -55,25 +55,6 @@ PartitionValidMbr (
     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;
     }
-- 
2.21.0.windows.1

Re: [PATCH 2/3] MdeModulePkg/PartitionDxe: Remove the check for special MBR

[Ni, Ray]

Zhichao,
Can you please just revert the fix you recently added?

> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Tuesday, August 11, 2020 2:43 PM
> 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 2/3] MdeModulePkg/PartitionDxe: Remove the check for special MBR
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> 
> 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         | 19 -------------------
>  1 file changed, 19 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> index 3830af1ea7..822bf03e92 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> @@ -55,25 +55,6 @@ PartitionValidMbr (
>      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;
>      }
> --
> 2.21.0.windows.1

Re: [edk2-devel] [PATCH 2/3] MdeModulePkg/PartitionDxe: Remove the check for special MBR

[Gao, Zhichao]

I also add some variables to calculate StartingLBA and SizeInLBA instead of calculate them when they are needed.
I am fine to revert the whole changes. Just make you aware of this.

Thanks,
Zhichao

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Tuesday, August 11, 2020 4:06 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Gary Lin <glin@suse.com>; Andrew Fish <afish@apple.com>
> Subject: Re: [edk2-devel] [PATCH 2/3] MdeModulePkg/PartitionDxe: Remove
> the check for special MBR
> 
> Zhichao,
> Can you please just revert the fix you recently added?
> 
> > -----Original Message-----
> > From: Gao, Zhichao <zhichao.gao@intel.com>
> > Sent: Tuesday, August 11, 2020 2:43 PM
> > 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 2/3] MdeModulePkg/PartitionDxe: Remove the check for
> > special MBR
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> >
> > 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         | 19 -------------------
> >  1 file changed, 19 deletions(-)
> >
> > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> > b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> > index 3830af1ea7..822bf03e92 100644
> > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> > @@ -55,25 +55,6 @@ PartitionValidMbr (
> >      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;
> >      }
> > --
> > 2.21.0.windows.1
> 
> 
> 

Re: [edk2-devel] [PATCH 2/3] MdeModulePkg/PartitionDxe: Remove the check for special MBR

[Ni, Ray]

I prefer to directly revert the patch. It simplifies the change history.

> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Tuesday, August 11, 2020 4:29 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Gary Lin <glin@suse.com>; Andrew Fish <afish@apple.com>
> Subject: RE: [edk2-devel] [PATCH 2/3] MdeModulePkg/PartitionDxe: Remove
> the check for special MBR
> 
> I also add some variables to calculate StartingLBA and SizeInLBA instead of
> calculate them when they are needed.
> I am fine to revert the whole changes. Just make you aware of this.
> 
> Thanks,
> Zhichao
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> > Sent: Tuesday, August 11, 2020 4:06 PM
> > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> > Gary Lin <glin@suse.com>; Andrew Fish <afish@apple.com>
> > Subject: Re: [edk2-devel] [PATCH 2/3] MdeModulePkg/PartitionDxe: Remove
> > the check for special MBR
> >
> > Zhichao,
> > Can you please just revert the fix you recently added?
> >
> > > -----Original Message-----
> > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > Sent: Tuesday, August 11, 2020 2:43 PM
> > > 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 2/3] MdeModulePkg/PartitionDxe: Remove the check for
> > > special MBR
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
> > >
> > > 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         | 19 -------------------
> > >  1 file changed, 19 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> > > b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> > > index 3830af1ea7..822bf03e92 100644
> > > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> > > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
> > > @@ -55,25 +55,6 @@ PartitionValidMbr (
> > >      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;
> > >      }
> > > --
> > > 2.21.0.windows.1
> >
> >
> > 
> 

[PATCH 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>
---
 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 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander

[Ni, Ray]

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

> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Tuesday, August 11, 2020 2:43 PM
> 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 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>
> ---
>  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 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec

[Gary Lin]

On Tue, Aug 11, 2020 at 02:42:59PM +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.
> 
Thanks for the patches. After applying this patch series, the firmware
recognizes openSUSE/SUSE iso images again.

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: Remove the check for special MBR
>   MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander
> 
>  .../Universal/Disk/PartitionDxe/Mbr.c         | 19 ----------------
>  .../Universal/Disk/PartitionDxe/Partition.c   | 22 ++++++++++---------
>  2 files changed, 12 insertions(+), 29 deletions(-)
> 
> -- 
> 2.21.0.windows.1
>