From: Alexander Graf <agraf@csgraf.de>

The SMCCC 1.3 spec section 5.2 says

  The Unknown SMC Function Identifier is a sign-extended value of (-1)
  that is returned in the R0, W0 or X0 registers. An implementation must
  return this error code when it receives:

    * An SMC or HVC call with an unknown Function Identifier
    * An SMC or HVC call for a removed Function Identifier
    * An SMC64/HVC64 call from AArch32 state

To comply with these statements, let's always return -1 when we encounter
an unknown HVC or SMC call.

Signed-off-by: Alexander Graf <agraf@csgraf.de>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/psci.c | 35 ++++++-----------------------------
 1 file changed, 6 insertions(+), 29 deletions(-)

diff --git a/target/arm/psci.c b/target/arm/psci.c
index 6709e280133..b279c0b9a45 100644
--- a/target/arm/psci.c
+++ b/target/arm/psci.c
@@ -27,15 +27,13 @@
 
 bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
 {
-    /* Return true if the r0/x0 value indicates a PSCI call and
-     * the exception type matches the configured PSCI conduit. This is
-     * called before the SMC/HVC instruction is executed, to decide whether
-     * we should treat it as a PSCI call or with the architecturally
+    /*
+     * Return true if the exception type matches the configured PSCI conduit.
+     * This is called before the SMC/HVC instruction is executed, to decide
+     * whether we should treat it as a PSCI call or with the architecturally
      * defined behaviour for an SMC or HVC (which might be UNDEF or trap
      * to EL2 or to EL3).
      */
-    CPUARMState *env = &cpu->env;
-    uint64_t param = is_a64(env) ? env->xregs[0] : env->regs[0];
 
     switch (excp_type) {
     case EXCP_HVC:
@@ -52,27 +50,7 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
         return false;
     }
 
-    switch (param) {
-    case QEMU_PSCI_0_2_FN_PSCI_VERSION:
-    case QEMU_PSCI_0_2_FN_MIGRATE_INFO_TYPE:
-    case QEMU_PSCI_0_2_FN_AFFINITY_INFO:
-    case QEMU_PSCI_0_2_FN64_AFFINITY_INFO:
-    case QEMU_PSCI_0_2_FN_SYSTEM_RESET:
-    case QEMU_PSCI_0_2_FN_SYSTEM_OFF:
-    case QEMU_PSCI_0_1_FN_CPU_ON:
-    case QEMU_PSCI_0_2_FN_CPU_ON:
-    case QEMU_PSCI_0_2_FN64_CPU_ON:
-    case QEMU_PSCI_0_1_FN_CPU_OFF:
-    case QEMU_PSCI_0_2_FN_CPU_OFF:
-    case QEMU_PSCI_0_1_FN_CPU_SUSPEND:
-    case QEMU_PSCI_0_2_FN_CPU_SUSPEND:
-    case QEMU_PSCI_0_2_FN64_CPU_SUSPEND:
-    case QEMU_PSCI_0_1_FN_MIGRATE:
-    case QEMU_PSCI_0_2_FN_MIGRATE:
-        return true;
-    default:
-        return false;
-    }
+    return true;
 }
 
 void arm_handle_psci_call(ARMCPU *cpu)
@@ -194,10 +172,9 @@ void arm_handle_psci_call(ARMCPU *cpu)
         break;
     case QEMU_PSCI_0_1_FN_MIGRATE:
     case QEMU_PSCI_0_2_FN_MIGRATE:
+    default:
         ret = QEMU_PSCI_RET_NOT_SUPPORTED;
         break;
-    default:
-        g_assert_not_reached();
     }
 
 err:
-- 
2.20.1

