From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.6464.1593075746634797823 for ; Thu, 25 Jun 2020 02:02:26 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=jH4iFdsS; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1593075745; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=6jBo7ACUB0zaQtNGGooCgUeofmpLVwfKANJpWaV8iKE=; b=jH4iFdsSRU5BDmiw3fesrkP7bLM8lzg2SGHIPAx1vy7khmOhlKz1vlFw1U+bIFQXZ43ypp ENMzVbwzL5eNg284DpNmd1c9ZmtjZQz2wJEIERk5xmOMmgyKe5E/r0Evka4WfIqwrDBSGc 9hWAuF34IWxc7eEvWolGWP8omwa0MZQ= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-157-kQCx-ge1PY2XZVAlQrT4rQ-1; Thu, 25 Jun 2020 05:02:17 -0400 X-MC-Unique: kQCx-ge1PY2XZVAlQrT4rQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8001280400D; Thu, 25 Jun 2020 09:02:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-16.ams2.redhat.com [10.36.115.16]) by smtp.corp.redhat.com (Postfix) with ESMTP id C34692B4B0; Thu, 25 Jun 2020 09:02:14 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler To: "Ni, Ray" , "devel@edk2.groups.io" , "Gao, Zhichao" Cc: "Wu, Hao A" , "Wang, Jian J" , "Gao, Liming" References: <20200624055610.13984-1-zhichao.gao@intel.com> <734D49CCEBEEF84792F5B80ED585239D5C5D1BBB@SHSMSX104.ccr.corp.intel.com> <2f85dc90-41ae-0b51-4512-d65dbdd143b5@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5C5D2639@SHSMSX104.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 25 Jun 2020 11:02:13 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C5D2639@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 >> Sent: Wednesday, June 24, 2020 7:58 PM >> To: devel@edk2.groups.io; Ni, Ray ; Gao, Zhichao >> Cc: Wu, Hao A ; Wang, Jian J ; Gao, Liming >> 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 >>>> Sent: Wednesday, June 24, 2020 1:56 PM >>>> To: devel@edk2.groups.io >>>> Cc: Wu, Hao A ; Ni, Ray ; Wang, Jian J ; Gao, Liming >>>> >>>> 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 >>>> Cc: Ray Ni >>>> Cc: Jian J Wang >>>> Cc: Liming Gao >>>> Signed-off-by: Zhichao Gao >>>> --- >>>> .../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.
>>>> +Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.
>>>> 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 >>> >>> >>> >>> >