On Tue, 21 Jan 2025 at 18:02, Sughosh Ganu <sughosh.g...@linaro.org> wrote:
>
> On Tue, 21 Jan 2025 at 21:25, Ilias Apalodimas
> <ilias.apalodi...@linaro.org> wrote:
> >
> > Hi Sughosh
> >
> > On Mon, 20 Jan 2025 at 12:51, Sughosh Ganu <sughosh.g...@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.
> >
> > I haven't followed up all the discussions yet, but I do have a question.
> > Are blkmap devices now unconditionally added in a pmem node? (if they
> > are backed by memory)
>
> Yes, all memory backed blkmap devices are being added as pmem nodes.
Alright, I don't think we want that. We want a boolean to the mapping
function, that cmd tools can conditionally set. Not all cases want to
preserve memory for the OS. The EFI part can set that flag to true.
I need to do some more testing, but a quick test in QEMU had this:
vda: vda1
nd_pmem namespace0.0: unable to guarantee persistence of writes
nd_pmem namespace0.0: could not reserve region [mem 0x40200000-0x547fffff]
nd_pmem namespace0.0: probe with driver nd_pmem failed with error -16
AFAICT the kernel behaves differently depending on its config and the
only reliable way to make this work, is to remove the mapping from the
EFI memory map. But that further complicates things because we can't
have the blkmap functions randomly call EFI functions....
Regards
/Ilias
>
> -sughosh
>
> >
> > Thanks
> > /Ilias
> >
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.g...@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(-)
> > >
> > > 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();
> > >
> > > --
> > > 2.34.1
> > >
