2022-04-06 11:22:40

by Bean Huo

[permalink] [raw]
Subject: Re: [PATCH v2 0/4] rpmb subsystem, uapi and virtio-rpmb driver

On Tue, 2022-04-05 at 16:43 +0100, Alex Bennée wrote:
>
> Bean Huo <[email protected]> writes:
>
> > Hi Alex,
> >
> > Thanks for this unified RPMB interface, I wanted to verify this on
> > our
> > UFS, it seems you didn't add the UFS access interface in this
> > version
> > from your userspace tools, right?
>
> No I didn't but it should be easy enough to add some function pointer
> redirection everywhere one of the op_* functions calls a vrpmb_*
> function. Do you already have a UFS RPMB device driver?
>

Hi Alex,
Thanks for your feedback.

We now access UFS RPMB through the RPMB LUN BSG device, RPMB is a well-
known LU and we have a userspace tool to access it.

I see that if we're going to use your interface, "static struct
rpmb_ops" should be registered from a lower-level driver, for example
in a UFS driver, yes there should be no problem with this registration,
but I don't know with the current way Compared, what are the advantages
to add a driver. maybe the main advantage is that we will have an
unified user space tool for RPMB. right?

Kind regards,
Bean


2022-04-06 15:40:05

by Alex Bennée

[permalink] [raw]
Subject: Re: [PATCH v2 0/4] rpmb subsystem, uapi and virtio-rpmb driver


Bean Huo <[email protected]> writes:

> On Tue, 2022-04-05 at 16:43 +0100, Alex Bennée wrote:
>>
>> Bean Huo <[email protected]> writes:
>>
>> > Hi Alex,
>> >
>> > Thanks for this unified RPMB interface, I wanted to verify this on
>> > our
>> > UFS, it seems you didn't add the UFS access interface in this
>> > version
>> > from your userspace tools, right?
>>
>> No I didn't but it should be easy enough to add some function pointer
>> redirection everywhere one of the op_* functions calls a vrpmb_*
>> function. Do you already have a UFS RPMB device driver?
>>
>
> Hi Alex,
> Thanks for your feedback.
>
> We now access UFS RPMB through the RPMB LUN BSG device, RPMB is a well-
> known LU and we have a userspace tool to access it.
>
> I see that if we're going to use your interface, "static struct
> rpmb_ops" should be registered from a lower-level driver, for example
> in a UFS driver, yes there should be no problem with this registration,
> but I don't know with the current way Compared, what are the advantages
> to add a driver. maybe the main advantage is that we will have an
> unified user space tool for RPMB. right?

Pretty much. The main issue for virtio-rpmb is it doesn't really fit
neatly into the block stack because all it does is the RPMB part so a
non-block orientate API makes sense.

Can you point be to where the UFS driver does it's current RPMB stuff?

>
> Kind regards,
> Bean


--
Alex Bennée

2022-04-06 20:12:35

by Bean Huo

[permalink] [raw]
Subject: Re: [PATCH v2 0/4] rpmb subsystem, uapi and virtio-rpmb driver

On Wed, 2022-04-06 at 12:22 +0100, Alex Bennée wrote:
>
> Bean Huo <[email protected]> writes:
>
> > On Tue, 2022-04-05 at 16:43 +0100, Alex Bennée wrote:
> > >
> > > Bean Huo <[email protected]> writes:
> > >
> > > > Hi Alex,
> > > >
> > > > Thanks for this unified RPMB interface, I wanted to verify this
> > > > on
> > > > our
> > > > UFS, it seems you didn't add the UFS access interface in this
> > > > version
> > > > from your userspace tools, right?
> > >
> > > No I didn't but it should be easy enough to add some function
> > > pointer
> > > redirection everywhere one of the op_* functions calls a vrpmb_*
> > > function. Do you already have a UFS RPMB device driver?
> > >
> >
> > Hi Alex,
> > Thanks for your feedback.
> >
> > We now access UFS RPMB through the RPMB LUN BSG device, RPMB is a
> > well-
> > known LU and we have a userspace tool to access it.
> >
> > I see that if we're going to use your interface, "static struct
> > rpmb_ops" should be registered from a lower-level driver, for
> > example
> > in a UFS driver, yes there should be no problem with this
> > registration,
> > but I don't know with the current way Compared, what are the
> > advantages
> > to add a driver. maybe the main advantage is that we will have an
> > unified user space tool for RPMB. right?
>
> Pretty much. The main issue for virtio-rpmb is it doesn't really fit
> neatly into the block stack because all it does is the RPMB part so a
> non-block orientate API makes sense.
>
> Can you point be to where the UFS driver does it's current RPMB
> stuff?
>

