Re: [RFC PATCH v4 4/4] scsi: ufs-qcom: add Inline Crypto Engine support

[Thara Gopinath <thara.gopinath@linaro.org>]

On 5/29/20 1:13 PM, Eric Biggers wrote:
> On Fri, May 29, 2020 at 11:54:18AM -0400, Thara Gopinath wrote:
>> On 5/7/20 2:08 PM, Eric Biggers wrote:
>>> On Thu, May 07, 2020 at 11:04:35AM -0700, Eric Biggers wrote:
>>>> Hi Thara,
>>>>
>>>> On Thu, May 07, 2020 at 08:36:58AM -0400, Thara Gopinath wrote:
>>>>>
>>>>>
>>>>> On 5/1/20 12:51 AM, Eric Biggers wrote:
>>>>>> From: Eric Biggers <ebiggers@google.com>
>>>>>>
>>>>>> Add support for Qualcomm Inline Crypto Engine (ICE) to ufs-qcom.
>>>>>>
>>>>>> The standards-compliant parts, such as querying the crypto capabilities
>>>>>> and enabling crypto for individual UFS requests, are already handled by
>>>>>> ufshcd-crypto.c, which itself is wired into the blk-crypto framework.
>>>>>> However, ICE requires vendor-specific init, enable, and resume logic,
>>>>>> and it requires that keys be programmed and evicted by vendor-specific
>>>>>> SMC calls.  Make the ufs-qcom driver handle these details.
>>>>>>
>>>>>> I tested this on Dragonboard 845c, which is a publicly available
>>>>>> development board that uses the Snapdragon 845 SoC and runs the upstream
>>>>>> Linux kernel.  This is the same SoC used in the Pixel 3 and Pixel 3 XL
>>>>>> phones.  This testing included (among other things) verifying that the
>>>>>> expected ciphertext was produced, both manually using ext4 encryption
>>>>>> and automatically using a block layer self-test I've written.
>>>>> Hello Eric,
>>>>>
>>>>> I am interested in testing out this series on 845, 855 and if possile on 865
>>>>> platforms. Can you give me some more details about your testing please.
>>>>>
>>>>
>>>> Great!  You can test this with fscrypt, a.k.a. ext4 or f2fs encryption.
>>>>
>>>> A basic manual test would be:
>>>>
>>>> 1. Build a kernel with:
>>>>
>>>> 	CONFIG_BLK_INLINE_ENCRYPTION=y
>>>> 	CONFIG_FS_ENCRYPTION=y
>>>> 	CONFIG_FS_ENCRYPTION_INLINE_CRYPT=y
>>>
>>> Sorry, I forgot: 'CONFIG_SCSI_UFS_CRYPTO=y' is needed too.
>>
>> Hi Eric,
>>
>> I tested this manually on db845c, sm8150-mtp and sm8250-mtp.(I added the dts
>> file entries for 8150 and 8250).
>>
>> I also ran OsBench test case createfiles[1] on the above platforms.
>> Following are the results on a non encrypted and encrypted directory on the
>> same file system(lower the number better)
>>
>> 			8250-MTP	8150-MTP	DB845
>>
>> nonencrypt_dir(us) 	55.3108954	26.8323124    69.5709552
>> encrypt_dir(us) 	70.0214426	37.5411254    92.3818296
>>
>>
>>
>> 1. https://github.com/mbitsnbites/osbench/blob/master/README.md
>>
> 
> Great, thanks for testing.
> 
> Note that the benchmark you ran (creating many small files, then deleting them)
> mostly tests the performance of filenames encryption and directory operations,
> not file contents encryption.  Inline encryption is only used for file contents.
> 
> In fact, since that benchmark doesn't sync the files before deleting them, there
> is no guarantee that any file contents are actually written to disk, and hence
> no guarantee that inline encryption got used at all.

Hi Eric,

The results are particularly interesting if you think a sync is not 
happening. There should not be any performance regression in this case 
between the two directories. I can try some reading/writing performance 
tests rather than creating files testing.

> 
> It would be more relevant to test the performance of reading/writing file data.
> 
> Also, did you try doing any correctness tests?  (See what I suggested earlier.)

I did correctness test as part of manual tests by diffing the content of 
the copied files and verifying them. I did not run any other tests you 
mentioned. Feel free to add my Tested-by in the next version you send out.

> 
> - Eric
> 

-- 
Warm Regards
Thara