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

["Gao, Zhichao" <zhichao.gao@intel.com>]

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