From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.2421.1689183815385178213 for ; Wed, 12 Jul 2023 10:43:35 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=kAcpHTyx; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: osde@linux.microsoft.com) Received: from [10.137.194.171] (unknown [131.107.1.171]) by linux.microsoft.com (Postfix) with ESMTPSA id D32E921C44E1; Wed, 12 Jul 2023 10:43:34 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com D32E921C44E1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1689183814; bh=hTwHwD3pKvxwGb/jNu0nVDHyIlE7iC+iw9NjI9gBYpE=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=kAcpHTyxMp4c+PRyosaH/islgR91eKfb5D/Z+MRa+jDMfJIQUesRxihqN6EHgLqBO Kyasl9UiagwAGvDRjAXPEyko5kV/f6aGmK6KBnB3F4qbIUS+ezF2Vt+5YYxpyinAdU wku9eyUWxvP/T7QAzuan99Z23tf7DbIag9sS4dcs= Message-ID: Date: Wed, 12 Jul 2023 10:43:34 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [edk2-devel] [edk2-platforms][PATCH V1 06/20] ArmPkg: Add support for FFA_MEM_PERM_GET/SET ABIs To: devel@edk2.groups.io, nishant.sharma@arm.com Cc: Ard Biesheuvel , Sami Mujawar , Thomas Abraham , Sayanta Pattanayak , Achin Gupta References: <20230711143658.781597-1-nishant.sharma@arm.com> <20230711143658.781597-7-nishant.sharma@arm.com> From: "Oliver Smith-Denny" In-Reply-To: <20230711143658.781597-7-nishant.sharma@arm.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 7/11/2023 7:36 AM, Nishant Sharma wrote: > From: Achin Gupta > > This patch uses the FFA_MEM_PERM_GET/SET ABIs to tweak the permissions of a > set of pages if FF-A v1.1 and above is supported by the SPMC. For FF-A v1.0 > the previous method through FFA_MSG_SEND_DIRECT_REQ/RESP is used. > > Signed-off-by: Achin Gupta > Signed-off-by: Nishant Sharma > --- > ArmPkg/Include/IndustryStandard/ArmFfaSvc.h | 2 + > ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c | 132 +++++++++++++++++--- > 2 files changed, 120 insertions(+), 14 deletions(-) > > diff --git a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h > index c80d783fad3f..7987678c996e 100644 > --- a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h > +++ b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h > @@ -23,6 +23,8 @@ > #define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64 0xC4000070 > #define ARM_SVC_ID_FFA_SUCCESS_AARCH32 0x84000061 > #define ARM_SVC_ID_FFA_SUCCESS_AARCH64 0xC4000061 > +#define ARM_SVC_ID_FFA_MEM_PERM_SET_AARCH32 0x84000089 > +#define ARM_SVC_ID_FFA_MEM_PERM_GET_AARCH32 0x84000088 > #define ARM_SVC_ID_FFA_ERROR_AARCH32 0x84000060 > #define ARM_SVC_ID_FFA_ERROR_AARCH64 0xC4000060 > #define ARM_SVC_ID_FFA_MSG_WAIT_AARCH32 0x8400006B > diff --git a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c > index 1a41a289ef17..76ef214bcb85 100644 > --- a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c > +++ b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c > @@ -25,6 +25,66 @@ > #include > #include > > + > +/** > + Utility function to determine whether ABIs in FF-A v1.1 to set and get > + memory permissions can be used. Ideally, this should be invoked once in the > + library constructor and set a flag that can be used at runtime. However, the > + StMM Core invokes this library before constructors are called and before the > + StMM image itself is relocated. > + > + @retval EFI_SUCCESS The availability of ABIs was correctly determined. > + @retval Other value Software stack is misconfigured. > + > +**/ > +STATIC > +BOOLEAN > +UseFfaMemPermAbis ( > + VOID > + ) > +{ > + ARM_SVC_ARGS SvcArgs; > + UINT32 SpmcFfaVersion; > + STATIC UINT16 SpmcMajorVer = 0; > + STATIC UINT16 SpmcMinorVer = 0; > + > + // Use prefetched version info. if either is not 0, then the version is > + // already fetched. > + if ((SpmcMajorVer | SpmcMinorVer) != 0) { > + return (SpmcMajorVer == SPM_MAJOR_VERSION_FFA) && (SpmcMinorVer >= SPM_MINOR_VERSION_FFA); > + } > + > + // Nothing to do if FF-A has not be enabled nit: "been enabled" > + if (FixedPcdGet32 (PcdFfaEnable) == 0) { > + return FALSE; > + } > + > + // Prepare the message parameters. > + ZeroMem (&SvcArgs, sizeof (ARM_SVC_ARGS)); > + SvcArgs.Arg0 = ARM_SVC_ID_FFA_VERSION_AARCH32; > + SvcArgs.Arg1 = FFA_VERSION_COMPILED; > + > + // Invoke the ABI > + ArmCallSvc (&SvcArgs); > + > + // Check if FF-A is supported and what version > + SpmcFfaVersion = SvcArgs.Arg0; > + > + // Damn! FF-A is not supported at all even though we specified v1.0 as our > + // version. However, the feature flag has been turned on. This is a > + // misconfigured software stack. So, return an error and assert in a debug build. > + if (SpmcFfaVersion == ARM_FFA_SPM_RET_NOT_SUPPORTED) { > + ASSERT (0); It would be nice to either have the assert be self documenting (ASSERT (SpmcFfaVersion == ARM_FFA_SPM_RET_NOT_SUPPORTED)) or to add a print here. > + return FALSE; > + } > + > + SpmcMajorVer = (SpmcFfaVersion >> FFA_VERSION_MAJOR_SHIFT) & FFA_VERSION_MAJOR_MASK; > + SpmcMinorVer = (SpmcFfaVersion >> FFA_VERSION_MINOR_SHIFT) & FFA_VERSION_MINOR_MASK; > + > + return (SpmcMajorVer == SPM_MAJOR_VERSION_FFA) && (SpmcMinorVer >= SPM_MINOR_VERSION_FFA); > +} > + > + > /** Send memory permission request to target. > > @param [in, out] SvcArgs Pointer to SVC arguments to send. On > @@ -55,6 +115,36 @@ SendMemoryPermissionRequest ( > > ArmCallSvc (SvcArgs); > if (FixedPcdGet32 (PcdFfaEnable) != 0) { > + > + // Check if FF-A memory permission ABIs were used. > + if (UseFfaMemPermAbis()) { > + switch (SvcArgs->Arg0) { > + > + case ARM_SVC_ID_FFA_ERROR_AARCH32: > + case ARM_SVC_ID_FFA_ERROR_AARCH64: > + switch (SvcArgs->Arg2) { > + case ARM_FFA_SPM_RET_INVALID_PARAMETERS: > + return EFI_INVALID_PARAMETER; > + case ARM_FFA_SPM_RET_NOT_SUPPORTED: > + return EFI_UNSUPPORTED; > + default: > + // Undefined error code received. > + ASSERT (0); For these two default cases, can we print out what error code we received? Thanks, Oliver > + return EFI_INVALID_PARAMETER; > + } > + > + case ARM_SVC_ID_FFA_SUCCESS_AARCH32: > + case ARM_SVC_ID_FFA_SUCCESS_AARCH64: > + *RetVal = SvcArgs->Arg2; > + return EFI_SUCCESS; > + > + default: > + // Undefined error code received. > + ASSERT (0); > + return EFI_INVALID_PARAMETER; > + } > + } > + > // Get/Set memory attributes is an atomic call, with > // StandaloneMm at S-EL0 being the caller and the SPM > // core being the callee. Thus there won't be a > @@ -164,12 +254,18 @@ GetMemoryPermissions ( > // See [1], Section 13.5.5.1 MM_SP_MEMORY_ATTRIBUTES_GET_AARCH64. > ZeroMem (&SvcArgs, sizeof (ARM_SVC_ARGS)); > if (FixedPcdGet32 (PcdFfaEnable) != 0) { > - // See [2], Section 10.2 FFA_MSG_SEND_DIRECT_REQ. > - SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ; > - SvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID; > - SvcArgs.Arg2 = 0; > - SvcArgs.Arg3 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES; > - SvcArgs.Arg4 = BaseAddress; > + // Check if FF-A memory permission ABIs can be used. > + if (UseFfaMemPermAbis()) { > + SvcArgs.Arg0 = ARM_SVC_ID_FFA_MEM_PERM_GET_AARCH32; > + SvcArgs.Arg1 = BaseAddress; > + } else { > + // See [2], Section 10.2 FFA_MSG_SEND_DIRECT_REQ. > + SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ; > + SvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID; > + SvcArgs.Arg2 = 0; > + SvcArgs.Arg3 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES; > + SvcArgs.Arg4 = BaseAddress; > + } > } else { > SvcArgs.Arg0 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES; > SvcArgs.Arg1 = BaseAddress; > @@ -219,14 +315,22 @@ RequestMemoryPermissionChange ( > // See [1], Section 13.5.5.2 MM_SP_MEMORY_ATTRIBUTES_SET_AARCH64. > ZeroMem (&SvcArgs, sizeof (ARM_SVC_ARGS)); > if (FixedPcdGet32 (PcdFfaEnable) != 0) { > - // See [2], Section 10.2 FFA_MSG_SEND_DIRECT_REQ. > - SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ; > - SvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID; > - SvcArgs.Arg2 = 0; > - SvcArgs.Arg3 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES; > - SvcArgs.Arg4 = BaseAddress; > - SvcArgs.Arg5 = EFI_SIZE_TO_PAGES (Length); > - SvcArgs.Arg6 = Permissions; > + // Check if FF-A memory permission ABIs can be used. > + if (UseFfaMemPermAbis()) { > + SvcArgs.Arg0 = ARM_SVC_ID_FFA_MEM_PERM_SET_AARCH32; > + SvcArgs.Arg1 = BaseAddress; > + SvcArgs.Arg2 = EFI_SIZE_TO_PAGES (Length); > + SvcArgs.Arg3 = Permissions; > + } else { > + // See [2], Section 10.2 FFA_MSG_SEND_DIRECT_REQ. > + SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ; > + SvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID; > + SvcArgs.Arg2 = 0; > + SvcArgs.Arg3 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES; > + SvcArgs.Arg4 = BaseAddress; > + SvcArgs.Arg5 = EFI_SIZE_TO_PAGES (Length); > + SvcArgs.Arg6 = Permissions; > + } > } else { > SvcArgs.Arg0 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES; > SvcArgs.Arg1 = BaseAddress;