On Fri, 30 Oct 2026 at 07:31, Eric Auger <[email protected]> wrote:
>
> In case *_arm_host_cpu_features() fails, we currently
> record that state in cpu->host_cpu_probe_failed to later
> report the error on realize().
>
> Replace the boolean by an error handle to allow
> more precise error reports.
>
> Signed-off-by: Eric Auger <[email protected]>
> Suggested-by: Peter Maydell <[email protected]>
> ---
> target/arm/cpu.h | 5 +++--
> target/arm/cpu.c | 9 +++++----
> target/arm/hvf/hvf.c | 3 ++-
> target/arm/kvm.c | 24 +++++++++++-------------
> target/arm/whpx/whpx-all.c | 3 ++-
> 5 files changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 5c5f551dac..dae79539b5 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1075,10 +1075,11 @@ struct ArchCPU {
> /* Uniprocessor system with MP extensions */
> bool mp_is_up;
>
> - /* True if we tried kvm_arm_host_cpu_features() during CPU instance_init
> + /*
> + * Set if we tried *_set_cpu_features_from_host() during CPU
> instance_init
> * and the probe failed (so we need to report the error in realize)
> */
> - bool host_cpu_probe_failed;
> + Error *host_cpu_probe_err;
>
> /* QOM property to indicate we should use the back-compat CNTFRQ default
> */
> bool backcompat_cntfrq;
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index aba45b66f8..67d48ebc07 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1838,12 +1838,13 @@ static void arm_cpu_realizefn(DeviceState *dev, Error
> **errp)
> * then it's possible that might have failed in the initfn, but
> * this is the first point where we can report it.
> */
> - if (cpu->host_cpu_probe_failed) {
> + if (cpu->host_cpu_probe_err) {
> if (!kvm_enabled() && !hvf_enabled() && !whpx_enabled()) {
> - error_setg(errp, "The 'host' CPU type can only be used with KVM,
> HVF or WHPX");
> - } else {
> - error_setg(errp, "Failed to retrieve host CPU features");
> + error_append_hint(&cpu->host_cpu_probe_err,
> + "The 'host' CPU type can only be used with
> KVM, HVF or WHPX\n");
I don't think we should be doing this as error_append_hint(). If
the accelerator isn't one that supports 'host' then "host
can only be used with KVM, HVF or WHPX" is the entire and
correct thing to report to the user, and whatever specifically
might have been the trigger for us to notice this is irrelevant.
> }
> + error_propagate(errp, cpu->host_cpu_probe_err);
> + cpu->host_cpu_probe_err = NULL;
> return;
> }
Don't we also need to free the Error in finalize, to catch the case
where we did "instance_init -> finalize" and don't get here ?
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 4b947e9ba5..2f2a46644d 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -1287,7 +1287,8 @@ void hvf_arm_set_cpu_features_from_host(ARMCPU *cpu)
> * We can't report this error yet, so flag that we need to
> * in arm_cpu_realizefn().
> */
> - cpu->host_cpu_probe_failed = true;
> + error_setg(&cpu->host_cpu_probe_err,
> + "failed to retrieve host CPU features");
> return;
> }
> }
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index b4b20c5dc2..fc9551411e 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -312,7 +312,7 @@ static uint32_t kvm_arm_sve_get_vls(int fd)
> return vls[0] & MAKE_64BIT_MASK(0, ARM_MAX_VQ);
> }
>
> -static void kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> +static bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> {
> /* Identify the feature bits corresponding to the host CPU, and
> * fill out the ARMHostCPUClass fields accordingly. To do this
> @@ -330,7 +330,7 @@ static void
> kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> ahcf->dtb_compatible = "arm,armv8";
>
> if (!kvm_enabled()) {
> - return;
> + return false;
> }
>
> /*
> @@ -372,7 +372,7 @@ static void
> kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> }
>
> if (!kvm_arm_create_scratch_host_vcpu(fdarray, &init)) {
> - return;
> + return false;
> }
>
> int fd = fdarray[2];
> @@ -502,7 +502,7 @@ static void
> kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> kvm_arm_destroy_scratch_host_vcpu(fdarray);
>
> if (err < 0) {
> - return;
> + return false;
> }
>
> /*
> @@ -521,27 +521,25 @@ static void
> kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>
> ahcf->target = init.target;
> ahcf->features = features;
> + return true;
> }
I don't think you need to change the return type of this function
and introduce a static probe_ok variable in the caller. The
thing that tells you that the probe succeeded is that it set
ahcf->target to something other than QEMU_KVM_ARM_TARGET_NONE,
which it only does right at the very end when it has definitely
succeeded.
(It is a bit obscure that the contract between kvm_arm_get_host_cpu_features()
and its caller is "ahcf->dtb_compatible NULL means we haven't tried a
probe yet; ahcf->kvm_target still TARGET_NONE after probe means the
probe failed". We could probably add a comment about this, but that's
separate from what this patch is doing.)
> void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
> {
> CPUARMState *env = &cpu->env;
> + static bool probe_ok;
>
> if (!arm_host_cpu_features.dtb_compatible) {
> - kvm_arm_get_host_cpu_features(&arm_host_cpu_features);
> + probe_ok = kvm_arm_get_host_cpu_features(&arm_host_cpu_features);
> }
>
> - cpu->kvm_target = arm_host_cpu_features.target;
> -
> - if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE) {
> - /*
> - * We can't report this error yet, so flag that we need to
> - * in arm_cpu_realizefn().
> - */
> - cpu->host_cpu_probe_failed = true;
> + if (!probe_ok) {
> + error_setg(&cpu->host_cpu_probe_err,
> + "failed to retrieve host CPU features");
> return;
> }
>
> + cpu->kvm_target = arm_host_cpu_features.target;
> cpu->dtb_compatible = arm_host_cpu_features.dtb_compatible;
> cpu->isar = arm_host_cpu_features.isar;
> cpu->sve_vq.supported = arm_host_cpu_features.sve_vq_supported;
-- PMM
