Re: [PATCH] mmc: rpmb: do not force a retune before RPMB switch

[Adrian Hunter <adrian.hunter@intel.com>]

On 2/01/24 12:41, Jorge Ramirez-Ortiz, Foundries wrote:
> On 11/12/23 09:00:06, Jorge Ramirez-Ortiz, Foundries wrote:
>> On 06/12/23 11:00:47, Jorge Ramirez-Ortiz, Foundries wrote:
>>> On 06/12/23 07:02:43, Avri Altman wrote:
>>>>>
>>>>> On 4/12/23 17:01, Jorge Ramirez-Ortiz wrote:
>>>>>> Requesting a retune before switching to the RPMB partition has been
>>>>>> observed to cause CRC errors on the RPMB reads (-EILSEQ).
>>>>>
>>>>> There are still 2 concerns:
>>>>> 1) We don't really know the root cause.  Have you determined if here are
>>>>> CRC errors in the main partition also?
>>>
>>> right, and I don't disagree with that.
>>>
>>> As a test I created a 4GB file from /dev/random which I then copied
>>> several times (dd if= ....)
>>>
>>> root@uz3cg-dwg-sec:/sys/kernel/debug/mmc0# cat err_stats
>>> # Command Timeout Occurred:      0
>>> # Command CRC Errors Occurred:   0
>>> # Data Timeout Occurred:         0
>>> # Data CRC Errors Occurred:      0
>>> # Auto-Cmd Error Occurred:       0
>>> # ADMA Error Occurred:   0
>>> # Tuning Error Occurred:         0
>>> # CMDQ RED Errors:       0
>>> # CMDQ GCE Errors:       0
>>> # CMDQ ICCE Errors:      0
>>> # Request Timedout:      0
>>> # CMDQ Request Timedout:         0
>>> # ICE Config Errors:     0
>>> # Controller Timedout errors:    0
>>> # Unexpected IRQ errors:         0
>>>
>>> However as soon as I access RPMB and fails (it takes just a few tries) I see:
>>>
>>> I/TC: RPMB: Using generated key
>>> [   86.902118] sdhci-arasan ff160000.mmc: __mmc_blk_ioctl_cmd: data error -84
>>> E/TC:? 0
>>> E/TC:? 0 TA panicked with code 0xffff0000
>>> E/LD:  Status of TA 22250a54-0bf1-48fe-8002-7b20f1c9c9b1
>>> E/LD:   arch: aarch64
>>> E/LD:  region  0: va 0xc0004000 pa 0x7e200000 size 0x002000 flags rw-s (ldelf)
>>> E/LD:  region  1: va 0xc0006000 pa 0x7e202000 size 0x008000 flags r-xs (ldelf)
>>> E/LD:  region  2: va 0xc000e000 pa 0x7e20a000 size 0x001000 flags rw-s (ldelf)
>>> E/LD:  region  3: va 0xc000f000 pa 0x7e20b000 size 0x004000 flags rw-s (ldelf)
>>> E/LD:  region  4: va 0xc0013000 pa 0x7e20f000 size 0x001000 flags r--s
>>> E/LD:  region  5: va 0xc0014000 pa 0x7e22c000 size 0x005000 flags rw-s (stack)
>>> E/LD:  region  6: va 0xc0019000 pa 0x818ea9ba8 size 0x002000 flags rw-- (param)
>>> E/LD:  region  7: va 0xc001b000 pa 0x818e97ba8 size 0x001000 flags rw-- (param)
>>> E/LD:  region  8: va 0xc004f000 pa 0x00001000 size 0x014000 flags r-xs [0]
>>> E/LD:  region  9: va 0xc0063000 pa 0x00015000 size 0x008000 flags rw-s [0]
>>> E/LD:   [0] 22250a54-0bf1-48fe-8002-7b20f1c9c9b1 @ 0xc004f000
>>> E/LD:  Call stack:
>>> E/LD:   0xc0051a14
>>> E/LD:   0xc004f31c
>>> E/LD:   0xc0052d40
>>> E/LD:   0xc004f624
>>>
>>> root@uz3cg-dwg-sec:/var/rootdirs/home/fio# cat /sys/kernel/debug/mmc0/err_stats
>>> # Command Timeout Occurred:      0
>>> # Command CRC Errors Occurred:   0
>>> # Data Timeout Occurred:         0
>>> # Data CRC Errors Occurred:      1
>>> # Auto-Cmd Error Occurred:       0
>>> # ADMA Error Occurred:   0
>>> # Tuning Error Occurred:         0
>>> # CMDQ RED Errors:       0
>>> # CMDQ GCE Errors:       0
>>> # CMDQ ICCE Errors:      0
>>> # Request Timedout:      0
>>> # CMDQ Request Timedout:         0
>>> # ICE Config Errors:     0
>>> # Controller Timedout errors:    0
>>> # Unexpected IRQ errors:         0
>>>
>>>>> 2) Forcing this on everyone
>>>>>
>>>>> The original idea was that because re-tuning cannot be done in RPMB, the
>>>>> need to re-rune in RPMB could be avoided by always re-tuning before
>>>>> switching to RPMB and then switching straight back. IIRC re-tuning should
>>>>> guarantee at least 4MB more I/O without issue.
>>>> Performance is hardly an issue in the context of RPMB access -
>>>> For most cases it’s a single frame.
>>>
>>> Yes, the security use case typically stores hashes, variables
>>> (bootcount, upgrade_available, versions, that sort of thing) and
>>> certificates in RPMB.
>>>
>>> Since you mentioned, I am seeing that tuning before switching to RPMB
>>> has an impact on performance. As a practical test, just reading a 6 byte
>>> variable incurs in 50ms penalty in kernel space due to the need to
>>> retune 5 times. Not great since the request is coming from a Trusted
>>> Application via OP-TEE through the supplicant meaning this TEE thread
>>> (they are statically allocated CFG_NUM_THREADS) will be reserved for
>>> quite a bit of time.
>>>
>>> Roughly:
>>> TA --> OP-TEE (core) --> TEE-supplicant --> Kernel (>50ms) --> OP-TEE --> TA
>>
>> To add more detail to the timing above, when using RPMB, OP-TEE stores
>> the secure filesystem on RPMB as well, so accessing one of the variables
>> stored in the filesystem consists on a number (~5) of individual RPMB
>> requests (each one forcing a retune, each retune taking around 10ms).
> 
> Adrian, please could you comment on the above.
> 
> The current code is a performance drag for systems that implement their
> secure filesystems on RPMB (i.e: OP-TEE) causing each read operation (of
> variables consisting of a few bytes stored in such a filesystem) to
> perform 5 consecutive retune requests.
> 
> I am just thinking whether the original use case that forces a call to
> retune prior to processing the RPMB request remains valid.

