[U-Boot] [PATCH v2 2/3] efi_loader: enumerate disk devices every time

[AKASHI Takahiro]

Heinrich,

On Thu, Jan 24, 2019 at 10:19:28PM +0100, Heinrich Schuchardt wrote:
> On 1/24/19 9:18 PM, Simon Glass wrote:
> > Hi Takahiro,
> > 
> > On Thu, 24 Jan 2019 at 13:53, AKASHI Takahiro
> > <takahiro.akashi at linaro.org> wrote:
> >>
> >> Simon,
> >>
> >> On Thu, Jan 24, 2019 at 10:58:53AM +1300, Simon Glass wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Wed, 23 Jan 2019 at 10:05, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>>>
> >>>> On 1/22/19 8:39 PM, Simon Glass wrote:
> >>>>> Hi Alex,
> >>>>>
> >>>>> On Tue, 22 Jan 2019 at 22:08, Alexander Graf <agraf at suse.de> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 22.01.19 09:29, AKASHI Takahiro wrote:
> >>>>>>> Alex, Simon,
> >>>>>>>
> >>>>>>> Apologies for my slow response on this matter,
> >>>>>>>
> >>>>>>> On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 11.01.19 05:29, AKASHI Takahiro wrote:
> >>>>>>>>> Alex, Heinrich and Simon,
> >>>>>>>>>
> >>>>>>>>> Thank you for your comments, they are all valuable but also make me
> >>>>>>>>> confused as different people have different requirements :)
> >>>>>>>>> I'm not sure that all of us share the same *ultimate* goal here.
> >>>>>>>>
> >>>>>>>> The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
> >>>>>>>
> >>>>>>> I don't still understand what "merge" means very well.
> >>>>>>
> >>>>>> It basically means that "struct efi_object" moves into "struct udevice".
> >>>>>> Every udevice instance of type UCLASS_BLK would expose the block and
> >>>>>> device_path protocols.
> >>>>>>
> >>>>>> This will be a slightly bigger rework, but eventually allows us to
> >>>>>> basically get rid of efi_init_obj_list() I think.
> >>>>>
> >>>>> I envisaged something like:
> >>>>>
> >>>>> - EFI objects have their own UCLASS_EFI uclass
> >>>>> - DM uclasses which support EFI would create a child EFI device (e.g.
> >>>>> a UCLASS_EFI child of each UCLASS_BLK device)
> >>>>> - EFI-uclass devices would thus be bound as needed
> >>>>> - Probing an EFI device would causes its parents to be probed
> >>>>> - We can use all the existing DM hooks (probe, remove, parent/child
> >>>>> data, operations), to implement EFI things
> >>>>>
> >>>>> I'm assuming that a small percentage of devices would have EFI
> >>>>> children, so that this is more efficient than trying to merge the data
> >>>>> structures. It also allows EFI to maintain some separate from the core
> >>>>> DM code.
> >>>>
> >>>> Dear Simon,
> >>>>
> >>>> thanks to your suggestions.
> >>>>
> >>>> I am not yet convinced that an UCLASS_EFI child device will be helpful.
> >>>> It is not an EFI object.
> >>>>
> >>>> A DM uclass is the equivalent to an EFI driver, i.e. a handle with the
> >>>> EFI_DRIVER_BINDING_PROTOCOL installed on it [1]. So the natural thing
> >>>> for a uclass supporting EFI would be to provide such a handle.
> >>>>
> >>>> For the actual devices we also need handles.
> >>>
> >>> Yes but I understand why this is problematic?
> >>>
> >>>>
> >>>> In the EFI world partitions are devices. How does this fit into your
> >>>> picture?
> >>>
> >>> Perhaps we could consider adding a UCLASS_PARTITION? The rework of the
> >>> FS layer may not be too much - at least once we drop the non-BLK code
> >>> (as you say at [1]).
> >>
> >> IMO, instead of considering UCLASS_PARTITION,
> >> 1) add IF_TYPE_BLK_PARTITION and set its type_class_id to UCLASS_BLK
> >>    So any partition has a parent node (I don't know the correct language
> >>    in DM world) that is a real raw disk device, and yet is seen
> >>    as a UCLASS_BLK device, or
> > 
> > Well 'parent node' is used in device tree. For driver model, we say
> > 'parent device'.
> > 
> > In fact enum if_type is not so necessary in the DM world. We could
> > instead use the uclass of the parent. For example, we could use
> > UCLASS_MMC instead of IF_TYPE_MMC.
> > 
> > So I'm not sure that it is right to set the if_type to UCLASS_BLK.
> > There are many U-Boot commands which number the different 'interface
> > types' separately. For example 'mmc' devices create a block device
> > with blk_desc containing devnum values numbered from 0, as do 'sata'
> > devices, etc. So what does it mean to have a generic 'block' interface
> > type?
> > 
> >>
> >> 2) create a block device (blk_desc) for each partition, either in bind/probe
> >>    or in enumerating devices, as I mentioned in the previous e-mail
> > 
> > Here you are really just creating a device of UCLASS_BLK, since there
> > is a 1:1 correspondence between a UCLASS_BLK device and blk_desc.
> > 
> > struct blk_desc *desc = dev_get_uclass_platdata(blk_dev);
> > 
> >>
> >> What's different between (1) and (2),
> >> we may enumerate block devices and identify the given one by scanning
> >> a UCLASS_BLK list with (1), while by scanning a blk_desc list with (2)
> >> at do_load()/fs_set_blk_dev().
> > 
> > As above these are really the same, in that a blk_desc can only exist
> > as an attachment to a UCLASS_BLK / block device.
> > 
> >>
> >> # In any way, we will need
> >> a) a bi-directional link between
> >>      UCLASS_BLK          efi_obj
> >>          or        and       or
> >>      blk_desc            efi_disk_obj, and
> >> b) a event notification mechanism, in your language, as as to maintain
> >>    (create/delete) those links
> >>
> >> If you agree to approach (1) or (2), I will be able to start a prototyping.
> > 
> > I think those two are actually the same, but I suspect I misunderstand
> > something.
> > 
> > If you look at how Unix handles devices versus partitions:
> > 
> > /dev/sdb - whole block device
> > /dev/sdb1 - first partition
> > /dev/sdb2 - second partition
> > 
> > So having the partitions as separate devices at least has precedent. I
> > do think it would work pretty well in U-Boot too.
> > 
> > Another thought might be that we make the block devices be siblings of
> > the 'whole' block device instead of children. Then a SATA device may
> > have 4 children, for example, all UCLASS_BLK, with the first one
> > containing the whole device contents and the other three containing
> > subsets of the first.
> 
> There are different types of devices nodes in the UEFI spec.

