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

From: Bean Huo <beanhuo@iokpp.de>
To: Avri Altman <Avri.Altman@wdc.com>,
	"alim.akhtar@samsung.com" <alim.akhtar@samsung.com>,
	"jejb@linux.ibm.com" <jejb@linux.ibm.com>,
	"martin.petersen@oracle.com" <martin.petersen@oracle.com>,
	"stanley.chu@mediatek.com" <stanley.chu@mediatek.com>,
	"beanhuo@micron.com" <beanhuo@micron.com>,
	"bvanassche@acm.org" <bvanassche@acm.org>,
	"tomas.winkler@intel.com" <tomas.winkler@intel.com>,
	"daejun7.park@samsung.com" <daejun7.park@samsung.com>,
	"quic_cang@quicinc.com" <quic_cang@quicinc.com>,
	"quic_nguyenb@quicinc.com" <quic_nguyenb@quicinc.com>,
	"quic_xiaosenh@quicinc.com" <quic_xiaosenh@quicinc.com>,
	"quic_richardp@quicinc.com" <quic_richardp@quicinc.com>,
	"quic_asutoshd@quicinc.com" <quic_asutoshd@quicinc.com>,
	"hare@suse.de" <hare@suse.de>
Cc: "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH v1 2/2] ufs: core: Add advanced RPMB support in ufs_bsg
Date: Tue, 08 Nov 2022 20:28:29 +0100	[thread overview]
Message-ID: <fa3905bff8ba47b0ed45f9221dd1d033ca553179.camel@iokpp.de> (raw)
In-Reply-To: <DM6PR04MB657518129522996B2B5C9640FC3F9@DM6PR04MB6575.namprd04.prod.outlook.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.
> 

> 
> 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. 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. 

> 
> 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