Received-SPF: pass (google.com: domain of linux-kernel+bounces-106063-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <136e6054-0e08-4a26-8aa4-0572bdf6fd4f@arm.com>
Date: Mon, 18 Mar 2024 11:22:43 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH 03/28] arm64: RME: Add wrappers for RMI calls
Content-Language: en-GB
To: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>,
 kvm@vger.kernel.org, kvmarm@lists.linux.dev
Cc: Catalin Marinas <catalin.marinas@arm.com>, Marc Zyngier <maz@kernel.org>,
 Will Deacon <will@kernel.org>, James Morse <james.morse@arm.com>,
 Oliver Upton <oliver.upton@linux.dev>,
 Suzuki K Poulose <suzuki.poulose@arm.com>, Zenghui Yu
 <yuzenghui@huawei.com>, linux-arm-kernel@lists.infradead.org,
 linux-kernel@vger.kernel.org, Joey Gouly <joey.gouly@arm.com>,
 Alexandru Elisei <alexandru.elisei@arm.com>,
 Christoffer Dall <christoffer.dall@arm.com>, Fuad Tabba <tabba@google.com>,
 linux-coco@lists.linux.dev
References: <20230127112248.136810-1-suzuki.poulose@arm.com>
 <20230127112932.38045-1-steven.price@arm.com>
 <20230127112932.38045-4-steven.price@arm.com>
 <aaf10e76-657f-4667-a920-e71b93419efd@os.amperecomputing.com>
From: Steven Price <steven.price@arm.com>
In-Reply-To: <aaf10e76-657f-4667-a920-e71b93419efd@os.amperecomputing.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 18/03/2024 07:03, Ganapatrao Kulkarni wrote:
> 
> Hi Steven,
> 
> On 27-01-2023 04:59 pm, Steven Price wrote:
>> The wrappers make the call sites easier to read and deal with the
>> boiler plate of handling the error codes from the RMM.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>>   arch/arm64/include/asm/rmi_cmds.h | 259 ++++++++++++++++++++++++++++++
>>   1 file changed, 259 insertions(+)
>>   create mode 100644 arch/arm64/include/asm/rmi_cmds.h
>>
>> diff --git a/arch/arm64/include/asm/rmi_cmds.h
>> b/arch/arm64/include/asm/rmi_cmds.h
>> new file mode 100644
>> index 000000000000..d5468ee46f35
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/rmi_cmds.h

[...]

>> +static inline int rmi_rtt_read_entry(unsigned long rd, unsigned long
>> map_addr,
>> +                     unsigned long level, struct rtt_entry *rtt)
>> +{
>> +    struct arm_smccc_1_2_regs regs = {
>> +        SMC_RMI_RTT_READ_ENTRY,
>> +        rd, map_addr, level
>> +    };
>> +
>> +    arm_smccc_1_2_smc(&regs, &regs);
>> +
>> +    rtt->walk_level = regs.a1;
>> +    rtt->state = regs.a2 & 0xFF;
>> +    rtt->desc = regs.a3;
>> +    rtt->ripas = regs.a4 & 1;
>> +
>> +    return regs.a0;
>> +}
>> +
>> +static inline int rmi_rtt_set_ripas(unsigned long rd, unsigned long rec,
>> +                    unsigned long map_addr, unsigned long level,
>> +                    unsigned long ripas)
>> +{
>> +    struct arm_smccc_res res;
>> +
>> +    arm_smccc_1_1_invoke(SMC_RMI_RTT_SET_RIPAS, rd, rec, map_addr,
>> level,
>> +                 ripas, &res);
>> +
>> +    return res.a0;
>> +}
>> +
>> +static inline int rmi_rtt_unmap_unprotected(unsigned long rd,
>> +                        unsigned long map_addr,
>> +                        unsigned long level)
>> +{
>> +    struct arm_smccc_res res;
>> +
>> +    arm_smccc_1_1_invoke(SMC_RMI_RTT_UNMAP_UNPROTECTED, rd, map_addr,
>> +                 level, &res);
>> +
>> +    return res.a0;
>> +}
>> +
>> +static inline phys_addr_t rmi_rtt_get_phys(struct rtt_entry *rtt)
>> +{
>> +    return rtt->desc & GENMASK(47, 12);
>> +}
>> +
>> +#endif
> 
> Can we please replace all occurrence of "unsigned long" with u64?

I'm conflicted here. On the one hand I agree with you - it would be
better to use types that are sized according to the RMM spec. However,
this file is a thin wrapper around the low-level SMC calls, and the
SMCCC interface is a bunch of "unsigned longs" (e.g. look at struct
arm_smccc_1_2_regs).

In particular it could be broken to use smaller types (e.g. char/u8) as
it would potentially permit the compiler to leave 'junk' in the top part
of the register.

So the question becomes whether to stick with the SMCCC interface sizes
(unsigned long) or use our knowledge that it must be a 64 bit platform
(RMM isn't support for 32 bit) and therefore use u64. My (mild)
preference is for unsigned long because it makes it obvious how this
relates to the SMCCC interface it's using. It also seems like it would
ease compatibility if (/when?) 128 bit registers become a thing.

> Also as per spec, RTT level is Int64, can we change accordingly?

Here, however, I agree you've definitely got a point. level should be
signed as (at least in theory) it could be negative.

> Please CC me in future cca patch-sets.
> gankulkarni@os.amperecomputing.com

I will do, thanks for the review.

Steve