Re: [PATCH v2 04/39] target/i386: decouple cpu_x86_inject_mce() from Monitor

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Jun 26, 2026 at 01:19:05AM +0400, Marc-André Lureau wrote:
> Replace monitor_printf() error reporting with the standard **errp
> pattern. Drop the Monitor *mon parameter and return bool to indicate
> success, improving the function usage from non-HMP contexts. Update the
> KVM MCE injection path and hmp_mce() accordingly.
> 
> Notes:
>  - we may want to print the error in KVM path too
>  - multiple error prints are now accumulated with error hints
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  target/i386/cpu.h     |  4 ++--
>  target/i386/helper.c  | 49 ++++++++++++++++++++++++++++---------------------
>  target/i386/kvm/kvm.c |  4 ++--
>  target/i386/monitor.c |  6 ++++--
>  4 files changed, 36 insertions(+), 27 deletions(-)


>  typedef struct MCEInjectionParams {
> -    Monitor *mon;
> +    GString *err;

Why not have "Error **errp"

>      int bank;
>      uint64_t status;
>      uint64_t mcg_status;
> @@ -428,9 +428,9 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
>           * reporting is disabled
>           */
>          if ((cenv->mcg_cap & MCG_CTL_P) && cenv->mcg_ctl != ~(uint64_t)0) {
> -            monitor_printf(params->mon,
> -                           "CPU %d: Uncorrected error reporting disabled\n",
> -                           cs->cpu_index);
> +            g_string_append_printf(params->err,
> +                "CPU %d: Uncorrected error reporting disabled\n",
> +                cs->cpu_index);

and use  error_setg here and the other two places below.

>              return;
>          }
>  
> @@ -439,10 +439,9 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
>           * reporting is disabled for the bank
>           */
>          if (banks[0] != ~(uint64_t)0) {
> -            monitor_printf(params->mon,
> -                           "CPU %d: Uncorrected error reporting disabled for"
> -                           " bank %d\n",
> -                           cs->cpu_index, params->bank);
> +            g_string_append_printf(params->err,
> +                "CPU %d: Uncorrected error reporting disabled for bank %d\n",
> +                cs->cpu_index, params->bank);
>              return;
>          }
>  
> @@ -459,7 +458,7 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
>          if (need_reset) {
>              emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
>                                        recursive);
> -            monitor_printf(params->mon, "%s", msg);
> +            g_string_append_printf(params->err, "%s\n", msg);
>              qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
>              qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>              return;
> @@ -488,14 +487,16 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
>      emit_guest_memory_failure(MEMORY_FAILURE_ACTION_INJECT, ar, recursive);
>  }
>  
> -void cpu_x86_inject_mce(Monitor *mon, X86CPU *cpu, int bank,
> +bool cpu_x86_inject_mce(X86CPU *cpu, int bank,
>                          uint64_t status, uint64_t mcg_status, uint64_t addr,
> -                        uint64_t misc, int flags)
> +                        uint64_t misc, int flags, Error **errp)
>  {
> +    ERRP_GUARD();
>      CPUState *cs = CPU(cpu);
>      CPUX86State *cenv = &cpu->env;
> +    g_autoptr(GString) err = g_string_new(NULL);
>      MCEInjectionParams params = {
> -        .mon = mon,
> +        .err = err,

This indirection via GString feels overkill when we could
use directly pass 'errp', given the ERRP_GUARD we have

>          .bank = bank,
>          .status = status,
>          .mcg_status = mcg_status,
> @@ -506,21 +507,21 @@ void cpu_x86_inject_mce(Monitor *mon, X86CPU *cpu, int bank,
>      unsigned bank_num = cenv->mcg_cap & 0xff;
>  
>      if (!cenv->mcg_cap) {
> -        monitor_printf(mon, "MCE injection not supported\n");
> -        return;
> +        error_setg(errp, "MCE injection not supported");
> +        return false;
>      }
>      if (bank >= bank_num) {
> -        monitor_printf(mon, "Invalid MCE bank number\n");
> -        return;
> +        error_setg(errp, "Invalid MCE bank number");
> +        return false;
>      }
>      if (!(status & MCI_STATUS_VAL)) {
> -        monitor_printf(mon, "Invalid MCE status code\n");
> -        return;
> +        error_setg(errp, "Invalid MCE status code");
> +        return false;
>      }
>      if ((flags & MCE_INJECT_BROADCAST)
>          && !cpu_x86_support_mca_broadcast(cenv)) {
> -        monitor_printf(mon, "Guest CPU does not support MCA broadcast\n");
> -        return;
> +        error_setg(errp, "Guest CPU does not support MCA broadcast");
> +        return false;
>      }
>  
>      run_on_cpu(cs, do_inject_x86_mce, RUN_ON_CPU_HOST_PTR(&params));
> @@ -539,6 +540,12 @@ void cpu_x86_inject_mce(Monitor *mon, X86CPU *cpu, int bank,
>              run_on_cpu(other_cs, do_inject_x86_mce, RUN_ON_CPU_HOST_PTR(&params));
>          }
>      }
> +    if (params.err->len) {
> +        error_setg(errp, "Failed to inject MCE");
> +        error_append_hint(errp, "%s", params.err->str);
> +        return false;
> +    }

This could be just

  if (*params.errp) {
     return false;
  }

> +    return true;
>  }
>  
>  static inline target_ulong get_memio_eip(CPUX86State *env)

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|