Messages in this thread | | | Date | Tue, 22 Aug 2023 11:43:43 -0700 (PDT) | From | Shyam Saini <> | Subject | Re: [RFC, PATCH 1/1] rpmb: add Replay Protected Memory Block (RPMB) driver |
| |
Hi Bart,
sorry for not replying earlier, as I am very new to NVMe/UFS spec and was figuring out few details about them.
> On 7/21/23 18:40, Shyam Saini wrote: >> +config RPMB >> + tristate "RPMB partition interface" >> + help >> + Unified RPMB partition interface for RPMB capable devices such as >> + eMMC and UFS. Provides interface for in kernel security >> controllers to >> + access RPMB partition. >> + >> + If unsure, select N. > > Please also mention NVMe.
Sure, > Please change the word "partition" into "unit" to avoid confusion with the > concept "LBA range partition".
sure, in next iteration
>> +static DEFINE_IDA(rpmb_ida); > > How are accesses to this IDA serialized?
I will look into that.
>> +/** >> + * rpmb_get_capacity() - returns the capacity of the rpmb device >> + * @rdev: rpmb device >> + * >> + * Return: >> + * * capacity of the device in units of 128K, on success >> + * * -EINVAL on wrong parameters >> + * * -EOPNOTSUPP if device doesn't support the requested operation >> + * * < 0 if the operation fails >> + */ > > Why in units of 128 KiB?
I think UFS/eMMC RPMB spec suggests size of RPMB multiple of 128K and NVMe spec suggests RPMB Data Area to be multiple of 128K as well.
>> +/** >> + * rpmb_dev_find_by_device() - retrieve rpmb device from the parent >> device >> + * @parent: parent device of the rpmb device >> + * @target: RPMB target/region within the physical device >> + * >> + * Return: NULL if there is no rpmb device associated with the parent >> device >> + */ > > Can an NVMe controller have multiple RPMB units? From the NVMe specification: > "The controller may support multiple RPMB targets."
That we have to figure, I see NVMe device can have upto 7 RPMB targets/units
> Can rpmb_dev_find_by_device() be used if multiple RPMB units are associated > with a single controller?
That's not finalised yet, but we some ideas from Optee folks on the other replies.
>> +/** >> + * rpmb_dev_register - register RPMB partition with the RPMB subsystem >> + * @dev: storage device of the rpmb device >> + * @target: RPMB target/region within the physical device >> + * @ops: device specific operations >> + * >> + * Return: a pointer to rpmb device >> + */ >> +struct rpmb_dev *rpmb_dev_register(struct device *dev, u8 target, >> + const struct rpmb_ops *ops) >> +{ >> + struct rpmb_dev *rdev; >> + int id; >> + int ret; >> + >> + if (!dev || !ops) >> + return ERR_PTR(-EINVAL); >> + >> + if (!ops->program_key) >> + return ERR_PTR(-EINVAL); >> + >> + if (!ops->get_capacity) >> + return ERR_PTR(-EINVAL); >> + >> + if (!ops->get_write_counter) >> + return ERR_PTR(-EINVAL); >> + >> + if (!ops->write_blocks) >> + return ERR_PTR(-EINVAL); >> + >> + if (!ops->read_blocks) >> + return ERR_PTR(-EINVAL); >> + >> + rdev = kzalloc(sizeof(*rdev), GFP_KERNEL); >> + if (!rdev) >> + return ERR_PTR(-ENOMEM); >> + >> + id = ida_simple_get(&rpmb_ida, 0, 0, GFP_KERNEL); >> + if (id < 0) { >> + ret = id; >> + goto exit; >> + } >> + >> + mutex_init(&rdev->lock); >> + rdev->ops = ops; >> + rdev->id = id; >> + rdev->target = target; >> + >> + dev_set_name(&rdev->dev, "rpmb%d", id); >> + rdev->dev.class = &rpmb_class; >> + rdev->dev.parent = dev; >> + >> + rpmb_cdev_prepare(rdev); >> + >> + ret = device_register(&rdev->dev); >> + if (ret) >> + goto exit; >> + >> + rpmb_cdev_add(rdev); >> + >> + dev_dbg(&rdev->dev, "registered device\n"); >> + >> + return rdev; >> + >> +exit: >> + if (id >= 0) >> + ida_simple_remove(&rpmb_ida, id); >> + kfree(rdev); >> + return ERR_PTR(ret); >> +} > > How is user space software supposed to map an NVMe RPMB target ID to an RPMB > device name?
I am not sure, this driver aims to provide in kernel RPMB access APIs, user space support may be added later on, but i will look if the current RFC version has any implication on future user-space support.
>> +MODULE_AUTHOR("Intel Corporation"); > > Shouldn't this be the name of a person instead of the name of a company? >
Thanks, I will address that in next iteration.
Please keep posted your reviews and feedback.
Best Regards, Shyam
| |