Re: Functionality issues in UDF support

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

Yes. Your understanding is correct.

Thanks/Ray

> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, September 15, 2017 1:34 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Paulo Alcantara <pcacjr@zytor.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
> 
> So the BlockIo produced by Partition driver should be logic with
> EFI_BLOCK_IO_MEDIA.LogicPartition = TRUE, right?
> Then for the case, the BLK5 shouldn't be created at all, and then there will be
> no BLK6 and BLK7.
> 
>      BLK5: Alias(s):
>           PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)/VenMedia(C5BD4D42-
> 1A76-4996-8956-73CDA326CD0A)
> 
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Friday, September 15, 2017 1:08 PM
> To: Paulo Alcantara <pcacjr@zytor.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 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.
> 
> 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.