| Message ID | 20211001050228.55183-1-takahiro.akashi@linaro.org |
|---|---|
| Headers | show |
| Series | efi_loader: more tightly integrate UEFI disks to device model | expand |
Hi Takahiro, On Thu, 30 Sept 2021 at 23:02, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > The purpose of this RPC is to reignite the discussion about how UEFI > subystem would best be integrated into U-Boot device model. > In the past, I poposed a couple of patch series, the latest one[1], > while Heinrich revealed his idea[2], and the approach taken here is > something between them, with a focus on block device handlings. > > # The code is a PoC and not well tested yet. > > Disks in UEFI world: > ==================== > In general in UEFI world, accessing to any device is performed through > a 'protocol' interface which are installed to (or associated with) the device's > UEFI handle (or an opaque pointer to UEFI object data). Protocols are > implemented by either the UEFI system itself or UEFI drivers. > > For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk > hereafter). Currently, every efi_disk may have one of two origins: > a.U-Boot's block devices or related partitions > (lib/efi_loader/efi_disk.c) > b.UEFI objects which are implemented as a block device by UEFI drivers. > (lib/efi_driver/efi_block_device.c) > > All the efi_diskss as (a) will be enumelated and created only once at UEFI > subsystem initialization (efi_disk_register()), which is triggered by > first executing one of UEFI-related U-Boot commands, like "bootefi", > "setenv -e" or "efidebug". > EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops) > in the corresponding udevice(UCLASS_BLK). > > On the other hand, efi_disk as (b) will be created each time UEFI boot > services' connect_controller() is executed in UEFI app which, as a (device) > controller, gives the method to access the device's data, > ie. EFI_BLOCK_IO_PROTOCOL. > > >>> more details >>> > Internally, connect_controller() search for UEFI driver that can support > this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case, > then calls the driver's 'bind' interface, which eventually installs > the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object. > 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for > * creating additional partitions efi_disk's, and > * supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it. > <<< <<< > > Issues: > ======= > 1. While an efi_disk represents a device equally for either a whole disk > or a partition in UEFI world, the device model treats only a whole > disk as a real block device or udevice(UCLASS_BLK). > > 2. efi_disk holds and makes use of "blk_desc" data even though blk_desc > in plat_data is supposed to be private and not to be accessed outside > the device model. > # This issue, though, exists for all the implmenetation of U-Boot > # file systems as well. Yes, this was a migration convenience and we should be able to unpick it now. However we still have SPL_BLK so need to consider whether we can drop that. > > For efi_disk(a), > 3. A block device can be enumelated dynamically by 'scanning' a device bus > in U-Boot, but UEFI subsystem is not able to update efi_disks accordingly. > For examples, > => scsi rescan; efidebug devices > => usb start; efidebug devices ... (A) > (A) doesn't show any usb devices detected. > > => scsi rescan; efidebug boot add -b 0 TEST scsi 0:1 ... > => scsi rescan ... (B) > => bootefi bootmgr ... (C) > (C) may de-reference a bogus blk_desc pointer which has been freed by (B). > (Please note that "scsi rescan" removes all udevices/blk_desc and then > re-create them even if nothing is changed on a bus.) This is something your series will help with. > > For efi_disk(b), > 4. A controller (handle), combined with efi_block driver, has no > corresponding udevice as a parent of efi_disks in DM tree, unlike, say, > a scsi controller, even though it provides methods for block io perations. Can we just create a udevice? > 5. There is no way supported to remove efi_disk's even after > disconnect_controller() is called. > Do we want to remove disks? I don't really understand the context, but if EFI wants to forget about a disk, that should not affect the rest of U-Boot. > > My approach in this RFC: > ======================== > Due to functional differences in semantics, it would be difficult > to identify "udevice" structure as a handle in UEFI world. Instead, we will > have to somehow maintain a relationship between a udevice and a handle. > > 1-1. add a dedicated uclass, UCLASS_PARTITION, for partitions > Currently, the uclass for paritions is not a UCLASS_BLK. > It can be possible to define partitions as UCLASS_BLK > (with IF_TYPE_PARTION?), but > I'm afraid that it may introduce some chaos since udevice(UCLASS_BLK) > is tightly coupled with 'struct blk_desc' data which is still used > as a "structure to a whole disk" in a lot of interfaces. > (I hope that you understand what it means.) > > In DM tree, a UCLASS_PARTITON instance has a UCLASS_BLK parent: > For instance, > UCLASS_SCSI --- UCLASS_BLK --- UCLASS_PARTITION > (IF_TYPE_SCSI) | > +- struct blk_desc +- struct disk_part > +- scsi_blk_ops +- blk_part_ops Seems right to me. But I think a partition should also have a BLK child, so that only block devices can be read/written. In fact, perhaps a partition should be a child of the media device (e.g. MMC), not of the BLK device? That might make more sense. > > 1-2. create partition udevices in the context of device_probe() > part_init() is already called in blk_post_probe(). See the commit > d0851c893706 ("blk: Call part_init() in the post_probe() method"). > Why not enumelate partitions as well in there. Sounds reasonable. > > 2. add new block access interfaces, which takes "udevice" as a target device, > in U-Boot and use those functions to implement efi_disk operations > (i.e. EFI_BLOCK_IO_PROTOCOL). Nice. > > 3-1. maintain a bi-directional link by adding > - a UEFI handle pointer in "struct udevice" > - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj") We should generalise the API here, but otherwise seems right. See my comment on the patch. > > 3-2. use device model's post_probe/pre_remove hook to synchronize the lifetime > of efi_disk objects in UEFI world with the device model. We should add an event mechanism to generalise this a bit. See my comment on the patch. > > 4. I have no answer to issue(4) and (5) yet. See above. > > > Patchs: > ======= > For easy understandings, patches may be categorized into seperate groups > of changes. > > Patch#1-#9: DM: create udevices for partitions > Patch#11,#12: UEFI: use udevice interfaces > Patch#13-#16: UEFI: sync up with the device model for adding > Patch#17-#19: UEFI: sync up with the device model for removing > For removal case, we may need more consideration since removing handles > unconditionally may end up breaking integrity of handles > (some may still be held and referenced to by a UEFI app). > Patch#20-#22: UEFI: align efi_driver with changes by the integration > This looks great to me and is the first real step I have seen to actually solving the problem of all this duplication. I think we need a few more DM APIs as I suggest in a few of the patches. I am happy to help with these if you like. I wonder if it is possible to get some version of at least part of this code applied in this merge window? It helps to set things on a much better direction. I also worry that continuing to develop the EFI impl while it is such a sorry state is adding to the difficulty of fixing it up. Regards, Simon > > [1] https://lists.denx.de/pipermail/u-boot/2019-February/357923.html > [2] https://lists.denx.de/pipermail/u-boot/2021-June/452297.html > > AKASHI Takahiro (22): > scsi: call device_probe() after scanning > usb: storage: call device_probe() after scanning > mmc: call device_probe() after scanning > nvme: call device_probe() after scanning > sata: call device_probe() after scanning > block: ide: call device_probe() after scanning > dm: blk: add UCLASS_PARTITION > dm: blk: add a device-probe hook for scanning disk partitions > dm: blk: add read/write interfaces with udevice > efi_loader: disk: use udevice instead of blk_desc > dm: add a hidden link to efi object > efi_loader: remove !CONFIG_BLK code from efi_disk > efi_loader: disk: a helper function to create efi_disk objects from > udevice > dm: blk: call efi's device-probe hook > efi_loader: cleanup after efi_disk-dm integration > efi_loader: add efi_remove_handle() > efi_loader: efi_disk: a helper function to delete efi_disk objects > dm: blk: call efi's device-removal hook > efi_driver: align with efi_disk-dm integration > efi_driver: cleanup after efi_disk-dm integration > efi_selftest: block device: adjust dp for a test disk > (TEST) let dm-tree unchanged after block_io testing is done > > common/usb_storage.c | 6 + > drivers/ata/dwc_ahsata.c | 10 + > drivers/ata/fsl_sata.c | 11 + > drivers/ata/sata_mv.c | 9 + > drivers/ata/sata_sil.c | 12 + > drivers/block/blk-uclass.c | 249 ++++++++++++++++ > drivers/block/ide.c | 6 + > drivers/mmc/mmc-uclass.c | 7 + > drivers/nvme/nvme.c | 6 + > drivers/scsi/scsi.c | 10 + > include/blk.h | 15 + > include/dm/device.h | 4 + > include/dm/uclass-id.h | 1 + > include/efi_loader.h | 8 +- > lib/efi_driver/efi_block_device.c | 30 +- > lib/efi_loader/efi_boottime.c | 8 + > lib/efi_loader/efi_device_path.c | 29 ++ > lib/efi_loader/efi_disk.c | 286 ++++++++++--------- > lib/efi_loader/efi_setup.c | 5 - > lib/efi_selftest/efi_selftest_block_device.c | 28 +- > 20 files changed, 566 insertions(+), 174 deletions(-) > > -- > 2.33.0 >
On Sun, Oct 10, 2021 at 08:14:00AM -0600, Simon Glass wrote: > Hi Takahiro, > > On Thu, 30 Sept 2021 at 23:02, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > The purpose of this RPC is to reignite the discussion about how UEFI > > subystem would best be integrated into U-Boot device model. > > In the past, I poposed a couple of patch series, the latest one[1], > > while Heinrich revealed his idea[2], and the approach taken here is > > something between them, with a focus on block device handlings. > > > > # The code is a PoC and not well tested yet. > > > > Disks in UEFI world: > > ==================== > > In general in UEFI world, accessing to any device is performed through > > a 'protocol' interface which are installed to (or associated with) the device's > > UEFI handle (or an opaque pointer to UEFI object data). Protocols are > > implemented by either the UEFI system itself or UEFI drivers. > > > > For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk > > hereafter). Currently, every efi_disk may have one of two origins: > > a.U-Boot's block devices or related partitions > > (lib/efi_loader/efi_disk.c) > > b.UEFI objects which are implemented as a block device by UEFI drivers. > > (lib/efi_driver/efi_block_device.c) > > > > All the efi_diskss as (a) will be enumelated and created only once at UEFI > > subsystem initialization (efi_disk_register()), which is triggered by > > first executing one of UEFI-related U-Boot commands, like "bootefi", > > "setenv -e" or "efidebug". > > EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops) > > in the corresponding udevice(UCLASS_BLK). > > > > On the other hand, efi_disk as (b) will be created each time UEFI boot > > services' connect_controller() is executed in UEFI app which, as a (device) > > controller, gives the method to access the device's data, > > ie. EFI_BLOCK_IO_PROTOCOL. > > > > >>> more details >>> > > Internally, connect_controller() search for UEFI driver that can support > > this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case, > > then calls the driver's 'bind' interface, which eventually installs > > the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object. > > 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for > > * creating additional partitions efi_disk's, and > > * supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it. > > <<< <<< > > > > Issues: > > ======= > > 1. While an efi_disk represents a device equally for either a whole disk > > or a partition in UEFI world, the device model treats only a whole > > disk as a real block device or udevice(UCLASS_BLK). > > > > 2. efi_disk holds and makes use of "blk_desc" data even though blk_desc > > in plat_data is supposed to be private and not to be accessed outside > > the device model. > > # This issue, though, exists for all the implmenetation of U-Boot > > # file systems as well. > > Yes, this was a migration convenience and we should be able to unpick it now. > > However we still have SPL_BLK so need to consider whether we can drop that. To be clear here, in that I can hand-wave my way to seeing a use case for lib/efi_loader/ under SPL, it would not be in a world where we also still would be supporting the non-DM infrastructure, and is also not a near-term goal. -- Tom
Hi Tom, On Tue, 12 Oct 2021 at 09:00, Tom Rini <trini@konsulko.com> wrote: > > On Sun, Oct 10, 2021 at 08:14:00AM -0600, Simon Glass wrote: > > Hi Takahiro, > > > > On Thu, 30 Sept 2021 at 23:02, AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > > The purpose of this RPC is to reignite the discussion about how UEFI > > > subystem would best be integrated into U-Boot device model. > > > In the past, I poposed a couple of patch series, the latest one[1], > > > while Heinrich revealed his idea[2], and the approach taken here is > > > something between them, with a focus on block device handlings. > > > > > > # The code is a PoC and not well tested yet. > > > > > > Disks in UEFI world: > > > ==================== > > > In general in UEFI world, accessing to any device is performed through > > > a 'protocol' interface which are installed to (or associated with) the device's > > > UEFI handle (or an opaque pointer to UEFI object data). Protocols are > > > implemented by either the UEFI system itself or UEFI drivers. > > > > > > For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk > > > hereafter). Currently, every efi_disk may have one of two origins: > > > a.U-Boot's block devices or related partitions > > > (lib/efi_loader/efi_disk.c) > > > b.UEFI objects which are implemented as a block device by UEFI drivers. > > > (lib/efi_driver/efi_block_device.c) > > > > > > All the efi_diskss as (a) will be enumelated and created only once at UEFI > > > subsystem initialization (efi_disk_register()), which is triggered by > > > first executing one of UEFI-related U-Boot commands, like "bootefi", > > > "setenv -e" or "efidebug". > > > EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops) > > > in the corresponding udevice(UCLASS_BLK). > > > > > > On the other hand, efi_disk as (b) will be created each time UEFI boot > > > services' connect_controller() is executed in UEFI app which, as a (device) > > > controller, gives the method to access the device's data, > > > ie. EFI_BLOCK_IO_PROTOCOL. > > > > > > >>> more details >>> > > > Internally, connect_controller() search for UEFI driver that can support > > > this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case, > > > then calls the driver's 'bind' interface, which eventually installs > > > the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object. > > > 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for > > > * creating additional partitions efi_disk's, and > > > * supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it. > > > <<< <<< > > > > > > Issues: > > > ======= > > > 1. While an efi_disk represents a device equally for either a whole disk > > > or a partition in UEFI world, the device model treats only a whole > > > disk as a real block device or udevice(UCLASS_BLK). > > > > > > 2. efi_disk holds and makes use of "blk_desc" data even though blk_desc > > > in plat_data is supposed to be private and not to be accessed outside > > > the device model. > > > # This issue, though, exists for all the implmenetation of U-Boot > > > # file systems as well. > > > > Yes, this was a migration convenience and we should be able to unpick it now. > > > > However we still have SPL_BLK so need to consider whether we can drop that. > > To be clear here, in that I can hand-wave my way to seeing a use case > for lib/efi_loader/ under SPL, it would not be in a world where we also > still would be supporting the non-DM infrastructure, and is also not a > near-term goal. OK good. Perhaps we should add a migration method for SPL_BLK? It would be good to know where we are in terms of the size stuff. I don't see a lot of boards rushing to use of-platdata, though. Regards, Simon
On Tue, Oct 12, 2021 at 02:31:15PM -0600, Simon Glass wrote: > Hi Tom, > > On Tue, 12 Oct 2021 at 09:00, Tom Rini <trini@konsulko.com> wrote: > > > > On Sun, Oct 10, 2021 at 08:14:00AM -0600, Simon Glass wrote: > > > Hi Takahiro, > > > > > > On Thu, 30 Sept 2021 at 23:02, AKASHI Takahiro > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > The purpose of this RPC is to reignite the discussion about how UEFI > > > > subystem would best be integrated into U-Boot device model. > > > > In the past, I poposed a couple of patch series, the latest one[1], > > > > while Heinrich revealed his idea[2], and the approach taken here is > > > > something between them, with a focus on block device handlings. > > > > > > > > # The code is a PoC and not well tested yet. > > > > > > > > Disks in UEFI world: > > > > ==================== > > > > In general in UEFI world, accessing to any device is performed through > > > > a 'protocol' interface which are installed to (or associated with) the device's > > > > UEFI handle (or an opaque pointer to UEFI object data). Protocols are > > > > implemented by either the UEFI system itself or UEFI drivers. > > > > > > > > For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk > > > > hereafter). Currently, every efi_disk may have one of two origins: > > > > a.U-Boot's block devices or related partitions > > > > (lib/efi_loader/efi_disk.c) > > > > b.UEFI objects which are implemented as a block device by UEFI drivers. > > > > (lib/efi_driver/efi_block_device.c) > > > > > > > > All the efi_diskss as (a) will be enumelated and created only once at UEFI > > > > subsystem initialization (efi_disk_register()), which is triggered by > > > > first executing one of UEFI-related U-Boot commands, like "bootefi", > > > > "setenv -e" or "efidebug". > > > > EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops) > > > > in the corresponding udevice(UCLASS_BLK). > > > > > > > > On the other hand, efi_disk as (b) will be created each time UEFI boot > > > > services' connect_controller() is executed in UEFI app which, as a (device) > > > > controller, gives the method to access the device's data, > > > > ie. EFI_BLOCK_IO_PROTOCOL. > > > > > > > > >>> more details >>> > > > > Internally, connect_controller() search for UEFI driver that can support > > > > this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case, > > > > then calls the driver's 'bind' interface, which eventually installs > > > > the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object. > > > > 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for > > > > * creating additional partitions efi_disk's, and > > > > * supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it. > > > > <<< <<< > > > > > > > > Issues: > > > > ======= > > > > 1. While an efi_disk represents a device equally for either a whole disk > > > > or a partition in UEFI world, the device model treats only a whole > > > > disk as a real block device or udevice(UCLASS_BLK). > > > > > > > > 2. efi_disk holds and makes use of "blk_desc" data even though blk_desc > > > > in plat_data is supposed to be private and not to be accessed outside > > > > the device model. > > > > # This issue, though, exists for all the implmenetation of U-Boot > > > > # file systems as well. > > > > > > Yes, this was a migration convenience and we should be able to unpick it now. > > > > > > However we still have SPL_BLK so need to consider whether we can drop that. > > > > To be clear here, in that I can hand-wave my way to seeing a use case > > for lib/efi_loader/ under SPL, it would not be in a world where we also > > still would be supporting the non-DM infrastructure, and is also not a > > near-term goal. > > OK good. Perhaps we should add a migration method for SPL_BLK? It > would be good to know where we are in terms of the size stuff. I don't > see a lot of boards rushing to use of-platdata, though. What do you mean? Since we have platforms that need to support 12 or 16 KiB of space for SPL, we're not going to enforce SPL_DM. But those platforms can / do need to boot from MMC (SD card I think usually). In terms of platforms that could, but don't, enable SPL_BLK, that's just the platforms that disable SPL_BLK today as it defaults to enabled when possible. -- Tom
Hi Tom, On Tue, 12 Oct 2021 at 15:13, Tom Rini <trini@konsulko.com> wrote: > > On Tue, Oct 12, 2021 at 02:31:15PM -0600, Simon Glass wrote: > > Hi Tom, > > > > On Tue, 12 Oct 2021 at 09:00, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Sun, Oct 10, 2021 at 08:14:00AM -0600, Simon Glass wrote: > > > > Hi Takahiro, > > > > > > > > On Thu, 30 Sept 2021 at 23:02, AKASHI Takahiro > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > The purpose of this RPC is to reignite the discussion about how UEFI > > > > > subystem would best be integrated into U-Boot device model. > > > > > In the past, I poposed a couple of patch series, the latest one[1], > > > > > while Heinrich revealed his idea[2], and the approach taken here is > > > > > something between them, with a focus on block device handlings. > > > > > > > > > > # The code is a PoC and not well tested yet. > > > > > > > > > > Disks in UEFI world: > > > > > ==================== > > > > > In general in UEFI world, accessing to any device is performed through > > > > > a 'protocol' interface which are installed to (or associated with) the device's > > > > > UEFI handle (or an opaque pointer to UEFI object data). Protocols are > > > > > implemented by either the UEFI system itself or UEFI drivers. > > > > > > > > > > For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk > > > > > hereafter). Currently, every efi_disk may have one of two origins: > > > > > a.U-Boot's block devices or related partitions > > > > > (lib/efi_loader/efi_disk.c) > > > > > b.UEFI objects which are implemented as a block device by UEFI drivers. > > > > > (lib/efi_driver/efi_block_device.c) > > > > > > > > > > All the efi_diskss as (a) will be enumelated and created only once at UEFI > > > > > subsystem initialization (efi_disk_register()), which is triggered by > > > > > first executing one of UEFI-related U-Boot commands, like "bootefi", > > > > > "setenv -e" or "efidebug". > > > > > EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops) > > > > > in the corresponding udevice(UCLASS_BLK). > > > > > > > > > > On the other hand, efi_disk as (b) will be created each time UEFI boot > > > > > services' connect_controller() is executed in UEFI app which, as a (device) > > > > > controller, gives the method to access the device's data, > > > > > ie. EFI_BLOCK_IO_PROTOCOL. > > > > > > > > > > >>> more details >>> > > > > > Internally, connect_controller() search for UEFI driver that can support > > > > > this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case, > > > > > then calls the driver's 'bind' interface, which eventually installs > > > > > the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object. > > > > > 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for > > > > > * creating additional partitions efi_disk's, and > > > > > * supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it. > > > > > <<< <<< > > > > > > > > > > Issues: > > > > > ======= > > > > > 1. While an efi_disk represents a device equally for either a whole disk > > > > > or a partition in UEFI world, the device model treats only a whole > > > > > disk as a real block device or udevice(UCLASS_BLK). > > > > > > > > > > 2. efi_disk holds and makes use of "blk_desc" data even though blk_desc > > > > > in plat_data is supposed to be private and not to be accessed outside > > > > > the device model. > > > > > # This issue, though, exists for all the implmenetation of U-Boot > > > > > # file systems as well. > > > > > > > > Yes, this was a migration convenience and we should be able to unpick it now. > > > > > > > > However we still have SPL_BLK so need to consider whether we can drop that. > > > > > > To be clear here, in that I can hand-wave my way to seeing a use case > > > for lib/efi_loader/ under SPL, it would not be in a world where we also > > > still would be supporting the non-DM infrastructure, and is also not a > > > near-term goal. > > > > OK good. Perhaps we should add a migration method for SPL_BLK? It > > would be good to know where we are in terms of the size stuff. I don't > > see a lot of boards rushing to use of-platdata, though. > > What do you mean? Since we have platforms that need to support 12 or 16 > KiB of space for SPL, we're not going to enforce SPL_DM. But those > platforms can / do need to boot from MMC (SD card I think usually). > > In terms of platforms that could, but don't, enable SPL_BLK, that's just > the platforms that disable SPL_BLK today as it defaults to enabled when > possible. Well I wonder if we can use of-platdata and DM then perhaps some of these will fit. The OMAP platform I sent patches for was close to complete, I think, and I believe that was one of the tightest. Actually I cannot remember what board that was... The overhead is perhaps 7KB though, so maybe not suitable for 16KB. Regards, Simon
On Tue, Oct 12, 2021 at 05:37:45PM -0600, Simon Glass wrote: > Hi Tom, > > On Tue, 12 Oct 2021 at 15:13, Tom Rini <trini@konsulko.com> wrote: > > > > On Tue, Oct 12, 2021 at 02:31:15PM -0600, Simon Glass wrote: > > > Hi Tom, > > > > > > On Tue, 12 Oct 2021 at 09:00, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > On Sun, Oct 10, 2021 at 08:14:00AM -0600, Simon Glass wrote: > > > > > Hi Takahiro, > > > > > > > > > > On Thu, 30 Sept 2021 at 23:02, AKASHI Takahiro > > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > The purpose of this RPC is to reignite the discussion about how UEFI > > > > > > subystem would best be integrated into U-Boot device model. > > > > > > In the past, I poposed a couple of patch series, the latest one[1], > > > > > > while Heinrich revealed his idea[2], and the approach taken here is > > > > > > something between them, with a focus on block device handlings. > > > > > > > > > > > > # The code is a PoC and not well tested yet. > > > > > > > > > > > > Disks in UEFI world: > > > > > > ==================== > > > > > > In general in UEFI world, accessing to any device is performed through > > > > > > a 'protocol' interface which are installed to (or associated with) the device's > > > > > > UEFI handle (or an opaque pointer to UEFI object data). Protocols are > > > > > > implemented by either the UEFI system itself or UEFI drivers. > > > > > > > > > > > > For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk > > > > > > hereafter). Currently, every efi_disk may have one of two origins: > > > > > > a.U-Boot's block devices or related partitions > > > > > > (lib/efi_loader/efi_disk.c) > > > > > > b.UEFI objects which are implemented as a block device by UEFI drivers. > > > > > > (lib/efi_driver/efi_block_device.c) > > > > > > > > > > > > All the efi_diskss as (a) will be enumelated and created only once at UEFI > > > > > > subsystem initialization (efi_disk_register()), which is triggered by > > > > > > first executing one of UEFI-related U-Boot commands, like "bootefi", > > > > > > "setenv -e" or "efidebug". > > > > > > EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops) > > > > > > in the corresponding udevice(UCLASS_BLK). > > > > > > > > > > > > On the other hand, efi_disk as (b) will be created each time UEFI boot > > > > > > services' connect_controller() is executed in UEFI app which, as a (device) > > > > > > controller, gives the method to access the device's data, > > > > > > ie. EFI_BLOCK_IO_PROTOCOL. > > > > > > > > > > > > >>> more details >>> > > > > > > Internally, connect_controller() search for UEFI driver that can support > > > > > > this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case, > > > > > > then calls the driver's 'bind' interface, which eventually installs > > > > > > the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object. > > > > > > 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for > > > > > > * creating additional partitions efi_disk's, and > > > > > > * supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it. > > > > > > <<< <<< > > > > > > > > > > > > Issues: > > > > > > ======= > > > > > > 1. While an efi_disk represents a device equally for either a whole disk > > > > > > or a partition in UEFI world, the device model treats only a whole > > > > > > disk as a real block device or udevice(UCLASS_BLK). > > > > > > > > > > > > 2. efi_disk holds and makes use of "blk_desc" data even though blk_desc > > > > > > in plat_data is supposed to be private and not to be accessed outside > > > > > > the device model. > > > > > > # This issue, though, exists for all the implmenetation of U-Boot > > > > > > # file systems as well. > > > > > > > > > > Yes, this was a migration convenience and we should be able to unpick it now. > > > > > > > > > > However we still have SPL_BLK so need to consider whether we can drop that. > > > > > > > > To be clear here, in that I can hand-wave my way to seeing a use case > > > > for lib/efi_loader/ under SPL, it would not be in a world where we also > > > > still would be supporting the non-DM infrastructure, and is also not a > > > > near-term goal. > > > > > > OK good. Perhaps we should add a migration method for SPL_BLK? It > > > would be good to know where we are in terms of the size stuff. I don't > > > see a lot of boards rushing to use of-platdata, though. > > > > What do you mean? Since we have platforms that need to support 12 or 16 > > KiB of space for SPL, we're not going to enforce SPL_DM. But those > > platforms can / do need to boot from MMC (SD card I think usually). > > > > In terms of platforms that could, but don't, enable SPL_BLK, that's just > > the platforms that disable SPL_BLK today as it defaults to enabled when > > possible. > > Well I wonder if we can use of-platdata and DM then perhaps some of > these will fit. The OMAP platform I sent patches for was close to > complete, I think, and I believe that was one of the tightest. > Actually I cannot remember what board that was... > > The overhead is perhaps 7KB though, so maybe not suitable for 16KB. It's only disabled on the vboot config, on am335x platforms. Honestly, I would suggest a separate thread and ask the board maintainers in question why specifically it's off. I know for am335x_boneblack_vboot_defconfig it's inadvertent, it could be turned on no problem I suspect. -- Tom
The purpose of this RPC is to reignite the discussion about how UEFI subystem would best be integrated into U-Boot device model. In the past, I poposed a couple of patch series, the latest one[1], while Heinrich revealed his idea[2], and the approach taken here is something between them, with a focus on block device handlings. # The code is a PoC and not well tested yet. Disks in UEFI world: ==================== In general in UEFI world, accessing to any device is performed through a 'protocol' interface which are installed to (or associated with) the device's UEFI handle (or an opaque pointer to UEFI object data). Protocols are implemented by either the UEFI system itself or UEFI drivers. For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk hereafter). Currently, every efi_disk may have one of two origins: a.U-Boot's block devices or related partitions (lib/efi_loader/efi_disk.c) b.UEFI objects which are implemented as a block device by UEFI drivers. (lib/efi_driver/efi_block_device.c) All the efi_diskss as (a) will be enumelated and created only once at UEFI subsystem initialization (efi_disk_register()), which is triggered by first executing one of UEFI-related U-Boot commands, like "bootefi", "setenv -e" or "efidebug". EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops) in the corresponding udevice(UCLASS_BLK). On the other hand, efi_disk as (b) will be created each time UEFI boot services' connect_controller() is executed in UEFI app which, as a (device) controller, gives the method to access the device's data, ie. EFI_BLOCK_IO_PROTOCOL. >>> more details >>> Internally, connect_controller() search for UEFI driver that can support this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case, then calls the driver's 'bind' interface, which eventually installs the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object. 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for * creating additional partitions efi_disk's, and * supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it. <<< <<< Issues: ======= 1. While an efi_disk represents a device equally for either a whole disk or a partition in UEFI world, the device model treats only a whole disk as a real block device or udevice(UCLASS_BLK). 2. efi_disk holds and makes use of "blk_desc" data even though blk_desc in plat_data is supposed to be private and not to be accessed outside the device model. # This issue, though, exists for all the implmenetation of U-Boot # file systems as well. For efi_disk(a), 3. A block device can be enumelated dynamically by 'scanning' a device bus in U-Boot, but UEFI subsystem is not able to update efi_disks accordingly. For examples, => scsi rescan; efidebug devices => usb start; efidebug devices ... (A) (A) doesn't show any usb devices detected. => scsi rescan; efidebug boot add -b 0 TEST scsi 0:1 ... => scsi rescan ... (B) => bootefi bootmgr ... (C) (C) may de-reference a bogus blk_desc pointer which has been freed by (B). (Please note that "scsi rescan" removes all udevices/blk_desc and then re-create them even if nothing is changed on a bus.) For efi_disk(b), 4. A controller (handle), combined with efi_block driver, has no corresponding udevice as a parent of efi_disks in DM tree, unlike, say, a scsi controller, even though it provides methods for block io perations. 5. There is no way supported to remove efi_disk's even after disconnect_controller() is called. My approach in this RFC: ======================== Due to functional differences in semantics, it would be difficult to identify "udevice" structure as a handle in UEFI world. Instead, we will have to somehow maintain a relationship between a udevice and a handle. 1-1. add a dedicated uclass, UCLASS_PARTITION, for partitions Currently, the uclass for paritions is not a UCLASS_BLK. It can be possible to define partitions as UCLASS_BLK (with IF_TYPE_PARTION?), but I'm afraid that it may introduce some chaos since udevice(UCLASS_BLK) is tightly coupled with 'struct blk_desc' data which is still used as a "structure to a whole disk" in a lot of interfaces. (I hope that you understand what it means.) In DM tree, a UCLASS_PARTITON instance has a UCLASS_BLK parent: For instance, UCLASS_SCSI --- UCLASS_BLK --- UCLASS_PARTITION (IF_TYPE_SCSI) | +- struct blk_desc +- struct disk_part +- scsi_blk_ops +- blk_part_ops 1-2. create partition udevices in the context of device_probe() part_init() is already called in blk_post_probe(). See the commit d0851c893706 ("blk: Call part_init() in the post_probe() method"). Why not enumelate partitions as well in there. 2. add new block access interfaces, which takes "udevice" as a target device, in U-Boot and use those functions to implement efi_disk operations (i.e. EFI_BLOCK_IO_PROTOCOL). 3-1. maintain a bi-directional link by adding - a UEFI handle pointer in "struct udevice" - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj") 3-2. use device model's post_probe/pre_remove hook to synchronize the lifetime of efi_disk objects in UEFI world with the device model. 4. I have no answer to issue(4) and (5) yet. Patchs: ======= For easy understandings, patches may be categorized into seperate groups of changes. Patch#1-#9: DM: create udevices for partitions Patch#11,#12: UEFI: use udevice interfaces Patch#13-#16: UEFI: sync up with the device model for adding Patch#17-#19: UEFI: sync up with the device model for removing For removal case, we may need more consideration since removing handles unconditionally may end up breaking integrity of handles (some may still be held and referenced to by a UEFI app). Patch#20-#22: UEFI: align efi_driver with changes by the integration [1] https://lists.denx.de/pipermail/u-boot/2019-February/357923.html [2] https://lists.denx.de/pipermail/u-boot/2021-June/452297.html AKASHI Takahiro (22): scsi: call device_probe() after scanning usb: storage: call device_probe() after scanning mmc: call device_probe() after scanning nvme: call device_probe() after scanning sata: call device_probe() after scanning block: ide: call device_probe() after scanning dm: blk: add UCLASS_PARTITION dm: blk: add a device-probe hook for scanning disk partitions dm: blk: add read/write interfaces with udevice efi_loader: disk: use udevice instead of blk_desc dm: add a hidden link to efi object efi_loader: remove !CONFIG_BLK code from efi_disk efi_loader: disk: a helper function to create efi_disk objects from udevice dm: blk: call efi's device-probe hook efi_loader: cleanup after efi_disk-dm integration efi_loader: add efi_remove_handle() efi_loader: efi_disk: a helper function to delete efi_disk objects dm: blk: call efi's device-removal hook efi_driver: align with efi_disk-dm integration efi_driver: cleanup after efi_disk-dm integration efi_selftest: block device: adjust dp for a test disk (TEST) let dm-tree unchanged after block_io testing is done common/usb_storage.c | 6 + drivers/ata/dwc_ahsata.c | 10 + drivers/ata/fsl_sata.c | 11 + drivers/ata/sata_mv.c | 9 + drivers/ata/sata_sil.c | 12 + drivers/block/blk-uclass.c | 249 ++++++++++++++++ drivers/block/ide.c | 6 + drivers/mmc/mmc-uclass.c | 7 + drivers/nvme/nvme.c | 6 + drivers/scsi/scsi.c | 10 + include/blk.h | 15 + include/dm/device.h | 4 + include/dm/uclass-id.h | 1 + include/efi_loader.h | 8 +- lib/efi_driver/efi_block_device.c | 30 +- lib/efi_loader/efi_boottime.c | 8 + lib/efi_loader/efi_device_path.c | 29 ++ lib/efi_loader/efi_disk.c | 286 ++++++++++--------- lib/efi_loader/efi_setup.c | 5 - lib/efi_selftest/efi_selftest_block_device.c | 28 +- 20 files changed, 566 insertions(+), 174 deletions(-) -- 2.33.0