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
diff --git a/MAINTAINERS b/MAINTAINERS index 3e8e193ecc..28a34231bf 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -786,6 +786,12 @@ M: Alper Nebi Yasak alpernebiyasak@gmail.com S: Maintained F: tools/binman/
+BLKMAP +M: Tobias Waldekranz tobias@waldekranz.com +S: Maintained +F: drivers/block/blkmap.c +F: include/blkmap.h
BOOTDEVICE M: Simon Glass sjg@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 BLKhelpCreate virtual block devices that are backed by various sources,e.g. RAM, or parts of an existing block device. Though much morerudimentary, it borrows a lot of ideas from Linux's device mappersubsystem.Example use-cases:- Treat a region of RAM as a block device, i.e. a RAM disk. This let'syou extract files from filesystem images stored in RAM (perhaps as aresult of a TFTP transfer).- Create a virtual partition on an existing device. This let's youaccess filesystems that aren't stored at an exact partitionboundary. A common example is a filesystem image embedded in an FITimage.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 tobias@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-fil...
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 tobias@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