Re: [edk2-devel] ArmCallSmc() and SMCCC specification

["Marcin Juszkiewicz" <marcin.juszkiewicz@linaro.org>]

W dniu 3.06.2024 o 18:47, Leif Lindholm via groups.io pisze:

>> In 2020 we got version C of spec (and then D, E, F) which allows to 
>> use more registers:
>>
>>  > Allow R4—R7 (SMC32/HVC32) to be used as result registers.
>>  > Allow X8—X17 to be used as parameter registers in SMC64/HVC64.
>>  > Allow X4—X17 to be used as result registers in SMC64/HVC64.
>>
>> And I started to wonder how to update EDK2 to newer version of SMCCC 
>> spec as one of in-progress QemuSbsa SMC calls may return more than 4 
>> values.
> 
> Yes, definitely. This has been a wishlist item for some time, but in 
> reality we've only ever updated these when we needed new functionality.



>> ARM_SMC_ARGS in ArmSmcLib.h can be expanded to handle up to Arg17 in 
>> an easy way and guarded by "#if defined(__aarch64__)" to not change it 
>> on Arm32.

My example code for it (including comment update):

diff --git a/ArmPkg/Include/Library/ArmSmcLib.h b/ArmPkg/Include/Library/ArmSmcLib.h
index beef0175c35c..af27781b72f6 100644
--- a/ArmPkg/Include/Library/ArmSmcLib.h
+++ b/ArmPkg/Include/Library/ArmSmcLib.h
@@ -23,14 +23,31 @@ typedef struct {
    UINTN    Arg5;
    UINTN    Arg6;
    UINTN    Arg7;
+#if defined(__aarch64__)
+  UINTN    Arg8;
+  UINTN    Arg9;
+  UINTN    Arg10;
+  UINTN    Arg11;
+  UINTN    Arg12;
+  UINTN    Arg13;
+  UINTN    Arg14;
+  UINTN    Arg15;
+  UINTN    Arg16;
+  UINTN    Arg17;
+#endif
  } ARM_SMC_ARGS;

  /**
    Trigger an SMC call

-  SMC calls can take up to 7 arguments and return up to 4 return values.
-  Therefore, the 4 first fields in the ARM_SMC_ARGS structure are used
-  for both input and output values.
+  on SMC32/HVC32
+  - R0 is a function identifier, R1-R7 are arguments
+  - R0-R7 are results, R4-R7 must be preserved unless they contain results
+
+
+  on SMC64/HVC64
+  - W0 is a function identifier, X1-X17 are arguments
+  - X0-X17 are results, X4-X17 must be preserved unless they contain results

  **/
  VOID


>> Then ArmCallSmc() in {AArch64,Arm}/ArmSmc.S needs changes. But here it 
>> gets tricky.
>>
>> On Arm we preserve r4-r8 and restore them after call like spec says. 
>> Which we do not do on AArch64 as version B of spec did not required 
>> that (and this changed in version C).
>>
>> If we start handling more than 4 results then we need to know how many 
>> results are expected and restore rest of r4-r7/x4-x17 registers:
> 
> Yeah, it feels like we may want to add a version field or/and number of 
> registers to save/restore to a new struct. And we should definitely 
> align Arm/AArch64 behaviour.




>>  From what I saw in both edk2/ and edk2-platforms/ most of code uses 
>> ArmCallSmc() function with ARM_SMC_ARGS structure populared with 
>> arguments. ArmCallSmc[0-3]() are used by Smbios, Psci and QemuSbsa 
>> code only.
> 
> The ArmCallSmc[0-3] helpers were only added as shorthand for most common 
> cases. I don't think we should worry about how those work for making any 
> design decisions. Everything they do can be described by the main 
> ArmCallSmc functions and a few lines of struct stuffing.

>> Now the question is: how to handle change?
> 
> It could be that it would be easiest to add a new call function, and 
> maybe even ra new egister struct, than trying to adapt the existing ones 
> in ways that doesn't break existing users?
> 
> That is if we care about existing users. We could always modify it to 
> guarantee breakage and expect users to update their code. Since this is 
> a library, and not a protocol, we *mostly* don't need to worry about 
> users mixing prebuilt binaries using old structs with new functions.

>> We could add ArmCallSmc[4-17] but that name only tells how many 
>> arguments we pass to SMC call, not how many results we expect. Or 
>> should we add NumberOfResults argument to ArmCallSmc() to know which 
>> registers we should preserve and which are results? And how 
>> complicated this assembly function will become?
> 
> I don't think the assembly function needs to be that complicated. It'd 
> be a bit tedious with a bunch of tests, but...

Note: I do not know aarch64 assembly. Did some changes to ArmCallSmc().
I moved to use x18 register (preserved on stack) instead of x9 one. And
changed code to handle 8 results (without preserving x4-x7).

diff --git a/ArmPkg/Library/ArmSmcLib/AArch64/ArmSmc.S b/ArmPkg/Library/ArmSmcLib/AArch64/ArmSmc.S
index 4a8c2a8f59ea..0525a0a7887f 100644
--- a/ArmPkg/Library/ArmSmcLib/AArch64/ArmSmc.S
+++ b/ArmPkg/Library/ArmSmcLib/AArch64/ArmSmc.S
@@ -8,8 +8,10 @@
  #include <AsmMacroIoLibV8.h>

  ASM_FUNC(ArmCallSmc)
+  // preserve x18
+  str   x18, [sp, #-16]!
    // Push x0 on the stack - The stack must always be quad-word aligned
-  str   x0, [sp, #-16]!
+  str   x0, [sp, #-32]!

    // Load the SMC arguments values into the appropriate registers
    ldp   x6, x7, [x0, #48]
@@ -19,14 +21,18 @@ ASM_FUNC(ArmCallSmc)

    smc   #0

-  // Pop the ARM_SMC_ARGS structure address from the stack into x9
-  ldr   x9, [sp], #16
+  // Pop the ARM_SMC_ARGS structure address from the stack into x18
+  ldr   x18, [sp], #32

    // Store the SMC returned values into the ARM_SMC_ARGS structure.
-  // A SMC call can return up to 4 values - we do not need to store back x4-x7.
-  stp   x2, x3, [x9, #16]
-  stp   x0, x1, [x9, #0]
+  // A SMC call can return up to 18 values, let handle 8 for now
+  stp   x6, x7, [x18, #48]
+  stp   x4, x5, [x18, #32]
+  stp   x2, x3, [x18, #16]
+  stp   x0, x1, [x18, #0]

-  mov   x0, x9
+  mov   x0, x18
+  // restore x18
+  ldr   x18, [sp], #16

    ret


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119446): https://edk2.groups.io/g/devel/message/119446
Mute This Topic: https://groups.io/mt/106403741/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-