Re: Functionality issues in UDF support

["Zeng, Star" <star.zeng@intel.com>]

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.