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

["Ard Biesheuvel" <ardb@kernel.org>]

On Tue, 4 Jun 2024 at 14:37, Marcin Juszkiewicz
<marcin.juszkiewicz@linaro.org> wrote:
>
> 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).
>

You shouldn't use X18 - it is the so-called 'platform register' and
has a special meaning.

For example, this code could be called inside a runtime service
invoked by the OS with interrupts enabled, and the OS's interrupt
handler might misbehave if X18 was modified.


> 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]!
>

This code does not honour the ordinary calling convention - better to
stack and unstack the frame pointer and return address. That also
gives you X30 (the link register) as a scratch register.

// Create a stack frame
stp  x29, x30, [sp, #-16]!
mov  x29, sp

// Preserve X0 for later use
mov  x30, x0

// Load the SMC arguments values into the appropriate registers
ldp   x0, x1, [x30, #0]
ldp   x2, x3, [x30, #16]
ldp   x4, x5, [x30, #32]
ldp   x6, x7, [x30, #48]
ldp   x8, x9, [x30, #64]
ldp   x10, x11, [x30, #80]
ldp   x12, x13, [x30, #96]
ldp   x14, x15, [x30, #112]
ldp   x16, x17, [x30, #128]

smc   #0

// A SMC call can return up to 18 values.
stp   x0, x1, [x30, #0]
stp   x2, x3, [x30, #16]
stp   x4, x5, [x30, #32]
stp   x6, x7, [x30, #48]
stp   x8, x9, [x30, #64]
stp   x10, x11, [x30, #80]
stp   x12, x13, [x30, #96]
stp   x14, x15, [x30, #112]
stp   x16, x17, [x30, #128]

mov   x0, x30
ldp   x29, x30, [sp], #16
ret


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119448): https://edk2.groups.io/g/devel/message/119448
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]
-=-=-=-=-=-=-=-=-=-=-=-