Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler

["Ni, Ray" <ray.ni@intel.com>]

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