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 9F441C636CD for ; Tue, 7 Feb 2023 08:31:47 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 80FDC85855; Tue, 7 Feb 2023 09:31:44 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=waldekranz.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=waldekranz-com.20210112.gappssmtp.com header.i=@waldekranz-com.20210112.gappssmtp.com header.b="khK9aCB9"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 4CCCC85D59; Tue, 7 Feb 2023 09:31:42 +0100 (CET) Received: from mail-oo1-xc34.google.com (mail-oo1-xc34.google.com [IPv6:2607:f8b0:4864:20::c34]) (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 179FF85855 for ; Tue, 7 Feb 2023 09:31:38 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=waldekranz.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=tobias@waldekranz.com Received: by mail-oo1-xc34.google.com with SMTP id y81-20020a4a4554000000b0051a7cd153ddso567919ooa.10 for ; Tue, 07 Feb 2023 00:31:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=waldekranz-com.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:cc:to:from:from:to:cc:subject:date:message-id :reply-to; bh=akUvdJzHj5+d90X0ibNoLCO2+FdzdgkzP4x9qfroT7Y=; b=khK9aCB9D+kFPCZoa2J3gwdSsXwkRQTCBh+3PW7QxWv3MBfpmGEFib7u898W1GtAfL NK+ubsRXpxJzBXkyE16GDGDVEgGmoVVi1jAJ6Heor5ntp1RlYKASjj6CXZZIJtseAPbz ogIA+aOxTgYRcyB27MYeDtdXFhlguSmWG6p+ki+COmYr4lvAOgqVDduo+BsW5Wu86767 FACDIx73laoWaE3yS8ErzB7WiLeoFgRjoQ0A4OmXYIpFhO7bfFWw4yQJe3dnPy4nf8XY t145aaLs7W1j9JxO0WoUKV9PHazaralKJRAvL+LLsobaS7Y4wYb+AQ67byPDRekxkhZS Nhbw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=akUvdJzHj5+d90X0ibNoLCO2+FdzdgkzP4x9qfroT7Y=; b=JJ5HJGvilBkYAfCEriFPoKv1cRiqvv1kCHRU30uyw2vbjCwLV6afGjV11VWCmuhFV5 y5lKhvZp1EMq/nmrm18HVDAUMfwD4weewm9vbO88nDSU8KDlMdKzKiRzpr4qmTZMIVXx j2nw5YLGDMPrb8RTxCGFxMpof38UXh/NJyQF976IQQtqs2YM1p+QGxCubFQJKjENd7Ni GUIOmgEpRO5ey3kCl9Y1JnOfLZPk3ODHk0cCnuVs5WPQwMrKXJ4H++sA+HQV2Jb/YNCl kWLsvMa45qQMqfWE/QrVbtpe8Fmb0IT30ofid/mxFtP7tcbGECc2ekdpCe2SpI0qPqWA N7GQ== X-Gm-Message-State: AO0yUKVwW/By4mb9Yzj831wjX7br9u/Tczmcz4NA1TIJDVT4BlM5n0Nr V/CFp17Z9fvqFJu1wiUAyUPlKrrbmX89KM5p X-Google-Smtp-Source: AK7set+QC8N5baGz5m2eh/+dzWzAPop/0tJ0Hx1WMT1lhPM3IZWnYWIH+1cO+c3Qj/vyetaFk4TqNQ== X-Received: by 2002:a05:6820:449:b0:51a:7a15:9758 with SMTP id p9-20020a056820044900b0051a7a159758mr926658oou.5.1675758696189; Tue, 07 Feb 2023 00:31:36 -0800 (PST) Received: from wkz-x13 (a124.broadband3.quicknet.se. [46.17.184.124]) by smtp.gmail.com with ESMTPSA id k29-20020a4a851d000000b0049fd5c02d25sm5772177ooh.12.2023.02.07.00.31.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Feb 2023 00:31:35 -0800 (PST) From: Tobias Waldekranz To: Simon Glass Cc: xypron.glpk@gmx.de, ilias.apalodimas@linaro.org, u-boot@lists.denx.de Subject: Re: [PATCH 3/8] blk: blkmap: Add basic infrastructure In-Reply-To: References: <20230201181016.4145834-1-tobias@waldekranz.com> <20230201181016.4145834-4-tobias@waldekranz.com> <875ycjm6li.fsf@waldekranz.com> <87357jmc12.fsf@waldekranz.com> Date: Tue, 07 Feb 2023 09:31:32 +0100 Message-ID: <87zg9plvvf.fsf@waldekranz.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 On m=C3=A5n, feb 06, 2023 at 21:02, Simon Glass wrote: > Hi Tobias, > > On Mon, 6 Feb 2023 at 01:30, Tobias Waldekranz wr= ote: >> >> On fre, feb 03, 2023 at 17:20, Simon Glass wrote: >> > Hi Tobias, >> > >> > On Fri, 3 Feb 2023 at 02:38, Tobias Waldekranz = wrote: >> >> >> >> On ons, feb 01, 2023 at 13:20, Simon Glass wrote: >> >> > Hi Tobias, >> >> >> >> Hi Simon, >> >> >> >> Thanks for the review! >> >> >> >> > 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 bloc= ks >> >> >> 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 >> >> >> >> > >> > [..] >> > >> >> > 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 do= n't >> >> > need to do that. >> >> > >> >> > Note that the host commands support either an label or a devnum, wh= ich >> >> > I think is useful, so you might copy that? >> >> > >> >> >> >> I took a look at the hostfs implementation. I agree that labels are m= uch >> >> nicer than bare integers. However, for block maps the idea is to fit = in >> >> to the existing filesystem infrastructure. Addressing block devices >> >> using the " [:]" 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 =3D device_bind_driver(dm_root(), "blkmap_root", "blkm= ap", &dev); >> >> >> + if (err) >> >> >> + return NULL; >> >> >> + >> >> >> + err =3D device_probe(dev); >> >> >> + if (err) { >> >> >> + device_unbind(dev); >> >> >> + return NULL; >> >> >> + } >> >> > >> >> > Should not be needed as probing children will cause this to be prob= ed. >> >> > >> >> > 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 to supply to, e.g.: >> >> >> >> ls blkmap /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. >> >> But there is no connection to the devnum that is allocated internally by >> U-Boot. Here's an experiment I just ran: >> >> I created two squashfs images containing a single directory: >> >> zero.squashfs: >> i_am_zero >> >> one.squashfs: >> i_am_one >> >> Then I added a binding to them: >> >> =3D> host bind 1 one.squashfs >> =3D> host bind 0 zero.squashfs >> >> When accessing them, we see that the existing filesystem utilities work >> on the internally generated devnums, ignoring the labels: >> >> =3D> ls host 1 >> i_am_zero/ >> >> 0 file(s), 1 dir(s) >> >> =3D> ls host 0 >> i_am_one/ >> >> 0 file(s), 1 dir(s) >> >> =3D> >> >> Doesn't it therefore make more sense to stick with the established >> abstraction? > > It is pretty clear that this is a bit silly though :-) As in "this specific example will never be used in practice"? Sure :) My point was just that the approach to stick to integer labels is brittle, since there is no connection between the label and the devnum used by existing commands. Here's an even simpler example that might actually trip up a user: =3D> host bind 1 one.squashfs =3D> ls host 1 ** Bad device specification host 1 ** Couldn't find partition host 1 =3D> ls host 0 i_am_one/ 0 file(s), 1 dir(s) =3D> > I mean, right now, it would be easier to stick with numbered devices. > But we want to be able to support named devices (e.g. using struct > udevice->name). A good way to be forward compatible is to support a > label today. > > When we do get to it, the less code that has piled up and needs > converting, the more likely it is to happen. I completely understand, and agree with, the direction you want to take this. > Sure, you have the problem as above, but mostly people are only going > to use one of them, so it doesn't matter. > > We could also have a way of obtaining a number from a label, if you > want to go that far. But I suggest just telling people to use labels > like "1" and "0" which should work. Alright, well, if that is acceptable then I'll do it that way. For my own piece of mind, I think I'll also add some way of safely doing the reverse mapping for any label. Does the following look ok? blkmap get