Re: Functionality issues in UDF support

[Paulo Alcantara <pcacjr@zytor.com>]

Ray,

On September 15, 2017 3:02:11 AM GMT-03:00, "Ni, Ruiyu" <ruiyu.ni@intel.com> wrote:
>Paulo,
>When will you change the implementation to this way?

Unfortunately I can't promise you when I'd be doing this because, you know, I can only work on this in my free time (very short, ATM). Perhaps this weekend - but real life also happens and I may get preempted by something else.

Paulo

>
>
>Thanks/Ray
>
>> -----Original Message-----
>> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>> Sent: Friday, September 15, 2017 1:38 PM
>> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
>> Cc: edk2-devel@lists.01.org; Wu, Hao A <hao.a.wu@intel.com>; Zeng,
>Star
>> <star.zeng@intel.com>
>> Subject: RE: Functionality issues in UDF support
>> 
>> 
>> 
>> Hi,
>> 
>> On September 15, 2017 2:08:28 AM GMT-03:00, "Ni, Ruiyu"
>> <ruiyu.ni@intel.com> wrote:
>> >Hi Paulo,
>> >The responsibility of PartitionDxe driver is to partition the
>physical
>> >BLK (the entire CDROM) into logical BLKs (one BLK for each volume).
>> >It doesn't make sense to create a child BLK handle which still
>covers
>> >the entire CDROM.
>> >So as I suggested in below, PartitionInstallUdfChildHandles() should
>> >contain a for-loop to iterate all the volumes in the CDROM, and
>create
>> >child BLK handle for each volumes, but skipping Eltorito volume.
>> 
>> Ah, makes sense to me now. Thanks for clarifying it. So I agree with
>you.
>> 
>> In whatever you guys decide to do with it, count on me to give some
>help.
>> 
>> Thanks!
>> Paulo
>> 
>> >
>> >Thanks/Ray
>> >
>> >> -----Original Message-----
>> >> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>> >> Sent: Friday, September 15, 2017 12:58 PM
>> >> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek
><lersek@redhat.com>
>> >> Cc: edk2-devel@lists.01.org; Wu, Hao A <hao.a.wu@intel.com>; Zeng,
>> >Star
>> >> <star.zeng@intel.com>
>> >> Subject: Re: Functionality issues in UDF support
>> >>
>> >>
>> >>
>> >> Hi Ray,
>> >>
>> >> On September 15, 2017 12:33:04 AM GMT-03:00, "Ni, Ruiyu"
>> >> <ruiyu.ni@intel.com> wrote:
>> >> >Paulo,
>> >> >Before raising my questions, I'd like to confirm that for a
>single
>> >> >CD/DVD in UDF format, there might be multiple volumes.
>> >> >One of the volume could be Eltorito type.
>> >> >If my understanding is correct, please continue reading below.
>> >>
>> >> Right.
>> >>
>> >> >
>> >> >We found below mapping table using "map -r" shell command in a
>> >platform
>> >> >with only PartitionDxe change and without UdfDxe driver.
>> >> >It's a bug that <BLK6> and <BLK7/FS2> are created. Actually they
>are
>> >> >identical to <BLK3> and <BLK4/FS1>.
>> >> >
>> >> >--- Mapping table---
>> >> >     BLK2: Alias(s):
>> >> >          PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)
>> >> >
>> >> >     BLK3: Alias(s):
>> >> >         
>PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)/CDROM(0x0)
>> >> >      FS1: Alias(s):CD1a65535a1:;BLK4:
>> >> >         
>PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)/CDROM(0x1)
>> >> >
>> >> >     BLK5: Alias(s):
>> >>
>>PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)/VenMedia(C5BD4D42-
>> >> 1A76-4996-8956-73CDA326CD0A)
>> >> >     BLK6: Alias(s):
>> >>
>>PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)/VenMedia(C5BD4D42-
>> >> 1A76-4996-8956-73CDA326CD0A)/CDROM(0x0)
>> >> >      FS2: Alias(s):CD1a65535ab:;BLK7:
>> >>
>>PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)/VenMedia(C5BD4D42-
>> >> 1A76-
>> >> >4996-8956-73CDA326CD0A)/CDROM(0x1)
>> >> >--- End of mapping table ---
>> >> >
>> >> >After investigation, I found the UDF logic in Partition driver
>> >doesn't
>> >> >truly skip the Eltorito volume.
>> >> >The code flow is like below:
>> >> >
>> >> >  1.  <BLK2> is created by ScsiDiskDxe driver.
>> >> >  2.  By passing <BLK2> to PartitionDxe Start()
>> >> >*   <BLK3> and <BLK4/FS1> are created by PartitionDxe driver, by
>> >> >PartitionInstallElToritoChildHandles().
>> >> >*   <BLK5> is created by PartitionDxe driver, by
>> >> >PartitionInstallUdfChildHandles().
>> >> >  3.  By passing <BLK5> to PartitionDxe Start()
>> >> >*   <BLK6> and <BLK7/FS2> are created by PartitionDxe driver, by
>> >> >PartitionInstallElToritoChildHandles().
>> >> >
>> >> >I think step 2.a is not correct if my understanding to UDF is
>> >correct.
>> >> >The PartitionInstallUdfChildHandles() should iterate all volumes
>in
>> >the
>> >> >media and creates the child BLK handle for each volume, but
>skipping
>> >> >Eltorito volume.
>> >> >
>> >> >Instead, the current implementation just creates one child BLK
>> >handle
>> >> >for the entire media.
>> >> >To avoid reclusively creating  child BLK handle, the
>> >> >PartitionInstallUdfChildHandles() contains a logic to do nothing
>> >when
>> >> >the handle is created by ParititonDxe driver. The logic is not
>> >needed
>> >> >when the implementation follows my suggestion above.
>> >> >Due to this, step 3.a creates the additional but shouldn't-exist
>BLK
>> >> >handles <BLK6> and <BLK7/FS2>.
>> >> >
>> >> >UdfDxe driver is supposed to Start() on each volume and produce
>> >> >SimpleFileSystem protocol.
>> >>
>> >> It seems that PartitionInstallUdfChildHandles() indeed skips the
>> >ElTorito
>> >> partitions otherwise you'd see the ".../CDROM(0x1)/VenMedia()" and
>> >> ".../CDROM(0x0)/VenMedia()" device paths *also* in the mapping
>> >output.
>> >>
>> >> If I understand correctly, the problem is that when we create a
>child
>> >handle
>> >> for an UDF volume, Partition driver will execute again, parse the
>> >newly
>> >> created UDF handle, find again an ElTorito partition and then
>install
>> >a new
>> >> child handle (VenMedia()/CDROM())
>> >>
>> >> So, the logic of skipping of ElTorito partitions in
>> >> PartitionInstallUdfChildHandles() is not enough. Unfortunately we
>> >can't
>> >> handle UDF bridge disks (ElTorito + UDF) entirely in
>> >> PartitionInstallUdfChildHandles() -- we should probably also skip
>UDF
>> >device
>> >> paths in PartitionInstallElToritoChildHandles().
>> >>
>> >> Does it make sense to you, Ray? Thanks for raising this up.
>> >>
>> >> Paulo
>> >>
>> >> >
>> >> >Do you agree with my above suggestions?
>> >> >
>> >> >Laszlo,
>> >> >I understood your needs of this UDF support. But as you can see
>> >there
>> >> >are many build failures and even functionality issues due to this
>> >> >support.
>> >> >I am not sure how the other open source project handles such
>cases.
>> >> >But I am thinking maybe we could move the whole UDF support to
>> >> >edk2-staging firstly and move it back after all the issues are
>> >> >resolved.
>> >> >What's your suggestion?
>> >> >
>> >> >Thanks/Ray
>> >>
>> >> --
>> >> Sent from my Android device with K-9 Mail. Please excuse my
>brevity.
>> 
>> --
>> Sent from my Android device with K-9 Mail. Please excuse my brevity.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.