Re: [RFC PATCH v1 05/30] ArmPkg & ArmVirtPkg: Make PcdMonitorConduitHvc a dynamic PCD

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Ard,

Thank you for the feedback.
Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 10/05/2023, 12:39, "Ard Biesheuvel" <ardb@kernel.org <mailto:ardb@kernel.org>> wrote:


On Tue, 25 Apr 2023 at 18:04, Sami Mujawar <sami.mujawar@arm.com <mailto:sami.mujawar@arm.com>> wrote:
>
> The monitor call conduit is fixed for a platform firmware in
> most scenarios. For a normal virtual machine guest firmware,
> the default conduit is HVC. However, for Arm CCA the Realm
> code must use SMC as the conduit.
>
> To have a common code base for Guest/Virtual firmware to be used
> by both normal VMs and Realm VMs, make PcdMonitorConduitHvc as a
> dynamic PCD. This allows the firmware to detect if it is running
> in a Realm and it can configure the PcdMonitorConduitHvc as FALSE
> (i.e. to use SMC as the conduit when running in a Realm).
>
> Also update the ArmVirtPkg/ArmVirtKvmTool.dsc workspace to move
> the PcdMonitorConduitHvc in the PcdsDynamic section to prevent
> the build from breaking.
>


Do you mean realm VMs will use SMC even for PSCI calls etc?
[SAMI] For Realm code a SMC is required for PSCI calls.
Following extract from Section A1.3 of the RMM A-bet0 specification (https://developer.arm.com/documentation/den0137/1-0bet0/?lang=en)

        The RMM exposes the following interfaces, which are accessed via SMC instructions, to Realms:
        * The Realm Services Interface (RSI), which provides services used to manage resources allocated to the
           Realm, and to request an attestation report.
        * The Power State Coordination Interface (PSCI), which provides services used to control power states of
           VPEs within a Realm. Note that the HVC conduit for PSCI is not supported for Realms.

[/SAMI]

The change looks fine to me, given that other platforms that rely on
the default will still get a fixed PCD after this change.




> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com <mailto:sami.mujawar@arm.com>>
> ---
> ArmPkg/ArmPkg.dec | 10 +++++-----
> ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.c | 4 ++--
> ArmVirtPkg/ArmVirtKvmTool.dsc | 4 ++--
> 3 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index f17ba913e6de1326d49b93d6a15378ff2f522d24..0730533e512d60fcba19c4cfa84944061d16f02e 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -139,11 +139,6 @@ [PcdsFeatureFlag.common]
> # Define if the GICv3 controller should use the GICv2 legacy
> gArmTokenSpaceGuid.PcdArmGicV3WithV2Legacy|FALSE|BOOLEAN|0x00000042
>
> - ## Define the conduit to use for monitor calls.
> - # Default PcdMonitorConduitHvc = FALSE, conduit = SMC
> - # If PcdMonitorConduitHvc = TRUE, conduit = HVC
> - gArmTokenSpaceGuid.PcdMonitorConduitHvc|FALSE|BOOLEAN|0x00000047
> -
> [PcdsFeatureFlag.ARM]
> # Whether to map normal memory as non-shareable. FALSE is the safe choice, but
> # TRUE may be appropriate to fix performance problems if you don't care about
> @@ -393,6 +388,11 @@ [PcdsFixedAtBuild.common, PcdsDynamic.common]
> gArmTokenSpaceGuid.PcdPciBusMin|0x0|UINT32|0x00000059
> gArmTokenSpaceGuid.PcdPciBusMax|0x0|UINT32|0x0000005A
>
> + ## Define the conduit to use for monitor calls.
> + # Default PcdMonitorConduitHvc = FALSE, conduit = SMC
> + # If PcdMonitorConduitHvc = TRUE, conduit = HVC
> + gArmTokenSpaceGuid.PcdMonitorConduitHvc|FALSE|BOOLEAN|0x00000047
> +
> [PcdsDynamicEx]
> #
> # This dynamic PCD hold the GUID of a firmware FFS which contains
> diff --git a/ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.c b/ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.c
> index 741f5c615744dc5cc5381ff3848078f93858dd2b..221724125ce3a8f351a55a81f441409a99bcb5cf 100644
> --- a/ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.c
> +++ b/ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.c
> @@ -1,7 +1,7 @@
> /** @file
> Arm Monitor Library.
>
> - Copyright (c) 2022, Arm Limited. All rights reserved.<BR>
> + Copyright (c) 2022 - 2023, Arm Limited. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -26,7 +26,7 @@ ArmMonitorCall (
> IN OUT ARM_MONITOR_ARGS *Args
> )
> {
> - if (FeaturePcdGet (PcdMonitorConduitHvc)) {
> + if (PcdGetBool (PcdMonitorConduitHvc)) {
> ArmCallHvc ((ARM_HVC_ARGS *)Args);
> } else {
> ArmCallSmc ((ARM_SMC_ARGS *)Args);
> diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
> index d2228a95726b24fe5c2edfbc84b1f5c23a85feba..467e5c166e1bbad3acbae78f53c225f5bac525a9 100644
> --- a/ArmVirtPkg/ArmVirtKvmTool.dsc
> +++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
> @@ -117,8 +117,6 @@ [PcdsFeatureFlag.common]
> # Use MMIO for accessing RTC controller registers.
> gPcAtChipsetPkgTokenSpaceGuid.PcdRtcUseMmio|TRUE
>
> - gArmTokenSpaceGuid.PcdMonitorConduitHvc|TRUE
> -
> [PcdsFixedAtBuild.common]
> gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x8000000F
>
> @@ -237,6 +235,8 @@ [PcdsDynamicDefault.common]
> gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister64|0x0
> gPcAtChipsetPkgTokenSpaceGuid.PcdRtcTargetRegister64|0x0
>
> + gArmTokenSpaceGuid.PcdMonitorConduitHvc|TRUE
> +
> ################################################################################
> #
> # Components Section - list of all EDK II Modules needed by this Platform
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>