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

[Bart Van Assche]

On 4/6/22 10:19, Bean Huo wrote:
> On Wed, 2022-04-06 at 12:22 +0100, Alex Bennée wrote:
> > Bean Huo <huobean@xxxxxxxxx> writes:
> >
> > > On Tue, 2022-04-05 at 16:43 +0100, Alex Bennée wrote:
> > > > Bean Huo <huobean@xxxxxxxxx> 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.