From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id F309537AA7D; Mon, 21 Sep 2026 10:03:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789984982; cv=none; b=kiVsZ1youFvb1hqedyVTyqPo7MGg9WKJsmkYmStQuYQ0mJDE28fEGnCKTd3WL5tURDzGtPsXvTgynr1Y4GPXaiDlLHnom8zkfYzKJDUbYGoHfdxDRazu3QOMYYev52wfWJH6sJRJ8jBq8xETDVkn7Xy7FapBtKlUf9i/K/ArWmY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789984982; c=relaxed/simple; bh=2Bi9HNrID2cE7gcX784v6dveXii59tEhPSRzCiZgdgs=; h=Message-ID:Date:MIME-Version:Subject:From:To:Cc:References: In-Reply-To:Content-Type; b=Cp2zgcDvy38TsT+ihAER6tqiPPlPd88/GGxvuFhhPdoULduXqbKIvoFQLkgRIdl/lrXih0NGwjb5MNTEtDgeXwnAoUjKsVn+pIYUDp59x7H+3mbhHLAB/lyCamS5PTIyAJzpDre2zkbecKlcRqkWjBv4ZEu2gLRsXT5EsWTSNvA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b=blOJwDP8; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b="blOJwDP8" Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id BC1CC1CE0; Mon, 21 Sep 2026 03:02:56 -0700 (PDT) Received: from [10.0.128.141] (unknown [10.0.128.141]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7A6DB3F86F; Mon, 21 Sep 2026 03:02:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=arm.com; s=foss; t=1789984980; bh=2Bi9HNrID2cE7gcX784v6dveXii59tEhPSRzCiZgdgs=; h=Date:Subject:From:To:Cc:References:In-Reply-To:From; b=blOJwDP8A/onBBJLnCPxUp9mIb8MzS7wS1+7wngcIelq7GcWJvaTjSVFF1Ytq/w3R x1av8/QUs0lhT01IoJteJ3chVaJ+FHMaSGYVCI5C4ujUt7eSTRkRLr5foUzByLlrWt EwI7qlCQh+NLED+A0IIYKTIEedohHbTqb02OPxBY= Message-ID: <45fb9520-7003-412f-9ab1-14e2a762c816@arm.com> Date: Mon, 21 Sep 2026 11:02:56 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v18 1/7] firmware: arm_rmm: Add SMC definitions for calling the RMM Content-Language: en-GB From: Suzuki K Poulose To: Jonathan Cameron Cc: kvm@vger.kernel.org, kvmarm@lists.linux.dev, maz@kernel.org, will@kernel.org, catalin.marinas@arm.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, steven.price@arm.com, aneesh.kumar@kernel.org, oupton@kernel.org, gshan@redhat.com, joey.gouly@arm.com, tabba@google.com, yuzenghui@huawei.com, linux-coco@lists.linux.dev, gankulkarni@os.amperecomputing.com, sdonthineni@nvidia.com, alpergun@google.com, fj0570is@fujitsu.com, WeiLin.Chang@arm.com, lpieralisi@kernel.org, enju.kohei@fujitsu.com References: <20260912083611.2513845-1-suzuki.poulose@arm.com> <20260912083611.2513845-2-suzuki.poulose@arm.com> <178978126540.2352296.16845657485139032765.b4-review@b4> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 21/09/2026 10:27, Suzuki K Poulose wrote: > On 19/09/2026 02:27, Jonathan Cameron wrote: >>> The RMM (Realm Management Monitor) provides functionality that can be >>> accessed by SMC calls from the host. >>> >>> The SMC definitions are based on DEN0137[1] version 2.0-bet3 >>> >>> [1] https://developer.arm.com/documentation/den0137/2-0bet3/ >>> >>> Signed-off-by: Steven Price >>> Signed-off-by: Suzuki K Poulose >> >> With Gavin's nitpicks and the GENMASK_ULL() from sashiko, just a few >> comments inline.  Mostly on subtle inconsistencies that really don't >> matter that much. >> >>>   include/linux/arm-smccc-rmi.h | 497 ++++++++++++++++++++++++++++++++++ >>>   1 file changed, 497 insertions(+) >>>   create mode 100644 include/linux/arm-smccc-rmi.h >>> >>> diff --git a/include/linux/arm-smccc-rmi.h b/include/linux/arm-smccc- >>> rmi.h >>> new file mode 100644 >>> index 000000000000..214d6228dfc2 >>> --- /dev/null >>> +++ b/include/linux/arm-smccc-rmi.h >>> @@ -0,0 +1,497 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +/* >>> + * Copyright (C) 2023-2026 ARM Ltd. >>> + * >>> + * The values and structures in this file are from the Realm >>> Management Monitor >>> + * specification (DEN0137) version 2.0-bet3: >>> + * https://developer.arm.com/documentation/den0137/2-0bet3/ >>> + */ >>> + >>> +#ifndef __LINUX_ARM_SMCCC_RMI_H_ >>> +#define __LINUX_ARM_SMCCC_RMI_H_ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include >>> + >>> +#define SMC_RMI_CALL(func)                \ >>> +    ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,        \ >>> +               ARM_SMCCC_SMC_64,        \ >>> +               ARM_SMCCC_OWNER_STANDARD,    \ >>> +               (func)) >> >> Obviously it is v18 so probably a future thing but nothing about this >> is RMI specific.  Could be used for ARM_SMCCC_TRNG_RND64 for instance. >> I'm not entirely sure what we'd call such a macro >> >> ARM_SMCCC_CALL_VAL64_STD() maybe? > > ARM_SMCCC_STD_CALL64_VAL() ? > > But, I would leave it as a wider cleanup in the tree as a separate > series. > >> >> I'm not just not keen on macros whose names to me hint at something >> special. FWIW this also matches SMC_RSI_FID() and FFA_SMC64(). >> Isn't it nice when we have a predictable naming scheme :) >> >> FID (for Function IDentifier) is in the spec, so maybe? >> >> Anyhow, I don't really care that much. >> >>> + >>> +#define SMC_RMI_VERSION                SMC_RMI_CALL(0x0150) >>> + >> >> >>> +#define RMI_ABI_MAJOR_VERSION    2 >>> +#define RMI_ABI_MINOR_VERSION    0 >>> + >>> +#define RMI_ABI_VERSION_GET_MAJOR(version) ((version) >> 16) >> >> I'd mask it.  Mostly because that would shout that it is only 15 bits. >> > > Ack > >>> +#define RMI_ABI_VERSION_GET_MINOR(version) ((version) & 0xFFFF) >>> +#define RMI_ABI_VERSION(major, minor)      (((major) << 16) | (minor)) >> >> I'd go all in on FIELD_PREP() / FIELD_GET() + GENMASK just for the >> sake of consistency + not having to be careful that everything is >> checked for fit to keep the LLM bots happy. >> >>> + >>> +#define RMI_RETURN_STATUS_MASK        GENMASK(7, 0) >>> +#define RMI_RETURN_INDEX_MASK        GENMASK(15, 8) >>> +#define RMI_RETURN_MEMREQ_MASK        GENMASK(9, 8) >>> +#define RMI_RETURN_CAN_CANCEL_MASK    BIT(10) >>> + >>> +#define RMI_RETURN_STATUS(ret) >>> FIELD_GET(RMI_RETURN_STATUS_MASK, ret) >>> +#define RMI_RETURN_INDEX(ret) >>> FIELD_GET(RMI_RETURN_INDEX_MASK, ret) >> >> What's this one?  I can't find anything in the spec that matches it >> and as far as I can tell you don't use it in this series. > > This is coming from RmiResultDataLevel. See RmiResult type. > This was renamed after we introduce the RmiResultDataIncomplete. > It is used in the KVM code to find the "level" where a command > failed/walked. > > I could rename it to RMI_RESULT_DATA_LEVEL() ? > Similarly RMI_RESULT_STATUS instead of RMI_RETURN_* > >> >>> +#define RMI_RETURN_MEMREQ(ret) >>> FIELD_GET(RMI_RETURN_MEMREQ_MASK, ret) >>> +#define RMI_RETURN_CAN_CANCEL(ret) >>> FIELD_GET(RMI_RETURN_CAN_CANCEL_MASK, ret) > >> These are obscure enough to find in the spec I'd give a comment just >> to save the sanity of anyone looking for them. > > As above, they are really RMI_RESULT_DATA_INCOMPLETE_* > >> >>> +/* >>> + * Note many of these fields are smaller than u64 but all fields >>> have u64 >>> + * alignment, so use u64 to ensure correct alignment. >> >> Obviously this is only going to run on arm64 so it's not critical, but >> more generally u64s aren't always 64 bit aligned.  So if you 'really' >> care aligned_u64 is there to ensure it.  Meh, arm64 so fine. > > Agreed, I am worried about the churn in the consumer code. Also, like > you said, this is only for ARM64. So, I would pass it. > >> >>> + */ >>> +struct rmm_config { >>> +    union { /* 0x0 */ >>> +        struct { >>> +            u64 tracking_region_size; >>> +            u64 rmi_granule_size; >>> +        }; >>> +        u8 sizer[SZ_4K]; >>> +    }; >>> +}; >>> + >>> +static_assert(sizeof(struct rmm_config) == SZ_4K); >>> + >>> +#define RMI_REALM_PARAM_FLAG_SVE        BIT(1) >>> +#define RMI_REALM_PARAM_FLAG_PMU        BIT(2) >>> +#define RMI_REALM_PARAM_FLAG_DA            BIT(3) >>> +#define RMI_REALM_PARAM_FLAG_LFA_POLICY        GENMASK(6, 5) >>> +#define RMI_REALM_PARAM_FLAG_MEC_POLICY        GENMASK(8, 7) >> >> Tiny bit inconsistent. When do you decide _MASK is needed and when >> not?  Seems a little too random for multibit fields. > > I will try to clean this up. Actually, this is used when we don't consume them from RMM. i.e., we never call FIELD_GET() on them. But thats not a reason for not being consistent in naming. I can fix it Cheers Suzuki