On ons, feb 01, 2023 at 13:20, Simon Glass <s...@chromium.org> wrote: > Hi Tobias,
Hi Simon, Thanks for the review! > On Wed, 1 Feb 2023 at 11:10, Tobias Waldekranz <tob...@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 <tob...@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 >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 3e8e193ecc..28a34231bf 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -786,6 +786,12 @@ M: Alper Nebi Yasak <alpernebiya...@gmail.com> >> S: Maintained >> F: tools/binman/ >> >> +BLKMAP >> +M: Tobias Waldekranz <tob...@waldekranz.com> >> +S: Maintained >> +F: drivers/block/blkmap.c >> +F: include/blkmap.h >> + >> BOOTDEVICE >> M: Simon Glass <s...@chromium.org> >> S: Maintained >> diff --git a/disk/part.c b/disk/part.c >> index d449635254..35300df590 100644 >> --- a/disk/part.c >> +++ b/disk/part.c >> @@ -140,6 +140,7 @@ void dev_print(struct blk_desc *dev_desc) >> case UCLASS_NVME: >> case UCLASS_PVBLOCK: >> case UCLASS_HOST: >> + case UCLASS_BLKMAP: >> printf ("Vendor: %s Rev: %s Prod: %s\n", >> dev_desc->vendor, >> dev_desc->revision, >> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig >> index e95da48bdc..5a1aeb3d2b 100644 >> --- a/drivers/block/Kconfig >> +++ b/drivers/block/Kconfig >> @@ -67,6 +67,24 @@ config BLOCK_CACHE >> it will prevent repeated reads from directory structures and other >> filesystem data structures. >> >> +config BLKMAP >> + bool "Composable virtual block devices (blkmap)" >> + depends on BLK >> + help >> + Create virtual block devices that are backed by various sources, >> + e.g. RAM, or parts of an existing block device. Though much more >> + rudimentary, it borrows a lot of ideas from Linux's device mapper >> + subsystem. >> + >> + Example use-cases: >> + - Treat a region of RAM as a block device, i.e. a RAM disk. This >> let's >> + you extract files from filesystem images stored in RAM (perhaps >> as a >> + result of a TFTP transfer). >> + - Create a virtual partition on an existing device. This let's you >> + access filesystems that aren't stored at an exact partition >> + boundary. A common example is a filesystem image embedded in an >> FIT >> + image. >> + >> config SPL_BLOCK_CACHE >> bool "Use block device cache in SPL" >> depends on SPL_BLK >> diff --git a/drivers/block/Makefile b/drivers/block/Makefile >> index f12447d78d..a161d145fd 100644 >> --- a/drivers/block/Makefile >> +++ b/drivers/block/Makefile >> @@ -14,6 +14,7 @@ obj-$(CONFIG_IDE) += ide.o >> endif >> obj-$(CONFIG_SANDBOX) += sandbox.o host-uclass.o host_dev.o >> obj-$(CONFIG_$(SPL_TPL_)BLOCK_CACHE) += blkcache.o >> +obj-$(CONFIG_BLKMAP) += blkmap.o >> >> obj-$(CONFIG_EFI_MEDIA) += efi-media-uclass.o >> obj-$(CONFIG_EFI_MEDIA_SANDBOX) += sb_efi_media.o >> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c >> index c69fc4d518..cb73faaeda 100644 >> --- a/drivers/block/blk-uclass.c >> +++ b/drivers/block/blk-uclass.c >> @@ -32,6 +32,7 @@ static struct { >> { UCLASS_EFI_LOADER, "efiloader" }, >> { UCLASS_VIRTIO, "virtio" }, >> { UCLASS_PVBLOCK, "pvblock" }, >> + { UCLASS_BLKMAP, "blkmap" }, >> }; >> >> static enum uclass_id uclass_name_to_iftype(const char *uclass_idname) >> diff --git a/drivers/block/blkmap.c b/drivers/block/blkmap.c >> new file mode 100644 >> index 0000000000..a6ba07404c >> --- /dev/null >> +++ b/drivers/block/blkmap.c >> @@ -0,0 +1,275 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright (c) 2023 Addiva Elektronik >> + * Author: Tobias Waldekranz <tob...@waldekranz.com> >> + */ >> + >> +#include <common.h> >> +#include <blk.h> >> +#include <blkmap.h> >> +#include <dm.h> >> +#include <dm/device-internal.h> >> +#include <dm/lists.h> >> +#include <dm/root.h> > > The three above should go at the end: > > https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files I should've read more carefully, sorry. Fixing. >> +#include <malloc.h> >> +#include <part.h> >> + >> +struct blkmap; >> + >> +struct blkmap_slice { >> + struct list_head node; >> + >> + lbaint_t blknr; >> + lbaint_t blkcnt; >> + >> + ulong (*read)(struct blkmap *bm, struct blkmap_slice *bms, >> + lbaint_t blknr, lbaint_t blkcnt, void *buffer); >> + ulong (*write)(struct blkmap *bm, struct blkmap_slice *bms, >> + lbaint_t blknr, lbaint_t blkcnt, const void *buffer); >> + void (*destroy)(struct blkmap *bm, struct blkmap_slice *bms); >> +}; > > Please comment these fully in Sphinx style Will do. >> + >> +struct blkmap { >> + struct udevice *dev; >> + struct list_head slices; >> +}; >> + >> +static bool blkmap_slice_contains(struct blkmap_slice *bms, lbaint_t blknr) >> +{ >> + return (blknr >= bms->blknr) && (blknr < (bms->blknr + bms->blkcnt)); > > lots of brackets! > >> +} >> + >> +static bool blkmap_slice_available(struct blkmap *bm, struct blkmap_slice >> *new) >> +{ >> + struct blkmap_slice *bms; >> + lbaint_t first, last; >> + >> + first = new->blknr; >> + last = new->blknr + new->blkcnt - 1; >> + >> + list_for_each_entry(bms, &bm->slices, node) { >> + if (blkmap_slice_contains(bms, first) || >> + blkmap_slice_contains(bms, last) || >> + blkmap_slice_contains(new, bms->blknr) || >> + blkmap_slice_contains(new, bms->blknr + bms->blkcnt - 1)) >> + return false; >> + } >> + >> + return true; >> +} >> + >> +static struct blkmap *blkmap_from_devnum(int devnum) > > I don't really like the use of devnum everywhere. Can we instead limit > the devnum stuff to the cmdline implementation, and use a struct > udevice everywhere else? Sure, I'll change it. > The devum can be allocated when the UCLASS_BLK device is created. > >> +{ >> + struct udevice *dev; >> + int err; >> + >> + err = blk_find_device(UCLASS_BLKMAP, devnum, &dev); >> + >> + return err ? NULL : dev_get_priv(dev); >> +} >> + >> +static int blkmap_add(struct blkmap *bm, struct blkmap_slice *new) >> +{ >> + struct blk_desc *bd = dev_get_uclass_plat(bm->dev); >> + struct list_head *insert = &bm->slices; >> + struct blkmap_slice *bms; >> + >> + if (!blkmap_slice_available(bm, new)) >> + return -EBUSY; >> + >> + list_for_each_entry(bms, &bm->slices, node) { >> + if (bms->blknr < new->blknr) >> + continue; >> + >> + insert = &bms->node; >> + break; >> + } >> + >> + list_add_tail(&new->node, insert); >> + >> + /* Disk might have grown, update the size */ >> + bms = list_last_entry(&bm->slices, struct blkmap_slice, node); >> + bd->lba = bms->blknr + bms->blkcnt; >> + return 0; >> +} >> + >> +static struct udevice *blkmap_root(void) > > 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... >> +{ >> + 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> /boot It seems like labels are a hostfs-specific feature, or am I missing something? >> +{ >> + 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. 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? >> + if (!bm) >> + return -ENOMEM; >> + >> + err = blk_create_devicef(root, "blkmap_blk", "blk", UCLASS_BLKMAP, >> + devnum, 512, 0, &bm->dev); >> + if (err) >> + goto err_free; >> + >> + bd = dev_get_uclass_plat(bm->dev); >> + >> + /* EFI core isn't keen on zero-sized disks, so we lie. This is >> + * updated with the correct size once the user adds a >> + * mapping. >> + */ >> + bd->lba = 1; > > if (CONFIG_IS_ENABLED(EFI_LOADER)) > > ? Right. >> + >> + dev_set_priv(bm->dev, bm); >> + INIT_LIST_HEAD(&bm->slices); >> + >> + err = blk_probe_or_unbind(bm->dev); >> + if (err) >> + goto err_remove; >> + >> + return bd->devnum; >> + >> +err_remove: >> + device_remove(bm->dev, DM_REMOVE_NORMAL); >> +err_free: >> + free(bm); >> + return err; >> +} >> + >> +int blkmap_destroy(int devnum) > > label > >> +{ >> + struct blkmap_slice *bms, *tmp; >> + struct blkmap *bm; >> + int err; >> + >> + bm = blkmap_from_devnum(devnum); >> + if (!bm) >> + return -ENODEV; >> + >> + err = device_remove(bm->dev, DM_REMOVE_NORMAL); >> + if (err) >> + return err; >> + >> + err = device_unbind(bm->dev); >> + if (err) >> + return err; >> + >> + list_for_each_entry_safe(bms, tmp, &bm->slices, node) { >> + list_del(&bms->node); >> + free(bms); >> + } >> + >> + free(bm); >> + return 0; >> +} >> + >> +static ulong blkmap_read_slice(struct blkmap *bm, struct blkmap_slice *bms, >> + lbaint_t blknr, lbaint_t blkcnt, void *buffer) >> +{ >> + lbaint_t nr, cnt; >> + >> + nr = blknr - bms->blknr; >> + cnt = (blkcnt < bms->blkcnt) ? blkcnt : bms->blkcnt; >> + return bms->read(bm, bms, nr, cnt, buffer); >> +} >> + >> +static ulong blkmap_read(struct udevice *dev, lbaint_t blknr, lbaint_t >> blkcnt, >> + void *buffer) >> +{ >> + struct blk_desc *bd = dev_get_uclass_plat(dev); >> + struct blkmap *bm = dev_get_priv(dev); >> + struct blkmap_slice *bms; >> + lbaint_t cnt, total = 0; >> + >> + list_for_each_entry(bms, &bm->slices, node) { >> + if (!blkmap_slice_contains(bms, blknr)) >> + continue; >> + >> + cnt = blkmap_read_slice(bm, bms, blknr, blkcnt, buffer); >> + blknr += cnt; >> + blkcnt -= cnt; >> + buffer += cnt << bd->log2blksz; >> + total += cnt; >> + } >> + >> + return total; >> +} >> + >> +static ulong blkmap_write_slice(struct blkmap *bm, struct blkmap_slice *bms, >> + lbaint_t blknr, lbaint_t blkcnt, >> + const void *buffer) >> +{ >> + lbaint_t nr, cnt; >> + >> + nr = blknr - bms->blknr; >> + cnt = (blkcnt < bms->blkcnt) ? blkcnt : bms->blkcnt; >> + return bms->write(bm, bms, nr, cnt, buffer); >> +} >> + >> +static ulong blkmap_write(struct udevice *dev, lbaint_t blknr, lbaint_t >> blkcnt, >> + const void *buffer) >> +{ >> + struct blk_desc *bd = dev_get_uclass_plat(dev); >> + struct blkmap *bm = dev_get_priv(dev); >> + struct blkmap_slice *bms; >> + lbaint_t cnt, total = 0; >> + >> + list_for_each_entry(bms, &bm->slices, node) { >> + if (!blkmap_slice_contains(bms, blknr)) >> + continue; >> + >> + cnt = blkmap_write_slice(bm, bms, blknr, blkcnt, buffer); >> + blknr += cnt; >> + blkcnt -= cnt; >> + buffer += cnt << bd->log2blksz; >> + total += cnt; >> + } >> + >> + return total; >> +} >> + >> +static const struct blk_ops blkmap_ops = { >> + .read = blkmap_read, >> + .write = blkmap_write, >> +}; >> + >> +U_BOOT_DRIVER(blkmap_blk) = { >> + .name = "blkmap_blk", >> + .id = UCLASS_BLK, >> + .ops = &blkmap_ops, >> +}; >> + >> +U_BOOT_DRIVER(blkmap_root) = { >> + .name = "blkmap_root", >> + .id = UCLASS_BLKMAP, >> +}; >> + >> +UCLASS_DRIVER(blkmap) = { >> + .id = UCLASS_BLKMAP, >> + .name = "blkmap", >> +}; >> diff --git a/include/blkmap.h b/include/blkmap.h >> new file mode 100644 >> index 0000000000..37c0c31c3f >> --- /dev/null >> +++ b/include/blkmap.h >> @@ -0,0 +1,15 @@ >> +/* SPDX-License-Identifier: GPL-2.0+ */ >> +/* >> + * Copyright (c) 2023 Addiva Elektronik >> + * Author: Tobias Waldekranz <tob...@waldekranz.com> >> + */ >> + >> +#ifndef _BLKMAP_H >> +#define _BLKMAP_H >> + >> +#include <stdbool.h> >> + >> +int blkmap_create(int devnum); >> +int blkmap_destroy(int devnum); > > full comments for exported functions Fixing. >> + >> +#endif /* _BLKMAP_H */ >> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h >> index 33e43c20db..576237b954 100644 >> --- a/include/dm/uclass-id.h >> +++ b/include/dm/uclass-id.h >> @@ -37,6 +37,7 @@ enum uclass_id { >> UCLASS_AUDIO_CODEC, /* Audio codec with control and data path */ >> UCLASS_AXI, /* AXI bus */ >> UCLASS_BLK, /* Block device */ >> + UCLASS_BLKMAP, /* Composable virtual block device */ >> UCLASS_BOOTCOUNT, /* Bootcount backing store */ >> UCLASS_BOOTDEV, /* Boot device for locating an OS to boot */ >> UCLASS_BOOTMETH, /* Bootmethod for booting an OS */ >> diff --git a/include/efi_loader.h b/include/efi_loader.h >> index 4560b0d04c..59687f44de 100644 >> --- a/include/efi_loader.h >> +++ b/include/efi_loader.h >> @@ -134,6 +134,10 @@ static inline efi_status_t efi_launch_capsules(void) >> #define U_BOOT_GUID \ >> EFI_GUID(0xe61d73b9, 0xa384, 0x4acc, \ >> 0xae, 0xab, 0x82, 0xe8, 0x28, 0xf3, 0x62, 0x8b) >> +/* GUID used as root for blkmap devices */ >> +#define U_BOOT_BLKMAP_DEV_GUID \ >> + EFI_GUID(0x4cad859d, 0xd644, 0x42ff, \ >> + 0x87, 0x0b, 0xc0, 0x2e, 0xac, 0x05, 0x58, 0x63) >> /* GUID used as host device on sandbox */ >> #define U_BOOT_HOST_DEV_GUID \ >> EFI_GUID(0xbbe4e671, 0x5773, 0x4ea1, \ >> diff --git a/lib/efi_loader/efi_device_path.c >> b/lib/efi_loader/efi_device_path.c >> index 3b267b713e..4b4c96bc2e 100644 >> --- a/lib/efi_loader/efi_device_path.c >> +++ b/lib/efi_loader/efi_device_path.c > > Please put this EFI stuff in a separate patch. Will do. >> @@ -21,6 +21,9 @@ >> #include <asm-generic/unaligned.h> >> #include <linux/compat.h> /* U16_MAX */ >> >> +#ifdef CONFIG_BLKMAP >> +const efi_guid_t efi_guid_blkmap_dev = U_BOOT_BLKMAP_DEV_GUID; >> +#endif >> #ifdef CONFIG_SANDBOX >> const efi_guid_t efi_guid_host_dev = U_BOOT_HOST_DEV_GUID; >> #endif >> @@ -573,6 +576,16 @@ __maybe_unused static unsigned int dp_size(struct >> udevice *dev) >> */ >> return dp_size(dev->parent) >> + sizeof(struct efi_device_path_vendor) + 1; >> +#endif >> +#ifdef CONFIG_BLKMAP >> + case UCLASS_BLKMAP: >> + /* >> + * blkmap devices will be represented as a vendor >> + * device node with an extra byte for the device >> + * number. >> + */ >> + return dp_size(dev->parent) >> + + sizeof(struct efi_device_path_vendor) + 1; >> #endif >> default: >> return dp_size(dev->parent); >> @@ -631,6 +644,23 @@ __maybe_unused static void *dp_fill(void *buf, struct >> udevice *dev) >> #endif >> case UCLASS_BLK: >> switch (dev->parent->uclass->uc_drv->id) { >> +#ifdef CONFIG_BLKMAP >> + case UCLASS_BLKMAP: { >> + struct efi_device_path_vendor *dp; >> + struct blk_desc *desc = dev_get_uclass_plat(dev); >> + >> + dp_fill(buf, dev->parent); >> + dp = buf; >> + ++dp; >> + dp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE; >> + dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR; >> + dp->dp.length = sizeof(*dp) + 1; >> + memcpy(&dp->guid, &efi_guid_blkmap_dev, >> + sizeof(efi_guid_t)); >> + dp->vendor_data[0] = desc->devnum; >> + return &dp->vendor_data[1]; >> + } >> +#endif >> #ifdef CONFIG_SANDBOX >> case UCLASS_HOST: { >> /* stop traversing parents at this point: */ >> -- >> 2.34.1 >> > > Regards, > Simon