On fre, feb 03, 2023 at 17:20, Simon Glass sjg@chromium.org wrote:
Hi Tobias,
On Fri, 3 Feb 2023 at 02:38, Tobias Waldekranz tobias@waldekranz.com wrote:
On ons, feb 01, 2023 at 13:20, Simon Glass sjg@chromium.org wrote:
Hi Tobias,
Hi Simon,
Thanks for the review!
On Wed, 1 Feb 2023 at 11:10, Tobias Waldekranz tobias@waldekranz.com wrote:
blkmaps are loosely modeled on Linux's device mapper subsystem. The basic idea is that you can create virtual block devices whose blocks can be backed by a plethora of sources that are user configurable.
This change just adds the basic infrastructure for creating and removing blkmap devices. Subsequent changes will extend this to add support for actual mappings.
Signed-off-by: Tobias Waldekranz tobias@waldekranz.com
MAINTAINERS | 6 + disk/part.c | 1 + drivers/block/Kconfig | 18 ++ drivers/block/Makefile | 1 + drivers/block/blk-uclass.c | 1 + drivers/block/blkmap.c | 275 +++++++++++++++++++++++++++++++ include/blkmap.h | 15 ++ include/dm/uclass-id.h | 1 + include/efi_loader.h | 4 + lib/efi_loader/efi_device_path.c | 30 ++++ 10 files changed, 352 insertions(+) create mode 100644 drivers/block/blkmap.c create mode 100644 include/blkmap.h
[..]
This needs to be created as part of DM. See how host_create_device() works. It attaches something to the uclass and then creates child devices from there. It also operations (struct host_ops) but you don't need to do that.
Note that the host commands support either an label or a devnum, which I think is useful, so you might copy that?
I took a look at the hostfs implementation. I agree that labels are much nicer than bare integers. However, for block maps the idea is to fit in to the existing filesystem infrastructure. Addressing block devices using the "<interface> <dev>[:<part>]" pattern seems very well established...
You can still do that, so long as the labels are "0" and "1", etc. But it lets us move to a more flexible system in future.
+{
static struct udevice *dev;int err;if (dev)return dev;err = device_bind_driver(dm_root(), "blkmap_root", "blkmap", &dev);if (err)return NULL;err = device_probe(dev);if (err) {device_unbind(dev);return NULL;}Should not be needed as probing children will cause this to be probed.
So this function just becomes
uclass_first_device(UCLASS_BLKDEV, &
return dev;+}
+int blkmap_create(int devnum)
Again, please drop the use of devnum and use devices. Here you could use a label, perhaps?
...which is why I don't think a label is going to fly here. Let's say I create a new ramdisk with a label instead, e.g.:
blkmap create rd blkmap map rd 0 0x100 mem ${loadaddr}How do I know which <dev> to supply to, e.g.:
ls blkmap <dev> /bootIt seems like labels are a hostfs-specific feature, or am I missing something?
We have the same problem with hostfs, since we have not implemented labels in block devices. For now you must use integer labels. But we will get there.
But there is no connection to the devnum that is allocated internally by U-Boot. Here's an experiment I just ran:
I created two squashfs images containing a single directory:
zero.squashfs: i_am_zero
one.squashfs: i_am_one
Then I added a binding to them:
=> host bind 1 one.squashfs => host bind 0 zero.squashfs
When accessing them, we see that the existing filesystem utilities work on the internally generated devnums, ignoring the labels:
=> ls host 1 i_am_zero/
0 file(s), 1 dir(s)
=> ls host 0 i_am_one/
0 file(s), 1 dir(s)
=>
Doesn't it therefore make more sense to stick with the established abstraction?
+{
struct udevice *root;Please don't use that name , as we use it for the DM root device. How about bm_parent? It isn't actually a tree of devices, so root doesn't sound right to me anyway.
Alright, I'll change it.
struct blk_desc *bd;struct blkmap *bm;int err;if (devnum >= 0 && blkmap_from_devnum(devnum))return -EBUSY;root = blkmap_root();if (!root)return -ENODEV;bm = calloc(1, sizeof(*bm));Can this be attached to the device as private data, so avoiding the malloc()?
Maybe, I'm not familiar enough with the U-Boot internals.
As it is now, all blkmaps are children of a single "blkmap_root" device. I chose that approach so that devnums would be allocated from a single pool.
Well, driver model handles this for you (see dev_seq()). You have a single uclass so can attach your 'overall' blkmap data to that. Then each device can have its own private data attached.
The only requirement is that BLK devices have a parent (so we know the media type). I had understood that you had one blkmap for each block map. If that is true, then you don't need to have a parent one as well. You can use the BLKMAP uclass to hold any overall data.
AFAIK, that would mean having to store it in the "blkmap_blk" device, but I thought that its private data was owned by the block subsystem?
[..]
Regards, Simon