Re: [PATCH v11 07/10] hvf: arm: Implement PSCI handling

From: Peter Maydell <peter.maydell@linaro.org>
To: Alexander Graf <agraf@csgraf.de>
Cc: "Eduardo Habkost" <ehabkost@redhat.com>,
	"Sergio Lopez" <slp@redhat.com>,
	"Peter Collingbourne" <pcc@google.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Cameron Esfahani" <dirty@apple.com>,
	"Roman Bolshakov" <r.bolshakov@yadro.com>,
	qemu-arm <qemu-arm@nongnu.org>, "Frank Yang" <lfy@google.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: Re: [PATCH v11 07/10] hvf: arm: Implement PSCI handling
Date: Thu, 16 Sep 2021 13:27:25 +0100	[thread overview]
Message-ID: <CAFEAcA9JWdh_y4SPtnNOdywLkyN-L4vAqfic_LsxtgCgdTSyPQ@mail.gmail.com> (raw)
In-Reply-To: <20210915181049.27597-8-agraf@csgraf.de>

On Wed, 15 Sept 2021 at 19:10, Alexander Graf <agraf@csgraf.de> wrote:
>
> We need to handle PSCI calls. Most of the TCG code works for us,
> but we can simplify it to only handle aa64 mode and we need to
> handle SUSPEND differently.
>
> This patch takes the TCG code as template and duplicates it in HVF.
>
> To tell the guest that we support PSCI 0.2 now, update the check in
> arm_cpu_initfn() as well.
>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> Reviewed-by: Sergio Lopez <slp@redhat.com>

> +    case QEMU_PSCI_0_2_FN_SYSTEM_RESET:
> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +        /* QEMU reset and shutdown are async requests, but PSCI
> +         * mandates that we never return from the reset/shutdown
> +         * call, so power the CPU off now so it doesn't execute
> +         * anything further.
> +         */

QEMU coding standard wants the opening "/*" of a block comment on its
own line.

> @@ -898,14 +1011,29 @@ int hvf_vcpu_exec(CPUState *cpu)
>          break;
>      case EC_AA64_HVC:
>          cpu_synchronize_state(cpu);
> -        trace_hvf_unknown_hvf(env->xregs[0]);
> -        /* SMCCC 1.3 section 5.2 says every unknown SMCCC call returns -1 */
> -        env->xregs[0] = -1;
> +        if ((arm_cpu->psci_conduit != QEMU_PSCI_CONDUIT_HVC) ||
> +            !hvf_handle_psci_call(cpu)) {
> +            trace_hvf_unknown_hvf(env->xregs[0]);
> +            /* SMCCC 1.3 section 5.2 says every unknown SMCCC call returns -1 */
> +            env->xregs[0] = -1;
> +        }
>          break;
>      case EC_AA64_SMC:
>          cpu_synchronize_state(cpu);
> -        trace_hvf_unknown_smc(env->xregs[0]);
> -        hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized());
> +
> +        if (arm_cpu->psci_conduit == QEMU_PSCI_CONDUIT_SMC) {
> +            advance_pc = true;
> +
> +            if (!hvf_handle_psci_call(cpu)) {
> +                trace_hvf_unknown_smc(env->xregs[0]);
> +                /* SMCCC 1.3 section 5.2 says every unknown SMCCC call returns -1 */
> +                env->xregs[0] = -1;
> +            }
> +        } else {
> +            trace_hvf_unknown_smc(env->xregs[0]);
> +            hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized());
> +        }

We seem to have ended up with an odd asymmetry in the way the
code for HVC and SMC has been structured.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM