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?