On Thu, 30 Sept 2021 at 16:12, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> From: Alexander Graf <agraf@csgraf.de>
>
> The SMCCC 1.3 spec section 5.2 says
>
>   The Unknown SMC Function Identifier is a sign-extended value of (-1)
>   that is returned in the R0, W0 or X0 registers. An implementation must
>   return this error code when it receives:
>
>     * An SMC or HVC call with an unknown Function Identifier
>     * An SMC or HVC call for a removed Function Identifier
>     * An SMC64/HVC64 call from AArch32 state
>
> To comply with these statements, let's always return -1 when we encounter
> an unknown HVC or SMC call.

TL/DR: I propose to revert this for 6.2.

This change turns out to cause regressions, for instance on the
imx6ul boards as described here:
https://lore.kernel.org/qemu-devel/c8b89685-7490-328b-51a3-48711c140a84@tribudubois.net/

The primary cause of that regression is that the guest code running
at EL3 expects SMCs (not related to PSCI) to do what they would if
our PSCI emulation was not present at all, but after this change
they instead set a value in R0/X0 and continue.

I had a look at fixing this, which involves deferring the "do we
want to enable PSCI emulation?" decision into the hw/arm/boot.c
code (which is the only place we finally figure out whether we're
going to be booting the guest into EL3 or not). I have some
more-or-less working prototype code, but in the course of writing
it I discovered a much harder to fix issue:

The highbank board both:
 (1) wants to enable PSCI emulation
 (2) has a bit of guest code that it wants to run at EL3 and
     to perform SMC calls that trap to the monitor vector table:
     this is the boot stub code that is written to memory by
     arm_write_secure_board_setup_dummy_smc() and which the
     highbank board enables by setting bootinfo->secure_board_setup

We can't satisfy both of those and also have the PSCI emulation
handle all SMC instruction executions regardless of function
identifier value.

There is probably a solution to this, but I'm not sure what it
is right now (it might involve having QEMU manually do the things
that we currently have the arm_write_secure_board_setup_dummy_smc
write guest code to do) and it's going to require digging through
what the highbank board actually is supposed to do here. Given
that we're already in the release cycle for 6.2, I think the
safest and simplest approach is to revert this patch for now,
which just takes us back to the behaviour we've always had
in previous releases. We can then take our time to figure out
how to clean up this mess in 7.0.

thanks
-- PMM

On 18.11.21 22:57, Peter Maydell wrote:
> On Thu, 30 Sept 2021 at 16:12, Peter Maydell <peter.maydell@linaro.org> wrote:
>> From: Alexander Graf <agraf@csgraf.de>
>>
>> The SMCCC 1.3 spec section 5.2 says
>>
>>    The Unknown SMC Function Identifier is a sign-extended value of (-1)
>>    that is returned in the R0, W0 or X0 registers. An implementation must
>>    return this error code when it receives:
>>
>>      * An SMC or HVC call with an unknown Function Identifier
>>      * An SMC or HVC call for a removed Function Identifier
>>      * An SMC64/HVC64 call from AArch32 state
>>
>> To comply with these statements, let's always return -1 when we encounter
>> an unknown HVC or SMC call.
> TL/DR: I propose to revert this for 6.2.
>
> This change turns out to cause regressions, for instance on the
> imx6ul boards as described here:
> https://lore.kernel.org/qemu-devel/c8b89685-7490-328b-51a3-48711c140a84@tribudubois.net/
>
> The primary cause of that regression is that the guest code running
> at EL3 expects SMCs (not related to PSCI) to do what they would if
> our PSCI emulation was not present at all, but after this change
> they instead set a value in R0/X0 and continue.
>
> I had a look at fixing this, which involves deferring the "do we
> want to enable PSCI emulation?" decision into the hw/arm/boot.c
> code (which is the only place we finally figure out whether we're
> going to be booting the guest into EL3 or not). I have some
> more-or-less working prototype code, but in the course of writing
> it I discovered a much harder to fix issue:
>
> The highbank board both:
>   (1) wants to enable PSCI emulation
>   (2) has a bit of guest code that it wants to run at EL3 and
>       to perform SMC calls that trap to the monitor vector table:
>       this is the boot stub code that is written to memory by
>       arm_write_secure_board_setup_dummy_smc() and which the
>       highbank board enables by setting bootinfo->secure_board_setup
>
> We can't satisfy both of those and also have the PSCI emulation
> handle all SMC instruction executions regardless of function
> identifier value.
>
> There is probably a solution to this, but I'm not sure what it
> is right now (it might involve having QEMU manually do the things
> that we currently have the arm_write_secure_board_setup_dummy_smc
> write guest code to do) and it's going to require digging through
> what the highbank board actually is supposed to do here. Given
> that we're already in the release cycle for 6.2, I think the
> safest and simplest approach is to revert this patch for now,
> which just takes us back to the behaviour we've always had
> in previous releases. We can then take our time to figure out
> how to clean up this mess in 7.0.


