Functionality issues in UDF support

Functionality issues in UDF support

[Ni, Ruiyu]

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.

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.

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

Re: Functionality issues in UDF support

[Paulo Alcantara]

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.

Re: Functionality issues in UDF support

[Ni, Ruiyu]

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.

Re: Functionality issues in UDF support

[Zeng, Star]

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.

Re: Functionality issues in UDF support

[Ni, Ruiyu]

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.

Re: Functionality issues in UDF support

[Paulo Alcantara]

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.

Re: Functionality issues in UDF support

[Ni, Ruiyu]

Paulo,
When will you change the implementation to this way?


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.

Re: Functionality issues in UDF support

[Paulo Alcantara]

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.

Re: Functionality issues in UDF support

[Zeng, Star]

How serious is this issue?
Only causing more BLKs are shown, but not impacting Eltorito? Or impacting Eltorito definitely?



Thanks,
Star
-----Original Message-----
From: Paulo Alcantara [mailto:pcacjr@zytor.com] 
Sent: Friday, September 15, 2017 2:26 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



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.

Re: Functionality issues in UDF support

[Ni, Ruiyu]

Star,
Eltorito file system access should work.
But UDF support may not work as expected if there are multiple
volumes in the CDROM.

Thanks/Ray

> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, September 15, 2017 3:01 PM
> To: Paulo Alcantara <pcacjr@zytor.com>; 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
> 
> How serious is this issue?
> Only causing more BLKs are shown, but not impacting Eltorito? Or impacting
> Eltorito definitely?
> 
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
> Sent: Friday, September 15, 2017 2:26 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
> 
> 
> 
> 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.

Re: Functionality issues in UDF support

[Ni, Ruiyu]

Paulo,
Can current code support multiple volumes in CDROM?

Thanks/Ray

> -----Original Message-----
> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
> Sent: Friday, September 15, 2017 2:26 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
> 
> 
> 
> 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.

Re: Functionality issues in UDF support

[Paulo Alcantara]

On September 15, 2017 4:10:10 AM GMT-03:00, "Ni, Ruiyu" <ruiyu.ni@intel.com> wrote:
>Paulo,
>Can current code support multiple volumes in CDROM?

No. It can handle only a single UDF logical volume. I think I never found an UDF disk or image with more than a LVD (Logical Volume Descriptior). Although I think I also added some comment in the code that it currently handles only one logical volume per disk (no access to the code right now, sorry). Other than that, there should be also a DEBUG() or perhaps an ASSERT() in case more than one is found.

For supporting multiple logical volumes, we would have to follow your suggestion and change Partition driver to install a child handle for each LVD found in an UDF fs.

Thanks!
Paulo

>
>Thanks/Ray
>
>> -----Original Message-----
>> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>> Sent: Friday, September 15, 2017 2:26 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
>> 
>> 
>> 
>> 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.

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

Re: Functionality issues in UDF support

[Ni, Ruiyu]

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


Thanks/Ray

> -----Original Message-----
> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
> Sent: Friday, September 15, 2017 3:22 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao,
> Jiewen <jiewen.yao@intel.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
> 
> 
> 
> On September 15, 2017 4:10:10 AM GMT-03:00, "Ni, Ruiyu"
> <ruiyu.ni@intel.com> wrote:
> >Paulo,
> >Can current code support multiple volumes in CDROM?
> 
> No. It can handle only a single UDF logical volume. I think I never found an
> UDF disk or image with more than a LVD (Logical Volume Descriptior).
> Although I think I also added some comment in the code that it currently
> handles only one logical volume per disk (no access to the code right now,
> sorry). Other than that, there should be also a DEBUG() or perhaps an
> ASSERT() in case more than one is found.
> 
> For supporting multiple logical volumes, we would have to follow your
> suggestion and change Partition driver to install a child handle for each LVD
> found in an UDF fs.
> 
> Thanks!
> Paulo
> 
> >
> >Thanks/Ray
> >
> >> -----Original Message-----
> >> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
> >> Sent: Friday, September 15, 2017 2:26 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
> >>
> >>
> >>
> >> 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.
> 
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: Functionality issues in UDF support

[Laszlo Ersek]

Hello All,

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

after reading through this thread:

(1) I believed that those parts of the UDF enablement that could
actually interfere with other parts of the firmware, such as driver
model and PartitionDxe integration, were thoroughly vetted. At this
point I'm unsure if this bug specifically would have been possible to
catch in review, but at this point it doesn't really matter.

(2) If the bug *regresses* preexistent functionality, then UDF support
should urgently be turned into a conditional feature. I thought that
adding -D UDF_ENABLE to platform DSC files would be enough, but in this
case, for PartitionDxe, looks like a new FeaturePCD would be necessary,
at a minimum?

