[PATCH v4 0/2] Qemu/Sbsa versioning

[PATCH v4 0/2] Qemu/Sbsa versioning

[Marcin Juszkiewicz]

Qemu has versioning for sbsa-ref platform. TF-A reads it from provided
DeviceTree and provides as SMC.

Version 0.0 (QEMU 8.0)  provides platform version information.
Version 0.1 (in review) provides GIC information.

Next versions will bring more changes to make use of development done
during last years.

Requires "SbsaQemu: make GIC base address Pcds dynamic" by Leif
Lindholm to be able to change values with those from QEMU.

QEMU part: https://lists.gnu.org/archive/html/qemu-arm/2023-05/msg00517.html
TF-A part: https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/20871

Marcin Juszkiewicz (2):
  Platform/SbsaQemu: read platform version
  Platform/SbsaQemu: read GIC base from TF-A

 Platform/Qemu/SbsaQemu/SbsaQemu.dsc           |  3 +
 .../SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c | 57 +++++++++++++++++++
 .../SbsaQemuPlatformDxe.inf                   |  9 +++
 Silicon/Qemu/SbsaQemu/SbsaQemu.dec            |  3 +
 4 files changed, 72 insertions(+)

-- 
2.40.1

[PATCH 1/2] Platform/SbsaQemu: read platform version

[Marcin Juszkiewicz]

Qemu has versioning for sbsa-ref platform. TF-A reads it from provided
DeviceTree and provides as SMC.

This change adds reading platform version into EDK2.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 Platform/Qemu/SbsaQemu/SbsaQemu.dsc           |  3 +++
 .../SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c | 27 +++++++++++++++++++
 .../SbsaQemuPlatformDxe.inf                   |  5 ++++
 Silicon/Qemu/SbsaQemu/SbsaQemu.dec            |  3 +++
 4 files changed, 38 insertions(+)

diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
index 0bd0df4f0239..b88729ad8ad6 100644
--- a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
+++ b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
@@ -558,6 +558,9 @@ DEFINE NETWORK_HTTP_BOOT_ENABLE       = FALSE
   gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisAssetTag|L"AT0000"
   gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisSKU|L"SK0000"
 
+  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMajor|0x0
+  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMinor|0x0
+
 ################################################################################
 #
 # Components Section - list of all EDK II Modules needed by this Platform
diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c
index b7270a07abbd..199766c7014a 100644
--- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c
+++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c
@@ -7,15 +7,25 @@
 *
 **/
 
+#include <Library/ArmSmcLib.h>
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
 #include <Library/NonDiscoverableDeviceRegistrationLib.h>
 #include <Library/PcdLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiDriverEntryPoint.h>
+#include <IndustryStandard/ArmStdSmc.h>
 
 #include <Protocol/FdtClient.h>
 
