From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 07C6AC636D4 for ; Wed, 1 Feb 2023 21:21:53 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 0C67385A42; Wed, 1 Feb 2023 22:21:43 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="Fg0FDJ8A"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 2969A85D58; Wed, 1 Feb 2023 22:21:38 +0100 (CET) Received: from mail-yw1-x112e.google.com (mail-yw1-x112e.google.com [IPv6:2607:f8b0:4864:20::112e]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 7A37885D33 for ; Wed, 1 Feb 2023 21:21:53 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-yw1-x112e.google.com with SMTP id 00721157ae682-4c24993965eso261389037b3.12 for ; Wed, 01 Feb 2023 12:21:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=ZXxjRXlhcg4/u3J79/xds4u2xc+FkZqiKc/KTEU8370=; b=Fg0FDJ8Ael1np1b9C+QrB18SEZCYAWffEAsrGd7t18fuJ/Eiernwo5AcQJn/A8TivR gXyQdBYsH38gL4LV0bDrdEqH1G3G+IOTJWyO+4dlVq59hqpEC8xsDAF7vm7GL0exwhNQ LckHhR+ARWKAtWAQPbb7JwR3I0i/JoD5xcftY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=ZXxjRXlhcg4/u3J79/xds4u2xc+FkZqiKc/KTEU8370=; b=L+CCJesuxYTylpCPNv8Iag5LMdzdrCuRwRWMucg0N20MnjAKADKOVkzNhxfhFqYbrm sdYJSx99dE6R5sCJIT6rADtgfrqD/7X2MbSTHaW6H8FnEVZAkTx+kYJxkF4InNPi1LsG d34w9qY35PiUhApwE7gPnkIDPuBSLqlo9CLDhhWXyaux8t9urxwO1NzeIr9zMTGHQlyA uwLBQpScQwpiW5uzyDge0gBx+l4u0lMMh1uDN4FsfJtfY0sNHsI8yeSC4amhJong24/W dOz+NIQVpq2/9qSC7cTdqMT6n/mWs/UclD15+Oc98uh5gH3fcUEhFwfg7ZrnsOA71AOb cLDA== X-Gm-Message-State: AO0yUKU3sifflL/H4drp8Y0uoHzRrUMRcfX7cZjWgKB/Zd8QzGcpDEvw BxBEpiyvEq0AmrD3MZ7PSgh8erQujvviMc57xfpYaD2u/+i5rAwR X-Google-Smtp-Source: AK7set/svy2Ig5lkIkzrrdsbQp7c1tgCBP7WMFfFIwg9Kl5IiP02WQ/6q27Op3Fhdi/+blQkV/onpJTrNy6e60NQS7k= X-Received: by 2002:a05:690c:591:b0:506:484c:ccf9 with SMTP id bo17-20020a05690c059100b00506484cccf9mr425425ywb.118.1675282898511; Wed, 01 Feb 2023 12:21:38 -0800 (PST) MIME-Version: 1.0 References: <20230201181016.4145834-1-tobias@waldekranz.com> <20230201181016.4145834-4-tobias@waldekranz.com> In-Reply-To: <20230201181016.4145834-4-tobias@waldekranz.com> From: Simon Glass Date: Wed, 1 Feb 2023 13:20:59 -0700 Message-ID: Subject: Re: [PATCH 3/8] blk: blkmap: Add basic infrastructure To: Tobias Waldekranz Cc: xypron.glpk@gmx.de, ilias.apalodimas@linaro.org, u-boot@lists.denx.de Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean Hi Tobias, On Wed, 1 Feb 2023 at 11:10, Tobias Waldekranz 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 > --- > 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 > S: Maintained > F: tools/binman/ > > +BLKMAP > +M: Tobias Waldekranz > +S: Maintained > +F: drivers/block/blkmap.c > +F: include/blkmap.h > + > BOOTDEVICE > M: Simon Glass > 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 > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include The three above should go at the end: https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files > +#include > +#include > + > +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 > + > +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? 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? > +{ > + 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? > +{ > + 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. > + 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()? > + 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)) ? > + > + 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 > + */ > + > +#ifndef _BLKMAP_H > +#define _BLKMAP_H > + > +#include > + > +int blkmap_create(int devnum); > +int blkmap_destroy(int devnum); full comments for exported functions > + > +#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. > @@ -21,6 +21,9 @@ > #include > #include /* 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