On Tue, 4 Jun 2024 at 14:37, Marcin Juszkiewicz
<marcin.juszkiew...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
