On 23/07/26 09:33:39, Neil Armstrong wrote:
> On 7/22/26 20:44, Jorge Ramirez wrote:
> > On 22/07/26 20:12:19, Jorge Ramirez wrote:
> > > On 22/07/26 17:43:16, Neil Armstrong wrote:
> > > > On 7/22/26 16:12, Jorge Ramirez wrote:
> > > > > On 22/07/26 10:31:04, [email protected] wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On 7/22/26 08:07, Jorge Ramirez-Ortiz wrote:
> > > > > > > OP-TEE's legacy RPMB supplicant interface assumes the RPMB device
> > > > > > > is eMMC
> > > > > > > and derives the key from an eMMC-shaped device ID, so it can never
> > > > > > > reproduce the key Linux derives for a UFS device; secure storage
> > > > > > > on
> > > > > > > UFS-only platforms instead needs the transport-agnostic RPMB
> > > > > > > subsystem
> > > > > > > interface, through which the normal world describes the real RPMB
> > > > > > > device
> > > > > > > to OP-TEE. Add a UFS backend for it so OP-TEE derives a key
> > > > > > > matching Linux
> > > > > > > and can use RPMB secure storage on UFS, leaving the legacy eMMC
> > > > > > > path
> > > > > > > untouched.
> > > > > > >
> > > > > > > This subsystem backend currently supports UFS drives only; eMMC
> > > > > > > still uses
> > > > > > > the legacy interface. As the legacy interface is dropped in
> > > > > > > favour of the
> > > > > > > subsystem one, an eMMC backend should be added here so both
> > > > > > > transports are
> > > > > > > served through a single RPMB path.
> > > > > > >
> > > > > > > Signed-off-by: Jorge Ramirez-Ortiz
> > > > > > > <[email protected]>
> > > > > > > ---
> > > > > > > drivers/tee/optee/Makefile | 1 +
> > > > > > > drivers/tee/optee/optee_msg_supplicant.h | 8 ++
> > > > > > > drivers/tee/optee/optee_private.h | 41 ++++++++
> > > > > > > drivers/tee/optee/rpmb.c | 124
> > > > > > > +++++++++++++++++++++++
> > > > > >
> > > > > > Why not rpmb_ufs ?
> > > > >
> > > > >
> > > > > because in the future (and IMO) rpmb.c should support UFS and EMMC
> > > > > using
> > > > > the new probe interface (eventually removing rpmb_legacy).
> > > >
> > > > Let's see in the future, if somehow there's need for a common "new"
> > > > and emmc only legacy some rework will be needed anyway. The eMMC RPMB
> > > > hasn't been updated for a very long time...
> > > >
> > > > Using the driver model to register an rpmb device attached to an eMMC
> > > > or UFS controller would the cleanest way to handle that, but we're not
> > > > here now.
> > > >
> > > > >
> > > > > >
> > > > > > It's fine to have 2 backend implementations, the emmc is
> > > > > > implementing the
> > > > > > legacy optee API, just add a comment in the eMMC file about that.
> > > > > >
> > > > > > Neil
> > > > > >
> > > > > > > drivers/tee/optee/supplicant.c | 9 ++
> > > > > > > 5 files changed, 183 insertions(+)
> > > > > > > create mode 100644 drivers/tee/optee/rpmb.c
> > > > > > >
> > > > > > > diff --git a/drivers/tee/optee/Makefile
> > > > > > > b/drivers/tee/optee/Makefile
> > > > > > > index 8321cf53a19..7af19834c00 100644
> > > > > > > --- a/drivers/tee/optee/Makefile
> > > > > > > +++ b/drivers/tee/optee/Makefile
> > > > > > > @@ -4,3 +4,4 @@ obj-$(CONFIG_OPTEE) += core.o
> > > > > > > obj-y += supplicant.o
> > > > > > > obj-$(CONFIG_DM_I2C) += i2c.o
> > > > > > > obj-$(CONFIG_SUPPORT_EMMC_RPMB) += rpmb_legacy.o
> > > > > > > +obj-$(CONFIG_SUPPORT_UFS_RPMB) += rpmb.o
> > > > > > > diff --git a/drivers/tee/optee/optee_msg_supplicant.h
> > > > > > > b/drivers/tee/optee/optee_msg_supplicant.h
> > > > > > > index 963cfd47824..b720d5d7b3f 100644
> > > > > > > --- a/drivers/tee/optee/optee_msg_supplicant.h
> > > > > > > +++ b/drivers/tee/optee/optee_msg_supplicant.h
> > > > > > > @@ -152,6 +152,14 @@
> > > > > > > */
> > > > > > > #define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> > > > > > > +#define OPTEE_MSG_RPC_CMD_RPMB_PROBE_RESET 22
> > > > > > > +#define OPTEE_MSG_RPC_CMD_RPMB_PROBE_NEXT 23
> > > > > > > +#define OPTEE_MSG_RPC_CMD_RPMB_FRAMES 24
> > > > > > > +
> > > > > > > +#define OPTEE_RPC_SHM_TYPE_APPL 0
> > > > > > > +
> > > > > > > +#define OPTEE_RPC_RPMB_UFS 1
> > > > > > > +
> > > > > > > /*
> > > > > > > * Was OPTEE_MSG_RPC_CMD_SQL_FS, which isn't supported any
> > > > > > > longer
> > > > > > > */
> > > > > > > diff --git a/drivers/tee/optee/optee_private.h
> > > > > > > b/drivers/tee/optee/optee_private.h
> > > > > > > index 1f07a27ee4b..2888257aefe 100644
> > > > > > > --- a/drivers/tee/optee/optee_private.h
> > > > > > > +++ b/drivers/tee/optee/optee_private.h
> > > > > > > @@ -9,6 +9,17 @@
> > > > > > > #include <tee.h>
> > > > > > > #include <log.h>
> > > > > > > +#ifdef CONFIG_SUPPORT_UFS_RPMB
> > > > > > > +/**
> > > > > > > + * struct optee_private - OP-TEE driver private data
> > > > > > > + * @rpmb_next_region: next UFS RPMB region to report on
> > > > > > > PROBE_NEXT
> > > > > > > + * @rpmb_cur_region: UFS RPMB region selected by the last
> > > > > > > PROBE_NEXT
> > > > > > > + */
> > > > > > > +struct optee_private {
> > > > > > > + unsigned int rpmb_next_region;
> > > > > > > + unsigned int rpmb_cur_region;
> > > > > > > +};
> > > > > > > +#else
> > > > > > > /**
> > > > > > > * struct optee_private - OP-TEE driver private data
> > > > > > > * @rpmb_mmc: mmc device for the RPMB partition
> > > > > > > @@ -22,6 +33,7 @@ struct optee_private {
> > > > > > > int rpmb_dev_id;
> > > > > > > int rpmb_original_part;
> > > > > > > };
> > > > > > > +#endif
> > > > > > > struct optee_msg_arg;
> > > > > > > @@ -60,6 +72,35 @@ static inline void
> > > > > > > optee_suppl_rpmb_release(struct udevice *dev)
> > > > > > > }
> > > > > > > #endif
> > > > > > > +#ifdef CONFIG_SUPPORT_UFS_RPMB
> > > > > > > +void optee_suppl_cmd_rpmb_probe_reset(struct udevice *dev,
> > > > > > > + struct optee_msg_arg *arg);
> > > > > > > +
> > > > > > > +void optee_suppl_cmd_rpmb_probe_next(struct udevice *dev,
> > > > > > > + struct optee_msg_arg *arg);
> > > > > > > +
> > > > > > > +void optee_suppl_cmd_rpmb_frames(struct udevice *dev,
> > > > > > > + struct optee_msg_arg *arg);
> > > > > > > +#else
> > > > > > > +static inline void optee_suppl_cmd_rpmb_probe_reset(struct
> > > > > > > udevice *dev,
> > > > > > > + struct
> > > > > > > optee_msg_arg *arg)
> > > > > > > +{
> > > > > > > + arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static inline void optee_suppl_cmd_rpmb_probe_next(struct
> > > > > > > udevice *dev,
> > > > > > > + struct optee_msg_arg
> > > > > > > *arg)
> > > > > > > +{
> > > > > > > + arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static inline void optee_suppl_cmd_rpmb_frames(struct udevice
> > > > > > > *dev,
> > > > > > > + struct optee_msg_arg
> > > > > > > *arg)
> > > > > > > +{
> > > > > > > + arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
> > > > > > > +}
> > > > > > > +#endif
> > > > > > > +
> > > > > > > #ifdef CONFIG_DM_I2C
> > > > > > > /**
> > > > > > > * optee_suppl_cmd_i2c_transfer() - route I2C requests to an
> > > > > > > I2C chip
> > > > > > > diff --git a/drivers/tee/optee/rpmb.c b/drivers/tee/optee/rpmb.c
> > > > > > > new file mode 100644
> > > > > > > index 00000000000..9c3e0f031ad
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/tee/optee/rpmb.c
> > > > > > > @@ -0,0 +1,124 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > > +
> > > > > > > +#include <dm.h>
> > > > > > > +#include <tee.h>
> > > > > > > +#include <ufs.h>
> > > > > > > +
> > > > > > > +#include "optee_msg.h"
> > > > > > > +#include "optee_msg_supplicant.h"
> > > > > > > +#include "optee_private.h"
> > > > > > > +
> > > > > > > +#define UFS_RPMB_CONTROLLER 0
> > > > > >
> > > > > > Can this be a config option instead which defaults to 0 ?
> > > > >
> > > > > um, I dont think so - this assumes there is only one controller. If
> > > > > there
> > > > > are more than one, we would need a mechanism to select which one from
> > > > > the enumerated values (maybe a DT alias (ie ufs-rpmb ?) I think the
> > > > > binding order
> > > > > changes as we enable/disable devices)
> > > >
> > > > It could collide with other SCSI devices like SATA devices over PCIe.
> > > >
> > > > Usually we try not to hardcode the devices ids.
> > >
> > >
> > > right...shall I go ahead with a DT alias approach?
> >
> >
> > something like this?
> >
> >
> > 8 ⋮ 8 │ #include "optee_msg_supplicant.h"
> > 9 ⋮ 9 │ #include "optee_private.h"
> > 10 ⋮ 10 │
> > 11 ⋮ │-#define UFS_RPMB_CONTROLLER 0
> > ⋮ 11 │+static int optee_rpmb_get_scsi_dev(struct udevice **scsi_devp)
> > ⋮ 12 │+{
> > ⋮ 13 │+ struct udevice *ufs_dev, *scsi_dev;
> > ⋮ 14 │+ ofnode node;
> > ⋮ 15 │+ int ret;
> > ⋮ 16 │+
> > ⋮ 17 │+ node = ofnode_get_aliases_node("ufs-rpmb");
> > ⋮ 18 │+ if (ofnode_valid(node))
> > ⋮ 19 │+ ret = uclass_get_device_by_ofnode(UCLASS_UFS,
> > node, &ufs_dev);
> > ⋮ 20 │+ else
> > ⋮ 21 │+ ret = uclass_get_device(UCLASS_UFS, 0,
> > &ufs_dev);
> > ⋮ 22 │+ if (ret)
> > ⋮ 23 │+ return ret;
> > ⋮ 24 │+
> > ⋮ 25 │+ ret = device_get_child(ufs_dev, 0, &scsi_dev);
> > ⋮ 26 │+ if (ret)
> > ⋮ 27 │+ return ret;
> > ⋮ 28 │+
> > ⋮ 29 │+ *scsi_devp = scsi_dev;
> > ⋮ 30 │+
> > ⋮ 31 │+ return 0;
> > ⋮ 32 │+}
> > 12 ⋮ 33 │
> > 13 ⋮ 34 │ void optee_suppl_cmd_rpmb_probe_reset(struct udevice *dev,
> > 14 ⋮ 35 │ struct optee_msg_arg *arg)
> >
> > ────────────────────────────────────────────────────────────────┐
> > • 76: void optee_suppl_cmd_rpmb_probe_next(struct udevice *dev, │
> > ────────────────────────────────────────────────────────────────┘
> > 55 ⋮ 76 │ return;
> > 56 ⋮ 77 │ }
> > 57 ⋮ 78 │
> > 58 ⋮ │- if (uclass_get_device(UCLASS_SCSI, UFS_RPMB_CONTROLLER,
> > &scsi_dev)) {
> > ⋮ 79 │+ if (optee_rpmb_get_scsi_dev(&scsi_dev)) {
> > 59 ⋮ 80 │ arg->ret = TEE_ERROR_ITEM_NOT_FOUND;
> > 60 ⋮ 81 │ return;
> > 61 ⋮ 82 │ }
> >
> > ─────────────────────────────────────────────────────────────┐
> > • 124: void optee_suppl_cmd_rpmb_frames(struct udevice *dev, │
> > ─────────────────────────────────────────────────────────────┘
> > 103⋮ 124│ return;
> > 104⋮ 125│ }
> > 105⋮ 126│
> > 106⋮ │- if (uclass_get_device(UCLASS_SCSI, UFS_RPMB_CONTROLLER,
> > &scsi_dev)) {
> > ⋮ 127│+ if (optee_rpmb_get_scsi_dev(&scsi_dev)) {
> > 107⋮ 128│ arg->ret = TEE_ERROR_ITEM_NOT_FOUND;
> > 108⋮ 129│ return;
> > 109⋮ 130│ }
> >
>
> No I was just thinking as a Kconfig option along CONFIG_SUPPORT_UFS_RPMB but
> I just didn't find where you added the CONFIG_SUPPORT_UFS_RPMB option...
>
> Neil
well ok, sure I can do the Kconfig change.
I havent enabled RPMB just yet in configs/qcom_lemans_defconfig until
the OP-TEE support is merged.
This is what I have on my development branch:
# Route OP-TEE RPMB secure storage over the UFS RPMB (no eMMC on this SoC).
CONFIG_SUPPORT_UFS_RPMB=y
