| Message ID | 20210201100903.17309-24-cfontana@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i386 cleanup PART 2 | expand |
On 2/1/21 11:09 AM, Claudio Fontana wrote: > overall, all devices' realize functions take an Error **errp, but return void. > > hw/core/qdev.c code, which realizes devices, therefore does: > > local_err = NULL; > dc->realize(dev, &local_err); > if (local_err != NULL) { > goto fail; > } > > However, we can improve at least accel_cpu to return a meaningful bool value. > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > --- > include/hw/core/accel-cpu.h | 2 +- > include/qemu/accel.h | 2 +- > target/i386/host-cpu.h | 2 +- > accel/accel-common.c | 6 +++--- > cpu.c | 5 +++-- > target/i386/host-cpu.c | 25 ++++++++++++++----------- > target/i386/kvm/kvm-cpu.c | 4 ++-- > target/i386/tcg/tcg-cpu.c | 6 ++++-- > 8 files changed, 29 insertions(+), 23 deletions(-) ... > diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c > index 9cfe56ce41..4ea9e354ea 100644 > --- a/target/i386/host-cpu.c > +++ b/target/i386/host-cpu.c > @@ -50,7 +50,7 @@ static void host_cpu_enable_cpu_pm(X86CPU *cpu) > env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR; > } > > -static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu, Error **errp) > +static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu) > { > uint32_t host_phys_bits = host_cpu_phys_bits(); > uint32_t phys_bits = cpu->phys_bits; > @@ -77,18 +77,10 @@ static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu, Error **errp) > } > } > > - if (phys_bits && > - (phys_bits > TARGET_PHYS_ADDR_SPACE_BITS || > - phys_bits < 32)) { > - error_setg(errp, "phys-bits should be between 32 and %u " > - " (but is %u)", > - TARGET_PHYS_ADDR_SPACE_BITS, phys_bits); > - } > - > return phys_bits; > } > > -void host_cpu_realizefn(CPUState *cs, Error **errp) > +bool host_cpu_realizefn(CPUState *cs, Error **errp) > { > X86CPU *cpu = X86_CPU(cs); > CPUX86State *env = &cpu->env; > @@ -97,8 +89,19 @@ void host_cpu_realizefn(CPUState *cs, Error **errp) > host_cpu_enable_cpu_pm(cpu); > } > if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) { > - cpu->phys_bits = host_cpu_adjust_phys_bits(cpu, errp); > + uint32_t phys_bits = host_cpu_adjust_phys_bits(cpu); > + > + if (phys_bits && > + (phys_bits > TARGET_PHYS_ADDR_SPACE_BITS || > + phys_bits < 32)) { > + error_setg(errp, "phys-bits should be between 32 and %u " > + " (but is %u)", > + TARGET_PHYS_ADDR_SPACE_BITS, phys_bits); Please this change in a preliminary patch (preferably), or comment it in the commit description. Either ways: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > + return false; > + } > + cpu->phys_bits = phys_bits; > } > + return true; > }
Claudio Fontana <cfontana@suse.de> writes: > overall, all devices' realize functions take an Error **errp, but return void. > > hw/core/qdev.c code, which realizes devices, therefore does: > > local_err = NULL; > dc->realize(dev, &local_err); > if (local_err != NULL) { > goto fail; > } > > However, we can improve at least accel_cpu to return a meaningful bool value. > > Signed-off-by: Claudio Fontana <cfontana@suse.de> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
diff --git a/include/hw/core/accel-cpu.h b/include/hw/core/accel-cpu.h index 24a6697412..5dbfd79955 100644 --- a/include/hw/core/accel-cpu.h +++ b/include/hw/core/accel-cpu.h @@ -32,7 +32,7 @@ typedef struct AccelCPUClass { void (*cpu_class_init)(CPUClass *cc); void (*cpu_instance_init)(CPUState *cpu); - void (*cpu_realizefn)(CPUState *cpu, Error **errp); + bool (*cpu_realizefn)(CPUState *cpu, Error **errp); } AccelCPUClass; #endif /* ACCEL_CPU_H */ diff --git a/include/qemu/accel.h b/include/qemu/accel.h index da0c8ab523..4f4c283f6f 100644 --- a/include/qemu/accel.h +++ b/include/qemu/accel.h @@ -89,6 +89,6 @@ void accel_cpu_instance_init(CPUState *cpu); * @cpu: The CPU that needs to call accel-specific cpu realization. * @errp: currently unused. */ -void accel_cpu_realizefn(CPUState *cpu, Error **errp); +bool accel_cpu_realizefn(CPUState *cpu, Error **errp); #endif /* QEMU_ACCEL_H */ diff --git a/target/i386/host-cpu.h b/target/i386/host-cpu.h index b47bc0943f..6a9bc918ba 100644 --- a/target/i386/host-cpu.h +++ b/target/i386/host-cpu.h @@ -12,7 +12,7 @@ void host_cpu_instance_init(X86CPU *cpu); void host_cpu_max_instance_init(X86CPU *cpu); -void host_cpu_realizefn(CPUState *cs, Error **errp); +bool host_cpu_realizefn(CPUState *cs, Error **errp); void host_cpu_vendor_fms(char *vendor, int *family, int *model, int *stepping); diff --git a/accel/accel-common.c b/accel/accel-common.c index 0f6fb4fb66..d77c09d7b5 100644 --- a/accel/accel-common.c +++ b/accel/accel-common.c @@ -98,14 +98,14 @@ void accel_cpu_instance_init(CPUState *cpu) } } -void accel_cpu_realizefn(CPUState *cpu, Error **errp) +bool accel_cpu_realizefn(CPUState *cpu, Error **errp) { CPUClass *cc = CPU_GET_CLASS(cpu); if (cc->accel_cpu && cc->accel_cpu->cpu_realizefn) { - /* NB: errp parameter is unused currently */ - cc->accel_cpu->cpu_realizefn(cpu, errp); + return cc->accel_cpu->cpu_realizefn(cpu, errp); } + return true; } static const TypeInfo accel_cpu_type = { diff --git a/cpu.c b/cpu.c index 25e6fbfa2c..34a0484bf4 100644 --- a/cpu.c +++ b/cpu.c @@ -130,8 +130,9 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp) CPUClass *cc = CPU_GET_CLASS(cpu); cpu_list_add(cpu); - accel_cpu_realizefn(cpu, errp); - + if (!accel_cpu_realizefn(cpu, errp)) { + return; + } #ifdef CONFIG_TCG /* NB: errp parameter is unused currently */ if (tcg_enabled()) { diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c index 9cfe56ce41..4ea9e354ea 100644 --- a/target/i386/host-cpu.c +++ b/target/i386/host-cpu.c @@ -50,7 +50,7 @@ static void host_cpu_enable_cpu_pm(X86CPU *cpu) env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR; } -static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu, Error **errp) +static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu) { uint32_t host_phys_bits = host_cpu_phys_bits(); uint32_t phys_bits = cpu->phys_bits; @@ -77,18 +77,10 @@ static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu, Error **errp) } } - if (phys_bits && - (phys_bits > TARGET_PHYS_ADDR_SPACE_BITS || - phys_bits < 32)) { - error_setg(errp, "phys-bits should be between 32 and %u " - " (but is %u)", - TARGET_PHYS_ADDR_SPACE_BITS, phys_bits); - } - return phys_bits; } -void host_cpu_realizefn(CPUState *cs, Error **errp) +bool host_cpu_realizefn(CPUState *cs, Error **errp) { X86CPU *cpu = X86_CPU(cs); CPUX86State *env = &cpu->env; @@ -97,8 +89,19 @@ void host_cpu_realizefn(CPUState *cs, Error **errp) host_cpu_enable_cpu_pm(cpu); } if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) { - cpu->phys_bits = host_cpu_adjust_phys_bits(cpu, errp); + uint32_t phys_bits = host_cpu_adjust_phys_bits(cpu); + + if (phys_bits && + (phys_bits > TARGET_PHYS_ADDR_SPACE_BITS || + phys_bits < 32)) { + error_setg(errp, "phys-bits should be between 32 and %u " + " (but is %u)", + TARGET_PHYS_ADDR_SPACE_BITS, phys_bits); + return false; + } + cpu->phys_bits = phys_bits; } + return true; } #define CPUID_MODEL_ID_SZ 48 diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c index c23bbe6c50..c660ad4293 100644 --- a/target/i386/kvm/kvm-cpu.c +++ b/target/i386/kvm/kvm-cpu.c @@ -18,7 +18,7 @@ #include "kvm_i386.h" #include "hw/core/accel-cpu.h" -static void kvm_cpu_realizefn(CPUState *cs, Error **errp) +static bool kvm_cpu_realizefn(CPUState *cs, Error **errp) { X86CPU *cpu = X86_CPU(cs); CPUX86State *env = &cpu->env; @@ -41,7 +41,7 @@ static void kvm_cpu_realizefn(CPUState *cs, Error **errp) MSR_IA32_UCODE_REV); } } - host_cpu_realizefn(cs, errp); + return host_cpu_realizefn(cs, errp); } /* diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c index 1d3d6d1c6a..23e1f5f0c3 100644 --- a/target/i386/tcg/tcg-cpu.c +++ b/target/i386/tcg/tcg-cpu.c @@ -96,7 +96,7 @@ static void x86_cpu_machine_done(Notifier *n, void *unused) } } -static void tcg_cpu_realizefn(CPUState *cs, Error **errp) +static bool tcg_cpu_realizefn(CPUState *cs, Error **errp) { X86CPU *cpu = X86_CPU(cs); @@ -132,12 +132,14 @@ static void tcg_cpu_realizefn(CPUState *cs, Error **errp) /* ... SMRAM with higher priority, linked from /machine/smram. */ cpu->machine_done.notify = x86_cpu_machine_done; qemu_add_machine_init_done_notifier(&cpu->machine_done); + return true; } #else /* CONFIG_USER_ONLY */ -static void tcg_cpu_realizefn(CPUState *cs, Error **errp) +static bool tcg_cpu_realizefn(CPUState *cs, Error **errp) { + return true; } #endif /* !CONFIG_USER_ONLY */
overall, all devices' realize functions take an Error **errp, but return void. hw/core/qdev.c code, which realizes devices, therefore does: local_err = NULL; dc->realize(dev, &local_err); if (local_err != NULL) { goto fail; } However, we can improve at least accel_cpu to return a meaningful bool value. Signed-off-by: Claudio Fontana <cfontana@suse.de> --- include/hw/core/accel-cpu.h | 2 +- include/qemu/accel.h | 2 +- target/i386/host-cpu.h | 2 +- accel/accel-common.c | 6 +++--- cpu.c | 5 +++-- target/i386/host-cpu.c | 25 ++++++++++++++----------- target/i386/kvm/kvm-cpu.c | 4 ++-- target/i386/tcg/tcg-cpu.c | 6 ++++-- 8 files changed, 29 insertions(+), 23 deletions(-)