Please note that the following situation is also a regression: some UDF
media is present in the system, and now other media (or functionality)
stops working or gets confused. This is a regression relative to
previous state, where UDF media would simply not be recognized, but
everything else would work.

(3) If the bug does not regress preexistent functionality (in the above
sense), then I think it's OK to file a BZ for the issue, and resolve it
as time allows.


Ray, Star, what is your assessment about point (2) -- is this bug that
kind of regression? If so, what options do you consider acceptable or
best for mitigating it, in PartitionDxe? FeaturePCD perhaps?

Thanks,
Laszlo

Re: Functionality issues in UDF support

[Laszlo Ersek]

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,

from the discussion it looks like the issue only hits with multi-volume
media. Paulo stated he had never seen such media. Can you share your
reproducer media perhaps (even privately, if it can be done legally), or
else provide instructions to create, or obtain, such media?

Thanks!
Laszlo

Re: Functionality issues in UDF support

[Ni, Ruiyu]

Laszlo,
I may not state the problem very clearly.
I saw two problems:
1. ParitionDxe shouldn't create BLK handle for the entire media again. Our QA complains
     several new but unexpected BLK devices are seen. This bug is easy to fix and needs to
     be fixed ASAP. So I created a BZ (https://bugzilla.tianocore.org/show_bug.cgi?id=707).

2. PartitionDxe should handle multiple LVDs in a UDF CDROM. No one complains this.
     I am fine to accept the limitation. So I even didn't create a BZ.
     Anyone who wants this support can submit a BZ.
     I guess following the same pattern of existing code to support it should be easy.

Thanks,
Ray

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Friday, September 15, 2017 7:03 PM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Yao, Jiewen <jiewen.yao@intel.com>
Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] Functionality issues in UDF support

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,

from the discussion it looks like the issue only hits with multi-volume media. Paulo stated he had never seen such media. Can you share your reproducer media perhaps (even privately, if it can be done legally), or else provide instructions to create, or obtain, such media?

Thanks!
Laszlo

Re: Functionality issues in UDF support

[Laszlo Ersek]

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 from Mantis#1835 (I'm at
liberty to quote it because I wrote it):

 > 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.

Thank you,
Laszlo

Re: Functionality issues in UDF support

[Paulo Alcantara]

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.

> 
>   > 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.

Thanks!
Paulo

Re: Functionality issues in UDF support

[Ni, Ruiyu]

Laszlo,
Please do not add a PCD for this. Too many PCDs are no good to the project.

发自我的 iPhone

> 在 2017年9月16日，上午12:27，Paulo Alcantara <pcacjr@zytor.com> 写道：
> 
> 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.
> 
>>  > 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.
> 
> Thanks!
> Paulo

Re: Functionality issues in UDF support

[Laszlo Ersek]

On 09/15/17 18:40, Ni, Ruiyu wrote:
> Laszlo,
> Please do not add a PCD for this. Too many PCDs are no good to the project.

I understand that new MdeModulePkg PCDs are not liked, but what do you
propose instead? If we simply revert the PartitionDxe changes, then
people that want to experiment with general UDF support under OVMF won't
be able to do that at all.

I'm in the process of adding -D UDF_ENABLE to OvmfPkg, ArmVirtPkg, and
Nt32Pkg, which would control both the FeaturePCD and the inclusion of
UdfDxe in the build. If you disagree with the FeaturePCD, I can stop
working on this, but I don't know what the alternative is. "Fix it
immediately" is not an alternative; we can't do that. If you want to
revert the change, it's your prerogative, but that will prevent
everybody from testing gradual UDF improvements. (No 3rd parties build
OVMF from any staging branches, so if the feature is only available on a
staging branch, it might as well not exist, for the outside world.)

Thanks
Laszlo

Re: Functionality issues in UDF support

[Yao, Jiewen]

Hi Laszlo and Ruiyu
I can think 1 possible alternative, for your consideration only.

1)       Move the feature to OvmfPkg.

As such, it won't block us at this moment.

Once the UDF solution has good quality, we can move it back to MdeModulePkg.

Thank you
Yao Jiewen

From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Saturday, September 16, 2017 12:51 AM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; Paulo Alcantara <pcacjr@zytor.com>
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] Functionality issues in UDF support

On 09/15/17 18:40, Ni, Ruiyu wrote:
> Laszlo,
> Please do not add a PCD for this. Too many PCDs are no good to the project.

I understand that new MdeModulePkg PCDs are not liked, but what do you
propose instead? If we simply revert the PartitionDxe changes, then
people that want to experiment with general UDF support under OVMF won't
be able to do that at all.

I'm in the process of adding -D UDF_ENABLE to OvmfPkg, ArmVirtPkg, and
Nt32Pkg, which would control both the FeaturePCD and the inclusion of
UdfDxe in the build. If you disagree with the FeaturePCD, I can stop
working on this, but I don't know what the alternative is. "Fix it
immediately" is not an alternative; we can't do that. If you want to
revert the change, it's your prerogative, but that will prevent
everybody from testing gradual UDF improvements. (No 3rd parties build
OVMF from any staging branches, so if the feature is only available on a
staging branch, it might as well not exist, for the outside world.)

Thanks
Laszlo

Re: Functionality issues in UDF support

[Yao, Jiewen]

Hi Laszlo/Ruiyu/Star
I went through the whole conversation and have some thought:

For staging, I quote the description from https://github.com/tianocore/edk2-staging

I think UDF matches all below criteria. "edk2 required quality criteria" is the key.

We have lots of features there - such as StandaloneSmm, CustomizedSecureBoot, RiscV, ResetSystem, StructurePcd.

All those features need more validation.
We are following the rule and I think UDF feature can follow the same rule.

If there is quality concern and the quality concern cannot be resolved in a short period of time, the staging tree is a better choice.
(NOTE: "Fix it immediately" is not an alternative; we can't do that.)



========================================================
This repository is used by EDK II as a staging location for new
features that are not yet ready for inclusion in EDK II.

Introduction
=================
Need place on tianocore.org where new features that are not ready for product
integration can be checked in for evaluation by the EDK II community prior to
adding to the edk2 trunk.  This serves several purposes:

* Encourage source code to be shared earlier in the development process
* Allow source code to be shared that does not yet meet all edk2 required quality criteria
* Allow source code to be shared so the EDK II community can help finish and validate new features
* Provide a location to hold new features until they are deemed ready for product integration
* Provide a location to hold new features until there is a natural point in edk2 release cycle to fully validate the new feature.



From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
Sent: Saturday, September 16, 2017 7:39 AM
To: Laszlo Ersek <lersek@redhat.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Paulo Alcantara <pcacjr@zytor.com>
Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] Functionality issues in UDF support

Hi Laszlo and Ruiyu
I can think 1 possible alternative, for your consideration only.

1)       Move the feature to OvmfPkg.

As such, it won't block us at this moment.

Once the UDF solution has good quality, we can move it back to MdeModulePkg.

Thank you
Yao Jiewen

From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Saturday, September 16, 2017 12:51 AM
To: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Paulo Alcantara <pcacjr@zytor.com<mailto:pcacjr@zytor.com>>
Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
Subject: Re: [edk2] Functionality issues in UDF support

On 09/15/17 18:40, Ni, Ruiyu wrote:
> Laszlo,
> Please do not add a PCD for this. Too many PCDs are no good to the project.

I understand that new MdeModulePkg PCDs are not liked, but what do you
propose instead? If we simply revert the PartitionDxe changes, then
people that want to experiment with general UDF support under OVMF won't
be able to do that at all.

I'm in the process of adding -D UDF_ENABLE to OvmfPkg, ArmVirtPkg, and
Nt32Pkg, which would control both the FeaturePCD and the inclusion of
UdfDxe in the build. If you disagree with the FeaturePCD, I can stop
working on this, but I don't know what the alternative is. "Fix it
immediately" is not an alternative; we can't do that. If you want to
revert the change, it's your prerogative, but that will prevent
everybody from testing gradual UDF improvements. (No 3rd parties build
OVMF from any staging branches, so if the feature is only available on a
staging branch, it might as well not exist, for the outside world.)

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel

Re: Functionality issues in UDF support

[Laszlo Ersek]

Hi Jiewen,

thank you for looking into this. I'm replying below quite at length;
please bear with me. At the end I'm going to propose a new alternative
to the PCD:

On 09/16/17 01:58, Yao, Jiewen wrote:
> Hi Laszlo/Ruiyu/Star
> I went through the whole conversation and have some thought:
> 
> For staging, I quote the description from https://github.com/tianocore/edk2-staging
> 
> I think UDF matches all below criteria. "edk2 required quality criteria" is the key.
> 
> We have lots of features there - such as StandaloneSmm, CustomizedSecureBoot, RiscV, ResetSystem, StructurePcd.
> 
> All those features need more validation.
> We are following the rule and I think UDF feature can follow the same rule.
> 
> If there is quality concern and the quality concern cannot be resolved in a short period of time, the staging tree is a better choice.
> (NOTE: "Fix it immediately" is not an alternative; we can't do that.)

Moving the UDF feature, including the PartitionDxe changes, to
edk2-staging would require the following:


(1) A maintainer that owns and maintains the UDF branch.

- I definitely cannot be this person, first because I have no UDF
knowledge, and second because I have no time for any more maintainer
responsibilities than I currently have. Again, my interest in UDF is
*only* as a user.

- I assume Ray will also have no time for maintaining a UDF feature
branch in edk2-staging.

- If Paulo is willing to become a maintainer for the UDF branch in
edk2-staging, that's good. Assuming he is *allowed* to become such a
maintainer.

(According to the edk2-staging description, periodic rebases to
edk2/master are required, hence Paulo would be required to dedicate time
to maintaining "edk2-staging/udf" on an on-going basis.)


(2) The UDF-related commits would have to be reverted from edk2, and
applied to edk2-staging/udf.

The first part (the reverts) would be bad. The commits that have gone
into UDF support thus far -- namely the build error fixes from everyone,
and there's also a pending style improvement series from Hao Wu -- have
already improved UDF a lot, and would have never happened in the staging
repository.

Furthermore, let me explain how most end-users (upstream users) consume
OVMF:
- edk2 maintainers push patches to edk2/master,
- various people (Ard and others from Linaro, my colleague Gerd Hoffmann
on his own time, probably others)  operate build bots / CI environments
that fetch edk2 master every day, and build RPM, DEB etc. packages for OVMF
- end-users set up YUM or APT repositories on their machines, pointing
to these packages, and periodically update their OVMF packages.

This means we have a significant user base. If we break something in
edk2/master, the build bots report build failures, and the users report
functional failures. This is good for edk2/master -- not just for
OvmfPkg, but for many other modules built into the OVMF binary.

If we move UDF to edk2-staging/udf, then UDF support will never reach
these end-users. The people that operate the CI environments will not
repeatedly merge edk2-staging/udf into edk2/master on their end, just so
they can build UDF support into OVMF. Such merges can even fail, if
rebasing the edk2-staging/udf branch to the current edk2/master branch
requires *manual* conflict resolution, and the edk2-staging/udf
maintainer is busy.

--*--

I understand that you guys absolutely don't want a new FeaturePCD. How
about the following alternative. When the edk2-staging repository was
initially discussed, I mentioned to Mike that Staging or Experimental
directories would also be a possibility, inside edk2/master. So the
below is a variant of that idea:

(a) In the "MdeModulePkg/Universal/Disk/PartitionDxe" directory, create
another directory called "Experimental".

(b) Move the "Udf.c" source file to:

  MdeModulePkg/Universal/Disk/PartitionDxe/Experimental/Udf.c

(c) Create a file called "Stubs.c":

  MdeModulePkg/Universal/Disk/PartitionDxe/Stubs.c

This file would contain an implementation of
PartitionInstallUdfChildHandles() that returned EFI_NOT_FOUND
*unconditionally*, and did nothing else.

(d) In the "PartitionDxe.inf" file, replace "Udf.c" with "Stubs.c",
under [Sources].

(e) Create "PartitionExperimentalDxe.inf" in the same directory where
"PartitionDxe.inf" is:

  MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
  MdeModulePkg/Universal/Disk/PartitionDxe/PartitionExperimentalDxe.inf

The second INF file would be identical to "PartitionDxe.inf", except it
would include "Experimental/Udf.c", rather than "Stubs.c".


The end result is that the PartitionDxe driver is built with just a UDF
stub function that does nothing, and the PartitionExperimentalDxe driver
is built with UDF support. The current regression would be fixed for all
platforms immediately that use

  MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf

No new PCD for MdeModulePkg, just a new INF file, and a stub file.

In turn, OvmfPkg, ArmVirtPkg, and Nt32Pkg could accept -D UDF_ENABLE,
and select the proper INF file, based on UDF_ENABLE.

The -D UDF_ENABLE flag would also be very easy to add, for the people
that operate the CI environments / build bots.


If this were acceptable, I'd be glad to send patches for it.

If it's not acceptable, then I think we should simply revert the UDF
patches from edk2, and *not* bother about creating an edk2-staging/udf
branch. I don't believe that CI operators and end-users would care about
edk2-staging/udf, even if Paulo spent the time and rebased that branch
to edk2/master very frequently. (Above, you mentioned the staging
branches StandaloneSmm, CustomizedSecureBoot, RiscV, ResetSystem,
StructurePcd -- and those are branches that *I* don't know about.)

So, can we do "PartitionExperimentalDxe.inf"?

Thanks!
Laszlo



> ========================================================
> This repository is used by EDK II as a staging location for new
> features that are not yet ready for inclusion in EDK II.
> 
> Introduction
> =================
> Need place on tianocore.org where new features that are not ready for product
> integration can be checked in for evaluation by the EDK II community prior to
> adding to the edk2 trunk.  This serves several purposes:
> 
> * Encourage source code to be shared earlier in the development process
> * Allow source code to be shared that does not yet meet all edk2 required quality criteria
> * Allow source code to be shared so the EDK II community can help finish and validate new features
> * Provide a location to hold new features until they are deemed ready for product integration
> * Provide a location to hold new features until there is a natural point in edk2 release cycle to fully validate the new feature.
> 
> 
> 
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
> Sent: Saturday, September 16, 2017 7:39 AM
> To: Laszlo Ersek <lersek@redhat.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Paulo Alcantara <pcacjr@zytor.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] Functionality issues in UDF support
> 
> Hi Laszlo and Ruiyu
> I can think 1 possible alternative, for your consideration only.
> 
> 1)       Move the feature to OvmfPkg.
> 
> As such, it won't block us at this moment.
> 
> Once the UDF solution has good quality, we can move it back to MdeModulePkg.
> 
> Thank you
> Yao Jiewen
> 
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Saturday, September 16, 2017 12:51 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Paulo Alcantara <pcacjr@zytor.com<mailto:pcacjr@zytor.com>>
> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
> Subject: Re: [edk2] Functionality issues in UDF support
> 
> On 09/15/17 18:40, Ni, Ruiyu wrote:
>> Laszlo,
>> Please do not add a PCD for this. Too many PCDs are no good to the project.
> 
> I understand that new MdeModulePkg PCDs are not liked, but what do you
> propose instead? If we simply revert the PartitionDxe changes, then
> people that want to experiment with general UDF support under OVMF won't
> be able to do that at all.
> 
> I'm in the process of adding -D UDF_ENABLE to OvmfPkg, ArmVirtPkg, and
> Nt32Pkg, which would control both the FeaturePCD and the inclusion of
> UdfDxe in the build. If you disagree with the FeaturePCD, I can stop
> working on this, but I don't know what the alternative is. "Fix it
> immediately" is not an alternative; we can't do that. If you want to
> revert the change, it's your prerogative, but that will prevent
> everybody from testing gradual UDF improvements. (No 3rd parties build
> OVMF from any staging branches, so if the feature is only available on a
> staging branch, it might as well not exist, for the outside world.)
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: Functionality issues in UDF support

[Laszlo Ersek]

Jiewen,

On 09/16/17 01:38, Yao, Jiewen wrote:
> Hi Laszlo and Ruiyu
> I can think 1 possible alternative, for your consideration only.
> 
> 1)       Move the feature to OvmfPkg.
> 
> As such, it won't block us at this moment.
> 
> Once the UDF solution has good quality, we can move it back to MdeModulePkg.

the urgent issue is not with the UdfDxe driver; a platform can simply
choose not to include the UdfDxe driver. The problem is that the
UDF-related changes in PartitionDxe have caused a regression in
PartitionDxe, and PartitionDxe is something that all platforms must include.

The feature PCD that I suggested would turn the (currently buggy) UDF
logic in PartitionDxe into a no-op by default.

Thanks
Laszlo


> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Saturday, September 16, 2017 12:51 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Paulo Alcantara <pcacjr@zytor.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] Functionality issues in UDF support
> 
> On 09/15/17 18:40, Ni, Ruiyu wrote:
>> Laszlo,
>> Please do not add a PCD for this. Too many PCDs are no good to the project.
> 
> I understand that new MdeModulePkg PCDs are not liked, but what do you
> propose instead? If we simply revert the PartitionDxe changes, then
> people that want to experiment with general UDF support under OVMF won't
> be able to do that at all.
> 
> I'm in the process of adding -D UDF_ENABLE to OvmfPkg, ArmVirtPkg, and
> Nt32Pkg, which would control both the FeaturePCD and the inclusion of
> UdfDxe in the build. If you disagree with the FeaturePCD, I can stop
> working on this, but I don't know what the alternative is. "Fix it
> immediately" is not an alternative; we can't do that. If you want to
> revert the change, it's your prerogative, but that will prevent
> everybody from testing gradual UDF improvements. (No 3rd parties build
> OVMF from any staging branches, so if the feature is only available on a
> staging branch, it might as well not exist, for the outside world.)
> 
> Thanks
> Laszlo
> 

Re: Functionality issues in UDF support

[Laszlo Ersek]

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