linux-kernel - Re: [PATCH v2 10/16] crypto: qce - Add support for lock aquire,lock release api.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9a40cce6-03cb-eea3-7621-7a89d0478a07@quicinc.com>
Date: Wed, 21 Aug 2024 10:44:02 +0530
From: Md Sadre Alam <quic_mdalam@...cinc.com>
To: Bjorn Andersson <andersson@...nel.org>
CC: <vkoul@...nel.org>, <robh@...nel.org>, <krzk+dt@...nel.org>,
        <conor+dt@...nel.org>, <konradybcio@...nel.org>,
        <thara.gopinath@...il.com>, <herbert@...dor.apana.org.au>,
        <davem@...emloft.net>, <gustavoars@...nel.org>,
        <u.kleine-koenig@...gutronix.de>, <kees@...nel.org>,
        <agross@...nel.org>, <linux-arm-msm@...r.kernel.org>,
        <dmaengine@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-crypto@...r.kernel.org>,
        <quic_srichara@...cinc.com>, <quic_varada@...cinc.com>,
        <quic_utiwari@...cinc.com>
Subject: Re: [PATCH v2 10/16] crypto: qce - Add support for lock aquire,lock
 release api.



On 8/16/2024 10:08 PM, Bjorn Andersson wrote:
> On Thu, Aug 15, 2024 at 02:27:19PM GMT, Md Sadre Alam wrote:
>> Add support for lock acquire and lock release api.
>> When multiple EE's(Execution Environment) want to access
>> CE5 then there will be race condition b/w multiple EE's.
>>
>> Since each EE's having their dedicated BAM pipe, BAM allows
>> Locking and Unlocking on BAM pipe. So if one EE's requesting
>> for CE5 access then that EE's first has to LOCK the BAM pipe
>> while setting LOCK bit on command descriptor and then access
>> it. After finishing the request EE's has to UNLOCK the BAM pipe
>> so in this way we race condition will not happen.
> 
> Does the lock/unlock need to happen on a dummy access before and after
> the actual sequence? Is it not sufficient to lock/unlock on the first
> and last operation?
   The locking/unlocking has to happen on command descriptor only, If we
   need locking/unlocking on data descriptor then we have to use dummy
   command descriptor only as per Hardware Programming Guide.

   Hardware Programming Guide state as below:
   Pipe Locking and Unlocking should appear ONLY in Command-Descriptor.
   In case a Lock is required on a Data Descriptor this can be implemented
   by a dummy Command-Descriptor with Lock/Unlock bit asserted preceding/
   following the Data Descriptor.
> 
> Please squash this with the previous commit, if kept as explicit
> operations, please squash it with the previous patch that introduces the
> flags.
   Ok
> 
>>
>> Added these two API qce_bam_acquire_lock() and qce_bam_release_lock()
>> for the same.
>>
>> Signed-off-by: Md Sadre Alam <quic_mdalam@...cinc.com>
>> ---
>>
>> Change in [v2]
>>
>> * No chnage
>>
>> Change in [v1]
>>
>> * Added initial support for lock_acquire and lock_release
>>    api.
>>
>>   drivers/crypto/qce/common.c | 36 ++++++++++++++++++++++++++++++++++++
>>   drivers/crypto/qce/core.h   |  2 ++
>>   2 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c
>> index ff96f6ba1fc5..a8eaffe41101 100644
>> --- a/drivers/crypto/qce/common.c
>> +++ b/drivers/crypto/qce/common.c
>> @@ -617,3 +617,39 @@ void qce_get_version(struct qce_device *qce, u32 *major, u32 *minor, u32 *step)
>>   	*minor = (val & CORE_MINOR_REV_MASK) >> CORE_MINOR_REV_SHIFT;
>>   	*step = (val & CORE_STEP_REV_MASK) >> CORE_STEP_REV_SHIFT;
>>   }
>> +
>> +int qce_bam_acquire_lock(struct qce_device *qce)
>> +{
>> +	int ret;
>> +
>> +	qce_clear_bam_transaction(qce);
> 
> It's not entirely obvious that a "lock" operation will invalidate any
> pending operations.
   qce_clear_bam_transaction() api is not going to invalidate any pending
   thransaction. This is just an internal api which will set bam_transaction
   structure to 0 before starting new bam transaction.
> 
>> +
>> +	/* This is just a dummy write to acquire lock on bam pipe */
>> +	qce_write_reg_dma(qce, REG_AUTH_SEG_CFG, 0, 1);
>> +
>> +	ret = qce_submit_cmd_desc(qce, QCE_DMA_DESC_FLAG_LOCK);
>> +	if (ret) {
>> +		dev_err(qce->dev, "Error in Locking cmd descriptor\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int qce_bam_release_lock(struct qce_device *qce)
> 
> What would be a reasonable response from the caller if this release
> operation returns a failure? How do you expect it to recover?
   If unlocking bam pipe failed means its a bam failure and we can abort
   the current transaction.
> 
>> +{
>> +	int ret;
>> +
>> +	qce_clear_bam_transaction(qce);
>> +
> 
> In particularly not on "unlock".
   qce_clear_bam_transaction() this is just to initialize with 0
   for bam transaction structure before any new transaction start.
> 
> Regards,
> Bjorn
> 
>> +	/* This just dummy write to release lock on bam pipe*/
>> +	qce_write_reg_dma(qce, REG_AUTH_SEG_CFG, 0, 1);
>> +
>> +	ret = qce_submit_cmd_desc(qce, QCE_DMA_DESC_FLAG_UNLOCK);
>> +	if (ret) {
>> +		dev_err(qce->dev, "Error in Un-Locking cmd descriptor\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/crypto/qce/core.h b/drivers/crypto/qce/core.h
>> index bf28dedd1509..d01d810b60ad 100644
>> --- a/drivers/crypto/qce/core.h
>> +++ b/drivers/crypto/qce/core.h
>> @@ -68,4 +68,6 @@ int qce_read_reg_dma(struct qce_device *qce, unsigned int offset, void *buff,
>>   void qce_clear_bam_transaction(struct qce_device *qce);
>>   int qce_submit_cmd_desc(struct qce_device *qce, unsigned long flags);
>>   struct qce_bam_transaction *qce_alloc_bam_txn(struct qce_dma_data *dma);
>> +int qce_bam_acquire_lock(struct qce_device *qce);
>> +int qce_bam_release_lock(struct qce_device *qce);
>>   #endif /* _CORE_H_ */
>> -- 
>> 2.34.1
>>