public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Paulo Alcantara <pcacjr@zytor.com>
To: "Ni, Ruiyu" <ruiyu.ni@intel.com>, Laszlo Ersek <lersek@redhat.com>
Cc: "edk2-devel@lists.01.org" <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
Date: Fri, 15 Sep 2017 03:26:09 -0300	[thread overview]
Message-ID: <1C01A5C1-09DE-4747-BA65-4EB668D76094@zytor.com> (raw)
In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5BA4212E@SHSMSX151.ccr.corp.intel.com>



Ray,

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

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

Paulo

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

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


  reply	other threads:[~2017-09-15  6:25 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-15  3:33 Functionality issues in UDF support Ni, Ruiyu
2017-09-15  4:57 ` Paulo Alcantara
2017-09-15  5:08   ` Ni, Ruiyu
2017-09-15  5:33     ` Zeng, Star
2017-09-15  5:35       ` Ni, Ruiyu
2017-09-15  5:37     ` Paulo Alcantara
2017-09-15  6:02       ` Ni, Ruiyu
2017-09-15  6:26         ` Paulo Alcantara [this message]
2017-09-15  7:01           ` Zeng, Star
2017-09-15  7:08             ` Ni, Ruiyu
2017-09-15  7:10           ` Ni, Ruiyu
2017-09-15  7:22             ` Paulo Alcantara
2017-09-15  7:38               ` Ni, Ruiyu
2017-09-15 10:26                 ` Laszlo Ersek
2017-09-15 11:03                 ` Laszlo Ersek
2017-09-15 12:45                   ` Ni, Ruiyu
2017-09-15 15:51                 ` Laszlo Ersek
2017-09-15 16:27                   ` Paulo Alcantara
2017-09-15 16:40                     ` Ni, Ruiyu
2017-09-15 16:51                       ` Laszlo Ersek
2017-09-15 23:38                         ` Yao, Jiewen
2017-09-15 23:58                           ` Yao, Jiewen
2017-09-16 21:19                             ` Laszlo Ersek
2017-09-16 20:21                           ` Laszlo Ersek
2017-09-15 16:45                     ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1C01A5C1-09DE-4747-BA65-4EB668D76094@zytor.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox