* [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler @ 2020-06-24 5:56 Gao, Zhichao 2020-06-24 8:32 ` Ni, Ray 0 siblings, 1 reply; 10+ messages in thread From: Gao, Zhichao @ 2020-06-24 5:56 UTC (permalink / raw) To: devel; +Cc: Hao A Wu, Ray Ni, Jian J Wang, Liming Gao REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823 Some linux iso like Redhat would contain both MBR info in the first 512 bytes and volume info at the beginning of 32KB offset. But the partition driver would only choose one of the GPT, MBR, and UDF(el torito compatible) to install the partition. That would lose one info for such linux ISO during one connect. And UDF(el torito compatible) is not conflicted with MBR/GPT. So partition driver should check UDF and MBR/GPT separately. Cc: Hao A Wu <hao.a.wu@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Liming Gao <liming.gao@intel.com> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> --- .../Universal/Disk/PartitionDxe/Partition.c | 52 +++++++++++++------ 1 file changed, 37 insertions(+), 15 deletions(-) diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c index d1c878ad2e..562490db4f 100644 --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c @@ -5,7 +5,7 @@ MBR, and GPT partition schemes are supported. Copyright (c) 2018 Qualcomm Datacenter Technologies, Inc. -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> +Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.<BR> SPDX-License-Identifier: BSD-2-Clause-Patent **/ @@ -39,7 +39,6 @@ EFI_DRIVER_BINDING_PROTOCOL gPartitionDriverBinding = { PARTITION_DETECT_ROUTINE mPartitionDetectRoutineTable[] = { PartitionInstallGptChildHandles, PartitionInstallMbrChildHandles, - PartitionInstallUdfChildHandles, NULL }; @@ -189,6 +188,8 @@ PartitionDriverBindingStart ( { EFI_STATUS Status; EFI_STATUS OpenStatus; + EFI_STATUS UdfStatus; + EFI_STATUS GptMbrStatus; EFI_BLOCK_IO_PROTOCOL *BlockIo; EFI_BLOCK_IO2_PROTOCOL *BlockIo2; EFI_DISK_IO_PROTOCOL *DiskIo; @@ -300,26 +301,47 @@ PartitionDriverBindingStart ( if (BlockIo->Media->MediaPresent || (BlockIo->Media->RemovableMedia && !BlockIo->Media->LogicalPartition)) { // - // Try for GPT, then legacy MBR partition types, and then UDF and El Torito. - // If the media supports a given partition type install child handles to - // represent the partitions described by the media. + // Try for UDF (El TOrito compatible) and GPT/MBR partition types. + // If the media supports a given partition type, it would install child handles + // to represent the partitions described by the media. + // Notes: GPT is conflicted with MBR. It would only install one of them to describe + // the partition. UDF (or El Torito) can exist with MBR, so need to check it along with + // GPT/MBR. // + UdfStatus = PartitionInstallUdfChildHandles ( + This, + ControllerHandle, + DiskIo, + DiskIo2, + BlockIo, + BlockIo2, + ParentDevicePath + ); + Routine = &mPartitionDetectRoutineTable[0]; while (*Routine != NULL) { - Status = (*Routine) ( - This, - ControllerHandle, - DiskIo, - DiskIo2, - BlockIo, - BlockIo2, - ParentDevicePath - ); - if (!EFI_ERROR (Status) || Status == EFI_MEDIA_CHANGED || Status == EFI_NO_MEDIA) { + GptMbrStatus = (*Routine) ( + This, + ControllerHandle, + DiskIo, + DiskIo2, + BlockIo, + BlockIo2, + ParentDevicePath + ); + if (!EFI_ERROR (GptMbrStatus) || GptMbrStatus == EFI_MEDIA_CHANGED || GptMbrStatus == EFI_NO_MEDIA) { break; } Routine++; } + + if (!EFI_ERROR (UdfStatus) || !EFI_ERROR (GptMbrStatus)) { + Status = EFI_SUCCESS; + } else if (UdfStatus == EFI_MEDIA_CHANGED || GptMbrStatus == EFI_MEDIA_CHANGED) { + Status = EFI_MEDIA_CHANGED; + } else if (UdfStatus == EFI_NO_MEDIA || GptMbrStatus == EFI_NO_MEDIA) { + Status = EFI_NO_MEDIA; + } } // // In the case that the driver is already started (OpenStatus == EFI_ALREADY_STARTED), -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler 2020-06-24 5:56 [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler Gao, Zhichao @ 2020-06-24 8:32 ` Ni, Ray 2020-06-24 11:57 ` [edk2-devel] " Laszlo Ersek 0 siblings, 1 reply; 10+ messages in thread From: Ni, Ray @ 2020-06-24 8:32 UTC (permalink / raw) To: Gao, Zhichao, devel@edk2.groups.io; +Cc: Wu, Hao A, Wang, Jian J, Gao, Liming Zhichao, Will two BlockIo child devices cover the same range in the ISO after your patch? I don't think that's a valid behavior. Thanks, Ray > -----Original Message----- > From: Gao, Zhichao <zhichao.gao@intel.com> > Sent: Wednesday, June 24, 2020 1:56 PM > To: devel@edk2.groups.io > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming > <liming.gao@intel.com> > Subject: [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823 > > Some linux iso like Redhat would contain both MBR info in the first > 512 bytes and volume info at the beginning of 32KB offset. > But the partition driver would only choose one of the GPT, MBR, and > UDF(el torito compatible) to install the partition. That would lose > one info for such linux ISO during one connect. And UDF(el torito > compatible) is not conflicted with MBR/GPT. So partition driver should > check UDF and MBR/GPT separately. > > Cc: Hao A Wu <hao.a.wu@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Liming Gao <liming.gao@intel.com> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > --- > .../Universal/Disk/PartitionDxe/Partition.c | 52 +++++++++++++------ > 1 file changed, 37 insertions(+), 15 deletions(-) > > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > index d1c878ad2e..562490db4f 100644 > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > @@ -5,7 +5,7 @@ > MBR, and GPT partition schemes are supported. > > Copyright (c) 2018 Qualcomm Datacenter Technologies, Inc. > -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -39,7 +39,6 @@ EFI_DRIVER_BINDING_PROTOCOL gPartitionDriverBinding = { > PARTITION_DETECT_ROUTINE mPartitionDetectRoutineTable[] = { > PartitionInstallGptChildHandles, > PartitionInstallMbrChildHandles, > - PartitionInstallUdfChildHandles, > NULL > }; > > @@ -189,6 +188,8 @@ PartitionDriverBindingStart ( > { > EFI_STATUS Status; > EFI_STATUS OpenStatus; > + EFI_STATUS UdfStatus; > + EFI_STATUS GptMbrStatus; > EFI_BLOCK_IO_PROTOCOL *BlockIo; > EFI_BLOCK_IO2_PROTOCOL *BlockIo2; > EFI_DISK_IO_PROTOCOL *DiskIo; > @@ -300,26 +301,47 @@ PartitionDriverBindingStart ( > if (BlockIo->Media->MediaPresent || > (BlockIo->Media->RemovableMedia && !BlockIo->Media->LogicalPartition)) { > // > - // Try for GPT, then legacy MBR partition types, and then UDF and El Torito. > - // If the media supports a given partition type install child handles to > - // represent the partitions described by the media. > + // Try for UDF (El TOrito compatible) and GPT/MBR partition types. > + // If the media supports a given partition type, it would install child handles > + // to represent the partitions described by the media. > + // Notes: GPT is conflicted with MBR. It would only install one of them to describe > + // the partition. UDF (or El Torito) can exist with MBR, so need to check it along with > + // GPT/MBR. > // > + UdfStatus = PartitionInstallUdfChildHandles ( > + This, > + ControllerHandle, > + DiskIo, > + DiskIo2, > + BlockIo, > + BlockIo2, > + ParentDevicePath > + ); > + > Routine = &mPartitionDetectRoutineTable[0]; > while (*Routine != NULL) { > - Status = (*Routine) ( > - This, > - ControllerHandle, > - DiskIo, > - DiskIo2, > - BlockIo, > - BlockIo2, > - ParentDevicePath > - ); > - if (!EFI_ERROR (Status) || Status == EFI_MEDIA_CHANGED || Status == EFI_NO_MEDIA) { > + GptMbrStatus = (*Routine) ( > + This, > + ControllerHandle, > + DiskIo, > + DiskIo2, > + BlockIo, > + BlockIo2, > + ParentDevicePath > + ); > + if (!EFI_ERROR (GptMbrStatus) || GptMbrStatus == EFI_MEDIA_CHANGED || GptMbrStatus == EFI_NO_MEDIA) { > break; > } > Routine++; > } > + > + if (!EFI_ERROR (UdfStatus) || !EFI_ERROR (GptMbrStatus)) { > + Status = EFI_SUCCESS; > + } else if (UdfStatus == EFI_MEDIA_CHANGED || GptMbrStatus == EFI_MEDIA_CHANGED) { > + Status = EFI_MEDIA_CHANGED; > + } else if (UdfStatus == EFI_NO_MEDIA || GptMbrStatus == EFI_NO_MEDIA) { > + Status = EFI_NO_MEDIA; > + } > } > // > // In the case that the driver is already started (OpenStatus == EFI_ALREADY_STARTED), > -- > 2.21.0.windows.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler 2020-06-24 8:32 ` Ni, Ray @ 2020-06-24 11:57 ` Laszlo Ersek 2020-06-25 0:21 ` Ni, Ray 0 siblings, 1 reply; 10+ messages in thread From: Laszlo Ersek @ 2020-06-24 11:57 UTC (permalink / raw) To: devel, ray.ni, Gao, Zhichao; +Cc: Wu, Hao A, Wang, Jian J, Gao, Liming On 06/24/20 10:32, Ni, Ray wrote: > Zhichao, > Will two BlockIo child devices cover the same range in the ISO after your patch? > > I don't think that's a valid behavior. Right; I'm surprised to find the Red Hat reference in both the commit message and the BZ. I can't say I understand the issue at hand, but are we sure this isn't a bug -- a standards conformance problem -- with the ISO in question? Zhichao, do you have a link to an ISO image that triggers the problem, and can you provide a hexdump of the part of the image that is either buggy, or triggers the problem in edk2? With a more specific issue description, I might be able to find someone at Red Hat who could comment with more insight than myself. Thanks! Laszlo > > Thanks, > Ray > >> -----Original Message----- >> From: Gao, Zhichao <zhichao.gao@intel.com> >> Sent: Wednesday, June 24, 2020 1:56 PM >> To: devel@edk2.groups.io >> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming >> <liming.gao@intel.com> >> Subject: [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler >> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823 >> >> Some linux iso like Redhat would contain both MBR info in the first >> 512 bytes and volume info at the beginning of 32KB offset. >> But the partition driver would only choose one of the GPT, MBR, and >> UDF(el torito compatible) to install the partition. That would lose >> one info for such linux ISO during one connect. And UDF(el torito >> compatible) is not conflicted with MBR/GPT. So partition driver should >> check UDF and MBR/GPT separately. >> >> Cc: Hao A Wu <hao.a.wu@intel.com> >> Cc: Ray Ni <ray.ni@intel.com> >> Cc: Jian J Wang <jian.j.wang@intel.com> >> Cc: Liming Gao <liming.gao@intel.com> >> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> >> --- >> .../Universal/Disk/PartitionDxe/Partition.c | 52 +++++++++++++------ >> 1 file changed, 37 insertions(+), 15 deletions(-) >> >> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >> b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >> index d1c878ad2e..562490db4f 100644 >> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >> @@ -5,7 +5,7 @@ >> MBR, and GPT partition schemes are supported. >> >> Copyright (c) 2018 Qualcomm Datacenter Technologies, Inc. >> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> >> +Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.<BR> >> SPDX-License-Identifier: BSD-2-Clause-Patent >> >> **/ >> @@ -39,7 +39,6 @@ EFI_DRIVER_BINDING_PROTOCOL gPartitionDriverBinding = { >> PARTITION_DETECT_ROUTINE mPartitionDetectRoutineTable[] = { >> PartitionInstallGptChildHandles, >> PartitionInstallMbrChildHandles, >> - PartitionInstallUdfChildHandles, >> NULL >> }; >> >> @@ -189,6 +188,8 @@ PartitionDriverBindingStart ( >> { >> EFI_STATUS Status; >> EFI_STATUS OpenStatus; >> + EFI_STATUS UdfStatus; >> + EFI_STATUS GptMbrStatus; >> EFI_BLOCK_IO_PROTOCOL *BlockIo; >> EFI_BLOCK_IO2_PROTOCOL *BlockIo2; >> EFI_DISK_IO_PROTOCOL *DiskIo; >> @@ -300,26 +301,47 @@ PartitionDriverBindingStart ( >> if (BlockIo->Media->MediaPresent || >> (BlockIo->Media->RemovableMedia && !BlockIo->Media->LogicalPartition)) { >> // >> - // Try for GPT, then legacy MBR partition types, and then UDF and El Torito. >> - // If the media supports a given partition type install child handles to >> - // represent the partitions described by the media. >> + // Try for UDF (El TOrito compatible) and GPT/MBR partition types. >> + // If the media supports a given partition type, it would install child handles >> + // to represent the partitions described by the media. >> + // Notes: GPT is conflicted with MBR. It would only install one of them to describe >> + // the partition. UDF (or El Torito) can exist with MBR, so need to check it along with >> + // GPT/MBR. >> // >> + UdfStatus = PartitionInstallUdfChildHandles ( >> + This, >> + ControllerHandle, >> + DiskIo, >> + DiskIo2, >> + BlockIo, >> + BlockIo2, >> + ParentDevicePath >> + ); >> + >> Routine = &mPartitionDetectRoutineTable[0]; >> while (*Routine != NULL) { >> - Status = (*Routine) ( >> - This, >> - ControllerHandle, >> - DiskIo, >> - DiskIo2, >> - BlockIo, >> - BlockIo2, >> - ParentDevicePath >> - ); >> - if (!EFI_ERROR (Status) || Status == EFI_MEDIA_CHANGED || Status == EFI_NO_MEDIA) { >> + GptMbrStatus = (*Routine) ( >> + This, >> + ControllerHandle, >> + DiskIo, >> + DiskIo2, >> + BlockIo, >> + BlockIo2, >> + ParentDevicePath >> + ); >> + if (!EFI_ERROR (GptMbrStatus) || GptMbrStatus == EFI_MEDIA_CHANGED || GptMbrStatus == EFI_NO_MEDIA) { >> break; >> } >> Routine++; >> } >> + >> + if (!EFI_ERROR (UdfStatus) || !EFI_ERROR (GptMbrStatus)) { >> + Status = EFI_SUCCESS; >> + } else if (UdfStatus == EFI_MEDIA_CHANGED || GptMbrStatus == EFI_MEDIA_CHANGED) { >> + Status = EFI_MEDIA_CHANGED; >> + } else if (UdfStatus == EFI_NO_MEDIA || GptMbrStatus == EFI_NO_MEDIA) { >> + Status = EFI_NO_MEDIA; >> + } >> } >> // >> // In the case that the driver is already started (OpenStatus == EFI_ALREADY_STARTED), >> -- >> 2.21.0.windows.1 > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler 2020-06-24 11:57 ` [edk2-devel] " Laszlo Ersek @ 2020-06-25 0:21 ` Ni, Ray 2020-06-25 9:02 ` Laszlo Ersek 0 siblings, 1 reply; 10+ messages in thread From: Ni, Ray @ 2020-06-25 0:21 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io, Gao, Zhichao Cc: Wu, Hao A, Wang, Jian J, Gao, Liming Laszlo, You are surprised because the issue is related to Redhat, or because the word "Redhat" should not be mentioned in the Bugzilla? http://manpages.ubuntu.com/manpages/bionic/man8/mkudffs.8.html is a tool that creates UDF file system with a compatible MBR table in the top 32KB specially for Windows. Search " WHOLE DISK VS PARTITION" in the web page. I think maybe edk2 Mbr logic should (but doesn't) skip MBR whose partition starts from LBA #0 (includes MBR itself). > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Wednesday, June 24, 2020 7:58 PM > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming <liming.gao@intel.com> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler > > On 06/24/20 10:32, Ni, Ray wrote: > > Zhichao, > > Will two BlockIo child devices cover the same range in the ISO after your patch? > > > > I don't think that's a valid behavior. > > Right; I'm surprised to find the Red Hat reference in both the commit > message and the BZ. > > I can't say I understand the issue at hand, but are we sure this isn't a > bug -- a standards conformance problem -- with the ISO in question? > > Zhichao, do you have a link to an ISO image that triggers the problem, > and can you provide a hexdump of the part of the image that is either > buggy, or triggers the problem in edk2? With a more specific issue > description, I might be able to find someone at Red Hat who could > comment with more insight than myself. > > Thanks! > Laszlo > > > > > Thanks, > > Ray > > > >> -----Original Message----- > >> From: Gao, Zhichao <zhichao.gao@intel.com> > >> Sent: Wednesday, June 24, 2020 1:56 PM > >> To: devel@edk2.groups.io > >> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming > >> <liming.gao@intel.com> > >> Subject: [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler > >> > >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823 > >> > >> Some linux iso like Redhat would contain both MBR info in the first > >> 512 bytes and volume info at the beginning of 32KB offset. > >> But the partition driver would only choose one of the GPT, MBR, and > >> UDF(el torito compatible) to install the partition. That would lose > >> one info for such linux ISO during one connect. And UDF(el torito > >> compatible) is not conflicted with MBR/GPT. So partition driver should > >> check UDF and MBR/GPT separately. > >> > >> Cc: Hao A Wu <hao.a.wu@intel.com> > >> Cc: Ray Ni <ray.ni@intel.com> > >> Cc: Jian J Wang <jian.j.wang@intel.com> > >> Cc: Liming Gao <liming.gao@intel.com> > >> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > >> --- > >> .../Universal/Disk/PartitionDxe/Partition.c | 52 +++++++++++++------ > >> 1 file changed, 37 insertions(+), 15 deletions(-) > >> > >> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > >> b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > >> index d1c878ad2e..562490db4f 100644 > >> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > >> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > >> @@ -5,7 +5,7 @@ > >> MBR, and GPT partition schemes are supported. > >> > >> Copyright (c) 2018 Qualcomm Datacenter Technologies, Inc. > >> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> > >> +Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.<BR> > >> SPDX-License-Identifier: BSD-2-Clause-Patent > >> > >> **/ > >> @@ -39,7 +39,6 @@ EFI_DRIVER_BINDING_PROTOCOL gPartitionDriverBinding = { > >> PARTITION_DETECT_ROUTINE mPartitionDetectRoutineTable[] = { > >> PartitionInstallGptChildHandles, > >> PartitionInstallMbrChildHandles, > >> - PartitionInstallUdfChildHandles, > >> NULL > >> }; > >> > >> @@ -189,6 +188,8 @@ PartitionDriverBindingStart ( > >> { > >> EFI_STATUS Status; > >> EFI_STATUS OpenStatus; > >> + EFI_STATUS UdfStatus; > >> + EFI_STATUS GptMbrStatus; > >> EFI_BLOCK_IO_PROTOCOL *BlockIo; > >> EFI_BLOCK_IO2_PROTOCOL *BlockIo2; > >> EFI_DISK_IO_PROTOCOL *DiskIo; > >> @@ -300,26 +301,47 @@ PartitionDriverBindingStart ( > >> if (BlockIo->Media->MediaPresent || > >> (BlockIo->Media->RemovableMedia && !BlockIo->Media->LogicalPartition)) { > >> // > >> - // Try for GPT, then legacy MBR partition types, and then UDF and El Torito. > >> - // If the media supports a given partition type install child handles to > >> - // represent the partitions described by the media. > >> + // Try for UDF (El TOrito compatible) and GPT/MBR partition types. > >> + // If the media supports a given partition type, it would install child handles > >> + // to represent the partitions described by the media. > >> + // Notes: GPT is conflicted with MBR. It would only install one of them to describe > >> + // the partition. UDF (or El Torito) can exist with MBR, so need to check it along with > >> + // GPT/MBR. > >> // > >> + UdfStatus = PartitionInstallUdfChildHandles ( > >> + This, > >> + ControllerHandle, > >> + DiskIo, > >> + DiskIo2, > >> + BlockIo, > >> + BlockIo2, > >> + ParentDevicePath > >> + ); > >> + > >> Routine = &mPartitionDetectRoutineTable[0]; > >> while (*Routine != NULL) { > >> - Status = (*Routine) ( > >> - This, > >> - ControllerHandle, > >> - DiskIo, > >> - DiskIo2, > >> - BlockIo, > >> - BlockIo2, > >> - ParentDevicePath > >> - ); > >> - if (!EFI_ERROR (Status) || Status == EFI_MEDIA_CHANGED || Status == EFI_NO_MEDIA) { > >> + GptMbrStatus = (*Routine) ( > >> + This, > >> + ControllerHandle, > >> + DiskIo, > >> + DiskIo2, > >> + BlockIo, > >> + BlockIo2, > >> + ParentDevicePath > >> + ); > >> + if (!EFI_ERROR (GptMbrStatus) || GptMbrStatus == EFI_MEDIA_CHANGED || GptMbrStatus == EFI_NO_MEDIA) { > >> break; > >> } > >> Routine++; > >> } > >> + > >> + if (!EFI_ERROR (UdfStatus) || !EFI_ERROR (GptMbrStatus)) { > >> + Status = EFI_SUCCESS; > >> + } else if (UdfStatus == EFI_MEDIA_CHANGED || GptMbrStatus == EFI_MEDIA_CHANGED) { > >> + Status = EFI_MEDIA_CHANGED; > >> + } else if (UdfStatus == EFI_NO_MEDIA || GptMbrStatus == EFI_NO_MEDIA) { > >> + Status = EFI_NO_MEDIA; > >> + } > >> } > >> // > >> // In the case that the driver is already started (OpenStatus == EFI_ALREADY_STARTED), > >> -- > >> 2.21.0.windows.1 > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler 2020-06-25 0:21 ` Ni, Ray @ 2020-06-25 9:02 ` Laszlo Ersek 2020-06-29 1:47 ` Gao, Zhichao 0 siblings, 1 reply; 10+ messages in thread From: Laszlo Ersek @ 2020-06-25 9:02 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io, Gao, Zhichao Cc: Wu, Hao A, Wang, Jian J, Gao, Liming On 06/25/20 02:21, Ni, Ray wrote: > Laszlo, > You are surprised because the issue is related to Redhat, or because > the word "Redhat" should not be mentioned in the Bugzilla? It's totally fine to mention Red Hat (with correct spelling) in the issue description. The more details, the better. I'm surprised that the issue *seems* specific to Red Hat. Earlier I had looked on-and-off on the scripts that generate (for example) the Fedora ISO images (so that they be both BIOS-bootable and UEFI-bootable), and I don't recall any particular tricks that would exploit gray areas in specs or so. It's possible that the *tools* the scripts use for formatting the ISOs have some bugs or quirks, of course. (But then I'm again a bit surprised for those issues or quirks to be RH-specific -- I just don't see a reason for us to diverge from the respective upstream code bases of those tools.) > http://manpages.ubuntu.com/manpages/bionic/man8/mkudffs.8.html > is a tool that creates UDF file system with a compatible MBR table in the > top 32KB specially for Windows. > > Search " WHOLE DISK VS PARTITION" in the web page. Thanks -- and, indeed, this confirms my point. The issue (or unexpected behavior at least) is not specific to Red Hat, it is part of the (most likely: upstream) mkudffs tool. (Case in point, the manual page you're quoting is from an Ubuntu distro.) I think a minimal reproducer using the mkudffs command (and maybe more commands) would be nice in the TianoCore BZ. Anyway, looking at the manual you've identified, it does seem like a gray area. It's a conflict between two specifications, apparently. Or perhaps more precisely, a bug in the UDF spec. (It seems really strange that a filesystem insists on being installed on a complete block device, and not on a partition described in a standard partition table.) The workaround described in the manual is presented as a workaround "for Windows' sake". I don't think that's fair; in my (uneducated?) opinion *any* file system should be happy to live inside a partition. I wouldn't "blame" Windows for making an assumption like this. Furthermore, the technical details of the workaround are really annoying: So the whole disk device and also first partition on disk points to same sectors. This means that the first partition recursively includes itself -- in other words, it includes the partition *table* that describes itself. :/ The manual then says, mkudffs option --bootarea=mbr put such MBR table for compatibility with Microsoft Windows systems into disk when formatting. So maybe there *is* a bug in the Red Hat ISO generator scripts; I'm not sure. I don't see a reason why "--bootarea=mbr" should ever be used with *removable* media (such as an ISO image). The mkudffs manual states that this trickery is useful only for non-removable media even on Windows: Microsoft Windows systems are unable to detect non-removable hard disks without MBR or GPT partition table. Removable disks do not have this restriction. (Personally I've been using an UDF-formatted USB pendrive for a long time now, because it supports 4G+ files, and is compatible with both Linux and Windows. There's no partition table on it; I formatted the pendrive myself, and I used neither a partition table nor the "--bootarea=mbr' flag.) Yet further, I also don't understand why this issue is being reported only now, and not before. TianoCore#2823 does not explain what it is exactly that fails, with RH ISOs. I've booted a whole lot of such ISOs over time (using OVMF). So is this -- that is, the assumed use of "--bootarea=mbr" -- a recent issue? Is it a regression with the ISOs? What is the bad symptom *precisely*, when the ISO image is formatted with "--bootarea=mbr"? The problem description is very lacking. Thanks, Laszlo > I think maybe edk2 Mbr logic should (but doesn't) skip MBR whose > partition starts from LBA #0 (includes MBR itself). > >> -----Original Message----- >> From: Laszlo Ersek <lersek@redhat.com> >> Sent: Wednesday, June 24, 2020 7:58 PM >> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com> >> Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming <liming.gao@intel.com> >> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler >> >> On 06/24/20 10:32, Ni, Ray wrote: >>> Zhichao, >>> Will two BlockIo child devices cover the same range in the ISO after your patch? >>> >>> I don't think that's a valid behavior. >> >> Right; I'm surprised to find the Red Hat reference in both the commit >> message and the BZ. >> >> I can't say I understand the issue at hand, but are we sure this isn't a >> bug -- a standards conformance problem -- with the ISO in question? >> >> Zhichao, do you have a link to an ISO image that triggers the problem, >> and can you provide a hexdump of the part of the image that is either >> buggy, or triggers the problem in edk2? With a more specific issue >> description, I might be able to find someone at Red Hat who could >> comment with more insight than myself. >> >> Thanks! >> Laszlo >> >>> >>> Thanks, >>> Ray >>> >>>> -----Original Message----- >>>> From: Gao, Zhichao <zhichao.gao@intel.com> >>>> Sent: Wednesday, June 24, 2020 1:56 PM >>>> To: devel@edk2.groups.io >>>> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming >>>> <liming.gao@intel.com> >>>> Subject: [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler >>>> >>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823 >>>> >>>> Some linux iso like Redhat would contain both MBR info in the first >>>> 512 bytes and volume info at the beginning of 32KB offset. >>>> But the partition driver would only choose one of the GPT, MBR, and >>>> UDF(el torito compatible) to install the partition. That would lose >>>> one info for such linux ISO during one connect. And UDF(el torito >>>> compatible) is not conflicted with MBR/GPT. So partition driver should >>>> check UDF and MBR/GPT separately. >>>> >>>> Cc: Hao A Wu <hao.a.wu@intel.com> >>>> Cc: Ray Ni <ray.ni@intel.com> >>>> Cc: Jian J Wang <jian.j.wang@intel.com> >>>> Cc: Liming Gao <liming.gao@intel.com> >>>> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> >>>> --- >>>> .../Universal/Disk/PartitionDxe/Partition.c | 52 +++++++++++++------ >>>> 1 file changed, 37 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >>>> b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >>>> index d1c878ad2e..562490db4f 100644 >>>> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >>>> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >>>> @@ -5,7 +5,7 @@ >>>> MBR, and GPT partition schemes are supported. >>>> >>>> Copyright (c) 2018 Qualcomm Datacenter Technologies, Inc. >>>> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> >>>> +Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.<BR> >>>> SPDX-License-Identifier: BSD-2-Clause-Patent >>>> >>>> **/ >>>> @@ -39,7 +39,6 @@ EFI_DRIVER_BINDING_PROTOCOL gPartitionDriverBinding = { >>>> PARTITION_DETECT_ROUTINE mPartitionDetectRoutineTable[] = { >>>> PartitionInstallGptChildHandles, >>>> PartitionInstallMbrChildHandles, >>>> - PartitionInstallUdfChildHandles, >>>> NULL >>>> }; >>>> >>>> @@ -189,6 +188,8 @@ PartitionDriverBindingStart ( >>>> { >>>> EFI_STATUS Status; >>>> EFI_STATUS OpenStatus; >>>> + EFI_STATUS UdfStatus; >>>> + EFI_STATUS GptMbrStatus; >>>> EFI_BLOCK_IO_PROTOCOL *BlockIo; >>>> EFI_BLOCK_IO2_PROTOCOL *BlockIo2; >>>> EFI_DISK_IO_PROTOCOL *DiskIo; >>>> @@ -300,26 +301,47 @@ PartitionDriverBindingStart ( >>>> if (BlockIo->Media->MediaPresent || >>>> (BlockIo->Media->RemovableMedia && !BlockIo->Media->LogicalPartition)) { >>>> // >>>> - // Try for GPT, then legacy MBR partition types, and then UDF and El Torito. >>>> - // If the media supports a given partition type install child handles to >>>> - // represent the partitions described by the media. >>>> + // Try for UDF (El TOrito compatible) and GPT/MBR partition types. >>>> + // If the media supports a given partition type, it would install child handles >>>> + // to represent the partitions described by the media. >>>> + // Notes: GPT is conflicted with MBR. It would only install one of them to describe >>>> + // the partition. UDF (or El Torito) can exist with MBR, so need to check it along with >>>> + // GPT/MBR. >>>> // >>>> + UdfStatus = PartitionInstallUdfChildHandles ( >>>> + This, >>>> + ControllerHandle, >>>> + DiskIo, >>>> + DiskIo2, >>>> + BlockIo, >>>> + BlockIo2, >>>> + ParentDevicePath >>>> + ); >>>> + >>>> Routine = &mPartitionDetectRoutineTable[0]; >>>> while (*Routine != NULL) { >>>> - Status = (*Routine) ( >>>> - This, >>>> - ControllerHandle, >>>> - DiskIo, >>>> - DiskIo2, >>>> - BlockIo, >>>> - BlockIo2, >>>> - ParentDevicePath >>>> - ); >>>> - if (!EFI_ERROR (Status) || Status == EFI_MEDIA_CHANGED || Status == EFI_NO_MEDIA) { >>>> + GptMbrStatus = (*Routine) ( >>>> + This, >>>> + ControllerHandle, >>>> + DiskIo, >>>> + DiskIo2, >>>> + BlockIo, >>>> + BlockIo2, >>>> + ParentDevicePath >>>> + ); >>>> + if (!EFI_ERROR (GptMbrStatus) || GptMbrStatus == EFI_MEDIA_CHANGED || GptMbrStatus == EFI_NO_MEDIA) { >>>> break; >>>> } >>>> Routine++; >>>> } >>>> + >>>> + if (!EFI_ERROR (UdfStatus) || !EFI_ERROR (GptMbrStatus)) { >>>> + Status = EFI_SUCCESS; >>>> + } else if (UdfStatus == EFI_MEDIA_CHANGED || GptMbrStatus == EFI_MEDIA_CHANGED) { >>>> + Status = EFI_MEDIA_CHANGED; >>>> + } else if (UdfStatus == EFI_NO_MEDIA || GptMbrStatus == EFI_NO_MEDIA) { >>>> + Status = EFI_NO_MEDIA; >>>> + } >>>> } >>>> // >>>> // In the case that the driver is already started (OpenStatus == EFI_ALREADY_STARTED), >>>> -- >>>> 2.21.0.windows.1 >>> >>> >>> >>> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler 2020-06-25 9:02 ` Laszlo Ersek @ 2020-06-29 1:47 ` Gao, Zhichao 2020-06-29 2:42 ` Ni, Ray 2020-07-02 9:42 ` Laszlo Ersek 0 siblings, 2 replies; 10+ messages in thread From: Gao, Zhichao @ 2020-06-29 1:47 UTC (permalink / raw) To: Laszlo Ersek, Ni, Ray, devel@edk2.groups.io Cc: Wu, Hao A, Wang, Jian J, Gao, Liming Hi Laszlo, Sorry, I didn't put the detail info about the issue. Let me descript here. The issue is not only for Red Hat. I found it with ubuntu 18.02 amd64 and Fedora-20-x86_64 ISO image as well. I didn't view all the linux iso images. Here is the issue: Using USB CD ROM with the linux ISO DVD. Run the platform and enter UEFI shell -> plug the USB CD ROM with the ISO DVD -> run "map -r". The CD's file system didn't show. It works fine with the normal boot (such as F2, F7 and auto boot) because it would run the *connect all* operation. That would run the partition driver serval times. First time the partition driver would install MBR partition info protocol with the device handle and skip the UDF check. The second time, it would fail the MBR check with *already started* and run UDF check next. That means for such ISO the UEFI would install two partition protocol, both MBR and UDF. But for shell environment, the USB hot plug handle would connect the USB device only once and missing the UDF check. I don't know why linux need the MBR info. Maybe for legacy compatible thinking (my opinion, may be wrong). If the GPT, MBR and UDF are conflict, then the original logic is fine. But in fact, GPT and MBR are conflict and GPT/MBR and UDF are not conflict. Thanks, Zhichao > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Thursday, June 25, 2020 5:02 PM > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Gao, Zhichao > <zhichao.gao@intel.com> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; > Gao, Liming <liming.gao@intel.com> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf > handler > > On 06/25/20 02:21, Ni, Ray wrote: > > Laszlo, > > You are surprised because the issue is related to Redhat, or because > > the word "Redhat" should not be mentioned in the Bugzilla? > > It's totally fine to mention Red Hat (with correct spelling) in the issue description. > The more details, the better. > > I'm surprised that the issue *seems* specific to Red Hat. Earlier I had looked on- > and-off on the scripts that generate (for example) the Fedora ISO images (so that > they be both BIOS-bootable and UEFI-bootable), and I don't recall any particular > tricks that would exploit gray areas in specs or so. > > It's possible that the *tools* the scripts use for formatting the ISOs have some > bugs or quirks, of course. (But then I'm again a bit surprised for those issues or > quirks to be RH-specific -- I just don't see a reason for us to diverge from the > respective upstream code bases of those tools.) > > > http://manpages.ubuntu.com/manpages/bionic/man8/mkudffs.8.html > > is a tool that creates UDF file system with a compatible MBR table in > > the top 32KB specially for Windows. > > > > Search " WHOLE DISK VS PARTITION" in the web page. > > Thanks -- and, indeed, this confirms my point. The issue (or unexpected behavior > at least) is not specific to Red Hat, it is part of the (most > likely: upstream) mkudffs tool. (Case in point, the manual page you're quoting is > from an Ubuntu distro.) > > I think a minimal reproducer using the mkudffs command (and maybe more > commands) would be nice in the TianoCore BZ. > > Anyway, looking at the manual you've identified, it does seem like a gray area. > It's a conflict between two specifications, apparently. Or perhaps more precisely, > a bug in the UDF spec. (It seems really strange that a filesystem insists on being > installed on a complete block device, and not on a partition described in a > standard partition table.) > > The workaround described in the manual is presented as a workaround "for > Windows' sake". I don't think that's fair; in my (uneducated?) opinion > *any* file system should be happy to live inside a partition. I wouldn't "blame" > Windows for making an assumption like this. > > Furthermore, the technical details of the workaround are really annoying: > > So the whole disk device and also first partition on disk > points to same sectors. > > This means that the first partition recursively includes itself -- in other words, it > includes the partition *table* that describes itself. :/ > > The manual then says, > > mkudffs option --bootarea=mbr put such MBR table for compatibility > with Microsoft Windows systems into disk when formatting. > > So maybe there *is* a bug in the Red Hat ISO generator scripts; I'm not sure. I > don't see a reason why "--bootarea=mbr" should ever be used with > *removable* media (such as an ISO image). The mkudffs manual states that this > trickery is useful only for non-removable media even on Windows: > > Microsoft Windows systems are unable to detect non-removable hard > disks without MBR or GPT partition table. Removable disks do not > have this restriction. > > (Personally I've been using an UDF-formatted USB pendrive for a long time now, > because it supports 4G+ files, and is compatible with both Linux and Windows. > There's no partition table on it; I formatted the pendrive myself, and I used > neither a partition table nor the "--bootarea=mbr' flag.) > > Yet further, I also don't understand why this issue is being reported only now, > and not before. TianoCore#2823 does not explain what it is exactly that fails, > with RH ISOs. I've booted a whole lot of such ISOs over time (using OVMF). > > So is this -- that is, the assumed use of "--bootarea=mbr" -- a recent issue? > > Is it a regression with the ISOs? > > What is the bad symptom *precisely*, when the ISO image is formatted with "-- > bootarea=mbr"? > > The problem description is very lacking. > > Thanks, > Laszlo > > > > I think maybe edk2 Mbr logic should (but doesn't) skip MBR whose > > partition starts from LBA #0 (includes MBR itself). > > > >> -----Original Message----- > >> From: Laszlo Ersek <lersek@redhat.com> > >> Sent: Wednesday, June 24, 2020 7:58 PM > >> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao > >> <zhichao.gao@intel.com> > >> Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J > >> <jian.j.wang@intel.com>; Gao, Liming <liming.gao@intel.com> > >> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate > >> the Udf handler > >> > >> On 06/24/20 10:32, Ni, Ray wrote: > >>> Zhichao, > >>> Will two BlockIo child devices cover the same range in the ISO after your > patch? > >>> > >>> I don't think that's a valid behavior. > >> > >> Right; I'm surprised to find the Red Hat reference in both the commit > >> message and the BZ. > >> > >> I can't say I understand the issue at hand, but are we sure this > >> isn't a bug -- a standards conformance problem -- with the ISO in question? > >> > >> Zhichao, do you have a link to an ISO image that triggers the > >> problem, and can you provide a hexdump of the part of the image that > >> is either buggy, or triggers the problem in edk2? With a more > >> specific issue description, I might be able to find someone at Red > >> Hat who could comment with more insight than myself. > >> > >> Thanks! > >> Laszlo > >> > >>> > >>> Thanks, > >>> Ray > >>> > >>>> -----Original Message----- > >>>> From: Gao, Zhichao <zhichao.gao@intel.com> > >>>> Sent: Wednesday, June 24, 2020 1:56 PM > >>>> To: devel@edk2.groups.io > >>>> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; > >>>> Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming > >>>> <liming.gao@intel.com> > >>>> Subject: [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf > >>>> handler > >>>> > >>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823 > >>>> > >>>> Some linux iso like Redhat would contain both MBR info in the first > >>>> 512 bytes and volume info at the beginning of 32KB offset. > >>>> But the partition driver would only choose one of the GPT, MBR, and > >>>> UDF(el torito compatible) to install the partition. That would lose > >>>> one info for such linux ISO during one connect. And UDF(el torito > >>>> compatible) is not conflicted with MBR/GPT. So partition driver > >>>> should check UDF and MBR/GPT separately. > >>>> > >>>> Cc: Hao A Wu <hao.a.wu@intel.com> > >>>> Cc: Ray Ni <ray.ni@intel.com> > >>>> Cc: Jian J Wang <jian.j.wang@intel.com> > >>>> Cc: Liming Gao <liming.gao@intel.com> > >>>> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > >>>> --- > >>>> .../Universal/Disk/PartitionDxe/Partition.c | 52 +++++++++++++------ > >>>> 1 file changed, 37 insertions(+), 15 deletions(-) > >>>> > >>>> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > >>>> b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > >>>> index d1c878ad2e..562490db4f 100644 > >>>> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > >>>> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > >>>> @@ -5,7 +5,7 @@ > >>>> MBR, and GPT partition schemes are supported. > >>>> > >>>> Copyright (c) 2018 Qualcomm Datacenter Technologies, Inc. > >>>> -Copyright (c) 2006 - 2018, Intel Corporation. All rights > >>>> reserved.<BR> > >>>> +Copyright (c) 2006 - 2020, Intel Corporation. All rights > >>>> +reserved.<BR> > >>>> SPDX-License-Identifier: BSD-2-Clause-Patent > >>>> > >>>> **/ > >>>> @@ -39,7 +39,6 @@ EFI_DRIVER_BINDING_PROTOCOL > >>>> gPartitionDriverBinding = { PARTITION_DETECT_ROUTINE > mPartitionDetectRoutineTable[] = { > >>>> PartitionInstallGptChildHandles, > >>>> PartitionInstallMbrChildHandles, > >>>> - PartitionInstallUdfChildHandles, > >>>> NULL > >>>> }; > >>>> > >>>> @@ -189,6 +188,8 @@ PartitionDriverBindingStart ( { > >>>> EFI_STATUS Status; > >>>> EFI_STATUS OpenStatus; > >>>> + EFI_STATUS UdfStatus; > >>>> + EFI_STATUS GptMbrStatus; > >>>> EFI_BLOCK_IO_PROTOCOL *BlockIo; > >>>> EFI_BLOCK_IO2_PROTOCOL *BlockIo2; > >>>> EFI_DISK_IO_PROTOCOL *DiskIo; > >>>> @@ -300,26 +301,47 @@ PartitionDriverBindingStart ( > >>>> if (BlockIo->Media->MediaPresent || > >>>> (BlockIo->Media->RemovableMedia && !BlockIo->Media- > >LogicalPartition)) { > >>>> // > >>>> - // Try for GPT, then legacy MBR partition types, and then UDF and El > Torito. > >>>> - // If the media supports a given partition type install child handles to > >>>> - // represent the partitions described by the media. > >>>> + // Try for UDF (El TOrito compatible) and GPT/MBR partition types. > >>>> + // If the media supports a given partition type, it would install child > handles > >>>> + // to represent the partitions described by the media. > >>>> + // Notes: GPT is conflicted with MBR. It would only install one of them to > describe > >>>> + // the partition. UDF (or El Torito) can exist with MBR, so need to check > it along with > >>>> + // GPT/MBR. > >>>> // > >>>> + UdfStatus = PartitionInstallUdfChildHandles ( > >>>> + This, > >>>> + ControllerHandle, > >>>> + DiskIo, > >>>> + DiskIo2, > >>>> + BlockIo, > >>>> + BlockIo2, > >>>> + ParentDevicePath > >>>> + ); > >>>> + > >>>> Routine = &mPartitionDetectRoutineTable[0]; > >>>> while (*Routine != NULL) { > >>>> - Status = (*Routine) ( > >>>> - This, > >>>> - ControllerHandle, > >>>> - DiskIo, > >>>> - DiskIo2, > >>>> - BlockIo, > >>>> - BlockIo2, > >>>> - ParentDevicePath > >>>> - ); > >>>> - if (!EFI_ERROR (Status) || Status == EFI_MEDIA_CHANGED || Status == > EFI_NO_MEDIA) { > >>>> + GptMbrStatus = (*Routine) ( > >>>> + This, > >>>> + ControllerHandle, > >>>> + DiskIo, > >>>> + DiskIo2, > >>>> + BlockIo, > >>>> + BlockIo2, > >>>> + ParentDevicePath > >>>> + ); > >>>> + if (!EFI_ERROR (GptMbrStatus) || GptMbrStatus == > >>>> + EFI_MEDIA_CHANGED || GptMbrStatus == EFI_NO_MEDIA) { > >>>> break; > >>>> } > >>>> Routine++; > >>>> } > >>>> + > >>>> + if (!EFI_ERROR (UdfStatus) || !EFI_ERROR (GptMbrStatus)) { > >>>> + Status = EFI_SUCCESS; > >>>> + } else if (UdfStatus == EFI_MEDIA_CHANGED || GptMbrStatus == > EFI_MEDIA_CHANGED) { > >>>> + Status = EFI_MEDIA_CHANGED; > >>>> + } else if (UdfStatus == EFI_NO_MEDIA || GptMbrStatus == > EFI_NO_MEDIA) { > >>>> + Status = EFI_NO_MEDIA; > >>>> + } > >>>> } > >>>> // > >>>> // In the case that the driver is already started (OpenStatus == > >>>> EFI_ALREADY_STARTED), > >>>> -- > >>>> 2.21.0.windows.1 > >>> > >>> > >>> > >>> > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler 2020-06-29 1:47 ` Gao, Zhichao @ 2020-06-29 2:42 ` Ni, Ray 2020-06-29 5:23 ` Gao, Zhichao 2020-07-02 9:42 ` Laszlo Ersek 1 sibling, 1 reply; 10+ messages in thread From: Ni, Ray @ 2020-06-29 2:42 UTC (permalink / raw) To: Gao, Zhichao, Laszlo Ersek, devel@edk2.groups.io Cc: Wu, Hao A, Wang, Jian J, Gao, Liming > -----Original Message----- > From: Gao, Zhichao <zhichao.gao@intel.com> > Sent: Monday, June 29, 2020 9:47 AM > To: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>; > devel@edk2.groups.io > Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; > Gao, Liming <liming.gao@intel.com> > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the > Udf handler > > Hi Laszlo, > > Sorry, I didn't put the detail info about the issue. Let me descript here. > > The issue is not only for Red Hat. I found it with ubuntu 18.02 amd64 and > Fedora-20-x86_64 ISO image as well. I didn't view all the linux iso images. > > Here is the issue: > Using USB CD ROM with the linux ISO DVD. Run the platform and enter UEFI > shell -> plug the USB CD ROM with the ISO DVD -> run "map -r". The CD's file > system didn't show. > > It works fine with the normal boot (such as F2, F7 and auto boot) because it > would run the *connect all* operation. That would run the partition driver > serval times. First time the partition driver would install MBR partition info > protocol with the device handle and skip the UDF check. The second time, it > would fail the MBR check with *already started* and run UDF check next. That > means for such ISO the UEFI would install two partition protocol, both MBR and > UDF. Zhichao, It sounds like a bug in partition driver that the second UDF check succeeds. > > But for shell environment, the USB hot plug handle would connect the USB > device only once and missing the UDF check. > > I don't know why linux need the MBR info. Maybe for legacy compatible > thinking (my opinion, may be wrong). > > If the GPT, MBR and UDF are conflict, then the original logic is fine. But in fact, > GPT and MBR are conflict and GPT/MBR and UDF are not conflict. > > Thanks, > Zhichao > > > -----Original Message----- > > From: Laszlo Ersek <lersek@redhat.com> > > Sent: Thursday, June 25, 2020 5:02 PM > > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Gao, Zhichao > > <zhichao.gao@intel.com> > > Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; > > Gao, Liming <liming.gao@intel.com> > > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the > Udf > > handler > > > > On 06/25/20 02:21, Ni, Ray wrote: > > > Laszlo, > > > You are surprised because the issue is related to Redhat, or because > > > the word "Redhat" should not be mentioned in the Bugzilla? > > > > It's totally fine to mention Red Hat (with correct spelling) in the issue > description. > > The more details, the better. > > > > I'm surprised that the issue *seems* specific to Red Hat. Earlier I had looked > on- > > and-off on the scripts that generate (for example) the Fedora ISO images (so > that > > they be both BIOS-bootable and UEFI-bootable), and I don't recall any > particular > > tricks that would exploit gray areas in specs or so. > > > > It's possible that the *tools* the scripts use for formatting the ISOs have some > > bugs or quirks, of course. (But then I'm again a bit surprised for those issues > or > > quirks to be RH-specific -- I just don't see a reason for us to diverge from the > > respective upstream code bases of those tools.) > > > > > http://manpages.ubuntu.com/manpages/bionic/man8/mkudffs.8.html > > > is a tool that creates UDF file system with a compatible MBR table in > > > the top 32KB specially for Windows. > > > > > > Search " WHOLE DISK VS PARTITION" in the web page. > > > > Thanks -- and, indeed, this confirms my point. The issue (or unexpected > behavior > > at least) is not specific to Red Hat, it is part of the (most > > likely: upstream) mkudffs tool. (Case in point, the manual page you're quoting > is > > from an Ubuntu distro.) > > > > I think a minimal reproducer using the mkudffs command (and maybe more > > commands) would be nice in the TianoCore BZ. > > > > Anyway, looking at the manual you've identified, it does seem like a gray > area. > > It's a conflict between two specifications, apparently. Or perhaps more > precisely, > > a bug in the UDF spec. (It seems really strange that a filesystem insists on > being > > installed on a complete block device, and not on a partition described in a > > standard partition table.) > > > > The workaround described in the manual is presented as a workaround "for > > Windows' sake". I don't think that's fair; in my (uneducated?) opinion > > *any* file system should be happy to live inside a partition. I wouldn't > "blame" > > Windows for making an assumption like this. > > > > Furthermore, the technical details of the workaround are really annoying: > > > > So the whole disk device and also first partition on disk > > points to same sectors. > > > > This means that the first partition recursively includes itself -- in other words, > it > > includes the partition *table* that describes itself. :/ > > > > The manual then says, > > > > mkudffs option --bootarea=mbr put such MBR table for compatibility > > with Microsoft Windows systems into disk when formatting. > > > > So maybe there *is* a bug in the Red Hat ISO generator scripts; I'm not sure. > I > > don't see a reason why "--bootarea=mbr" should ever be used with > > *removable* media (such as an ISO image). The mkudffs manual states that > this > > trickery is useful only for non-removable media even on Windows: > > > > Microsoft Windows systems are unable to detect non-removable hard > > disks without MBR or GPT partition table. Removable disks do not > > have this restriction. > > > > (Personally I've been using an UDF-formatted USB pendrive for a long time > now, > > because it supports 4G+ files, and is compatible with both Linux and Windows. > > There's no partition table on it; I formatted the pendrive myself, and I used > > neither a partition table nor the "--bootarea=mbr' flag.) > > > > Yet further, I also don't understand why this issue is being reported only now, > > and not before. TianoCore#2823 does not explain what it is exactly that fails, > > with RH ISOs. I've booted a whole lot of such ISOs over time (using OVMF). > > > > So is this -- that is, the assumed use of "--bootarea=mbr" -- a recent issue? > > > > Is it a regression with the ISOs? > > > > What is the bad symptom *precisely*, when the ISO image is formatted with > "-- > > bootarea=mbr"? > > > > The problem description is very lacking. > > > > Thanks, > > Laszlo > > > > > > > I think maybe edk2 Mbr logic should (but doesn't) skip MBR whose > > > partition starts from LBA #0 (includes MBR itself). > > > > > >> -----Original Message----- > > >> From: Laszlo Ersek <lersek@redhat.com> > > >> Sent: Wednesday, June 24, 2020 7:58 PM > > >> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao > > >> <zhichao.gao@intel.com> > > >> Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J > > >> <jian.j.wang@intel.com>; Gao, Liming <liming.gao@intel.com> > > >> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate > > >> the Udf handler > > >> > > >> On 06/24/20 10:32, Ni, Ray wrote: > > >>> Zhichao, > > >>> Will two BlockIo child devices cover the same range in the ISO after your > > patch? > > >>> > > >>> I don't think that's a valid behavior. > > >> > > >> Right; I'm surprised to find the Red Hat reference in both the commit > > >> message and the BZ. > > >> > > >> I can't say I understand the issue at hand, but are we sure this > > >> isn't a bug -- a standards conformance problem -- with the ISO in question? > > >> > > >> Zhichao, do you have a link to an ISO image that triggers the > > >> problem, and can you provide a hexdump of the part of the image that > > >> is either buggy, or triggers the problem in edk2? With a more > > >> specific issue description, I might be able to find someone at Red > > >> Hat who could comment with more insight than myself. > > >> > > >> Thanks! > > >> Laszlo > > >> > > >>> > > >>> Thanks, > > >>> Ray > > >>> > > >>>> -----Original Message----- > > >>>> From: Gao, Zhichao <zhichao.gao@intel.com> > > >>>> Sent: Wednesday, June 24, 2020 1:56 PM > > >>>> To: devel@edk2.groups.io > > >>>> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; > > >>>> Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming > > >>>> <liming.gao@intel.com> > > >>>> Subject: [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf > > >>>> handler > > >>>> > > >>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823 > > >>>> > > >>>> Some linux iso like Redhat would contain both MBR info in the first > > >>>> 512 bytes and volume info at the beginning of 32KB offset. > > >>>> But the partition driver would only choose one of the GPT, MBR, and > > >>>> UDF(el torito compatible) to install the partition. That would lose > > >>>> one info for such linux ISO during one connect. And UDF(el torito > > >>>> compatible) is not conflicted with MBR/GPT. So partition driver > > >>>> should check UDF and MBR/GPT separately. > > >>>> > > >>>> Cc: Hao A Wu <hao.a.wu@intel.com> > > >>>> Cc: Ray Ni <ray.ni@intel.com> > > >>>> Cc: Jian J Wang <jian.j.wang@intel.com> > > >>>> Cc: Liming Gao <liming.gao@intel.com> > > >>>> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > > >>>> --- > > >>>> .../Universal/Disk/PartitionDxe/Partition.c | 52 +++++++++++++------ > > >>>> 1 file changed, 37 insertions(+), 15 deletions(-) > > >>>> > > >>>> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > >>>> b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > >>>> index d1c878ad2e..562490db4f 100644 > > >>>> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > >>>> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > >>>> @@ -5,7 +5,7 @@ > > >>>> MBR, and GPT partition schemes are supported. > > >>>> > > >>>> Copyright (c) 2018 Qualcomm Datacenter Technologies, Inc. > > >>>> -Copyright (c) 2006 - 2018, Intel Corporation. All rights > > >>>> reserved.<BR> > > >>>> +Copyright (c) 2006 - 2020, Intel Corporation. All rights > > >>>> +reserved.<BR> > > >>>> SPDX-License-Identifier: BSD-2-Clause-Patent > > >>>> > > >>>> **/ > > >>>> @@ -39,7 +39,6 @@ EFI_DRIVER_BINDING_PROTOCOL > > >>>> gPartitionDriverBinding = { PARTITION_DETECT_ROUTINE > > mPartitionDetectRoutineTable[] = { > > >>>> PartitionInstallGptChildHandles, > > >>>> PartitionInstallMbrChildHandles, > > >>>> - PartitionInstallUdfChildHandles, > > >>>> NULL > > >>>> }; > > >>>> > > >>>> @@ -189,6 +188,8 @@ PartitionDriverBindingStart ( { > > >>>> EFI_STATUS Status; > > >>>> EFI_STATUS OpenStatus; > > >>>> + EFI_STATUS UdfStatus; > > >>>> + EFI_STATUS GptMbrStatus; > > >>>> EFI_BLOCK_IO_PROTOCOL *BlockIo; > > >>>> EFI_BLOCK_IO2_PROTOCOL *BlockIo2; > > >>>> EFI_DISK_IO_PROTOCOL *DiskIo; > > >>>> @@ -300,26 +301,47 @@ PartitionDriverBindingStart ( > > >>>> if (BlockIo->Media->MediaPresent || > > >>>> (BlockIo->Media->RemovableMedia && !BlockIo->Media- > > >LogicalPartition)) { > > >>>> // > > >>>> - // Try for GPT, then legacy MBR partition types, and then UDF and El > > Torito. > > >>>> - // If the media supports a given partition type install child handles to > > >>>> - // represent the partitions described by the media. > > >>>> + // Try for UDF (El TOrito compatible) and GPT/MBR partition types. > > >>>> + // If the media supports a given partition type, it would install child > > handles > > >>>> + // to represent the partitions described by the media. > > >>>> + // Notes: GPT is conflicted with MBR. It would only install one of > them to > > describe > > >>>> + // the partition. UDF (or El Torito) can exist with MBR, so need to > check > > it along with > > >>>> + // GPT/MBR. > > >>>> // > > >>>> + UdfStatus = PartitionInstallUdfChildHandles ( > > >>>> + This, > > >>>> + ControllerHandle, > > >>>> + DiskIo, > > >>>> + DiskIo2, > > >>>> + BlockIo, > > >>>> + BlockIo2, > > >>>> + ParentDevicePath > > >>>> + ); > > >>>> + > > >>>> Routine = &mPartitionDetectRoutineTable[0]; > > >>>> while (*Routine != NULL) { > > >>>> - Status = (*Routine) ( > > >>>> - This, > > >>>> - ControllerHandle, > > >>>> - DiskIo, > > >>>> - DiskIo2, > > >>>> - BlockIo, > > >>>> - BlockIo2, > > >>>> - ParentDevicePath > > >>>> - ); > > >>>> - if (!EFI_ERROR (Status) || Status == EFI_MEDIA_CHANGED || Status > == > > EFI_NO_MEDIA) { > > >>>> + GptMbrStatus = (*Routine) ( > > >>>> + This, > > >>>> + ControllerHandle, > > >>>> + DiskIo, > > >>>> + DiskIo2, > > >>>> + BlockIo, > > >>>> + BlockIo2, > > >>>> + ParentDevicePath > > >>>> + ); > > >>>> + if (!EFI_ERROR (GptMbrStatus) || GptMbrStatus == > > >>>> + EFI_MEDIA_CHANGED || GptMbrStatus == EFI_NO_MEDIA) { > > >>>> break; > > >>>> } > > >>>> Routine++; > > >>>> } > > >>>> + > > >>>> + if (!EFI_ERROR (UdfStatus) || !EFI_ERROR (GptMbrStatus)) { > > >>>> + Status = EFI_SUCCESS; > > >>>> + } else if (UdfStatus == EFI_MEDIA_CHANGED || GptMbrStatus == > > EFI_MEDIA_CHANGED) { > > >>>> + Status = EFI_MEDIA_CHANGED; > > >>>> + } else if (UdfStatus == EFI_NO_MEDIA || GptMbrStatus == > > EFI_NO_MEDIA) { > > >>>> + Status = EFI_NO_MEDIA; > > >>>> + } > > >>>> } > > >>>> // > > >>>> // In the case that the driver is already started (OpenStatus == > > >>>> EFI_ALREADY_STARTED), > > >>>> -- > > >>>> 2.21.0.windows.1 > > >>> > > >>> > > >>> > > >>> > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler 2020-06-29 2:42 ` Ni, Ray @ 2020-06-29 5:23 ` Gao, Zhichao 0 siblings, 0 replies; 10+ messages in thread From: Gao, Zhichao @ 2020-06-29 5:23 UTC (permalink / raw) To: Ni, Ray, Laszlo Ersek, devel@edk2.groups.io Cc: Wu, Hao A, Wang, Jian J, Gao, Liming > -----Original Message----- > From: Ni, Ray <ray.ni@intel.com> > Sent: Monday, June 29, 2020 10:42 AM > To: Gao, Zhichao <zhichao.gao@intel.com>; Laszlo Ersek <lersek@redhat.com>; > devel@edk2.groups.io > Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; > Gao, Liming <liming.gao@intel.com> > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf > handler > > > > > -----Original Message----- > > From: Gao, Zhichao <zhichao.gao@intel.com> > > Sent: Monday, June 29, 2020 9:47 AM > > To: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>; > > devel@edk2.groups.io > > Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J > > <jian.j.wang@intel.com>; Gao, Liming <liming.gao@intel.com> > > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate > > the Udf handler > > > > Hi Laszlo, > > > > Sorry, I didn't put the detail info about the issue. Let me descript here. > > > > The issue is not only for Red Hat. I found it with ubuntu 18.02 amd64 > > and > > Fedora-20-x86_64 ISO image as well. I didn't view all the linux iso images. > > > > Here is the issue: > > Using USB CD ROM with the linux ISO DVD. Run the platform and enter > > UEFI shell -> plug the USB CD ROM with the ISO DVD -> run "map -r". > > The CD's file system didn't show. > > > > It works fine with the normal boot (such as F2, F7 and auto boot) > > because it would run the *connect all* operation. That would run the > > partition driver serval times. First time the partition driver would > > install MBR partition info protocol with the device handle and skip > > the UDF check. The second time, it would fail the MBR check with > > *already started* and run UDF check next. That means for such ISO the > > UEFI would install two partition protocol, both MBR and UDF. > Zhichao, > It sounds like a bug in partition driver that the second UDF check succeeds. I agree with you. It is a bug when return *already start* and continue to check next partition format. That is why the issue is not observed when the add UDF support. Thanks, Zhichao > > > > > > But for shell environment, the USB hot plug handle would connect the > > USB device only once and missing the UDF check. > > > > I don't know why linux need the MBR info. Maybe for legacy compatible > > thinking (my opinion, may be wrong). > > > > If the GPT, MBR and UDF are conflict, then the original logic is fine. > > But in fact, GPT and MBR are conflict and GPT/MBR and UDF are not conflict. > > > > Thanks, > > Zhichao > > > > > -----Original Message----- > > > From: Laszlo Ersek <lersek@redhat.com> > > > Sent: Thursday, June 25, 2020 5:02 PM > > > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Gao, Zhichao > > > <zhichao.gao@intel.com> > > > Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J > > > <jian.j.wang@intel.com>; Gao, Liming <liming.gao@intel.com> > > > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: > > > Seperate the > > Udf > > > handler > > > > > > On 06/25/20 02:21, Ni, Ray wrote: > > > > Laszlo, > > > > You are surprised because the issue is related to Redhat, or > > > > because the word "Redhat" should not be mentioned in the Bugzilla? > > > > > > It's totally fine to mention Red Hat (with correct spelling) in the > > > issue > > description. > > > The more details, the better. > > > > > > I'm surprised that the issue *seems* specific to Red Hat. Earlier I > > > had looked > > on- > > > and-off on the scripts that generate (for example) the Fedora ISO > > > images (so > > that > > > they be both BIOS-bootable and UEFI-bootable), and I don't recall > > > any > > particular > > > tricks that would exploit gray areas in specs or so. > > > > > > It's possible that the *tools* the scripts use for formatting the > > > ISOs have some bugs or quirks, of course. (But then I'm again a bit > > > surprised for those issues > > or > > > quirks to be RH-specific -- I just don't see a reason for us to > > > diverge from the respective upstream code bases of those tools.) > > > > > > > http://manpages.ubuntu.com/manpages/bionic/man8/mkudffs.8.html > > > > is a tool that creates UDF file system with a compatible MBR table > > > > in the top 32KB specially for Windows. > > > > > > > > Search " WHOLE DISK VS PARTITION" in the web page. > > > > > > Thanks -- and, indeed, this confirms my point. The issue (or > > > unexpected > > behavior > > > at least) is not specific to Red Hat, it is part of the (most > > > likely: upstream) mkudffs tool. (Case in point, the manual page > > > you're quoting > > is > > > from an Ubuntu distro.) > > > > > > I think a minimal reproducer using the mkudffs command (and maybe > > > more > > > commands) would be nice in the TianoCore BZ. > > > > > > Anyway, looking at the manual you've identified, it does seem like a > > > gray > > area. > > > It's a conflict between two specifications, apparently. Or perhaps > > > more > > precisely, > > > a bug in the UDF spec. (It seems really strange that a filesystem > > > insists on > > being > > > installed on a complete block device, and not on a partition > > > described in a standard partition table.) > > > > > > The workaround described in the manual is presented as a workaround > > > "for Windows' sake". I don't think that's fair; in my (uneducated?) > > > opinion > > > *any* file system should be happy to live inside a partition. I > > > wouldn't > > "blame" > > > Windows for making an assumption like this. > > > > > > Furthermore, the technical details of the workaround are really annoying: > > > > > > So the whole disk device and also first partition on disk > > > points to same sectors. > > > > > > This means that the first partition recursively includes itself -- > > > in other words, > > it > > > includes the partition *table* that describes itself. :/ > > > > > > The manual then says, > > > > > > mkudffs option --bootarea=mbr put such MBR table for compatibility > > > with Microsoft Windows systems into disk when formatting. > > > > > > So maybe there *is* a bug in the Red Hat ISO generator scripts; I'm not sure. > > I > > > don't see a reason why "--bootarea=mbr" should ever be used with > > > *removable* media (such as an ISO image). The mkudffs manual states > > > that > > this > > > trickery is useful only for non-removable media even on Windows: > > > > > > Microsoft Windows systems are unable to detect non-removable hard > > > disks without MBR or GPT partition table. Removable disks do not > > > have this restriction. > > > > > > (Personally I've been using an UDF-formatted USB pendrive for a long > > > time > > now, > > > because it supports 4G+ files, and is compatible with both Linux and Windows. > > > There's no partition table on it; I formatted the pendrive myself, > > > and I used neither a partition table nor the "--bootarea=mbr' flag.) > > > > > > Yet further, I also don't understand why this issue is being > > > reported only now, and not before. TianoCore#2823 does not explain > > > what it is exactly that fails, with RH ISOs. I've booted a whole lot of such ISOs > over time (using OVMF). > > > > > > So is this -- that is, the assumed use of "--bootarea=mbr" -- a recent issue? > > > > > > Is it a regression with the ISOs? > > > > > > What is the bad symptom *precisely*, when the ISO image is formatted > > > with > > "-- > > > bootarea=mbr"? > > > > > > The problem description is very lacking. > > > > > > Thanks, > > > Laszlo > > > > > > > > > > I think maybe edk2 Mbr logic should (but doesn't) skip MBR whose > > > > partition starts from LBA #0 (includes MBR itself). > > > > > > > >> -----Original Message----- > > > >> From: Laszlo Ersek <lersek@redhat.com> > > > >> Sent: Wednesday, June 24, 2020 7:58 PM > > > >> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, > > > >> Zhichao <zhichao.gao@intel.com> > > > >> Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J > > > >> <jian.j.wang@intel.com>; Gao, Liming <liming.gao@intel.com> > > > >> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: > > > >> Seperate the Udf handler > > > >> > > > >> On 06/24/20 10:32, Ni, Ray wrote: > > > >>> Zhichao, > > > >>> Will two BlockIo child devices cover the same range in the ISO > > > >>> after your > > > patch? > > > >>> > > > >>> I don't think that's a valid behavior. > > > >> > > > >> Right; I'm surprised to find the Red Hat reference in both the > > > >> commit message and the BZ. > > > >> > > > >> I can't say I understand the issue at hand, but are we sure this > > > >> isn't a bug -- a standards conformance problem -- with the ISO in question? > > > >> > > > >> Zhichao, do you have a link to an ISO image that triggers the > > > >> problem, and can you provide a hexdump of the part of the image > > > >> that is either buggy, or triggers the problem in edk2? With a > > > >> more specific issue description, I might be able to find someone > > > >> at Red Hat who could comment with more insight than myself. > > > >> > > > >> Thanks! > > > >> Laszlo > > > >> > > > >>> > > > >>> Thanks, > > > >>> Ray > > > >>> > > > >>>> -----Original Message----- > > > >>>> From: Gao, Zhichao <zhichao.gao@intel.com> > > > >>>> Sent: Wednesday, June 24, 2020 1:56 PM > > > >>>> To: devel@edk2.groups.io > > > >>>> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; > > > >>>> Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming > > > >>>> <liming.gao@intel.com> > > > >>>> Subject: [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf > > > >>>> handler > > > >>>> > > > >>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823 > > > >>>> > > > >>>> Some linux iso like Redhat would contain both MBR info in the > > > >>>> first > > > >>>> 512 bytes and volume info at the beginning of 32KB offset. > > > >>>> But the partition driver would only choose one of the GPT, MBR, > > > >>>> and UDF(el torito compatible) to install the partition. That > > > >>>> would lose one info for such linux ISO during one connect. And > > > >>>> UDF(el torito > > > >>>> compatible) is not conflicted with MBR/GPT. So partition driver > > > >>>> should check UDF and MBR/GPT separately. > > > >>>> > > > >>>> Cc: Hao A Wu <hao.a.wu@intel.com> > > > >>>> Cc: Ray Ni <ray.ni@intel.com> > > > >>>> Cc: Jian J Wang <jian.j.wang@intel.com> > > > >>>> Cc: Liming Gao <liming.gao@intel.com> > > > >>>> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > > > >>>> --- > > > >>>> .../Universal/Disk/PartitionDxe/Partition.c | 52 +++++++++++++------ > > > >>>> 1 file changed, 37 insertions(+), 15 deletions(-) > > > >>>> > > > >>>> diff --git > > > >>>> a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > > >>>> b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > > >>>> index d1c878ad2e..562490db4f 100644 > > > >>>> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > > >>>> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > > >>>> @@ -5,7 +5,7 @@ > > > >>>> MBR, and GPT partition schemes are supported. > > > >>>> > > > >>>> Copyright (c) 2018 Qualcomm Datacenter Technologies, Inc. > > > >>>> -Copyright (c) 2006 - 2018, Intel Corporation. All rights > > > >>>> reserved.<BR> > > > >>>> +Copyright (c) 2006 - 2020, Intel Corporation. All rights > > > >>>> +reserved.<BR> > > > >>>> SPDX-License-Identifier: BSD-2-Clause-Patent > > > >>>> > > > >>>> **/ > > > >>>> @@ -39,7 +39,6 @@ EFI_DRIVER_BINDING_PROTOCOL > > > >>>> gPartitionDriverBinding = { PARTITION_DETECT_ROUTINE > > > mPartitionDetectRoutineTable[] = { > > > >>>> PartitionInstallGptChildHandles, > > > >>>> PartitionInstallMbrChildHandles, > > > >>>> - PartitionInstallUdfChildHandles, > > > >>>> NULL > > > >>>> }; > > > >>>> > > > >>>> @@ -189,6 +188,8 @@ PartitionDriverBindingStart ( { > > > >>>> EFI_STATUS Status; > > > >>>> EFI_STATUS OpenStatus; > > > >>>> + EFI_STATUS UdfStatus; > > > >>>> + EFI_STATUS GptMbrStatus; > > > >>>> EFI_BLOCK_IO_PROTOCOL *BlockIo; > > > >>>> EFI_BLOCK_IO2_PROTOCOL *BlockIo2; > > > >>>> EFI_DISK_IO_PROTOCOL *DiskIo; > > > >>>> @@ -300,26 +301,47 @@ PartitionDriverBindingStart ( > > > >>>> if (BlockIo->Media->MediaPresent || > > > >>>> (BlockIo->Media->RemovableMedia && !BlockIo->Media- > > > >LogicalPartition)) { > > > >>>> // > > > >>>> - // Try for GPT, then legacy MBR partition types, and then UDF and El > > > Torito. > > > >>>> - // If the media supports a given partition type install child handles to > > > >>>> - // represent the partitions described by the media. > > > >>>> + // Try for UDF (El TOrito compatible) and GPT/MBR partition types. > > > >>>> + // If the media supports a given partition type, it would > > > >>>> + install child > > > handles > > > >>>> + // to represent the partitions described by the media. > > > >>>> + // Notes: GPT is conflicted with MBR. It would only > > > >>>> + install one of > > them to > > > describe > > > >>>> + // the partition. UDF (or El Torito) can exist with MBR, > > > >>>> + so need to > > check > > > it along with > > > >>>> + // GPT/MBR. > > > >>>> // > > > >>>> + UdfStatus = PartitionInstallUdfChildHandles ( > > > >>>> + This, > > > >>>> + ControllerHandle, > > > >>>> + DiskIo, > > > >>>> + DiskIo2, > > > >>>> + BlockIo, > > > >>>> + BlockIo2, > > > >>>> + ParentDevicePath > > > >>>> + ); > > > >>>> + > > > >>>> Routine = &mPartitionDetectRoutineTable[0]; > > > >>>> while (*Routine != NULL) { > > > >>>> - Status = (*Routine) ( > > > >>>> - This, > > > >>>> - ControllerHandle, > > > >>>> - DiskIo, > > > >>>> - DiskIo2, > > > >>>> - BlockIo, > > > >>>> - BlockIo2, > > > >>>> - ParentDevicePath > > > >>>> - ); > > > >>>> - if (!EFI_ERROR (Status) || Status == EFI_MEDIA_CHANGED || > Status > > == > > > EFI_NO_MEDIA) { > > > >>>> + GptMbrStatus = (*Routine) ( > > > >>>> + This, > > > >>>> + ControllerHandle, > > > >>>> + DiskIo, > > > >>>> + DiskIo2, > > > >>>> + BlockIo, > > > >>>> + BlockIo2, > > > >>>> + ParentDevicePath > > > >>>> + ); > > > >>>> + if (!EFI_ERROR (GptMbrStatus) || GptMbrStatus == > > > >>>> + EFI_MEDIA_CHANGED || GptMbrStatus == EFI_NO_MEDIA) { > > > >>>> break; > > > >>>> } > > > >>>> Routine++; > > > >>>> } > > > >>>> + > > > >>>> + if (!EFI_ERROR (UdfStatus) || !EFI_ERROR (GptMbrStatus)) { > > > >>>> + Status = EFI_SUCCESS; > > > >>>> + } else if (UdfStatus == EFI_MEDIA_CHANGED || GptMbrStatus > > > >>>> + == > > > EFI_MEDIA_CHANGED) { > > > >>>> + Status = EFI_MEDIA_CHANGED; > > > >>>> + } else if (UdfStatus == EFI_NO_MEDIA || GptMbrStatus == > > > EFI_NO_MEDIA) { > > > >>>> + Status = EFI_NO_MEDIA; > > > >>>> + } > > > >>>> } > > > >>>> // > > > >>>> // In the case that the driver is already started > > > >>>> (OpenStatus == EFI_ALREADY_STARTED), > > > >>>> -- > > > >>>> 2.21.0.windows.1 > > > >>> > > > >>> > > > >>> > > > >>> > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler 2020-06-29 1:47 ` Gao, Zhichao 2020-06-29 2:42 ` Ni, Ray @ 2020-07-02 9:42 ` Laszlo Ersek 2020-07-06 2:55 ` Gao, Zhichao 1 sibling, 1 reply; 10+ messages in thread From: Laszlo Ersek @ 2020-07-02 9:42 UTC (permalink / raw) To: Gao, Zhichao, Ni, Ray, devel@edk2.groups.io Cc: Wu, Hao A, Wang, Jian J, Gao, Liming On 06/29/20 03:47, Gao, Zhichao wrote: > Hi Laszlo, > > Sorry, I didn't put the detail info about the issue. Let me descript here. > > The issue is not only for Red Hat. I found it with ubuntu 18.02 amd64 and Fedora-20-x86_64 ISO image as well. I didn't view all the linux iso images. OK. Thanks. > > Here is the issue: > Using USB CD ROM with the linux ISO DVD. Run the platform and enter UEFI shell -> plug the USB CD ROM with the ISO DVD -> run "map -r". The CD's file system didn't show. That's right. And, I've always thought that's by design. With all the ISO images I've checked in the UEFI shell before, my experience has been consistent: in the UEFI shell, the ElTorito boot image contents are *visible*, and the file system that the *OS* would show is *not* visible. I've always treated these two content-sets as separate file systems. When we "mount" the ISO under UEFI, we get the ElTorito boot image. When we mount the disk under an OS, we get the "real" file system. What's wrong with that? UEFI needs to be able to boot off of a UEFI-bootable CD-ROM. That requires support for the ElTorito boot image. That support exists. > > It works fine with the normal boot (such as F2, F7 and auto boot) because it would run the *connect all* operation. I think I disagree; ConnectAll is not the reason. Automatic boot works OK because the ElTorito image is processed correctly even *without* a ConnectAll, and the ElTorito image is all that's needed for successfully booting. I think you may be misled the fact that the ElTorito image and the OS-visible filesystem on the disk are *similar*. They have similar contents, but they are not identical. For UEFI booting, the OS-visible filesystem is completely ignored (and that's how it should be). > That would run the partition driver serval times. First time the partition driver would install MBR partition info protocol with the device handle and skip the UDF check. The second time, it would fail the MBR check with *already started* and run UDF check next. That means for such ISO the UEFI would install two partition protocol, both MBR and UDF. I still don't understand how UDF enters the picture. If I loop-mount the "Fedora-20-x86_64-DVD.iso" image on my laptop, the kernel logs the following: > ISO 9660 Extensions: Microsoft Joliet Level 3 > ISO 9660 Extensions: RRIP_1991A Also, the "mount" and "df" commands report the filesystem on the ISO image as "iso9660". It's not UDF. (I do have UDF media too; when I mount it, the kernel logs > UDF-fs: INFO Mounting volume '...', timestamp ... (...) and "mount" and "df" report "udf" as file system.) So I would say that a UDF filesystem should *never* be exposed for "Fedora-20-x86_64-DVD.iso" specifically, under UEFI. ... down-thread, Ray says, > It sounds like a bug in partition driver that the second UDF check > succeeds. And I think I agree. Thanks Laszlo > > But for shell environment, the USB hot plug handle would connect the USB device only once and missing the UDF check. > > I don't know why linux need the MBR info. Maybe for legacy compatible thinking (my opinion, may be wrong). > > If the GPT, MBR and UDF are conflict, then the original logic is fine. But in fact, GPT and MBR are conflict and GPT/MBR and UDF are not conflict. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler 2020-07-02 9:42 ` Laszlo Ersek @ 2020-07-06 2:55 ` Gao, Zhichao 0 siblings, 0 replies; 10+ messages in thread From: Gao, Zhichao @ 2020-07-06 2:55 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com, Ni, Ray Cc: Wu, Hao A, Wang, Jian J, Gao, Liming > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek > Sent: Thursday, July 2, 2020 5:42 PM > To: Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; > devel@edk2.groups.io > Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; > Gao, Liming <liming.gao@intel.com> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf > handler > > On 06/29/20 03:47, Gao, Zhichao wrote: > > Hi Laszlo, > > > > Sorry, I didn't put the detail info about the issue. Let me descript here. > > > > The issue is not only for Red Hat. I found it with ubuntu 18.02 amd64 and > Fedora-20-x86_64 ISO image as well. I didn't view all the linux iso images. > > OK. Thanks. > > > > > Here is the issue: > > Using USB CD ROM with the linux ISO DVD. Run the platform and enter UEFI > shell -> plug the USB CD ROM with the ISO DVD -> run "map -r". The CD's file > system didn't show. > > That's right. > > And, I've always thought that's by design. > > With all the ISO images I've checked in the UEFI shell before, my experience has > been consistent: in the UEFI shell, the ElTorito boot image contents are *visible*, > and the file system that the *OS* would show is *not* visible. > > I've always treated these two content-sets as separate file systems. > When we "mount" the ISO under UEFI, we get the ElTorito boot image. When we > mount the disk under an OS, we get the "real" file system. > > What's wrong with that? > > UEFI needs to be able to boot off of a UEFI-bootable CD-ROM. That requires > support for the ElTorito boot image. That support exists. UEFI can boot ElTorito image because it has a FAT file system. And UEFI would load the bootable image in the FAT. If the block device has FAT file system in them. It should be shown in the UEFI shell. For the above Linux ISO images, they have. > > > > > It works fine with the normal boot (such as F2, F7 and auto boot) because it > would run the *connect all* operation. > > I think I disagree; ConnectAll is not the reason. > > Automatic boot works OK because the ElTorito image is processed correctly even > *without* a ConnectAll, and the ElTorito image is all that's needed for > successfully booting. > > I think you may be misled the fact that the ElTorito image and the OS-visible > filesystem on the disk are *similar*. They have similar contents, but they are not > identical. For UEFI booting, the OS-visible filesystem is completely ignored (and > that's how it should be). Why I mention the *ConnectAll*, because the UEFI binding driver would be ran thru this function. That means the partition driver would run serval times. First time it runs, it would pass the MBR check. Second time it would failed the MBR because the same block already installed, and it would continue to check UDF (ElTorito compatible). So the connect behavior should be run at least twice. Otherwise, the ElTorito image would be missed. This is an incorrect behavior mention below. The USB hot plug handle would only connect the USB device only once. So the hot plug of USB CD ROM under shell would only show the MBR block info. If you run platform with USB CD ROM plugin all the time or hot plug USB CD ROM and run 'reconnnect -r', the CD block with FAT will appear. > > > That would run the partition driver serval times. First time the partition driver > would install MBR partition info protocol with the device handle and skip the UDF > check. The second time, it would fail the MBR check with *already started* and > run UDF check next. That means for such ISO the UEFI would install two partition > protocol, both MBR and UDF. > > I still don't understand how UDF enters the picture. If I loop-mount the "Fedora- > 20-x86_64-DVD.iso" image on my laptop, the kernel logs the > following: > > > ISO 9660 Extensions: Microsoft Joliet Level 3 ISO 9660 Extensions: > > RRIP_1991A > > Also, the "mount" and "df" commands report the filesystem on the ISO image as > "iso9660". It's not UDF. > > (I do have UDF media too; when I mount it, the kernel logs > > > UDF-fs: INFO Mounting volume '...', timestamp ... (...) > > and "mount" and "df" report "udf" as file system.) > > So I would say that a UDF filesystem should *never* be exposed for "Fedora-20- > x86_64-DVD.iso" specifically, under UEFI. Sorry for the unclear description. The UDF I mentioned is ElTorito compatible. There is one commit to merge the ElTorito into the UDF. See 01a68fd37e79c6c7e2f342d7d2c1349325110e99. > > ... down-thread, Ray says, > > > It sounds like a bug in partition driver that the second UDF check > > succeeds. > > And I think I agree. I didn't aware this is a bug before. I make the patch to do the MBR and UDF check at one connect operation. Now I plan to add the check for the MBR table check to skip the one added for windows compatibility. And fix the bug: if one partition check return already started, it would continue to check next routine partition check. Thanks, Zhichao > > Thanks > Laszlo > > > > > But for shell environment, the USB hot plug handle would connect the USB > device only once and missing the UDF check. > > > > I don't know why linux need the MBR info. Maybe for legacy compatible thinking > (my opinion, may be wrong). > > > > If the GPT, MBR and UDF are conflict, then the original logic is fine. But in fact, > GPT and MBR are conflict and GPT/MBR and UDF are not conflict. > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-07-06 2:55 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-06-24 5:56 [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler Gao, Zhichao 2020-06-24 8:32 ` Ni, Ray 2020-06-24 11:57 ` [edk2-devel] " Laszlo Ersek 2020-06-25 0:21 ` Ni, Ray 2020-06-25 9:02 ` Laszlo Ersek 2020-06-29 1:47 ` Gao, Zhichao 2020-06-29 2:42 ` Ni, Ray 2020-06-29 5:23 ` Gao, Zhichao 2020-07-02 9:42 ` Laszlo Ersek 2020-07-06 2:55 ` Gao, Zhichao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox