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

[Simon Glass]

Hi Tobias,

On Fri, 3 Feb 2023 at 02:38, Tobias Waldekranz <tobias at waldekranz.com> wrote:
>
> On ons, feb 01, 2023 at 13:20, Simon Glass <sjg at chromium.org> wrote:
> > Hi Tobias,
>
> Hi Simon,
>
> Thanks for the review!
>
> > On Wed, 1 Feb 2023 at 11:10, Tobias Waldekranz <tobias at 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 at 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> /boot
>
> It 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.

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