Re: [Qemu-devel] [PATCH RESEND v4 4/4] HMP: Introduce msr_get and msr_set HMP commands

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Julian Kirsch (git@kirschju.re) wrote:
> Add two new x86-specific HMP commands to read/write model specific
> registers (MSRs) of the current CPU.
> 
> Signed-off-by: Julian Kirsch <git@kirschju.re>
> ---
>  hmp-commands.hx              | 38 ++++++++++++++++++++++
>  include/monitor/hmp-target.h |  2 ++
>  target/i386/monitor.c        | 76 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 116 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 88192817b2..07a09120aa 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1568,6 +1568,44 @@ STEXI
>  Inject an MCE on the given CPU (x86 only).
>  ETEXI
>  
> +
> +#if defined(TARGET_I386)
> +
> +    {
> +        .name         = "msr_get",
> +        .args_type    = "msr_index:i",
> +        .params       = "msrindex",
> +        .help         = "get value of model specific register (MSR) on x86",
> +        .cmd          =  hmp_msr_get,
> +    },
> +
> +#endif
> +STEXI
> +@item msr-get @var{msridx}
> +@findex msr-get (x86)

Shouldn't the use of '-' be '_' in those to match the .name (and in the -set variant
below) ?

Dave

> +Print value of model specific register @var{msr_index} on current CPU (x86
> +only).
> +ETEXI
> +
> +
> +#if defined(TARGET_I386)
> +
> +    {
> +        .name         = "msr_set",
> +        .args_type    = "msr_index:i,value:l",
> +        .params       = "msrindex value",
> +        .help         = "set value of model specific register (MSR) on x86",
> +        .cmd          =  hmp_msr_set,
> +    },
> +
> +#endif
> +STEXI
> +@item msr-set @var{msr_index} @var{value}
> +@findex msr-set
> +Set value of model specific register @var{msr_index} on current CPU to
> +@var{value} (x86 only).
> +ETEXI
> +
>      {
>          .name       = "getfd",
>          .args_type  = "fdname:s",
> diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
> index 454e8ed155..a0da1eb05b 100644
> --- a/include/monitor/hmp-target.h
> +++ b/include/monitor/hmp-target.h
> @@ -46,5 +46,7 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict);
>  void hmp_mce(Monitor *mon, const QDict *qdict);
>  void hmp_info_local_apic(Monitor *mon, const QDict *qdict);
>  void hmp_info_io_apic(Monitor *mon, const QDict *qdict);
> +void hmp_msr_get(Monitor *mon, const QDict *qdict);
> +void hmp_msr_set(Monitor *mon, const QDict *qdict);
>  
>  #endif /* MONITOR_HMP_TARGET_H */
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 77ead60437..d33228416e 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -27,7 +27,9 @@
>  #include "monitor/hmp-target.h"
>  #include "hw/i386/pc.h"
>  #include "sysemu/kvm.h"
> +#include "sysemu/hw_accel.h"
>  #include "hmp.h"
> +#include "qapi/error.h"
>  
>  
>  static void print_pte(Monitor *mon, CPUArchState *env, hwaddr addr,
> @@ -651,3 +653,77 @@ void hmp_info_io_apic(Monitor *mon, const QDict *qdict)
>          ioapic_dump_state(mon, qdict);
>      }
>  }
> +
> +void hmp_msr_get(Monitor *mon, const QDict *qdict)
> +{
> +    bool valid = false;
> +    X86CPU *cpu;
> +    CPUState *cs;
> +    Error *err = NULL;
> +    uint64_t value;
> +
> +    uint32_t index = qdict_get_int(qdict, "msr_index");
> +
> +    /* N.B.: mon_get_cpu already synchronizes the CPU state */
> +    cs = mon_get_cpu();
> +    if (cs != NULL) {
> +        cpu = X86_CPU(cs);
> +
> +        value = x86_cpu_rdmsr(&cpu->env, index, &valid);
> +        if (valid) {
> +            monitor_printf(mon, "0x%016" PRIx64 "\n", value);
> +        } else {
> +            error_setg(&err, "Failed to read MSR 0x%08x", index);
> +        }
> +    } else {
> +        monitor_printf(mon, "No CPU available\n");
> +        return;
> +    }
> +
> +    if (err) {
> +        error_report_err(err);
> +    }
> +}
> +
> +void hmp_msr_set(Monitor *mon, const QDict *qdict)
> +{
> +    bool valid = false;
> +    X86CPU *cpu;
> +    CPUState *cs;
> +    Error *err = NULL;
> +    uint64_t new_value;
> +
> +    uint32_t index = qdict_get_int(qdict, "msr_index");
> +    uint64_t value = qdict_get_int(qdict, "value");
> +
> +    /* N.B.: mon_get_cpu already synchronizes the CPU state */
> +    cs = mon_get_cpu();
> +    if (cs != NULL) {
> +        cpu = X86_CPU(cs);
> +
> +        x86_cpu_wrmsr(&cpu->env, index, value, &valid);
> +        if (!valid) {
> +            error_setg(&err, "Failed to write MSR 0x%08x", index);
> +        }
> +#ifdef CONFIG_KVM
> +        else if (kvm_enabled()) {
> +            /* Force KVM to flush registers at KVM_PUT_FULL_STATE level. */
> +            cpu_synchronize_post_init(cs);
> +
> +            /* Read back the value from KVM to check if it flushed them. */
> +            cpu_synchronize_state(cs);
> +            new_value = x86_cpu_rdmsr(&cpu->env, index, &valid);
> +            if (new_value != value) {
> +                error_setg(&err, "Failed to flush MSR 0x%08x to KVM", index);
> +            }
> +        }
> +#endif
> +    } else {
> +        monitor_printf(mon, "No CPU available\n");
> +        return;
> +    }
> +
> +    if (err) {
> +        error_report_err(err);
> +    }
> +}
> -- 
> 2.11.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK