Re: Functionality issues in UDF support

[Laszlo Ersek <lersek@redhat.com>]

On 09/15/17 18:27, Paulo Alcantara wrote:
> Laszlo,
> 
> On 15/09/2017 12:51, Laszlo Ersek wrote:
>> On 09/15/17 09:38, Ni, Ruiyu wrote:
>>> Paulo,
>>> Supporting multiple LVDs can be considered as a long-term task, or
>>> can be added
>>> per request.
>>> But I think a urgent fix in PartitionDxe driver is to:
>>> 1. create child BLK only for the portion covered by the LVD
>>> 2. Do not create child BLK for LVD that's actually an Eltorito LVD
>>>
>>> I submitted a Bugzilla to record this:
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=707
>>
>> Ray,
>>
>> I've just assigned this bug to myself, for investigation.
>>
>> I think I understand the problem, and the algorithm that's needed for
>> solving the problem is not complex:
>>
>> - iterate over the logical volume descriptors
>> - if the current one is ElTorito, continue iterating
>> - if the current one is not ElTorito, stop, and then install a child for
>>    only the portion of the parent BlkIo (using *inclusive* start and stop
>>    block numbers) that are covered by the current logical volume
>>    descriptor
>> - if no LVD is found that is *not* ElTorito, then install no children,
>>    and report failure
>> - future extension: produce children for all non-ElTorito LVDs.
>>
>> The problem is that I have absolutely no knowledge of UDF specifics, and
>> it would take me quite a bit of time to learn enough to actually
>> implement the details for the above algorithm. I don't think this
>> breakage should be allowed to stay in the tree until either I learn
>> enough to fix it, or Paulo finds enough time to write the code (he
>> doesn't have to start learning from zero, but apparently can't find the
>> time to fix the breakage).
>>
>> If I understand correctly, this is exactly the kind of regression that
>> is *not* localized, it messes up the system. Booting from UDF *Bridge*
>> disks is *already* required by the UEFI spec, and it used to work before
>> the UDF-related changes. I will not quote section "13.3.2.1 ISO-9660 and
>> El Torito" of the UEFI-2.7 spec here, because it is confusing; instead I
>> will quote the improved / proposed wording froam Mantis#1835 (I'm at
>> liberty to quote it because I wrote it):
> 
> I can still boot my Windows 10 ISO image which is an UDF bridge disk
> image with the UDF changes. With or without UdfDxe driver. The problem
> is creating UDF child handles that occupy the entire medium instead of a
> single LVD, hence you can observe the wrong new partition handles &
> device paths from Ray's mapping out.

I understand that -- the regression is not necessarily an OS boot
regression, yet the fact that now the protocol database contains handles
and protocols that should not be there is a regression.

> 
>>
>>   > A DVD-ROM image formatted as required by the UDF 2.0 specification
>>> (OSTA Universal Disk Format Specification, Revision 2.0) shall be
>>> booted by UEFI if:
>>>
>>> - the DVD-ROM image conforms to the "UDF Bridge" format defined in the
>>> UDF 2.0 specification, and
>>>
>>> - the DVD-ROM image contains exactly one ISO-9660 file system, and
>>>
>>> - the ISO-9660 file system conforms to the "El Torito" Bootable CD-ROM
>>> Format Specification.
>>>
>>> Booting from a DVD-ROM satisfying these requirements is accomplished
>>> using the same methods as booting from a CD-ROM: the ISO-9660 file
>>> system shall be booted.
>>
>> Again, such disks / disk images could *already* be booted on edk2,
>> before changing PartitionDxe and adding UdfDxe. By introducing the
>> PartitionDxe changes, previous functionality is disturbed.
>>
>> Namely, according to your description at the start of this thread,
>> PartitionInstallUdfChildHandles() creates handles (with BlockIo
>> protocols and device paths) for such volumes -- ElTorito ones -- that
>> are already *owned* by other components in the firmware. In other words,
>> PartitionInstallUdfChildHandles() infringes on the ownership &
>> responsibilities of other components, when there is a UDF *Bridge*
>> volume in the system, which was handled correctly before.
>>
>>
>> This is the textbook definition of regression. My suggestion is that we
>> disable PartitionInstallUdfChildHandles() entirely, at least
>> temporarily. This should give Paulo enough time to fix the bug *well*. I
>> strongly suggest that we don't try to rush the fix; it could only lead
>> to even more problems.
>>
>> I'm going to submit a patch series that introduces a feature PCD and
>> short-circuits PartitionInstallUdfChildHandles() if the PCD is clear.
> 
> Since I can't fix it in a pleasant time, as well as it's not a simple
> fix, that might help us a lot.

Thank you for your agreement / understanding -- I'd *very much* like
general UDF support to mature to the point where it can be enabled by
default.

Thanks
Laszlo