Re: [Qemu-devel] [PATCH] arm: Fix SMC reporting to EL2 when QEMU provides PSCI

[Jan Kiszka]

On 2017-09-21 18:12, Peter Maydell wrote:
> On 18 September 2017 at 08:51, Jan Kiszka <address@hidden> wrote:
>> From: Jan Kiszka <address@hidden>
>>
>> This properly forwards SMC events to EL2 when PSCI is provided by QEMU
>> itself and, thus, ARM_FEATURE_EL3 is off.
>>
>> Found and tested with the Jailhouse hypervisor.
>>
>> Signed-off-by: Jan Kiszka <address@hidden>
>> ---
>>  target/arm/helper.c    | 2 +-
>>  target/arm/op_helper.c | 8 ++++----
>>  target/arm/psci.c      | 6 ++++++
>>  3 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 4f41841ef6..8c3929762c 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -3717,7 +3717,7 @@ static void hcr_write(CPUARMState *env, const 
>> ARMCPRegInfo *ri, uint64_t value)
>>
>>      if (arm_feature(env, ARM_FEATURE_EL3)) {
>>          valid_mask &= ~HCR_HCD;
>> -    } else {
>> +    } else if (cpu->psci_conduit != QEMU_PSCI_CONDUIT_SMC) {
>>          valid_mask &= ~HCR_TSC;
> 
> This looks odd, so it needs a comment I think.
>   /* Architecturally HCR.TSC is RES0 if EL3 is not implemented.
>    * However, if we're using the SMC PSCI conduit then QEMU is
>    * effectively acting like EL3 firmware and so the guest at
>    * EL2 should retain the ability to prevent EL1 from being
>    * able to make SMC calls into the ersatz firmware, so in
>    * that case HCR.TSC should be read/write.
>    */
> 

Ack.

>>      }
>>
>> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
>> index 6a60464ab9..4b0ef6a234 100644
>> --- a/target/arm/op_helper.c
>> +++ b/target/arm/op_helper.c
>> @@ -960,12 +960,12 @@ void HELPER(pre_smc)(CPUARMState *env, uint32_t 
>> syndrome)
>>          return;
>>      }
>>
>> -    if (!arm_feature(env, ARM_FEATURE_EL3)) {
>> -        /* If we have no EL3 then SMC always UNDEFs */
>> -        undef = true;
>> -    } else if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) {
>> +    if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) {
>>          /* In NS EL1, HCR controlled routing to EL2 has priority over SMD. 
>> */
>>          raise_exception(env, EXCP_HYP_TRAP, syndrome, 2);
>> +    } else if (!arm_feature(env, ARM_FEATURE_EL3)) {
>> +        /* If we have no EL3 then SMC always UNDEFs */
>> +        undef = true;
>>      }
>>
>>      if (undef) {
> 
> I think the logic in this function should be something like:
> 
>     if (!arm_feature(env, ARM_FEATURE_EL3) &&
>         cpu->psci_conduit != QEMU_PSCI_CONDUIT_SMC)) {
>         /* If we have no EL3 then SMC always UNDEFs and can't be
>          * trapped to EL2. PSCI-via-SMC is a sort of ersatz EL3
>          * firmware within QEMU, and we want an EL2 guest to be able
>          * to forbid its EL1 from making PSCI calls into QEMU's
>          * "firmware" via HCR.TSC, so for these purposes treat
>          *  PSCI-via-SMC as implying an EL3.
>          */
>         undef = true;
>     else if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) {
>         /* In NS EL1, HCR controlled routing to EL2 has priority over SMD.
>          * We also want an EL2 guest to be able to forbid its EL1 from
>          * making PSCI calls into QEMU's "firmware" via HCR.TSC.
>          */
>         raise_exception(env, EXCP_HYP_TRAP, syndrome, 2);
>     }
> 
>     if (undef && !arm_is_psci_call(cpu, EXCP_SMC)) {
>         /* If PSCI is enabled and this looks like a valid PSCI call then
>          * suppress the UNDEF -- we'll catch the SMC exception and
>          * implement the PSCI call behaviour there.
>          */
>         raise_exception(env, EXCP_UDEF, syn_uncategorized(),
>                         exception_target_el(env));
>     }
> 
> (Totally untested!)

I'll try this.

> 
>> diff --git a/target/arm/psci.c b/target/arm/psci.c
>> index fc34b263d3..637987ff46 100644
>> --- a/target/arm/psci.c
>> +++ b/target/arm/psci.c
>> @@ -35,6 +35,8 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
>>       */
>>      CPUARMState *env = &cpu->env;
>>      uint64_t param = is_a64(env) ? env->xregs[0] : env->regs[0];
>> +    int cur_el = arm_current_el(env);
>> +    bool secure = arm_is_secure(env);
>>
>>      switch (excp_type) {
>>      case EXCP_HVC:
>> @@ -46,6 +48,10 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
>>          if (cpu->psci_conduit != QEMU_PSCI_CONDUIT_SMC) {
>>              return false;
>>          }
>> +        if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) {
>> +            /* The EL2 will handle this. */
>> +            return false;
>> +        }
> 
> I don't think we should need to change this function -- its
> purpose is "does this look like a PSCI call", and if it's
> an SMC exception with the right magic parameters, then it
> does look like a PSCI call. Instead we should make the
> pre_smc helper choose "raise a hyp trap" if that's the right
> thing to be doing (see comment above for what I think is the
> right logic there).

If we remove this filter, we will have to patch arm_cpu_do_interrupt in
addition IIRC. I once had a version which had a similar ordering in
pre_smc as above, but it still didn't deliver the call to EL2.

BTW, my feeling is that things are not completely correct for the HVC
case as well, at least the ordering of checks. But I didn't try that yet.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux