Re: [PATCH v5 07/19] arm64: rsi: Add support for checking whether an MMIO is protected

[Gavin Shan <gshan@redhat.com>]

On 9/6/24 11:55 PM, Steven Price wrote:
> On 06/09/2024 05:32, Gavin Shan wrote:
>> On 8/19/24 11:19 PM, Steven Price wrote:
>>> From: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>
>>> On Arm CCA, with RMM-v1.0, all MMIO regions are shared. However, in
>>> the future, an Arm CCA-v1.0 compliant guest may be run in a lesser
>>> privileged partition in the Realm World (with Arm CCA-v1.1 Planes
>>> feature). In this case, some of the MMIO regions may be emulated
>>> by a higher privileged component in the Realm world, i.e, protected.
>>>
>>> Thus the guest must decide today, whether a given MMIO region is shared
>>> vs Protected and create the stage1 mapping accordingly. On Arm CCA, this
>>> detection is based on the "IPA State" (RIPAS == RIPAS_IO). Provide a
>>> helper to run this check on a given range of MMIO.
>>>
>>> Also, provide a arm64 helper which may be hooked in by other solutions.
>>>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>> ---
>>> New patch for v5
>>> ---
>>>    arch/arm64/include/asm/io.h       |  8 ++++++++
>>>    arch/arm64/include/asm/rsi.h      |  3 +++
>>>    arch/arm64/include/asm/rsi_cmds.h | 21 +++++++++++++++++++++
>>>    arch/arm64/kernel/rsi.c           | 26 ++++++++++++++++++++++++++
>>>    4 files changed, 58 insertions(+)

[...]

>>> diff --git a/arch/arm64/kernel/rsi.c b/arch/arm64/kernel/rsi.c
>>> index e968a5c9929e..381a5b9a5333 100644
>>> --- a/arch/arm64/kernel/rsi.c
>>> +++ b/arch/arm64/kernel/rsi.c
>>> @@ -67,6 +67,32 @@ void __init arm64_rsi_setup_memory(void)
>>>        }
>>>    }
>>>    +bool arm64_rsi_is_protected_mmio(phys_addr_t base, size_t size)
>>> +{
>>> +    enum ripas ripas;
>>> +    phys_addr_t end, top;
>>> +
>>> +    /* Overflow ? */
>>> +    if (WARN_ON(base + size < base))
>>> +        return false;
>>> +
>>> +    end = ALIGN(base + size, RSI_GRANULE_SIZE);
>>> +    base = ALIGN_DOWN(base, RSI_GRANULE_SIZE);
>>> +
>>> +    while (base < end) {
>>> +        if (WARN_ON(rsi_ipa_state_get(base, end, &ripas, &top)))
>>> +            break;
>>> +        if (WARN_ON(top <= base))
>>> +            break;
>>> +        if (ripas != RSI_RIPAS_IO)
>>> +            break;
>>> +        base = top;
>>> +    }
>>> +
>>> +    return (size && base >= end);
>>> +}
>>
>> I don't understand why @size needs to be checked here. Its initial value
>> taken from the input parameter should be larger than zero and its value
>> is never updated in the loop. So I'm understanding @size is always larger
>> than zero, and the condition would be something like below if I'm correct.
> 
> Yes you are correct. I'm not entirely sure why it was written that way.
> The only change dropping 'size' as you suggest is that a zero-sized
> region is considered protected. But I'd consider it a bug if this is
> called with size=0. I'll drop 'size' here.
> 

The check 'size == 0' could be squeezed to the overflow check if you agree.

     /* size == 0 or overflow */
     if (WARN_ON(base + size) <= base)
         return false;
     :
     
     return (base >= end);


>>         return (base >= end);     /* RSI_RIPAS_IO returned for all
>> granules */
>>
>> Another issue is @top is always zero with the latest tf-rmm. More details
>> are provided below.
> 
> That suggests that you are not actually using the 'latest' tf-rmm ;)
> (for some definition of 'latest' which might not be obvious!)
> 
>>From the cover letter:
> 
>> As mentioned above the new RMM specification means that corresponding
>> changes need to be made in the RMM, at this time these changes are still
>> in review (see 'topics/rmm-1.0-rel0-rc1'). So you'll need to fetch the
>> changes[3] from the gerrit instance until they are pushed to the main
>> branch.
>>
>> [3] https://review.trustedfirmware.org/c/TF-RMM/tf-rmm/+/30485
> 
> Sorry, I should probably have made this much more prominent in the cover
> letter.
> 
> Running something like...
> 
>   git fetch https://git.trustedfirmware.org/TF-RMM/tf-rmm.git \
> 	refs/changes/85/30485/11
> 
> ... should get you the latest. Hopefully these changes will get merged
> to the main branch soon.
> 

My bad. I didn't check the cover letter in time. With this specific TF-RMM branch,
I'm able to boot the guest with cca/host-v4 and cca/guest-v5. However, there are
messages indicating unhandled system register accesses, as below.

# ./start.sh
   Info: # lkvm run -k Image -m 256 -c 2 --name guest-152
   Info: Removed ghost socket file "/root/.lkvm//guest-152.sock".
[   rmm ] SMC_RMI_REALM_CREATE          882860000 880856000 > RMI_SUCCESS
[   rmm ] SMC_RMI_REC_AUX_COUNT         882860000 > RMI_SUCCESS 10
[   rmm ] SMC_RMI_REC_CREATE            882860000 88bdc5000 88bdc4000 > RMI_SUCCESS
[   rmm ] SMC_RMI_REC_CREATE            882860000 88bdd7000 88bdc4000 > RMI_SUCCESS
[   rmm ] SMC_RMI_REALM_ACTIVATE        882860000 > RMI_SUCCESS
[   rmm ] Unhandled write S2_0_C0_C2_2
[   rmm ] Unhandled write S3_3_C9_C14_0
[   rmm ] SMC_RSI_VERSION               10000 > RSI_SUCCESS 10000 10000
[   rmm ] SMC_RSI_REALM_CONFIG          82b2b000 > RSI_SUCCESS
[   rmm ] SMC_RSI_IPA_STATE_SET         80000000 90000000 1 0 > RSI_SUCCESS 90000000 0
[   rmm ] SMC_RSI_IPA_STATE_GET         1000000 1001000 > RSI_SUCCESS 1001000 0
      :
[    1.835570] DMA: preallocated 128 KiB GFP_KERNEL|GFP_DMA32 pool for atomic allocations
[    1.865993] audit: initializing netlink subsys (disabled)
[    1.891218] audit: type=2000 audit(0.492:1): state=initialized audit_enabled=0 res=1
[    1.899066] thermal_sys: Registered thermal governor 'step_wise'
[    1.920869] thermal_sys: Registered thermal governor 'power_allocator'
[    1.944151] cpuidle: using governor menu
[    1.988588] hw-breakpoint: found 16 breakpoint and 16 watchpoint registers.
[   rmm ] Unhandled write S2_0_C0_C0_5
[   rmm ] Unhandled write S2_0_C0_C0_4
[   rmm ] Unhandled write S2_0_C0_C1_5
[   rmm ] Unhandled write S2_0_C0_C1_4
[   rmm ] Unhandled write S2_0_C0_C2_5
      :
[   rmm ] Unhandled write S2_0_C0_C13_6
[   rmm ] Unhandled write S2_0_C0_C14_7
[   rmm ] Unhandled write S2_0_C0_C14_6
[   rmm ] Unhandled write S2_0_C0_C15_7
[   rmm ] Unhandled write S2_0_C0_C15_6

Thanks,
Gavin