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>
---
v6 -> v7:
- This patch integrates "arm: Set PSCI to 0.2 for HVF"
v7 -> v8:
- Do not advance for HVC, PC is already updated by hvf
- Fix checkpatch error
---
target/arm/cpu.c | 4 +-
target/arm/hvf/hvf.c | 123 ++++++++++++++++++++++++++++++++++--
target/arm/hvf/trace-events | 1 +
3 files changed, 122 insertions(+), 6 deletions(-)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 762d8a6d26..b202d06e09 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1079,8 +1079,8 @@ static void arm_cpu_initfn(Object *obj)
cpu->psci_version = 1; /* By default assume PSCI v0.1 */
cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE;
- if (tcg_enabled()) {
- cpu->psci_version = 2; /* TCG implements PSCI 0.2 */
+ if (tcg_enabled() || hvf_enabled()) {
+ cpu->psci_version = 2; /* TCG and HVF implement PSCI 0.2 */
}
}
diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index bce46f3ed8..65c33e2a14 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -25,6 +25,7 @@
#include "hw/irq.h"
#include "qemu/main-loop.h"
#include "sysemu/cpus.h"
+#include "arm-powerctl.h"
#include "target/arm/cpu.h"
#include "target/arm/internals.h"
#include "trace/trace-target_arm_hvf.h"
@@ -45,6 +46,8 @@
#define TMR_CTL_IMASK (1 << 1)
#define TMR_CTL_ISTATUS (1 << 2)
+static void hvf_wfi(CPUState *cpu);
+
typedef struct ARMHostCPUFeatures {
ARMISARegisters isar;
uint64_t features;
@@ -553,6 +556,110 @@ static void hvf_raise_exception(CPUARMState *env, uint32_t excp,
env->pc = addr;
}
+static int hvf_psci_cpu_off(ARMCPU *arm_cpu)
+{
+ int32_t ret = 0;
+ ret = arm_set_cpu_off(arm_cpu->mp_affinity);
+ assert(ret == QEMU_ARM_POWERCTL_RET_SUCCESS);
+
+ return 0;
+}
+
+static int hvf_handle_psci_call(CPUState *cpu)
+{
+ ARMCPU *arm_cpu = ARM_CPU(cpu);
+ CPUARMState *env = &arm_cpu->env;
+ uint64_t param[4] = {
+ env->xregs[0],
+ env->xregs[1],
+ env->xregs[2],
+ env->xregs[3]
+ };
+ uint64_t context_id, mpidr;
+ bool target_aarch64 = true;
+ CPUState *target_cpu_state;
+ ARMCPU *target_cpu;
+ target_ulong entry;
+ int target_el = 1;
+ int32_t ret = 0;
+
+ trace_hvf_psci_call(param[0], param[1], param[2], param[3],
+ arm_cpu->mp_affinity);
+
+ switch (param[0]) {
+ case QEMU_PSCI_0_2_FN_PSCI_VERSION:
+ ret = QEMU_PSCI_0_2_RET_VERSION_0_2;
+ break;
+ case QEMU_PSCI_0_2_FN_MIGRATE_INFO_TYPE:
+ ret = QEMU_PSCI_0_2_RET_TOS_MIGRATION_NOT_REQUIRED; /* No trusted OS */
+ break;
+ case QEMU_PSCI_0_2_FN_AFFINITY_INFO:
+ case QEMU_PSCI_0_2_FN64_AFFINITY_INFO:
+ mpidr = param[1];
+
+ switch (param[2]) {
+ case 0:
+ target_cpu_state = arm_get_cpu_by_id(mpidr);
+ if (!target_cpu_state) {
+ ret = QEMU_PSCI_RET_INVALID_PARAMS;
+ break;
+ }
+ target_cpu = ARM_CPU(target_cpu_state);
+
+ ret = target_cpu->power_state;
+ break;
+ default:
+ /* Everything above affinity level 0 is always on. */
+ ret = 0;
+ }
+ break;
+ 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.
+ */
+ return hvf_psci_cpu_off(arm_cpu);
+ case QEMU_PSCI_0_2_FN_SYSTEM_OFF:
+ qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+ return hvf_psci_cpu_off(arm_cpu);
+ case QEMU_PSCI_0_1_FN_CPU_ON:
+ case QEMU_PSCI_0_2_FN_CPU_ON:
+ case QEMU_PSCI_0_2_FN64_CPU_ON:
+ mpidr = param[1];
+ entry = param[2];
+ context_id = param[3];
+ ret = arm_set_cpu_on(mpidr, entry, context_id,
+ target_el, target_aarch64);
+ break;
+ case QEMU_PSCI_0_1_FN_CPU_OFF:
+ case QEMU_PSCI_0_2_FN_CPU_OFF:
+ return hvf_psci_cpu_off(arm_cpu);
+ case QEMU_PSCI_0_1_FN_CPU_SUSPEND:
+ case QEMU_PSCI_0_2_FN_CPU_SUSPEND:
+ case QEMU_PSCI_0_2_FN64_CPU_SUSPEND:
+ /* Affinity levels are not supported in QEMU */
+ if (param[1] & 0xfffe0000) {
+ ret = QEMU_PSCI_RET_INVALID_PARAMS;
+ break;
+ }
+ /* Powerdown is not supported, we always go into WFI */
+ env->xregs[0] = 0;
+ hvf_wfi(cpu);
+ break;
+ case QEMU_PSCI_0_1_FN_MIGRATE:
+ case QEMU_PSCI_0_2_FN_MIGRATE:
+ ret = QEMU_PSCI_RET_NOT_SUPPORTED;
+ break;
+ default:
+ return 1;
+ }
+
+ env->xregs[0] = ret;
+ return 0;
+}
+
static uint64_t hvf_sysreg_read(CPUState *cpu, uint32_t reg)
{
ARMCPU *arm_cpu = ARM_CPU(cpu);
@@ -716,6 +823,8 @@ int hvf_vcpu_exec(CPUState *cpu)
}
if (cpu->halted) {
+ /* On unhalt, we usually have CPU state changes. Prepare for them. */
+ cpu_synchronize_state(cpu);
return EXCP_HLT;
}
@@ -813,13 +922,19 @@ int hvf_vcpu_exec(CPUState *cpu)
break;
case EC_AA64_HVC:
cpu_synchronize_state(cpu);
- trace_hvf_unknown_hvf(env->xregs[0]);
- hvf_raise_exception(env, EXCP_UDEF, syn_uncategorized());
+ if (hvf_handle_psci_call(cpu)) {
+ trace_hvf_unknown_hvf(env->xregs[0]);
+ hvf_raise_exception(env, EXCP_UDEF, syn_uncategorized());
+ }
break;
case EC_AA64_SMC:
cpu_synchronize_state(cpu);
- trace_hvf_unknown_smc(env->xregs[0]);
- hvf_raise_exception(env, EXCP_UDEF, syn_uncategorized());
+ if (!hvf_handle_psci_call(cpu)) {
+ advance_pc = true;
+ } else {
+ trace_hvf_unknown_smc(env->xregs[0]);
+ hvf_raise_exception(env, EXCP_UDEF, syn_uncategorized());
+ }
break;
default:
cpu_synchronize_state(cpu);
diff --git a/target/arm/hvf/trace-events b/target/arm/hvf/trace-events
index 49a547dcf6..278b88cc62 100644
--- a/target/arm/hvf/trace-events
+++ b/target/arm/hvf/trace-events
@@ -8,3 +8,4 @@ hvf_sysreg_write(uint32_t reg, uint32_t op0, uint32_t op1, uint32_t crn, uint32_
hvf_unknown_hvf(uint64_t x0) "unknown HVC! 0x%016"PRIx64
hvf_unknown_smc(uint64_t x0) "unknown SMC! 0x%016"PRIx64
hvf_exit(uint64_t syndrome, uint32_t ec, uint64_t pc) "exit: 0x%"PRIx64" [ec=0x%x pc=0x%"PRIx64"]"
+hvf_psci_call(uint64_t x0, uint64_t x1, uint64_t x2, uint64_t x3, uint32_t cpuid) "PSCI Call x0=0x%016"PRIx64" x1=0x%016"PRIx64" x2=0x%016"PRIx64" x3=0x%016"PRIx64" cpu=0x%x"
--
2.30.1 (Apple Git-130)
On Wed, May 19, 2021 at 10:22:50PM +0200, Alexander Graf 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> > > --- > > v6 -> v7: > > - This patch integrates "arm: Set PSCI to 0.2 for HVF" > > v7 -> v8: > > - Do not advance for HVC, PC is already updated by hvf > - Fix checkpatch error > --- > target/arm/cpu.c | 4 +- > target/arm/hvf/hvf.c | 123 ++++++++++++++++++++++++++++++++++-- > target/arm/hvf/trace-events | 1 + > 3 files changed, 122 insertions(+), 6 deletions(-) Reviewed-by: Sergio Lopez <slp@redhat.com>
On Wed, 19 May 2021 at 21:23, 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>
>
> ---
>
> v6 -> v7:
>
> - This patch integrates "arm: Set PSCI to 0.2 for HVF"
>
> v7 -> v8:
>
> - Do not advance for HVC, PC is already updated by hvf
> - Fix checkpatch error
> +static int hvf_psci_cpu_off(ARMCPU *arm_cpu)
> +{
> + int32_t ret = 0;
> + ret = arm_set_cpu_off(arm_cpu->mp_affinity);
> + assert(ret == QEMU_ARM_POWERCTL_RET_SUCCESS);
> +
> + return 0;
If you're always returning 0 you might as well just make
it return void.
> +}
> +
> +static int hvf_handle_psci_call(CPUState *cpu)
I think the callsites would be clearer if you made the function
return true for "PSCI call handled", false for "not recognised,
give the guest an UNDEF". Code like
if (hvf_handle_psci_call(cpu)) {
stuff;
}
looks like the 'stuff' is for the "psci call handled" case,
which at the moment it isn't.
Either way, a comment for this function describing what its
return value semantics are would be useful.
> + ARMCPU *arm_cpu = ARM_CPU(cpu);
> + CPUARMState *env = &arm_cpu->env;
> + uint64_t param[4] = {
> + env->xregs[0],
> + env->xregs[1],
> + env->xregs[2],
> + env->xregs[3]
> + };
> + uint64_t context_id, mpidr;
> + bool target_aarch64 = true;
> + CPUState *target_cpu_state;
> + ARMCPU *target_cpu;
> + target_ulong entry;
> + int target_el = 1;
> + int32_t ret = 0;
> +
> + trace_hvf_psci_call(param[0], param[1], param[2], param[3],
> + arm_cpu->mp_affinity);
> +
> + switch (param[0]) {
> + case QEMU_PSCI_0_2_FN_PSCI_VERSION:
> + ret = QEMU_PSCI_0_2_RET_VERSION_0_2;
> + break;
> + case QEMU_PSCI_0_2_FN_MIGRATE_INFO_TYPE:
> + ret = QEMU_PSCI_0_2_RET_TOS_MIGRATION_NOT_REQUIRED; /* No trusted OS */
> + break;
> + case QEMU_PSCI_0_2_FN_AFFINITY_INFO:
> + case QEMU_PSCI_0_2_FN64_AFFINITY_INFO:
> + mpidr = param[1];
> +
> + switch (param[2]) {
> + case 0:
> + target_cpu_state = arm_get_cpu_by_id(mpidr);
> + if (!target_cpu_state) {
> + ret = QEMU_PSCI_RET_INVALID_PARAMS;
> + break;
> + }
> + target_cpu = ARM_CPU(target_cpu_state);
> +
> + ret = target_cpu->power_state;
> + break;
> + default:
> + /* Everything above affinity level 0 is always on. */
> + ret = 0;
> + }
> + break;
> + 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.
> + */
> + return hvf_psci_cpu_off(arm_cpu);
> + case QEMU_PSCI_0_2_FN_SYSTEM_OFF:
> + qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> + return hvf_psci_cpu_off(arm_cpu);
> + case QEMU_PSCI_0_1_FN_CPU_ON:
> + case QEMU_PSCI_0_2_FN_CPU_ON:
> + case QEMU_PSCI_0_2_FN64_CPU_ON:
> + mpidr = param[1];
> + entry = param[2];
> + context_id = param[3];
> + ret = arm_set_cpu_on(mpidr, entry, context_id,
> + target_el, target_aarch64);
> + break;
> + case QEMU_PSCI_0_1_FN_CPU_OFF:
> + case QEMU_PSCI_0_2_FN_CPU_OFF:
> + return hvf_psci_cpu_off(arm_cpu);
> + case QEMU_PSCI_0_1_FN_CPU_SUSPEND:
> + case QEMU_PSCI_0_2_FN_CPU_SUSPEND:
> + case QEMU_PSCI_0_2_FN64_CPU_SUSPEND:
> + /* Affinity levels are not supported in QEMU */
> + if (param[1] & 0xfffe0000) {
> + ret = QEMU_PSCI_RET_INVALID_PARAMS;
> + break;
> + }
> + /* Powerdown is not supported, we always go into WFI */
> + env->xregs[0] = 0;
> + hvf_wfi(cpu);
> + break;
> + case QEMU_PSCI_0_1_FN_MIGRATE:
> + case QEMU_PSCI_0_2_FN_MIGRATE:
> + ret = QEMU_PSCI_RET_NOT_SUPPORTED;
> + break;
> + default:
> + return 1;
> + }
> +
> + env->xregs[0] = ret;
> + return 0;
> +}
> +
> static uint64_t hvf_sysreg_read(CPUState *cpu, uint32_t reg)
> {
> ARMCPU *arm_cpu = ARM_CPU(cpu);
> @@ -716,6 +823,8 @@ int hvf_vcpu_exec(CPUState *cpu)
> }
>
> if (cpu->halted) {
> + /* On unhalt, we usually have CPU state changes. Prepare for them. */
> + cpu_synchronize_state(cpu);
> return EXCP_HLT;
> }
This seems odd. If I understand the control flow correctly, this
is neither:
(a) just before we're about to emulate a PSCI call so we need
the current values of the registers from hvf
(b) when we've just changed the CPU registers because we made
a PSCI call and we want to push them back to hvf
(c) when we're about to resume the CPU because it's gone from
halted to not-halted, which might also be a good time to push
register state back to hvf
So what's it for ?
My expectation would be that we would ensure the state is in sync
(ie that hvf has any local changes) before we call hv_cpu_run(),
and that we would go the other way before we access any local
register CPU struct state.
thanks
-- PMM
On 15.06.21 14:54, Peter Maydell wrote:
> On Wed, 19 May 2021 at 21:23, 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>
>>
>> ---
>>
>> v6 -> v7:
>>
>> - This patch integrates "arm: Set PSCI to 0.2 for HVF"
>>
>> v7 -> v8:
>>
>> - Do not advance for HVC, PC is already updated by hvf
>> - Fix checkpatch error
>
>> +static int /(ARMCPU *arm_cpu)
>> +{
>> + int32_t ret = 0;
>> + ret = arm_set_cpu_off(arm_cpu->mp_affinity);
>> + assert(ret == QEMU_ARM_POWERCTL_RET_SUCCESS);
>> +
>> + return 0;
> If you're always returning 0 you might as well just make
> it return void.
>
>> +}
>> +
>> +static int hvf_handle_psci_call(CPUState *cpu)
> I think the callsites would be clearer if you made the function
> return true for "PSCI call handled", false for "not recognised,
> give the guest an UNDEF". Code like
> if (hvf_handle_psci_call(cpu)) {
> stuff;
> }
>
> looks like the 'stuff' is for the "psci call handled" case,
> which at the moment it isn't.
This function merely follows standard C semantics along the lines of "0
means success, !0 is error". Isn't that what you would usually expect?
>
> Either way, a comment for this function describing what its
> return value semantics are would be useful.
Sure, I can add one :)
>
>> + ARMCPU *arm_cpu = ARM_CPU(cpu);
>> + CPUARMState *env = &arm_cpu->env;
>> + uint64_t param[4] = {
>> + env->xregs[0],
>> + env->xregs[1],
>> + env->xregs[2],
>> + env->xregs[3]
>> + };
>> + uint64_t context_id, mpidr;
>> + bool target_aarch64 = true;
>> + CPUState *target_cpu_state;
>> + ARMCPU *target_cpu;
>> + target_ulong entry;
>> + int target_el = 1;
>> + int32_t ret = 0;
>> +
>> + trace_hvf_psci_call(param[0], param[1], param[2], param[3],
>> + arm_cpu->mp_affinity);
>> +
>> + switch (param[0]) {
>> + case QEMU_PSCI_0_2_FN_PSCI_VERSION:
>> + ret = QEMU_PSCI_0_2_RET_VERSION_0_2;
>> + break;
>> + case QEMU_PSCI_0_2_FN_MIGRATE_INFO_TYPE:
>> + ret = QEMU_PSCI_0_2_RET_TOS_MIGRATION_NOT_REQUIRED; /* No trusted OS */
>> + break;
>> + case QEMU_PSCI_0_2_FN_AFFINITY_INFO:
>> + case QEMU_PSCI_0_2_FN64_AFFINITY_INFO:
>> + mpidr = param[1];
>> +
>> + switch (param[2]) {
>> + case 0:
>> + target_cpu_state = arm_get_cpu_by_id(mpidr);
>> + if (!target_cpu_state) {
>> + ret = QEMU_PSCI_RET_INVALID_PARAMS;
>> + break;
>> + }
>> + target_cpu = ARM_CPU(target_cpu_state);
>> +
>> + ret = target_cpu->power_state;
>> + break;
>> + default:
>> + /* Everything above affinity level 0 is always on. */
>> + ret = 0;
>> + }
>> + break;
>> + 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.
>> + */
>> + return hvf_psci_cpu_off(arm_cpu);
>> + case QEMU_PSCI_0_2_FN_SYSTEM_OFF:
>> + qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>> + return hvf_psci_cpu_off(arm_cpu);
>> + case QEMU_PSCI_0_1_FN_CPU_ON:
>> + case QEMU_PSCI_0_2_FN_CPU_ON:
>> + case QEMU_PSCI_0_2_FN64_CPU_ON:
>> + mpidr = param[1];
>> + entry = param[2];
>> + context_id = param[3];
>> + ret = arm_set_cpu_on(mpidr, entry, context_id,
>> + target_el, target_aarch64);
>> + break;
>> + case QEMU_PSCI_0_1_FN_CPU_OFF:
>> + case QEMU_PSCI_0_2_FN_CPU_OFF:
>> + return hvf_psci_cpu_off(arm_cpu);
>> + case QEMU_PSCI_0_1_FN_CPU_SUSPEND:
>> + case QEMU_PSCI_0_2_FN_CPU_SUSPEND:
>> + case QEMU_PSCI_0_2_FN64_CPU_SUSPEND:
>> + /* Affinity levels are not supported in QEMU */
>> + if (param[1] & 0xfffe0000) {
>> + ret = QEMU_PSCI_RET_INVALID_PARAMS;
>> + break;
>> + }
>> + /* Powerdown is not supported, we always go into WFI */
>> + env->xregs[0] = 0;
>> + hvf_wfi(cpu);
>> + break;
>> + case QEMU_PSCI_0_1_FN_MIGRATE:
>> + case QEMU_PSCI_0_2_FN_MIGRATE:
>> + ret = QEMU_PSCI_RET_NOT_SUPPORTED;
>> + break;
>> + default:
>> + return 1;
>> + }
>> +
>> + env->xregs[0] = ret;
>> + return 0;
>> +}
>> +
>> static uint64_t hvf_sysreg_read(CPUState *cpu, uint32_t reg)
>> {
>> ARMCPU *arm_cpu = ARM_CPU(cpu);
>> @@ -716,6 +823,8 @@ int hvf_vcpu_exec(CPUState *cpu)
>> }
>>
>> if (cpu->halted) {
>> + /* On unhalt, we usually have CPU state changes. Prepare for them. */
>> + cpu_synchronize_state(cpu);
>> return EXCP_HLT;
>> }
> This seems odd. If I understand the control flow correctly, this
> is neither:
> (a) just before we're about to emulate a PSCI call so we need
> the current values of the registers from hvf
> (b) when we've just changed the CPU registers because we made
> a PSCI call and we want to push them back to hvf
> (c) when we're about to resume the CPU because it's gone from
> halted to not-halted, which might also be a good time to push
> register state back to hvf
>
> So what's it for ?
I agree, it's pretty superfluous. I'll remove it :)
Alex
>
> My expectation would be that we would ensure the state is in sync
> (ie that hvf has any local changes) before we call hv_cpu_run(),
> and that we would go the other way before we access any local
> register CPU struct state.
>
> thanks
> -- PMM
On 9/12/21 1:36 PM, Alexander Graf wrote:
>> I think the callsites would be clearer if you made the function
>> return true for "PSCI call handled", false for "not recognised,
>> give the guest an UNDEF". Code like
>> if (hvf_handle_psci_call(cpu)) {
>> stuff;
>> }
>>
>> looks like the 'stuff' is for the "psci call handled" case,
>> which at the moment it isn't.
>
>
> This function merely follows standard C semantics along the lines of "0
> means success, !0 is error". Isn't that what you would usually expect?
No, not really. I expect stuff that returns error codes to return negative integers on
failure. I expect stuff that returns a boolean success/failure to return true on success.
r~
On 12.09.21 23:20, Richard Henderson wrote:
> On 9/12/21 1:36 PM, Alexander Graf wrote:
>>> I think the callsites would be clearer if you made the function
>>> return true for "PSCI call handled", false for "not recognised,
>>> give the guest an UNDEF". Code like
>>> if (hvf_handle_psci_call(cpu)) {
>>> stuff;
>>> }
>>>
>>> looks like the 'stuff' is for the "psci call handled" case,
>>> which at the moment it isn't.
>>
>>
>> This function merely follows standard C semantics along the lines of "0
>> means success, !0 is error". Isn't that what you would usually expect?
>
> No, not really. I expect stuff that returns error codes to return
> negative integers on failure. I expect stuff that returns a boolean
> success/failure to return true on success.
Fair, I'll change it to return -1 then. Thanks!
Alex
On 9/12/21 2:37 PM, Alexander Graf wrote:
>
> On 12.09.21 23:20, Richard Henderson wrote:
>> On 9/12/21 1:36 PM, Alexander Graf wrote:
>>>> I think the callsites would be clearer if you made the function
>>>> return true for "PSCI call handled", false for "not recognised,
>>>> give the guest an UNDEF". Code like
>>>> if (hvf_handle_psci_call(cpu)) {
>>>> stuff;
>>>> }
>>>>
>>>> looks like the 'stuff' is for the "psci call handled" case,
>>>> which at the moment it isn't.
>>>
>>>
>>> This function merely follows standard C semantics along the lines of "0
>>> means success, !0 is error". Isn't that what you would usually expect?
>>
>> No, not really. I expect stuff that returns error codes to return
>> negative integers on failure. I expect stuff that returns a boolean
>> success/failure to return true on success.
>
>
> Fair, I'll change it to return -1 then. Thanks!
Not quite the point I was making. If the only two return values are -1/0, then bool
false/true is in fact more appropriate.
r~
On 12.09.21 23:40, Richard Henderson wrote:
> On 9/12/21 2:37 PM, Alexander Graf wrote:
>>
>> On 12.09.21 23:20, Richard Henderson wrote:
>>> On 9/12/21 1:36 PM, Alexander Graf wrote:
>>>>> I think the callsites would be clearer if you made the function
>>>>> return true for "PSCI call handled", false for "not recognised,
>>>>> give the guest an UNDEF". Code like
>>>>> if (hvf_handle_psci_call(cpu)) {
>>>>> stuff;
>>>>> }
>>>>>
>>>>> looks like the 'stuff' is for the "psci call handled" case,
>>>>> which at the moment it isn't.
>>>>
>>>>
>>>> This function merely follows standard C semantics along the lines
>>>> of "0
>>>> means success, !0 is error". Isn't that what you would usually expect?
>>>
>>> No, not really. I expect stuff that returns error codes to return
>>> negative integers on failure. I expect stuff that returns a boolean
>>> success/failure to return true on success.
>>
>>
>> Fair, I'll change it to return -1 then. Thanks!
>
> Not quite the point I was making. If the only two return values are
> -1/0, then bool false/true is in fact more appropriate.
If the whole code base adheres to it, maybe. The big problem with using
true/false as return values for functions that don't make it very
explicit (have an is in their name or use gerund for example). QEMU uses
a lot of 0 == success internally.
If you now add bools to the a code base that already uses int returns a
lot, you always need to look up what a function actually returns if the
caller treats it as bool (if, ?, assert, etc), making code review and
reasoning about code flows extremely hard. I've had one too many
occasions where I called an innocently looking API and put the assert()
the wrong way around for example.
I don't think we can solve this problem here, but IMHO the only sensible
way to get to good readability would be to have functions that return
success/failure return a libc defined struct that indicates the error.
Then check for success explicitly on every caller site. Overloading bool
or int for success/failure return is just bad :).
Alex
+Markus
On 9/13/21 12:06 PM, Alexander Graf wrote:
> On 12.09.21 23:40, Richard Henderson wrote:
>> On 9/12/21 2:37 PM, Alexander Graf wrote:
>>>
>>> On 12.09.21 23:20, Richard Henderson wrote:
>>>> On 9/12/21 1:36 PM, Alexander Graf wrote:
>>>>>> I think the callsites would be clearer if you made the function
>>>>>> return true for "PSCI call handled", false for "not recognised,
>>>>>> give the guest an UNDEF". Code like
>>>>>> if (hvf_handle_psci_call(cpu)) {
>>>>>> stuff;
>>>>>> }
>>>>>>
>>>>>> looks like the 'stuff' is for the "psci call handled" case,
>>>>>> which at the moment it isn't.
>>>>>
>>>>>
>>>>> This function merely follows standard C semantics along the lines
>>>>> of "0
>>>>> means success, !0 is error". Isn't that what you would usually expect?
>>>>
>>>> No, not really. I expect stuff that returns error codes to return
>>>> negative integers on failure. I expect stuff that returns a boolean
>>>> success/failure to return true on success.
>>>
>>>
>>> Fair, I'll change it to return -1 then. Thanks!
>>
>> Not quite the point I was making. If the only two return values are
>> -1/0, then bool false/true is in fact more appropriate.
>
>
> If the whole code base adheres to it, maybe. The big problem with using
> true/false as return values for functions that don't make it very
> explicit (have an is in their name or use gerund for example). QEMU uses
> a lot of 0 == success internally.
>
> If you now add bools to the a code base that already uses int returns a
> lot, you always need to look up what a function actually returns if the
> caller treats it as bool (if, ?, assert, etc), making code review and
> reasoning about code flows extremely hard. I've had one too many
> occasions where I called an innocently looking API and put the assert()
> the wrong way around for example.
>
> I don't think we can solve this problem here, but IMHO the only sensible
> way to get to good readability would be to have functions that return
> success/failure return a libc defined struct that indicates the error.
> Then check for success explicitly on every caller site. Overloading bool
> or int for success/failure return is just bad :).
IIUC what we are trying to do lately is, if caller only expects
success/failure then return boolean, with error information in
Error* for eventual propagation. Only return libc errno if the
caller has to handle failures differently.
© 2016 - 2024 Red Hat, Inc.