Ugh :(. Conceptually, once you tell QEMU to handle PSCI, you're 
basically giving up that EL to it. It sounds almost as if what these 
boards (imx6ul + highbank) want is an EL4 they can call into to deflect 
PSCI calls into from EL3 they own. We would basically have to allocate a 
currently undefinied/reserved instruction as "QEMU SMC" and make the EL3 
code call that when it needs to call QEMU for PSCI operations. Or a PV 
MMIO device. Or a PV sysreg. But at the end of the day, EL3 calling into 
QEMU differently than on real hardware is paravirtualization.

I agree with the conclusion that we revert it for QEMU 6.2 though. The 2 
guest OSs I'm aware of that rely on the behavior in the patch / spec 
(Windows and VMware ESXi) require more QEMU modifications to be fully 
functional: SMC as default conduit for Windows and EL3 PSR exposure for 
ESXi. So neither of them would work out of the box with 6.2 as is.

Just to double check: Is the broken monitor code that expects QEMU to 
partially handle SMCs only ever injected into the guest by us or is 
there existing guest payload code for EL3 that makes the same assumption?


Alex

On Fri, 19 Nov 2021 at 10:35, Alexander Graf <agraf@csgraf.de> wrote:
> Ugh :(. Conceptually, once you tell QEMU to handle PSCI, you're
> basically giving up that EL to it. It sounds almost as if what these
> boards (imx6ul + highbank) want is an EL4 they can call into to deflect
> PSCI calls into from EL3 they own. We would basically have to allocate a
> currently undefinied/reserved instruction as "QEMU SMC" and make the EL3
> code call that when it needs to call QEMU for PSCI operations. Or a PV
> MMIO device. Or a PV sysreg. But at the end of the day, EL3 calling into
> QEMU differently than on real hardware is paravirtualization.

I think in practice what they're doing is "we want an emulated
firmware that provides PSCI and also stubs out some other SMCs".
(My extremely vague recollection is that the SMC in question was
some kind of "flush cache" operation on the l2x0 cache controller,
which was secure-mode-access only.) So probably the eventual
answer is going to be "PSCI setting x0 is OK, get rid of the
infrastructure for setting up the do-nothing-SMC-handler. But it's
too complicated and needs too much archaeology on why we added
this code to be doable for 6.2.

> Just to double check: Is the broken monitor code that expects QEMU to
> partially handle SMCs only ever injected into the guest by us or is
> there existing guest payload code for EL3 that makes the same assumption?

We only write the boot stub code that uses SMC if we're booting a
Linux kernel (ie not running the guest at EL3). So for guest EL3
code the highbank board is in the same bucket as the other
affected boards (mcimx6ul-evk, mcimx7d-sabre, orangepi, xlnx-zcu102,
plus for EL3 code loaded via -kernel: virt and xlnx-versal-virt):
maybe somebody had EL3 guest code that was assuming that QEMU
provided PSCI, but that would be something that could only run
on QEMU's model, not on the real hardware. So I'm OK with
breaking that if it exists.

(In particular to get SMP to work on these boards
with EL3 guest code we need to model a power controller or some
other way to get the secondary CPUs to power up, unless the EL3
code expects the "all cores start the bios code simultaneously"
pattern.)

-- PMM