Message-ID: <3ee316c9-0660-4b21-a02c-cda8fe9fd118@arm.com>
Date: Fri, 19 Apr 2024 12:18:21 +0100
From: Steven Price <steven.price@....com>
To: Suzuki K Poulose <suzuki.poulose@....com>, kvm@...r.kernel.org,
 kvmarm@...ts.linux.dev
Cc: Catalin Marinas <catalin.marinas@....com>, Marc Zyngier <maz@...nel.org>,
 Will Deacon <will@...nel.org>, James Morse <james.morse@....com>,
 Oliver Upton <oliver.upton@...ux.dev>, Zenghui Yu <yuzenghui@...wei.com>,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Joey Gouly <joey.gouly@....com>, Alexandru Elisei
 <alexandru.elisei@....com>, Christoffer Dall <christoffer.dall@....com>,
 Fuad Tabba <tabba@...gle.com>, linux-coco@...ts.linux.dev,
 Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com>
Subject: Re: [PATCH v2 06/43] arm64: RME: Add wrappers for RMI calls

On 16/04/2024 14:14, Suzuki K Poulose wrote:
> Hi Steven
> 
> On 12/04/2024 09:42, 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.
>>
> 
> I have compared the parameters and output values to that of the RMM spec
> and they match. There are some minor nits below.
> 
>> Signed-off-by: Steven Price <steven.price@....com>
>> ---
>>   arch/arm64/include/asm/rmi_cmds.h | 509 ++++++++++++++++++++++++++++++
>>   1 file changed, 509 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..c21414127e8e
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/rmi_cmds.h
>> @@ -0,0 +1,509 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2023 ARM Ltd.
>> + */
>> +
>> +#ifndef __ASM_RMI_CMDS_H
>> +#define __ASM_RMI_CMDS_H
>> +
>> +#include <linux/arm-smccc.h>
>> +
>> +#include <asm/rmi_smc.h>
>> +
>> +struct rtt_entry {
>> +    unsigned long walk_level;
>> +    unsigned long desc;
>> +    int state;
>> +    int ripas;
>> +};
>> +
> 
> ...
> 
>> +/**
>> + * rmi_data_destroy() - Destroy a Data Granule
>> + * @rd: PA of the RD
>> + * @ipa: IPA at which the granule is mapped in the guest
>> + * @data_out: PA of the granule which was destroyed
>> + * @top_out: Top IPA of non-live RTT entries
>> + *
>> + * Transitions the granule to DESTROYED state, the address cannot be
>> used by
>> + * the guest for the lifetime of the Realm.
>> + *
>> + * Return: RMI return code
>> + */
>> +static inline int rmi_data_destroy(unsigned long rd, unsigned long ipa,
>> +                   unsigned long *data_out,
>> +                   unsigned long *top_out)
>> +{
>> +    struct arm_smccc_res res;
>> +
>> +    arm_smccc_1_1_invoke(SMC_RMI_DATA_DESTROY, rd, ipa, &res);
>> +
>> +    *data_out = res.a1;
>> +    *top_out = res.a2;
> 
> minor nit: Do we need to be safer by checking the parameters before
> filling them in ? i.e.,
> 
>     if (ptr)
>         *ptr = result_out;
> 
> This applies for others calls below.

I had taken the approach of making all the out-parameters required (i.e.
non-NULL). But I guess I can switch over to allowing NULL - hopefully
the compiler will optimise these checks away, but there are some
situations where we are currently ignoring the extra out-parameters that
could be tidied up.

> 
>> +
>> +    return res.a0;
>> +}
> 
>> +
>> +/**
>> + * rmi_realm_destroy() - Destroy a Realm
>> + * @rd: PA of the RD
>> + *
>> + * Destroys a Realm, all objects belonging to the Realm must be
>> destroyed first.
>> + *
>> + * Return: RMI return code
>> + */
>> +static inline int rmi_realm_destroy(unsigned long rd)
>> +{
>> +    struct arm_smccc_res res;
>> +
>> +    arm_smccc_1_1_invoke(SMC_RMI_REALM_DESTROY, rd, &res);
>> +
>> +    return res.a0;
>> +}
>> +
>> +/**
>> + * rmi_rec_aux_count() - Get number of auxiliary Granules required
>> + * @rd: PA of the RD
>> + * @aux_count: Number of pages written to this pointer
>> + *
>> + * A REC may require extra auxiliary pages to be delegateed for the
>> RMM to
> 
> minor nit: "s/delegateed/delegated/"
> 
> ...
> 
>> +/**
>> + * rmi_rtt_read_entry() - Read an RTTE
>> + * @rd: PA of the RD
>> + * @ipa: IPA for which to read the RTTE
>> + * @level: RTT level at which to read the RTTE
>> + * @rtt: Output structure describing the RTTE
>> + *
>> + * Reads a RTTE (Realm Translation Table Entry).
>> + *
>> + * Return: RMI return code
>> + */
>> +static inline int rmi_rtt_read_entry(unsigned long rd, unsigned long
>> ipa,
>> +                     long level, struct rtt_entry *rtt)
>> +{
>> +    struct arm_smccc_1_2_regs regs = {
>> +        SMC_RMI_RTT_READ_ENTRY,
>> +        rd, ipa, level
>> +    };
>> +
>> +    arm_smccc_1_2_smc(&regs, &regs);
>> +
>> +    rtt->walk_level = regs.a1;
>> +    rtt->state = regs.a2 & 0xFF;
> 
> minor nit: We mask the state, but not the "ripas". Both of them are u8.
> For consistency, we should mask both or neither.

Good point - I'll mask ripas as well. I suspect this is a bug that crept
in when I was updating for the new RIPAS state.

>> +    rtt->desc = regs.a3;
>> +    rtt->ripas = regs.a4;
>> +
>> +    return regs.a0;
>> +}
>> +
> 
> ...
> 
>> +/**
>> + * rmi_rtt_get_phys() - Get the PA from a RTTE
>> + * @rtt: The RTTE
>> + *
>> + * Return: the physical address from a RTT entry.
>> + */
>> +static inline phys_addr_t rmi_rtt_get_phys(struct rtt_entry *rtt)
>> +{
>> +    return rtt->desc & GENMASK(47, 12);
>> +}
> 
> I guess this may need to change with the LPA2 support in RMM and must be
> used in conjunction with the "realm" object to make the correct
> conversion.

Actually this is currently unused, and there's a potential bug lurking
in realm_map_protected() where rtt->desc is assumed to be a valid
physical address. I'll move the function there and fix it up by also
taking a realm argument. I've tried to keep the realm structure out of
this file.

Thanks,

Steve

Powered by blists - more mailing lists