| Message ID | 20250120105045.1281262-5-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add pmem node for preserving distro ISO's | expand |
On mån, jan 20, 2025 at 16:20, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > Add information about the type of blkmap device in the blkmap > structure. Currently, the blkmap device is used for mapping to either > a memory based block device, or another block device (linear > mapping). Put information in the blkmap structure to identify if it is > associated with a memory or linear mapped device. Which can then be > used to take specific action based on the type of blkmap device. Is this restriction really necessary? Why should it not be possible to setup a block map like this: myblkmap: .--------. .-----. | slice0 +------> RAM | :--------: '-----' .-------. | slice1 +------------------> eMMC0 | :--------: .-------. '-------' | slice2 +------> eMMC1 | '........' '-------' Linux's "device mapper", after which blkmaps are modeled, works in this way. I.e. a blkmap is just a collection of slices, and it is up to each slice how its data is provided, meaning that the user is free to compose their virtual block device in whatever way they need. Looking at the pmem patch that follows this one, I am not able to find anything that would motivate restricting the functionality either.
On Mon, 20 Jan 2025 at 17:55, Tobias Waldekranz <tobias@waldekranz.com> wrote: > > On mån, jan 20, 2025 at 16:20, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > Add information about the type of blkmap device in the blkmap > > structure. Currently, the blkmap device is used for mapping to either > > a memory based block device, or another block device (linear > > mapping). Put information in the blkmap structure to identify if it is > > associated with a memory or linear mapped device. Which can then be > > used to take specific action based on the type of blkmap device. > > Is this restriction really necessary? Why should it not be possible to > setup a block map like this: > > myblkmap: > .--------. .-----. > | slice0 +------> RAM | > :--------: '-----' .-------. > | slice1 +------------------> eMMC0 | > :--------: .-------. '-------' > | slice2 +------> eMMC1 | > '........' '-------' > > Linux's "device mapper", after which blkmaps are modeled, works in this > way. I.e. a blkmap is just a collection of slices, and it is up to each > slice how its data is provided, meaning that the user is free to compose > their virtual block device in whatever way they need. The blkmap structure, the way it is designed, is pointing to the underlying block device. How can a single blkmap then be associated with slices of different types? Would that not contravene with the idea of a block device associating with a blkmap? > > Looking at the pmem patch that follows this one, I am not able to find > anything that would motivate restricting the functionality either. The subsequent patch is adding the persistent memory node to the device-tree. The pmem node that is to be added is the memory mapped blkmap device. The logic does check for the type of the blkmap device and then proceeds to add the pmem node only for the memory mapped blkmaps. -sughosh
On mån, jan 20, 2025 at 19:30, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > On Mon, 20 Jan 2025 at 17:55, Tobias Waldekranz <tobias@waldekranz.com> wrote: >> >> On mån, jan 20, 2025 at 16:20, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: >> > Add information about the type of blkmap device in the blkmap >> > structure. Currently, the blkmap device is used for mapping to either >> > a memory based block device, or another block device (linear >> > mapping). Put information in the blkmap structure to identify if it is >> > associated with a memory or linear mapped device. Which can then be >> > used to take specific action based on the type of blkmap device. >> >> Is this restriction really necessary? Why should it not be possible to >> setup a block map like this: >> >> myblkmap: >> .--------. .-----. >> | slice0 +------> RAM | >> :--------: '-----' .-------. >> | slice1 +------------------> eMMC0 | >> :--------: .-------. '-------' >> | slice2 +------> eMMC1 | >> '........' '-------' >> >> Linux's "device mapper", after which blkmaps are modeled, works in this >> way. I.e. a blkmap is just a collection of slices, and it is up to each >> slice how its data is provided, meaning that the user is free to compose >> their virtual block device in whatever way they need. > > The blkmap structure, the way it is designed, is pointing to the > underlying block device. How can a single blkmap then be associated The `struct udevice *blk` from `struct blkmap` is a reference to the block device which represents the block map itself ("myblkmap" in the picture above), not any lower device. > with slices of different types? Would that not contravene with the > idea of a block device associating with a blkmap? For slices which are linear mappings (and are thus backed by some other underlying block device), their reference to that lower device ("eMMC0" and "eMMC1" above) is stored in the `struct udevice *blk` member of `struct blkmap_linear`. Slices which are backed by memory does not have any reference to a lower device, but merely a pointer to the start of the mapping - `void *addr` in `struct blkmap_mem`. The overarching idea is that the block map does not have to know anything about the implementation of how any individual slice chooses to provide its data. It only knows about their sizes and offsets. Based on that information, it simply routes incoming read/write requests to the correct slice. >> >> Looking at the pmem patch that follows this one, I am not able to find >> anything that would motivate restricting the functionality either. > > The subsequent patch is adding the persistent memory node to the > device-tree. The pmem node that is to be added is the memory mapped > blkmap device. The logic does check for the type of the blkmap device > and then proceeds to add the pmem node only for the memory mapped > blkmaps. Sorry I am confused. Why do you need a block map device to add the pmem node to the device tree?
On Mon, 20 Jan 2025 at 20:06, Tobias Waldekranz <tobias@waldekranz.com> wrote: > > On mån, jan 20, 2025 at 19:30, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Mon, 20 Jan 2025 at 17:55, Tobias Waldekranz <tobias@waldekranz.com> wrote: > >> > >> On mån, jan 20, 2025 at 16:20, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > >> > Add information about the type of blkmap device in the blkmap > >> > structure. Currently, the blkmap device is used for mapping to either > >> > a memory based block device, or another block device (linear > >> > mapping). Put information in the blkmap structure to identify if it is > >> > associated with a memory or linear mapped device. Which can then be > >> > used to take specific action based on the type of blkmap device. > >> > >> Is this restriction really necessary? Why should it not be possible to > >> setup a block map like this: > >> > >> myblkmap: > >> .--------. .-----. > >> | slice0 +------> RAM | > >> :--------: '-----' .-------. > >> | slice1 +------------------> eMMC0 | > >> :--------: .-------. '-------' > >> | slice2 +------> eMMC1 | > >> '........' '-------' > >> > >> Linux's "device mapper", after which blkmaps are modeled, works in this > >> way. I.e. a blkmap is just a collection of slices, and it is up to each > >> slice how its data is provided, meaning that the user is free to compose > >> their virtual block device in whatever way they need. > > > > The blkmap structure, the way it is designed, is pointing to the > > underlying block device. How can a single blkmap then be associated > > The `struct udevice *blk` from `struct blkmap` is a reference to the > block device which represents the block map itself ("myblkmap" in the > picture above), not any lower device. Okay. I got confused with the comment associated with that member, which says, "Underlying block device". This I interpreted to be the block device that is associated with the blkmap structure. > > > with slices of different types? Would that not contravene with the > > idea of a block device associating with a blkmap? > > For slices which are linear mappings (and are thus backed by some other > underlying block device), their reference to that lower device ("eMMC0" > and "eMMC1" above) is stored in the `struct udevice *blk` member of > `struct blkmap_linear`. Okay. But then, the computation of the blocksize seems to be happening at the blkmap device level, which again implies having the same set of slices associated with the blkmap. Any reason why the blksize is not taken from the block device associated with that slice? That would make it clear that the slice mapping type is independent from the parent blkmap device. > > Slices which are backed by memory does not have any reference to a lower > device, but merely a pointer to the start of the mapping - `void *addr` > in `struct blkmap_mem`. > > The overarching idea is that the block map does not have to know > anything about the implementation of how any individual slice chooses to > provide its data. It only knows about their sizes and offsets. Based > on that information, it simply routes incoming read/write requests to > the correct slice. Okay. I think, for my solution, I will just need to move type identification to the slice, instead of the blkmap device. > > >> > >> Looking at the pmem patch that follows this one, I am not able to find > >> anything that would motivate restricting the functionality either. > > > > The subsequent patch is adding the persistent memory node to the > > device-tree. The pmem node that is to be added is the memory mapped > > blkmap device. The logic does check for the type of the blkmap device > > and then proceeds to add the pmem node only for the memory mapped > > blkmaps. > > Sorry I am confused. Why do you need a block map device to add the pmem > node to the device tree? This is needed to include the RAM based block device information in the device-tree as pmem node. The OS installer then uses this pmem device as the block device which contains the installation packages, and proceeds with the OS installation. -sughosh
On mån, jan 20, 2025 at 21:10, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > On Mon, 20 Jan 2025 at 20:06, Tobias Waldekranz <tobias@waldekranz.com> wrote: >> >> On mån, jan 20, 2025 at 19:30, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: >> > On Mon, 20 Jan 2025 at 17:55, Tobias Waldekranz <tobias@waldekranz.com> wrote: >> >> >> >> On mån, jan 20, 2025 at 16:20, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: >> >> > Add information about the type of blkmap device in the blkmap >> >> > structure. Currently, the blkmap device is used for mapping to either >> >> > a memory based block device, or another block device (linear >> >> > mapping). Put information in the blkmap structure to identify if it is >> >> > associated with a memory or linear mapped device. Which can then be >> >> > used to take specific action based on the type of blkmap device. >> >> >> >> Is this restriction really necessary? Why should it not be possible to >> >> setup a block map like this: >> >> >> >> myblkmap: >> >> .--------. .-----. >> >> | slice0 +------> RAM | >> >> :--------: '-----' .-------. >> >> | slice1 +------------------> eMMC0 | >> >> :--------: .-------. '-------' >> >> | slice2 +------> eMMC1 | >> >> '........' '-------' >> >> >> >> Linux's "device mapper", after which blkmaps are modeled, works in this >> >> way. I.e. a blkmap is just a collection of slices, and it is up to each >> >> slice how its data is provided, meaning that the user is free to compose >> >> their virtual block device in whatever way they need. >> > >> > The blkmap structure, the way it is designed, is pointing to the >> > underlying block device. How can a single blkmap then be associated >> >> The `struct udevice *blk` from `struct blkmap` is a reference to the >> block device which represents the block map itself ("myblkmap" in the >> picture above), not any lower device. > > Okay. I got confused with the comment associated with that member, > which says, "Underlying block device". This I interpreted to be the > block device that is associated with the blkmap structure. Yeah I agree that it could be made clearer :) >> >> > with slices of different types? Would that not contravene with the >> > idea of a block device associating with a blkmap? >> >> For slices which are linear mappings (and are thus backed by some other >> underlying block device), their reference to that lower device ("eMMC0" >> and "eMMC1" above) is stored in the `struct udevice *blk` member of >> `struct blkmap_linear`. > > Okay. But then, the computation of the blocksize seems to be happening > at the blkmap device level, which again implies having the same set of > slices associated with the blkmap. Any reason why the blksize is not > taken from the block device associated with that slice? That would > make it clear that the slice mapping type is independent from the > parent blkmap device. In the original series, only linear mappings to devices which used block sizes of 512 was supported, precisely because otherwise you need to do proper translation to work in all cases. I tried to argue this point on the list back then: https://lore.kernel.org/u-boot/875y3wohrt.fsf@waldekranz.com/ but I did not get my point across and the restriction was lifted anyway. >> >> Slices which are backed by memory does not have any reference to a lower >> device, but merely a pointer to the start of the mapping - `void *addr` >> in `struct blkmap_mem`. >> >> The overarching idea is that the block map does not have to know >> anything about the implementation of how any individual slice chooses to >> provide its data. It only knows about their sizes and offsets. Based >> on that information, it simply routes incoming read/write requests to >> the correct slice. > > Okay. I think, for my solution, I will just need to move type > identification to the slice, instead of the blkmap device. > >> >> >> >> >> Looking at the pmem patch that follows this one, I am not able to find >> >> anything that would motivate restricting the functionality either. >> > >> > The subsequent patch is adding the persistent memory node to the >> > device-tree. The pmem node that is to be added is the memory mapped >> > blkmap device. The logic does check for the type of the blkmap device >> > and then proceeds to add the pmem node only for the memory mapped >> > blkmaps. >> >> Sorry I am confused. Why do you need a block map device to add the pmem >> node to the device tree? > > This is needed to include the RAM based block device information in > the device-tree as pmem node. The OS installer then uses this pmem > device as the block device which contains the installation packages, > and proceeds with the OS installation. But even if the user has not setup a blkmap, don't you want to inject the pmem node in the DT anyway? All you need is the size and offset of the blob right? Is that not available from `image_setup_libfdt()`?
diff --git a/cmd/blkmap.c b/cmd/blkmap.c index 164f80f1387..1bf0747ab16 100644 --- a/cmd/blkmap.c +++ b/cmd/blkmap.c @@ -119,15 +119,23 @@ static int do_blkmap_map(struct cmd_tbl *cmdtp, int flag, static int do_blkmap_create(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { + enum blkmap_type type; const char *label; int err; - if (argc != 2) + if (argc != 3) return CMD_RET_USAGE; label = argv[1]; - err = blkmap_create(label, NULL); + if (!strcmp(argv[2], "linear")) + type = BLKMAP_LINEAR; + else if (!strcmp(argv[2], "mem")) + type = BLKMAP_MEM; + else + return CMD_RET_USAGE; + + err = blkmap_create(label, NULL, type); if (err) { printf("Unable to create \"%s\": %d\n", label, err); return CMD_RET_FAILURE; @@ -218,7 +226,7 @@ U_BOOT_CMD_WITH_SUBCMDS( "blkmap read <addr> <blk#> <cnt>\n" "blkmap write <addr> <blk#> <cnt>\n" "blkmap get <label> dev [<var>] - store device number in variable\n" - "blkmap create <label> - create device\n" + "blkmap create <label> <type> - create device(linear/mem)\n" "blkmap destroy <label> - destroy device\n" "blkmap map <label> <blk#> <cnt> linear <interface> <dev> <blk#> - device mapping\n" "blkmap map <label> <blk#> <cnt> mem <addr> - memory mapping\n", @@ -228,6 +236,6 @@ U_BOOT_CMD_WITH_SUBCMDS( U_BOOT_SUBCMD_MKENT(read, 5, 1, do_blkmap_common), U_BOOT_SUBCMD_MKENT(write, 5, 1, do_blkmap_common), U_BOOT_SUBCMD_MKENT(get, 5, 1, do_blkmap_get), - U_BOOT_SUBCMD_MKENT(create, 2, 1, do_blkmap_create), + U_BOOT_SUBCMD_MKENT(create, 3, 1, do_blkmap_create), U_BOOT_SUBCMD_MKENT(destroy, 2, 1, do_blkmap_destroy), U_BOOT_SUBCMD_MKENT(map, 32, 1, do_blkmap_map)); diff --git a/drivers/block/blkmap.c b/drivers/block/blkmap.c index 34eed1380dc..a817345b6bc 100644 --- a/drivers/block/blkmap.c +++ b/drivers/block/blkmap.c @@ -153,6 +153,9 @@ int blkmap_map_linear(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, struct blk_desc *bd, *lbd; int err; + if (bm->type != BLKMAP_LINEAR) + return log_msg_ret("Invalid blkmap type", -EINVAL); + bd = dev_get_uclass_plat(bm->blk); lbd = dev_get_uclass_plat(lblk); if (lbd->blksz != bd->blksz) { @@ -240,6 +243,9 @@ int __blkmap_map_mem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, struct blkmap_mem *bmm; int err; + if (bm->type != BLKMAP_MEM) + return log_msg_ret("Invalid blkmap type", -EINVAL); + bmm = malloc(sizeof(*bmm)); if (!bmm) return -ENOMEM; @@ -435,7 +441,8 @@ struct udevice *blkmap_from_label(const char *label) return NULL; } -int blkmap_create(const char *label, struct udevice **devp) +int blkmap_create(const char *label, struct udevice **devp, + enum blkmap_type type) { char *hname, *hlabel; struct udevice *dev; @@ -472,6 +479,7 @@ int blkmap_create(const char *label, struct udevice **devp) device_set_name_alloced(dev); bm = dev_get_plat(dev); bm->label = hlabel; + bm->type = type; if (devp) *devp = dev; diff --git a/drivers/block/blkmap_helper.c b/drivers/block/blkmap_helper.c index bfba14110d2..56cbe57d4aa 100644 --- a/drivers/block/blkmap_helper.c +++ b/drivers/block/blkmap_helper.c @@ -19,7 +19,7 @@ int blkmap_create_ramdisk(const char *label, ulong image_addr, ulong image_size, struct blk_desc *desc; struct udevice *bm_dev; - ret = blkmap_create(label, &bm_dev); + ret = blkmap_create(label, &bm_dev, BLKMAP_MEM); if (ret) { log_err("failed to create blkmap\n"); return ret; diff --git a/include/blkmap.h b/include/blkmap.h index d53095437fa..21169c30af1 100644 --- a/include/blkmap.h +++ b/include/blkmap.h @@ -9,6 +9,12 @@ #include <dm/lists.h> +/* Type of blkmap device, Linear or Memory */ +enum blkmap_type { + BLKMAP_LINEAR = 1, + BLKMAP_MEM, +}; + /** * struct blkmap - Block map * @@ -16,11 +22,13 @@ * * @label: Human readable name of this blkmap * @blk: Underlying block device + * @type: Type of blkmap device * @slices: List of slices associated with this blkmap */ struct blkmap { char *label; struct udevice *blk; + enum blkmap_type type; struct list_head slices; }; @@ -78,9 +86,11 @@ struct udevice *blkmap_from_label(const char *label); * * @label: Label of the new blkmap * @devp: If not NULL, updated with the address of the resulting device + * @type: Type of blkmap device to create * Returns: 0 on success, negative error code on failure */ -int blkmap_create(const char *label, struct udevice **devp); +int blkmap_create(const char *label, struct udevice **devp, + enum blkmap_type type); /** * blkmap_destroy() - Destroy blkmap diff --git a/test/dm/blkmap.c b/test/dm/blkmap.c index a6a0b4d4e20..06816cb4b54 100644 --- a/test/dm/blkmap.c +++ b/test/dm/blkmap.c @@ -56,7 +56,7 @@ static int dm_test_blkmap_read(struct unit_test_state *uts) struct udevice *dev, *blk; const struct mapping *m; - ut_assertok(blkmap_create("rdtest", &dev)); + ut_assertok(blkmap_create("rdtest", &dev, BLKMAP_MEM)); ut_assertok(blk_get_from_parent(dev, &blk)); /* Generate an ordered and an unordered pattern in memory */ @@ -85,7 +85,7 @@ static int dm_test_blkmap_write(struct unit_test_state *uts) struct udevice *dev, *blk; const struct mapping *m; - ut_assertok(blkmap_create("wrtest", &dev)); + ut_assertok(blkmap_create("wrtest", &dev, BLKMAP_MEM)); ut_assertok(blk_get_from_parent(dev, &blk)); /* Generate an ordered and an unordered pattern in memory */ @@ -114,7 +114,7 @@ static int dm_test_blkmap_slicing(struct unit_test_state *uts) { struct udevice *dev; - ut_assertok(blkmap_create("slicetest", &dev)); + ut_assertok(blkmap_create("slicetest", &dev, BLKMAP_MEM)); ut_assertok(blkmap_map_mem(dev, 8, 8, NULL)); @@ -140,19 +140,19 @@ static int dm_test_blkmap_creation(struct unit_test_state *uts) { struct udevice *first, *second; - ut_assertok(blkmap_create("first", &first)); + ut_assertok(blkmap_create("first", &first, BLKMAP_LINEAR)); /* Can't have two "first"s */ - ut_asserteq(-EBUSY, blkmap_create("first", &second)); + ut_asserteq(-EBUSY, blkmap_create("first", &second, BLKMAP_LINEAR)); /* But "second" should be fine */ - ut_assertok(blkmap_create("second", &second)); + ut_assertok(blkmap_create("second", &second, BLKMAP_LINEAR)); /* Once "first" is destroyed, we should be able to create it * again */ ut_assertok(blkmap_destroy(first)); - ut_assertok(blkmap_create("first", &first)); + ut_assertok(blkmap_create("first", &first, BLKMAP_LINEAR)); ut_assertok(blkmap_destroy(first)); ut_assertok(blkmap_destroy(second)); @@ -168,7 +168,7 @@ static int dm_test_cmd_blkmap(struct unit_test_state *uts) ut_assertok(run_command("blkmap info", 0)); ut_assert_console_end(); - ut_assertok(run_command("blkmap create ramdisk", 0)); + ut_assertok(run_command("blkmap create ramdisk mem", 0)); ut_assert_nextline("Created \"ramdisk\""); ut_assert_console_end();
Add information about the type of blkmap device in the blkmap structure. Currently, the blkmap device is used for mapping to either a memory based block device, or another block device (linear mapping). Put information in the blkmap structure to identify if it is associated with a memory or linear mapped device. Which can then be used to take specific action based on the type of blkmap device. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- Changes since V2: New patch cmd/blkmap.c | 16 ++++++++++++---- drivers/block/blkmap.c | 10 +++++++++- drivers/block/blkmap_helper.c | 2 +- include/blkmap.h | 12 +++++++++++- test/dm/blkmap.c | 16 ++++++++-------- 5 files changed, 41 insertions(+), 15 deletions(-)