Re: [PATCH 3/8] blk: blkmap: Add basic infrastructure

[Tobias Waldekranz <tobias@waldekranz.com>]

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 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 <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-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 <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