I am not sure what you are asking.

For some transfer modes, re-tuning is expected to deal with sampling
point drift over time, mainly due to temperature changes.  It is done
either periodically (tuning timer) or after a CRC error.

There is no reason to assume RPMB is immune from that.

Certainly re-tuning before switching to RPMB is not optimal for
performance, and we can leave that out, but a CRC error before
or during RPMB operations will *still* result in re-tuning
after switching back from RPMB.

In your case, re-tuning makes things worse, which is a bit of a
mystery.  Running the new re-tuning test would tell us whether
it makes things worse in general, or only for RPMB.

> 
> Independently of the fact that not doing so fixes the problem I was
> working on - and with the information I have - I dont think RPMB is
> generally used to store larger files (maybe you have more information
> about the average use case? are you aware of systems using RPMB to store
> binaries or images?)
> 
> I still I have to execute the test you shared some weeks ago. Bit of a
> pain to NFS boot this system...will try to do it this week.

Other options are to boot with an initrd only, or after boot switch
to a RAM-based file system.

I was waiting for this, since it is good to try to get closer to a
root cause, but as you point out, the change is good for performance
also, so I will Ack it.


> 
> TIA
> 
>>
>> BTW, I also tried delaying the timing between those consecutive retunes
>> (up to 1 second), but the issue still persisted.
>>
>>>
>>> Adrian, I couldn't find the original performance justification for
>>> enabling this feature globally. At which point do you think it becomes
>>> beneficial to retune before accessing RPMB?
>>
>> How should we proceed with this patch then? can it be merged as I
>> proposed? should I rewrite it differently? not sure what is next
>>
>> TIA
>> Jorge