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

[Steven Price <steven.price@arm.com>]

On 09/09/2024 00:53, Gavin Shan wrote:
> 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);
> 

Yes that makes sense, thanks for the suggestion.

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

To some extent unhandled system register accesses are expected. The
kernel will probe for features, and if the RMM doesn't support them it
will be emulating those registers as RAZ/WI. I believe RAZ/WI is an
appropriate emulation, so Linux won't have any trouble here, and I don't
think there's anything wrong with Linux probing these registers.

So the question really is whether the RMM needs to have dummy handlers
to silence the 'warnings'. They are currently output using 'INFO' so
priority - so will only be visible in a 'debug' build (or if the log
level has been explicitly set).

Steve

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