I think that you're talking things in a mixed (and confusing) language here.
You seems to use "device" as either "device path (protocol)"
and "block io (protocol)" unintentionally.

> Examples of messaging devices are Sata, NVMe, eMMC, SD, iSCSI,
> UsbMassStorage, SAS (serial attached SCSI), Scsi, Ata (ATAPI).

devices -> device paths

> Examples of media devices are HD (partition), CDROM, RamDisk.

ditto

> So a partition belongs into a different catagory of device then a disk
> drive.

Wait. Why "so?"
What do you mean "disk drive?"

"Messaging device path" is nothing but a location in a specific-type
of device interface that will connect the parent (probably "bus") and
the attached child (probably a device). For example, Scsi(0,0)
means a location of device with PUN=0 and LUN=0.

It will never tell us what device type (block device or whatever else)
it is.

> Here is the output of the EFI shell command `devtree` on a 2018 Lenovo
> computer:
> 
> Ctrl[C1] PciRoot(0x0)
>  Ctrl[1E7] NVM Express Controller
>   Ctrl[225] SAMSUNG MZVLW256HEHP
>    Ctrl[233] FAT File System
>    Ctrl[234]
> PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/NVMe(0x1,..)/HD(2,GPT,..)
>    Ctrl[235]
> PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/NVMe(0x1,..)/HD(3,GPT,..)
>    Ctrl[236]
> PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/NVMe(0x1,..)/HD(4,GPT,..)
>  Ctrl[1F7] AHCI Bus Driver
>   Ctrl[228] Crucial_CT525MX300SSD1
>    Ctrl[229] FAT File System
>    Ctrl[22A]
> PciRoot(0x0)/Pci(0x8,0x2)/Pci(0x0,0x0)/Sata(0x0,..)/HD(2,GPT,..)
>    Ctrl[22B]
> PciRoot(0x0)/Pci(0x8,0x2)/Pci(0x0,0x0)/Sata(0x0,..)/HD(3,GPT,..)
> 
> The disk Sata(0x0,0x0,0x0) or NVMe(0x1,<DevUID>) is the parent of its
> respective partitions. There is no pseudo partition device HD(0, ...)
> for block I/O to the disk.

I don't get your point.
What do you mean by "pseudo partition device for block I/O?"

Are you saying that a device represented by "sata(0x0,0x0,0x0)"
will not be able to be accessed via block io protocol?

I don't think so. UEFI spec v2.7, section 10.4.5 says,
"An EFI_BLOCK_IO_PROTOCOL is produced for both raw devices and partitions
on devices."

It is also true in u-boot UEFI implementation:
(I have two scsi disks here. Scsi(1,0) has two partitions.)
=> efi devices
Scanning disk ahci_scsi.id0lun0...
Scanning disk ahci_scsi.id1lun0...
Found 4 disks
Device           Device Path
================ ====================
000000007ef0afd0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
000000007ef0bb80 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
000000007ef0bf50 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
000000007ef0c090 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)
000000007ef0c2c0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)
=> efi dh
Handle           Protocols
================ ====================
000000007ef0afd0 Device Path, Device Path To Text, Device Path Utilities, Unicode Collation 2
000000007ef00b50 Driver Binding
000000007ef00bc0 Simple Text Output
000000007ef00c30 Simple Text Input, Simple Text Input Ex
000000007ef0bb80 Block IO, Device Path
000000007ef0bf50 Block IO, Device Path
000000007ef0c090 Block IO, Device Path, Simple File System
000000007ef0c2c0 Block IO, Device Path, Simple File System

> On U-Boot I see trees like:
> root_driver
> |-- pcie at 10000000
> |   |-- pci_0:0.0
> |   |-- e1000#0
> |   `-- ahci_pci
> |       `-- ahci_scsi
> |           `-- ahci_scsi.id0lun0
> 
> To keep the U-Boot and the UEFI model in sync we should define
> partitions as child devices of block devices.

What type of UCLASS do you suggest for a child device here,
UCLASS_PARTITION or UCLASS_BLK?

-Takahiro Akashi

> 
> Heinrich
> 
> > 
> > But it seems to me that having the partitions be children of a parent
> > BLK device makes more sense, since if you write to the parent you are
> > implicitly writing from one of the children. Also, if you write the
> > partition table, you do that with the parent BLK device, but that will
> > cause the children to be deleted and recreated. Also the operations on
> > a partition are not quite the same as those on a whole device: they
> > can only see a window into the whole device, so will likely add their
> > offset and then call the parent-device operations.
> > 
> > [..]
> > 
> > Regards,
> > Simon
> > 
>