It's the SCSI BSG driver, in fact, we don't have a dedicated UFS RPMB
driver in the kernel. RPMB is a well known LU, we are using userspace
tools to issue SCSI commands directly to the UFS RPMB LU via ioctl()
from the BSG device node in the /dev/sg/ folder.

Here is the BSG part of the code in the userspace tools:

io_hdr_v4.guard = 'Q';
io_hdr_v4.protocol = BSG_PROTOCOL_SCSI;
io_hdr_v4.subprotocol = BSG_SUB_PROTOCOL_SCSI_CMD;
io_hdr_v4.response = (__u64)sense_buffer;
io_hdr_v4.max_response_len = SENSE_BUFF_LEN;
io_hdr_v4.request_len = cmd_len;
io_hdr_v4.request = (__u64)cdb;


ioctl(fd, SG_IO, &io_hdr_v4))
...


> >
> > Kind regards,
> > Bean
>
>

2022-04-06 21:34:12

by Bean Huo

[permalink] [raw]
Subject: Re: [PATCH v2 0/4] rpmb subsystem, uapi and virtio-rpmb driver


>
> Can you point be to where the UFS driver does it's current RPMB
> stuff?
>

If you want to understand the UFS RPMB functionality/command sequence,
you can refer to the ufs-utils tool, it is much like mmc-utils.

https://github.com/westerndigitalcorporation/ufs-utils/blob/dev/ufs_rpmb.c

> >
> > Kind regards,
> > Bean
>
>

2022-04-06 21:50:26

by Bart Van Assche

[permalink] [raw]
Subject: Re: [PATCH v2 0/4] rpmb subsystem, uapi and virtio-rpmb driver

On 4/6/22 10:19, Bean Huo wrote:
> On Wed, 2022-04-06 at 12:22 +0100, Alex Bennée wrote:
>>
>> Bean Huo <[email protected]> writes:
>>
>>> On Tue, 2022-04-05 at 16:43 +0100, Alex Bennée wrote:
>>>>
>>>> Bean Huo <[email protected]> writes:
>>>>
>>>>> Hi Alex,
>>>>>
>>>>> Thanks for this unified RPMB interface, I wanted to verify this
>>>>> on
>>>>> our
>>>>> UFS, it seems you didn't add the UFS access interface in this
>>>>> version
>>>>> from your userspace tools, right?
>>>>
>>>> No I didn't but it should be easy enough to add some function
>>>> pointer
>>>> redirection everywhere one of the op_* functions calls a vrpmb_*
>>>> function. Do you already have a UFS RPMB device driver?
>>>>
>>>
>>> Hi Alex,
>>> Thanks for your feedback.
>>>
>>> We now access UFS RPMB through the RPMB LUN BSG device, RPMB is a
>>> well-
>>> known LU and we have a userspace tool to access it.
>>>
>>> I see that if we're going to use your interface, "static struct
>>> rpmb_ops" should be registered from a lower-level driver, for
>>> example
>>> in a UFS driver, yes there should be no problem with this
>>> registration,
>>> but I don't know with the current way Compared, what are the
>>> advantages
>>> to add a driver. maybe the main advantage is that we will have an
>>> unified user space tool for RPMB. right?
>>
>> Pretty much. The main issue for virtio-rpmb is it doesn't really fit
>> neatly into the block stack because all it does is the RPMB part so a
>> non-block orientate API makes sense.
>>
>> Can you point be to where the UFS driver does it's current RPMB
>> stuff?
>>
>
> It's the SCSI BSG driver, in fact, we don't have a dedicated UFS RPMB
> driver in the kernel. RPMB is a well known LU, we are using userspace
> tools to issue SCSI commands directly to the UFS RPMB LU via ioctl()
> from the BSG device node in the /dev/sg/ folder.
>
> Here is the BSG part of the code in the userspace tools:
>
> io_hdr_v4.guard = 'Q';
> io_hdr_v4.protocol = BSG_PROTOCOL_SCSI;
> io_hdr_v4.subprotocol = BSG_SUB_PROTOCOL_SCSI_CMD;
> io_hdr_v4.response = (__u64)sense_buffer;
> io_hdr_v4.max_response_len = SENSE_BUFF_LEN;
> io_hdr_v4.request_len = cmd_len;
> io_hdr_v4.request = (__u64)cdb;
>
>
> ioctl(fd, SG_IO, &io_hdr_v4))

Hi Bean,

I'm not sure where the above comes from? The Android RPMB client uses
slightly different code. Additionally, the retry loop around the
submission of SG/IO commands is very important. See also the
check_sg_io_hdr() call in send_ufs_rpmb_req(). See also
https://cs.android.com/android/platform/superproject/+/master:system/core/trusty/storage/proxy/rpmb.c

Thanks,

Bart.