* [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec
@ 2020-08-12 1:21 Gao, Zhichao
2020-08-12 1:21 ` [PATCH V2 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR Gao, Zhichao
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Gao, Zhichao @ 2020-08-12 1:21 UTC (permalink / raw)
To: devel; +Cc: Jian J Wang, Hao A Wu, Ray Ni, Gary Lin, Andrew Fish
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V2 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR
2020-08-12 1:21 [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec Gao, Zhichao
@ 2020-08-12 1:21 ` Gao, Zhichao
2020-08-12 5:22 ` [edk2-devel] " Ni, Ray
2020-08-12 6:37 ` Wu, Hao A
2020-08-12 1:21 ` [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert changes for the special MBR Gao, Zhichao
` (2 subsequent siblings)
3 siblings, 2 replies; 13+ messages in thread
From: Gao, Zhichao @ 2020-08-12 1:21 UTC (permalink / raw)
To: devel; +Cc: Jian J Wang, Hao A Wu, Ray Ni, Gary Lin, Andrew Fish
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert changes for the special MBR
2020-08-12 1:21 [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec Gao, Zhichao
2020-08-12 1:21 ` [PATCH V2 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR Gao, Zhichao
@ 2020-08-12 1:21 ` Gao, Zhichao
2020-08-12 5:23 ` [edk2-devel] " Ni, Ray
` (2 more replies)
2020-08-12 1:21 ` [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander Gao, Zhichao
2020-08-12 6:26 ` [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec Gary Lin
3 siblings, 3 replies; 13+ messages in thread
From: Gao, Zhichao @ 2020-08-12 1:21 UTC (permalink / raw)
To: devel; +Cc: Jian J Wang, Hao A Wu, Ray Ni, Gary Lin, Andrew Fish
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander
2020-08-12 1:21 [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec Gao, Zhichao
2020-08-12 1:21 ` [PATCH V2 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR Gao, Zhichao
2020-08-12 1:21 ` [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert changes for the special MBR Gao, Zhichao
@ 2020-08-12 1:21 ` Gao, Zhichao
2020-08-12 6:35 ` Wu, Hao A
2020-08-12 6:26 ` [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec Gary Lin
3 siblings, 1 reply; 13+ messages in thread
From: Gao, Zhichao @ 2020-08-12 1:21 UTC (permalink / raw)
To: devel; +Cc: Jian J Wang, Hao A Wu, Ray Ni, Gary Lin, Andrew Fish
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR
2020-08-12 1:21 ` [PATCH V2 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR Gao, Zhichao
@ 2020-08-12 5:22 ` Ni, Ray
2020-08-12 6:37 ` Wu, Hao A
1 sibling, 0 replies; 13+ messages in thread
From: Ni, Ray @ 2020-08-12 5:22 UTC (permalink / raw)
To: devel@edk2.groups.io, Gao, Zhichao
Cc: Wang, Jian J, Wu, Hao A, Gary Lin, Andrew Fish
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
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert changes for the special MBR
2020-08-12 1:21 ` [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert changes for the special MBR Gao, Zhichao
@ 2020-08-12 5:23 ` Ni, Ray
2020-08-12 6:38 ` Wu, Hao A
2020-08-12 6:38 ` Wu, Hao A
2 siblings, 0 replies; 13+ messages in thread
From: Ni, Ray @ 2020-08-12 5:23 UTC (permalink / raw)
To: devel@edk2.groups.io, Gao, Zhichao
Cc: Wang, Jian J, Wu, Hao A, Gary Lin, Andrew Fish
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
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec
2020-08-12 1:21 [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec Gao, Zhichao
` (2 preceding siblings ...)
2020-08-12 1:21 ` [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander Gao, Zhichao
@ 2020-08-12 6:26 ` Gary Lin
2020-08-12 11:04 ` [edk2-devel] " Laszlo Ersek
3 siblings, 1 reply; 13+ messages in thread
From: Gary Lin @ 2020-08-12 6:26 UTC (permalink / raw)
To: Zhichao Gao; +Cc: devel, Jian J Wang, Hao A Wu, Ray Ni, Andrew Fish
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander
2020-08-12 1:21 ` [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander Gao, Zhichao
@ 2020-08-12 6:35 ` Wu, Hao A
2020-08-13 1:38 ` Gao, Zhichao
0 siblings, 1 reply; 13+ messages in thread
From: Wu, Hao A @ 2020-08-12 6:35 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Wang, Jian J, Ni, Ray, Gary Lin, Andrew Fish
> -----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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR
2020-08-12 1:21 ` [PATCH V2 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR Gao, Zhichao
2020-08-12 5:22 ` [edk2-devel] " Ni, Ray
@ 2020-08-12 6:37 ` Wu, Hao A
1 sibling, 0 replies; 13+ messages in thread
From: Wu, Hao A @ 2020-08-12 6:37 UTC (permalink / raw)
To: devel@edk2.groups.io, Gao, Zhichao
Cc: Wang, Jian J, Ni, Ray, Gary Lin, Andrew Fish
> -----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
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert changes for the special MBR
2020-08-12 1:21 ` [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert changes for the special MBR Gao, Zhichao
2020-08-12 5:23 ` [edk2-devel] " Ni, Ray
@ 2020-08-12 6:38 ` Wu, Hao A
2020-08-12 6:38 ` Wu, Hao A
2 siblings, 0 replies; 13+ messages in thread
From: Wu, Hao A @ 2020-08-12 6:38 UTC (permalink / raw)
To: devel@edk2.groups.io, Gao, Zhichao
Cc: Wang, Jian J, Ni, Ray, Gary Lin, Andrew Fish
> -----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
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert changes for the special MBR
2020-08-12 1:21 ` [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert changes for the special MBR Gao, Zhichao
2020-08-12 5:23 ` [edk2-devel] " Ni, Ray
2020-08-12 6:38 ` Wu, Hao A
@ 2020-08-12 6:38 ` Wu, Hao A
2 siblings, 0 replies; 13+ messages in thread
From: Wu, Hao A @ 2020-08-12 6:38 UTC (permalink / raw)
To: devel@edk2.groups.io, Gao, Zhichao
Cc: Wang, Jian J, Ni, Ray, Gary Lin, Andrew Fish
> -----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
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec
2020-08-12 6:26 ` [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec Gary Lin
@ 2020-08-12 11:04 ` Laszlo Ersek
0 siblings, 0 replies; 13+ messages in thread
From: Laszlo Ersek @ 2020-08-12 11:04 UTC (permalink / raw)
To: devel, glin, Zhichao Gao; +Cc: Jian J Wang, Hao A Wu, Ray Ni, Andrew Fish
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
>>
>
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander
2020-08-12 6:35 ` Wu, Hao A
@ 2020-08-13 1:38 ` Gao, Zhichao
0 siblings, 0 replies; 13+ messages in thread
From: Gao, Zhichao @ 2020-08-13 1:38 UTC (permalink / raw)
To: Wu, Hao A, devel@edk2.groups.io
Cc: Wang, Jian J, Ni, Ray, Gary Lin, Andrew Fish
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-08-13 1:38 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-08-12 1:21 [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec Gao, Zhichao
2020-08-12 1:21 ` [PATCH V2 1/3] MdeModulePkg/PartitionDxe: Put the UDF check ahead of MBR Gao, Zhichao
2020-08-12 5:22 ` [edk2-devel] " Ni, Ray
2020-08-12 6:37 ` Wu, Hao A
2020-08-12 1:21 ` [PATCH V2 2/3] MdeModulePkg/PartitionDxe: Revert changes for the special MBR Gao, Zhichao
2020-08-12 5:23 ` [edk2-devel] " Ni, Ray
2020-08-12 6:38 ` Wu, Hao A
2020-08-12 6:38 ` Wu, Hao A
2020-08-12 1:21 ` [PATCH V2 3/3] MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child hander Gao, Zhichao
2020-08-12 6:35 ` Wu, Hao A
2020-08-13 1:38 ` Gao, Zhichao
2020-08-12 6:26 ` [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec Gary Lin
2020-08-12 11:04 ` [edk2-devel] " Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox