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

[Eduardo Habkost]

On Tue, Apr 18, 2017 at 09:39:46AM +0200, Julian Kirsch wrote:
> Add two new x86-specific HMP commands to read/write model specific
> registers (MSRs) of the current CPU.
> 
> Signed-off-by: Julian Kirsch <address@hidden>
[...]
> @@ -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);

Do we really need KVM_PUT_FULL_STATE here? The MSRs that can be
changed at runtime are supposed to be handled by
KVM_PUT_RUNTIME_STATE, aren't they?

We can also consider the approach suggested by Paolo: calling
KVM_SET_MSR directly (but then we need to ensure
cpu->kvm_vcpu_dirty is cleared before getting/setting the MSR).

On either case, we seem to need a wrapper that does the opposite
of cpu_synchronize_state(), here. Then we can choose between:
1) calling the wrapper and then calling KVM_{GET,SET}_MSR; or
2) calling x86_cpu_{rd,wr}msr() and then (optionally) calling the wrapper.

> +
> +            /* 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
> 

-- 
Eduardo