RE: [RFC PATCH v1 2/2] ufs: core: Add advanced RPMB support in ufs_bsg

[Avri Altman <Avri.Altman@wdc.com>]

> Avri,
> thanks for your comments and review.
> 
> 
> On Tue, 2022-11-08 at 19:09 +0000, Avri Altman wrote:
> > > Add advanced RPMB support in ufs_bsg. For these reasons, we try to
> > > implement Advanced RPMB in ufs_bsg:
> > > 1. According to the UFS specification, only one RPMB operation can
> > > be performed at any time. We can ensure this by using reserved slot
> > > and its dev_cmd sync operation protection mechanism.
> >
> > Regardless of its technical convenience, this approach unfortunately
> > breaks the spec.
> >
> > The spec say (please note the line numbers):
> >
> > ".....
> >
> > 5197 12.4.5.1 Advanced RPMB Message
> >
> > 5198 An Advanced RPMB Message is composed of an Advanced RPMB
> Meta
> > Information and a MAC/KEY in
> >
> > 5199 the EHS field in *COMMAND UPIU* and *RESPONSE UPIU*. Advanced
> > RPMB Data is delivered through
> >
> > ....."
> 
> > Moreover, in the examples that are provided, it is still expected to
> > be carried via SECURITY PROTOCOL IN and SECURITY PROTOCOL OUT,
> >
> > See e.g. Figure 12.15 — Authenticated Data Write Flow (in Advanced
> > RPMB Mode).
> >
> >
> not quite get what you meant here.
I meant that we should still build upius that contains security protocol commands.
 
> >
> 
> >
> > Therefore, wrapping the rpmb packets in a query-request upiu and
> > query-response upiu is not allowed.
> >
> >
> 
> no, I didn't wrap RPMB packet in query-request/response, it is inupiu_req
> and upiu_rsp, it is upiu command.
I was thinking you are using a query request upiu because you are using device management commands flow,
And specifically, I didn't see where you are setting ucd_req_ptr->sc.cdb,
Which should hold the security protocol cdb.

> Based on Bart's suggestion, we shouldn't
> change the current ufs_bsg structure. I think his concern is that if we change
> ufs_bsg structure, the user space tool also needs to change as well.
Adding command upiu support will not break any ufs-bsg tools, e.g. ufs-utils.
It would just add this capability.

Thanks,
Avri

> 
> >
> > Still, I agree that the approach you suggested, namely to rely on the
> > ufs-bsg driver, is the cleanest way to handle the advance rpmb access.
> >
> > However, IMHO, you need to do it is by adding command UPIU to the
> > ufs-bsg driver.
> >
> >
> 
> Yes, agree with you on this point. But we still need to use reserved slots for
> RPMB or command UPIU, we don't want to affect IO requests on the normal
> path.
> 
> One problem is that we didn't split the dev_manage command and the
> RPMB command in their completion handlers. I would like to change
> dev_man to passthrough or something else, and then split dev_man and
> RPMB, otherwise, they would be mixed in one dev_man completion handler.
> No technical issues here, just want to make it more readable and
> maintainable.
> 
> 
> 
> Kind regards,
> Bean
> 
> >
> > Thanks,
> >
> > Avri