+/* those probably should go into IndustryStandard/ArmStdSmc.h */
+#define SMC_FASTCALL       0x80000000
+#define SMC64_FUNCTION     (SMC_FASTCALL   | 0x40000000)
+#define SIP_FUNCTION       (SMC64_FUNCTION | 0x02000000)
+#define SIP_FUNCTION_ID(n) (SIP_FUNCTION   | (n))
+
+#define SIP_SVC_VERSION  SIP_FUNCTION_ID(1)
+
 EFI_STATUS
 EFIAPI
 InitializeSbsaQemuPlatformDxe (
@@ -26,6 +36,9 @@ InitializeSbsaQemuPlatformDxe (
   EFI_STATUS                     Status;
   UINTN                          Size;
   VOID*                          Base;
+  UINTN                          Arg0;
+  UINTN                          Arg1;
+  UINTN                          Result;
 
   DEBUG ((DEBUG_INFO, "%a: InitializeSbsaQemuPlatformDxe called\n", __FUNCTION__));
 
@@ -51,5 +64,19 @@ InitializeSbsaQemuPlatformDxe (
     return Status;
   }
 
+  Result = ArmCallSmc0 (SIP_SVC_VERSION, &Arg0, &Arg1, NULL);
+  if (Result == SMC_ARCH_CALL_SUCCESS)
+  {
+        Result = PcdSet32S (PcdPlatformVersionMajor, Arg0);
+        ASSERT_EFI_ERROR (Result);
+        Result = PcdSet32S (PcdPlatformVersionMinor, Arg1);
+        ASSERT_EFI_ERROR (Result);
+  }
+
+  Arg0 = PcdGet32 (PcdPlatformVersionMajor);
+  Arg1 = PcdGet32 (PcdPlatformVersionMinor);
+
+  DEBUG ((DEBUG_INFO, "Platform version: %d.%d\n", Arg0, Arg1));
+
   return EFI_SUCCESS;
 }
diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf
index 21d2135f6d17..1f2c8a9dd6af 100644
--- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf
+++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf
@@ -20,6 +20,7 @@
   SbsaQemuPlatformDxe.c
 
 [Packages]
+  ArmPkg/ArmPkg.dec
   ArmVirtPkg/ArmVirtPkg.dec
   EmbeddedPkg/EmbeddedPkg.dec
   MdeModulePkg/MdeModulePkg.dec
@@ -27,6 +28,7 @@
   Silicon/Qemu/SbsaQemu/SbsaQemu.dec
 
 [LibraryClasses]
+  ArmSmcLib
   PcdLib
   DebugLib
   NonDiscoverableDeviceRegistrationLib
@@ -36,6 +38,9 @@
   gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformAhciBase
   gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformAhciSize
 
+  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMajor
+  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMinor
+
 [Depex]
   TRUE
 
diff --git a/Silicon/Qemu/SbsaQemu/SbsaQemu.dec b/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
index 9448852967b6..5182978cf56d 100644
--- a/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
+++ b/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
@@ -67,3 +67,6 @@
   gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisManufacturer|L""|VOID*|0x0000011B
   gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisAssetTag|L""|VOID*|0x0000011C
   gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisSKU|L""|VOID*|0x0000011D
+
+  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMajor|0x0|UINT32|0x0000011E
+  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMinor|0x0|UINT32|0x0000011F
-- 
2.40.1

[PATCH 2/2] Platform/SbsaQemu: read GIC base from TF-A

[Marcin Juszkiewicz]

Qemu has versioning for sbsa-ref platform. TF-A reads data from provided
DeviceTree and provides as SMC.

This change adds reading GIC base addresses into EDK2.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 .../SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c | 30 +++++++++++++++++++
 .../SbsaQemuPlatformDxe.inf                   |  4 +++
 2 files changed, 34 insertions(+)

diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c
index 199766c7014a..1213268519c8 100644
--- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c
+++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c
@@ -25,6 +25,8 @@
 #define SIP_FUNCTION_ID(n) (SIP_FUNCTION   | (n))
 
 #define SIP_SVC_VERSION  SIP_FUNCTION_ID(1)
+#define SIP_SVC_GET_GICD SIP_FUNCTION_ID(2)
+#define SIP_SVC_GET_GICR SIP_FUNCTION_ID(3)
 
 EFI_STATUS
 EFIAPI
@@ -78,5 +80,33 @@ InitializeSbsaQemuPlatformDxe (
 
   DEBUG ((DEBUG_INFO, "Platform version: %d.%d\n", Arg0, Arg1));
 
+  /* If we are on platform version 0.0 then there is no more data for us in DeviceTree */
+  if ((Arg0 == 0) & (Arg1 == 0))
+  {
+    return EFI_SUCCESS;
+  }
+
+  Result = ArmCallSmc0 (SIP_SVC_GET_GICD, &Arg0, NULL, NULL);
+  if (Result == SMC_ARCH_CALL_SUCCESS)
+  {
+        Result = PcdSet64S (PcdGicDistributorBase, Arg0);
+        ASSERT_EFI_ERROR (Result);
+  }
+
+  Arg0 = PcdGet64 (PcdGicDistributorBase);
+
+  DEBUG ((DEBUG_INFO, "GICD base: 0x%x\n", Arg0));
+
+  Result = ArmCallSmc0 (SIP_SVC_GET_GICR, &Arg0, NULL, NULL);
+  if (Result == SMC_ARCH_CALL_SUCCESS)
+  {
+        Result = PcdSet64S (PcdGicRedistributorsBase, Arg0);
+        ASSERT_EFI_ERROR (Result);
+  }
+
+  Arg0 = PcdGet64 (PcdGicRedistributorsBase);
+
+  DEBUG ((DEBUG_INFO, "GICR base: 0x%x\n", Arg0));
+
   return EFI_SUCCESS;
 }
diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf
index 1f2c8a9dd6af..545794a8c7ff 100644
--- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf
+++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf
@@ -41,6 +41,10 @@
   gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMajor
   gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMinor
 
+  gArmTokenSpaceGuid.PcdGicDistributorBase
+  gArmTokenSpaceGuid.PcdGicRedistributorsBase
+
+
 [Depex]
   TRUE
 
-- 
2.40.1

Re: [PATCH 1/2] Platform/SbsaQemu: read platform version

[Rebecca Cran]

On 5/15/23 04:24, Marcin Juszkiewicz wrote:
> +  Result = ArmCallSmc0 (SIP_SVC_VERSION, &Arg0, &Arg1, NULL);
> +  if (Result == SMC_ARCH_CALL_SUCCESS)
> +  {
> +        Result = PcdSet32S (PcdPlatformVersionMajor, Arg0);
> +        ASSERT_EFI_ERROR (Result);
> +        Result = PcdSet32S (PcdPlatformVersionMinor, Arg1);
> +        ASSERT_EFI_ERROR (Result);
> +  }
> +
> +  Arg0 = PcdGet32 (PcdPlatformVersionMajor);
> +  Arg1 = PcdGet32 (PcdPlatformVersionMinor);

I didn't notice this before, but PcdSet32S returns a RETURN_STATUS, not 
UINTN so Result should be type RETURN_STATUS (and probably a different 
variable used for the result of ArmCallSmc0).

Also, you should use ASSERT_RETURN_ERROR not ASSERT_EFI_ERROR (I checked 
ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c 
to see how error handling is done there).


-- 
Rebecca Cran

Re: [PATCH 2/2] Platform/SbsaQemu: read GIC base from TF-A

[Rebecca Cran]

On 5/15/23 04:24, Marcin Juszkiewicz wrote:

> +  /* If we are on platform version 0.0 then there is no more data for us in DeviceTree */
> +  if ((Arg0 == 0) & (Arg1 == 0))
> +  {
> +    return EFI_SUCCESS;
> +  }

Should that be "&&" instead of "&"?


-- 

Rebecca Cran

Re: [PATCH 2/2] Platform/SbsaQemu: read GIC base from TF-A

[Marcin Juszkiewicz]

W dniu 15.05.2023 o 15:14, Rebecca Cran pisze:
> On 5/15/23 04:24, Marcin Juszkiewicz wrote:
> 
>> +  /* If we are on platform version 0.0 then there is no more data for 
>> us in DeviceTree */
>> +  if ((Arg0 == 0) & (Arg1 == 0))
>> +  {
>> +    return EFI_SUCCESS;
>> +  }
> 
> Should that be "&&" instead of "&"?

Ops, yes. Always forgetting does C has "if ( (this) and (that) )" and 
then use wrong &/&& variant.

Re: [PATCH 1/2] Platform/SbsaQemu: read platform version

[Leif Lindholm]

On Mon, May 15, 2023 at 12:24:22 +0200, Marcin Juszkiewicz wrote:
> Qemu has versioning for sbsa-ref platform. TF-A reads it from provided
> DeviceTree and provides as SMC.
> 
> This change adds reading platform version into EDK2.
> 
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> ---
>  Platform/Qemu/SbsaQemu/SbsaQemu.dsc           |  3 +++
>  .../SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c | 27 +++++++++++++++++++
>  .../SbsaQemuPlatformDxe.inf                   |  5 ++++
>  Silicon/Qemu/SbsaQemu/SbsaQemu.dec            |  3 +++
>  4 files changed, 38 insertions(+)
> 
> diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> index 0bd0df4f0239..b88729ad8ad6 100644
> --- a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> @@ -558,6 +558,9 @@ DEFINE NETWORK_HTTP_BOOT_ENABLE       = FALSE
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisAssetTag|L"AT0000"
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisSKU|L"SK0000"
>  
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMajor|0x0
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMinor|0x0
> +
>  ################################################################################
>  #
>  # Components Section - list of all EDK II Modules needed by this Platform
> diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c
> index b7270a07abbd..199766c7014a 100644
> --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c
> +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c
> @@ -7,15 +7,25 @@
>  *
>  **/
>  
> +#include <Library/ArmSmcLib.h>
>  #include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/NonDiscoverableDeviceRegistrationLib.h>
>  #include <Library/PcdLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/UefiDriverEntryPoint.h>
> +#include <IndustryStandard/ArmStdSmc.h>
>  
>  #include <Protocol/FdtClient.h>
>  
> +/* those probably should go into IndustryStandard/ArmStdSmc.h */
> +#define SMC_FASTCALL       0x80000000
> +#define SMC64_FUNCTION     (SMC_FASTCALL   | 0x40000000)
> +#define SIP_FUNCTION       (SMC64_FUNCTION | 0x02000000)
> +#define SIP_FUNCTION_ID(n) (SIP_FUNCTION   | (n))

Can you break this out into a separate patch for ArmStdSmc.h?
We're out of the freeze, so can merge without delay.

But please add SMC_ prefix on the last two macros for that.

> +
> +#define SIP_SVC_VERSION  SIP_FUNCTION_ID(1)

Could you break this out into a separate include file?
I'd suggest
Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuSmc.h
(we'll be adding more calls there later).

/
    Leif

> +
>  EFI_STATUS
>  EFIAPI
>  InitializeSbsaQemuPlatformDxe (
> @@ -26,6 +36,9 @@ InitializeSbsaQemuPlatformDxe (
>    EFI_STATUS                     Status;
>    UINTN                          Size;
>    VOID*                          Base;
> +  UINTN                          Arg0;
> +  UINTN                          Arg1;
> +  UINTN                          Result;
>  
>    DEBUG ((DEBUG_INFO, "%a: InitializeSbsaQemuPlatformDxe called\n", __FUNCTION__));
>  
> @@ -51,5 +64,19 @@ InitializeSbsaQemuPlatformDxe (
>      return Status;
>    }
>  
> +  Result = ArmCallSmc0 (SIP_SVC_VERSION, &Arg0, &Arg1, NULL);
> +  if (Result == SMC_ARCH_CALL_SUCCESS)
> +  {
> +        Result = PcdSet32S (PcdPlatformVersionMajor, Arg0);
> +        ASSERT_EFI_ERROR (Result);
> +        Result = PcdSet32S (PcdPlatformVersionMinor, Arg1);
> +        ASSERT_EFI_ERROR (Result);
> +  }
> +
> +  Arg0 = PcdGet32 (PcdPlatformVersionMajor);
> +  Arg1 = PcdGet32 (PcdPlatformVersionMinor);
> +
> +  DEBUG ((DEBUG_INFO, "Platform version: %d.%d\n", Arg0, Arg1));
> +
>    return EFI_SUCCESS;
>  }
> diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf
> index 21d2135f6d17..1f2c8a9dd6af 100644
> --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf
> +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf
> @@ -20,6 +20,7 @@
>    SbsaQemuPlatformDxe.c
>  
>  [Packages]
> +  ArmPkg/ArmPkg.dec
>    ArmVirtPkg/ArmVirtPkg.dec
>    EmbeddedPkg/EmbeddedPkg.dec
>    MdeModulePkg/MdeModulePkg.dec
> @@ -27,6 +28,7 @@
>    Silicon/Qemu/SbsaQemu/SbsaQemu.dec
>  
>  [LibraryClasses]
> +  ArmSmcLib
>    PcdLib
>    DebugLib
>    NonDiscoverableDeviceRegistrationLib
> @@ -36,6 +38,9 @@
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformAhciBase
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformAhciSize
>  
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMajor
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMinor
> +
>  [Depex]
>    TRUE
>  
> diff --git a/Silicon/Qemu/SbsaQemu/SbsaQemu.dec b/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> index 9448852967b6..5182978cf56d 100644
> --- a/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> +++ b/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> @@ -67,3 +67,6 @@
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisManufacturer|L""|VOID*|0x0000011B
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisAssetTag|L""|VOID*|0x0000011C
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisSKU|L""|VOID*|0x0000011D
> +
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMajor|0x0|UINT32|0x0000011E
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMinor|0x0|UINT32|0x0000011F
> -- 
> 2.40.1
>