From: Paolo Bonzini <pbonzini@redhat.com>
To: Eduardo Habkost <ehabkost@redhat.com>
Cc: Julian Kirsch <git@kirschju.re>,
qemu-devel@nongnu.org,
Peter Crosthwaite <crosthwaite.peter@gmail.com>,
Richard Henderson <rth@twiddle.net>,
"Dr . David Alan Gilbert" <dgilbert@redhat.com>,
Eric Blake <eblake@redhat.com>
Subject: Re: [Qemu-devel] [PATCH RESEND v4 1/4] X86: Move rdmsr/wrmsr functionality to standalone functions
Date: Wed, 19 Apr 2017 05:44:06 -0400 (EDT) [thread overview]
Message-ID: <311531997.14466224.1492595046139.JavaMail.zimbra@redhat.com> (raw)
In-Reply-To: <20170418181628.GQ25239@thinpad.lan.raisama.net>
> On Tue, Apr 18, 2017 at 05:38:01PM +0200, Paolo Bonzini wrote:
> > On 18/04/2017 17:28, Eduardo Habkost wrote:
> > > Hi,
> > >
> > > A few comments below:
> > >
> > > On Tue, Apr 18, 2017 at 09:39:43AM +0200, Julian Kirsch wrote:
> > >> Add two new functions to provide read/write access to model specific
> > >> registers
> > >> (MSRs) on x86. Move original functionality to new functions
> > >> x86_cpu_[rd|wr]msr. This enables other parts of the qemu codebase to
> > >> read/write MSRs by means of the newly introduced functions. The
> > >> helper_[rd|wr]msr functions in misc_helper.c now consist of stubs
> > >> extracting the arguments from the current state of the CPU registers and
> > >> then pass control to the new functions.
> >
> > I didn't receive patches 2-4 so apologies if this is wrong.
> >
> > rdmsr/wrmsr should not use the helpers on KVM; it should instead use the
> > KVM_GET_MSR and KVM_SET_MSR ioctls. For HAX there are similar APIs.
>
> This series seems to take a different approach: it get/sets the
> MSR value inside X86CPU, and expect it to be synced later by
> kvm_put_msrs().
>
> I think both approaches are valid, but this series needs a call
> to cpu_synchronize_state() before getting/setting the MSR value
> or it won't work correctly.
With Julian's choice, you rely on TCG code to filter out bad MSR writes
before they hit KVM. I don't think this is always possible; sometimes
it would restrict too little and cause problems when KVM_SET_MSR fails,
sometimes it would restrict too much (and in particular restrict the
choice of MSRs that you can write, because KVM supports more of them
than TCG).
> (On the other hand, if we make the new msr_set/msr_get commands
> call KVM_{GET,SET}_MSR directly, then it needs to ensure the VCPU
> state isn't dirty first.)
Good point.
Paolo
> >
> > As a first step, only supporting the new commands on TCG would be fine.
> >
> > One possibility is to add a dispatcher function in helper.c
> >
> > if (tcg_enabled()) {
> > return tcg_cpu_wrmsr(env, idx, val, valid);
> > }
> > if (kvm_enabled()) {
> > return kvm_cpu_wrmsr(env, idx, val, valid);
> > }
> > ...
> >
> > Paolo
> >
> > >> Signed-off-by: Julian Kirsch <git@kirschju.re>
> > >> ---
> > >> target/i386/cpu.h | 3 +
> > >> target/i386/helper.c | 303
> > >> ++++++++++++++++++++++++++++++++++++++++++++++
> > >> target/i386/misc_helper.c | 297
> > >> ++-------------------------------------------
> > >> 3 files changed, 317 insertions(+), 286 deletions(-)
> > >>
> > >> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > >> index 07401ad9fe..84b7080ade 100644
> > >> --- a/target/i386/cpu.h
> > >> +++ b/target/i386/cpu.h
> > >> @@ -1310,6 +1310,9 @@ int
> > >> x86_cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
> > >> void x86_cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list,
> > >> Error **errp);
> > >>
> > >> +uint64_t x86_cpu_rdmsr(CPUX86State *env, uint32_t idx, bool *valid);
> > >> +void x86_cpu_wrmsr(CPUX86State *env, uint32_t idx, uint64_t val, bool
> > >> *valid);
> > >> +
> > >> void x86_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function
> > >> cpu_fprintf,
> > >> int flags);
> > >>
> > >> diff --git a/target/i386/helper.c b/target/i386/helper.c
> > >> index e2af3404f2..9412fd180c 100644
> > >> --- a/target/i386/helper.c
> > >> +++ b/target/i386/helper.c
> > >> @@ -26,6 +26,7 @@
> > >> #include "sysemu/sysemu.h"
> > >> #include "sysemu/hw_accel.h"
> > >> #include "monitor/monitor.h"
> > >> +#include "hw/i386/apic.h"
> > >> #include "hw/i386/apic_internal.h"
> > >> #endif
> > >>
> > >> @@ -364,11 +365,313 @@ void x86_cpu_dump_local_apic_state(CPUState *cs,
> > >> FILE *f,
> > >> }
> > >> cpu_fprintf(f, " PPR 0x%02x\n", apic_get_ppr(s));
> > >> }
> > >> +
> > >> +void x86_cpu_wrmsr(CPUX86State *env, uint32_t idx, uint64_t val, bool
> > >> *valid)
> > >> +{
> > >> + *valid = true;
> > >> + /* FIXME: With KVM nabled, only report success if we are sure the
> > >> new value
> > >
> > > Typo: "KVM enabled".
> > >
> > >> + * will actually be written back by the KVM subsystem later. */
> > >> +
> > >> + switch (idx) {
> > >> + case MSR_IA32_SYSENTER_CS:
> > >> + env->sysenter_cs = val & 0xffff;
> > >> + break;
> > >> + case MSR_IA32_SYSENTER_ESP:
> > >> + env->sysenter_esp = val;
> > >> + break;
> > >> + case MSR_IA32_SYSENTER_EIP:
> > >> + env->sysenter_eip = val;
> > >> + break;
> > >> + case MSR_IA32_APICBASE:
> > >> + cpu_set_apic_base(x86_env_get_cpu(env)->apic_state, val);
> > >> + break;
> > >> + case MSR_EFER:
> > >> + {
> > >> + uint64_t update_mask;
> > >> +
> > >> + update_mask = 0;
> > >> + if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_SYSCALL)
> > >> {
> > >> + update_mask |= MSR_EFER_SCE;
> > >> + }
> > >> + if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
> > >> + update_mask |= MSR_EFER_LME;
> > >> + }
> > >> + if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_FFXSR) {
> > >> + update_mask |= MSR_EFER_FFXSR;
> > >> + }
> > >> + if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_NX) {
> > >> + update_mask |= MSR_EFER_NXE;
> > >> + }
> > >> + if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) {
> > >> + update_mask |= MSR_EFER_SVME;
> > >> + }
> > >> + if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_FFXSR) {
> > >> + update_mask |= MSR_EFER_FFXSR;
> > >> + }
> > >> + cpu_load_efer(env, (env->efer & ~update_mask) |
> > >> + (val & update_mask));
> > >> + }
> > >> + break;
> > >> + case MSR_STAR:
> > >> + env->star = val;
> > >> + break;
> > >> + case MSR_PAT:
> > >> + env->pat = val;
> > >> + break;
> > >> + case MSR_VM_HSAVE_PA:
> > >> + env->vm_hsave = val;
> > >> + break;
> > >> +#ifdef TARGET_X86_64
> > >> + case MSR_LSTAR:
> > >> + env->lstar = val;
> > >> + break;
> > >> + case MSR_CSTAR:
> > >> + env->cstar = val;
> > >> + break;
> > >> + case MSR_FMASK:
> > >> + env->fmask = val;
> > >> + break;
> > >> + case MSR_FSBASE:
> > >> + env->segs[R_FS].base = val;
> > >> + break;
> > >> + case MSR_GSBASE:
> > >> + env->segs[R_GS].base = val;
> > >> + break;
> > >> + case MSR_KERNELGSBASE:
> > >> + env->kernelgsbase = val;
> > >> + break;
> > >> +#endif
> > >> + case MSR_MTRRphysBase(0):
> > >> + case MSR_MTRRphysBase(1):
> > >> + case MSR_MTRRphysBase(2):
> > >> + case MSR_MTRRphysBase(3):
> > >> + case MSR_MTRRphysBase(4):
> > >> + case MSR_MTRRphysBase(5):
> > >> + case MSR_MTRRphysBase(6):
> > >> + case MSR_MTRRphysBase(7):
> > >> + env->mtrr_var[(idx - MSR_MTRRphysBase(0)) / 2].base = val;
> > >> + break;
> > >> + case MSR_MTRRphysMask(0):
> > >> + case MSR_MTRRphysMask(1):
> > >> + case MSR_MTRRphysMask(2):
> > >> + case MSR_MTRRphysMask(3):
> > >> + case MSR_MTRRphysMask(4):
> > >> + case MSR_MTRRphysMask(5):
> > >> + case MSR_MTRRphysMask(6):
> > >> + case MSR_MTRRphysMask(7):
> > >> + env->mtrr_var[(idx - MSR_MTRRphysMask(0)) / 2].mask = val;
> > >> + break;
> > >> + case MSR_MTRRfix64K_00000:
> > >> + env->mtrr_fixed[idx - MSR_MTRRfix64K_00000] = val;
> > >> + break;
> > >> + case MSR_MTRRfix16K_80000:
> > >> + case MSR_MTRRfix16K_A0000:
> > >> + env->mtrr_fixed[idx - MSR_MTRRfix16K_80000 + 1] = val;
> > >> + break;
> > >> + case MSR_MTRRfix4K_C0000:
> > >> + case MSR_MTRRfix4K_C8000:
> > >> + case MSR_MTRRfix4K_D0000:
> > >> + case MSR_MTRRfix4K_D8000:
> > >> + case MSR_MTRRfix4K_E0000:
> > >> + case MSR_MTRRfix4K_E8000:
> > >> + case MSR_MTRRfix4K_F0000:
> > >> + case MSR_MTRRfix4K_F8000:
> > >> + env->mtrr_fixed[idx - MSR_MTRRfix4K_C0000 + 3] = val;
> > >> + break;
> > >> + case MSR_MTRRdefType:
> > >> + env->mtrr_deftype = val;
> > >> + break;
> > >> + case MSR_MCG_STATUS:
> > >> + env->mcg_status = val;
> > >> + break;
> > >> + case MSR_MCG_CTL:
> > >> + if ((env->mcg_cap & MCG_CTL_P)
> > >> + && (val == 0 || val == ~(uint64_t)0)) {
> > >> + env->mcg_ctl = val;
> > >> + }
> > >
> > > [***]
> > >
> > > Should we set *valid = false if MCG_CTL_P is not set?
> > >
> > > Should we set *valid = false if 'val' is invalid?
> > >
> > >
> > >> + break;
> > >> + case MSR_TSC_AUX:
> > >> + env->tsc_aux = val;
> > >> + break;
> > >> + case MSR_IA32_MISC_ENABLE:
> > >> + env->msr_ia32_misc_enable = val;
> > >> + break;
> > >> + case MSR_IA32_BNDCFGS:
> > >> + /* FIXME: #GP if reserved bits are set. */
> > >> + /* FIXME: Extend highest implemented bit of linear address. */
> > >> + env->msr_bndcfgs = val;
> > >> + cpu_sync_bndcs_hflags(env);
> > >> + break;
> > >> + default:
> > >> + *valid = false;
> > >
> > > [*]
> > >
> > >> + if (idx >= MSR_MC0_CTL && idx < MSR_MC0_CTL +
> > >> + (4 * env->mcg_cap & 0xff)) {
> > >> + uint32_t offset = idx - MSR_MC0_CTL;
> > >> + if ((offset & 0x3) != 0
> > >> + || (val == 0 || val == ~(uint64_t)0)) {
> > >> + env->mce_banks[offset] = val;
> > >> + *valid = true;
> > >
> > > [**]
> > >
> > >> + }
> > >> + break;
> > >> + }
> > >
> > > If you set *valid = false here instead of setting it above[*],
> > > you don't need to set *valid = true again above[**].
> > >
> > > I don't know if we should set *valid = false if 'val' is invalid,
> > > though. You might want to move the 'break' statement to [**] to
> > > keep the behavior of this patch.
> > >
> > >
> > >> + break;
> > >> + }
> > >> +}
> > >> +
> > >> +uint64_t x86_cpu_rdmsr(CPUX86State *env, uint32_t idx, bool *valid)
> > >> +{
> > >> + uint64_t val;
> > >> + *valid = true;
> > >> +
> > >> + switch (idx) {
> > >> + case MSR_IA32_SYSENTER_CS:
> > >> + val = env->sysenter_cs;
> > >> + break;
> > >> + case MSR_IA32_SYSENTER_ESP:
> > >> + val = env->sysenter_esp;
> > >> + break;
> > >> + case MSR_IA32_SYSENTER_EIP:
> > >> + val = env->sysenter_eip;
> > >> + break;
> > >> + case MSR_IA32_APICBASE:
> > >> + val = cpu_get_apic_base(x86_env_get_cpu(env)->apic_state);
> > >> + break;
> > >> + case MSR_EFER:
> > >> + val = env->efer;
> > >> + break;
> > >> + case MSR_STAR:
> > >> + val = env->star;
> > >> + break;
> > >> + case MSR_PAT:
> > >> + val = env->pat;
> > >> + break;
> > >> + case MSR_VM_HSAVE_PA:
> > >> + val = env->vm_hsave;
> > >> + break;
> > >> + case MSR_IA32_PERF_STATUS:
> > >> + /* tsc_increment_by_tick */
> > >> + val = 1000ULL;
> > >> + /* CPU multiplier */
> > >> + val |= (((uint64_t)4ULL) << 40);
> > >> + break;
> > >> +#ifdef TARGET_X86_64
> > >> + case MSR_LSTAR:
> > >> + val = env->lstar;
> > >> + break;
> > >> + case MSR_CSTAR:
> > >> + val = env->cstar;
> > >> + break;
> > >> + case MSR_FMASK:
> > >> + val = env->fmask;
> > >> + break;
> > >> + case MSR_FSBASE:
> > >> + val = env->segs[R_FS].base;
> > >> + break;
> > >> + case MSR_GSBASE:
> > >> + val = env->segs[R_GS].base;
> > >> + break;
> > >> + case MSR_KERNELGSBASE:
> > >> + val = env->kernelgsbase;
> > >> + break;
> > >> + case MSR_TSC_AUX:
> > >> + val = env->tsc_aux;
> > >> + break;
> > >> +#endif
> > >> + case MSR_MTRRphysBase(0):
> > >> + case MSR_MTRRphysBase(1):
> > >> + case MSR_MTRRphysBase(2):
> > >> + case MSR_MTRRphysBase(3):
> > >> + case MSR_MTRRphysBase(4):
> > >> + case MSR_MTRRphysBase(5):
> > >> + case MSR_MTRRphysBase(6):
> > >> + case MSR_MTRRphysBase(7):
> > >> + val = env->mtrr_var[(idx - MSR_MTRRphysBase(0)) / 2].base;
> > >> + break;
> > >> + case MSR_MTRRphysMask(0):
> > >> + case MSR_MTRRphysMask(1):
> > >> + case MSR_MTRRphysMask(2):
> > >> + case MSR_MTRRphysMask(3):
> > >> + case MSR_MTRRphysMask(4):
> > >> + case MSR_MTRRphysMask(5):
> > >> + case MSR_MTRRphysMask(6):
> > >> + case MSR_MTRRphysMask(7):
> > >> + val = env->mtrr_var[(idx - MSR_MTRRphysMask(0)) / 2].mask;
> > >> + break;
> > >> + case MSR_MTRRfix64K_00000:
> > >> + val = env->mtrr_fixed[0];
> > >> + break;
> > >> + case MSR_MTRRfix16K_80000:
> > >> + case MSR_MTRRfix16K_A0000:
> > >> + val = env->mtrr_fixed[idx - MSR_MTRRfix16K_80000 + 1];
> > >> + break;
> > >> + case MSR_MTRRfix4K_C0000:
> > >> + case MSR_MTRRfix4K_C8000:
> > >> + case MSR_MTRRfix4K_D0000:
> > >> + case MSR_MTRRfix4K_D8000:
> > >> + case MSR_MTRRfix4K_E0000:
> > >> + case MSR_MTRRfix4K_E8000:
> > >> + case MSR_MTRRfix4K_F0000:
> > >> + case MSR_MTRRfix4K_F8000:
> > >> + val = env->mtrr_fixed[idx - MSR_MTRRfix4K_C0000 + 3];
> > >> + break;
> > >> + case MSR_MTRRdefType:
> > >> + val = env->mtrr_deftype;
> > >> + break;
> > >> + case MSR_MTRRcap:
> > >> + if (env->features[FEAT_1_EDX] & CPUID_MTRR) {
> > >> + val = MSR_MTRRcap_VCNT | MSR_MTRRcap_FIXRANGE_SUPPORT |
> > >> + MSR_MTRRcap_WC_SUPPORTED;
> > >> + } else {
> > >> + *valid = false;
> > >> + val = 0;
> > >
> > > This had a "XXX: exception?" comment on the original code, so I
> > > guess this matches the original intention behind it.
> > >
> > >> + }
> > >> + break;
> > >> + case MSR_MCG_CAP:
> > >> + val = env->mcg_cap;
> > >> + break;
> > >> + case MSR_MCG_CTL:
> > >> + if (env->mcg_cap & MCG_CTL_P) {
> > >> + val = env->mcg_ctl;
> > >> + } else {
> > >> + *valid = false;
> > >> + val = 0;
> > >
> > > I understand the intention behind setting *valid = false here,
> > > but the existing kvm_put_msrs() code tries to set MSR_MCG_CTL
> > > even if (mcg_cap & MCG_CTL_P) is not set. We need to keep that in
> > > mind if we convert kvm_{get,put}_msrs() to use the wrmsr/rdmsr
> > > helpers.
> > >
> > > Also, if x86_cpu_rdmsr() is set *valid = false here, we probably
> > > should set *valid = false at x86_cpu_wrmsr() too[***} for
> > > consistency.
> > >
> > >> + }
> > >> + break;
> > >> + case MSR_MCG_STATUS:
> > >> + val = env->mcg_status;
> > >> + break;
> > >> + case MSR_IA32_MISC_ENABLE:
> > >> + val = env->msr_ia32_misc_enable;
> > >> + break;
> > >> + case MSR_IA32_BNDCFGS:
> > >> + val = env->msr_bndcfgs;
> > >> + break;
> > >> + default:
> > >> + if (idx >= MSR_MC0_CTL && idx < MSR_MC0_CTL +
> > >> + (4 * env->mcg_cap & 0xff)) {
> > >> + val = env->mce_banks[idx - MSR_MC0_CTL];
> > >
> > > The original helper_rdmsr() code had a "offset" variable. Not
> > > important, as the new code is equivalent, but it would make the
> > > equivalency between x86_cpu_rdmsr() and x86_cpu_wrmsr() more
> > > visible.
> > >
> > >> + break;
> > >> + }
> > >> + *valid = false;
> > >> + val = 0;
> > >> + break;
> > >> + }
> > >> + return val;
> > >> +}
> > >> #else
> > >> void x86_cpu_dump_local_apic_state(CPUState *cs, FILE *f,
> > >> fprintf_function cpu_fprintf, int
> > >> flags)
> > >> {
> > >> }
> > >> +void x86_cpu_wrmsr(CPUX86State *env, uint32_t idx, uint64_t val, bool
> > >> *valid)
> > >> +{
> > >> + *valid = false;
> > >> +}
> > >> +uint64_t x86_cpu_rdmsr(CPUX86State *env, uint32_t idx, bool *valid)
> > >> +{
> > >> + *valid = false;
> > >> + return 0ULL;
> > >> +}
> > >> #endif /* !CONFIG_USER_ONLY */
> > >>
> > >> #define DUMP_CODE_BYTES_TOTAL 50
> > >> diff --git a/target/i386/misc_helper.c b/target/i386/misc_helper.c
> > >> index ca2ea09f54..fbbf9d146e 100644
> > >> --- a/target/i386/misc_helper.c
> > >> +++ b/target/i386/misc_helper.c
> > >> @@ -227,307 +227,32 @@ void helper_rdmsr(CPUX86State *env)
> > >> void helper_wrmsr(CPUX86State *env)
> > >> {
> > >> uint64_t val;
> > >> + bool res_valid;
> > >>
> > >> cpu_svm_check_intercept_param(env, SVM_EXIT_MSR, 1, GETPC());
> > >>
> > >> val = ((uint32_t)env->regs[R_EAX]) |
> > >> ((uint64_t)((uint32_t)env->regs[R_EDX]) << 32);
> > >>
> > >> - switch ((uint32_t)env->regs[R_ECX]) {
> > >> - case MSR_IA32_SYSENTER_CS:
> > >> - env->sysenter_cs = val & 0xffff;
> > >> - break;
> > >> - case MSR_IA32_SYSENTER_ESP:
> > >> - env->sysenter_esp = val;
> > >> - break;
> > >> - case MSR_IA32_SYSENTER_EIP:
> > >> - env->sysenter_eip = val;
> > >> - break;
> > >> - case MSR_IA32_APICBASE:
> > >> - cpu_set_apic_base(x86_env_get_cpu(env)->apic_state, val);
> > >> - break;
> > >> - case MSR_EFER:
> > >> - {
> > >> - uint64_t update_mask;
> > >> + x86_cpu_wrmsr(env, (uint32_t)env->regs[R_ECX], val, &res_valid);
> > >> +
> > >> + /* XXX: exception? */
> > >> + if (!res_valid) { }
> > >>
> > >> - update_mask = 0;
> > >> - if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_SYSCALL)
> > >> {
> > >> - update_mask |= MSR_EFER_SCE;
> > >> - }
> > >> - if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
> > >> - update_mask |= MSR_EFER_LME;
> > >> - }
> > >> - if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_FFXSR) {
> > >> - update_mask |= MSR_EFER_FFXSR;
> > >> - }
> > >> - if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_NX) {
> > >> - update_mask |= MSR_EFER_NXE;
> > >> - }
> > >> - if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) {
> > >> - update_mask |= MSR_EFER_SVME;
> > >> - }
> > >> - if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_FFXSR) {
> > >> - update_mask |= MSR_EFER_FFXSR;
> > >> - }
> > >> - cpu_load_efer(env, (env->efer & ~update_mask) |
> > >> - (val & update_mask));
> > >> - }
> > >> - break;
> > >> - case MSR_STAR:
> > >> - env->star = val;
> > >> - break;
> > >> - case MSR_PAT:
> > >> - env->pat = val;
> > >> - break;
> > >> - case MSR_VM_HSAVE_PA:
> > >> - env->vm_hsave = val;
> > >> - break;
> > >> -#ifdef TARGET_X86_64
> > >> - case MSR_LSTAR:
> > >> - env->lstar = val;
> > >> - break;
> > >> - case MSR_CSTAR:
> > >> - env->cstar = val;
> > >> - break;
> > >> - case MSR_FMASK:
> > >> - env->fmask = val;
> > >> - break;
> > >> - case MSR_FSBASE:
> > >> - env->segs[R_FS].base = val;
> > >> - break;
> > >> - case MSR_GSBASE:
> > >> - env->segs[R_GS].base = val;
> > >> - break;
> > >> - case MSR_KERNELGSBASE:
> > >> - env->kernelgsbase = val;
> > >> - break;
> > >> -#endif
> > >> - case MSR_MTRRphysBase(0):
> > >> - case MSR_MTRRphysBase(1):
> > >> - case MSR_MTRRphysBase(2):
> > >> - case MSR_MTRRphysBase(3):
> > >> - case MSR_MTRRphysBase(4):
> > >> - case MSR_MTRRphysBase(5):
> > >> - case MSR_MTRRphysBase(6):
> > >> - case MSR_MTRRphysBase(7):
> > >> - env->mtrr_var[((uint32_t)env->regs[R_ECX] -
> > >> - MSR_MTRRphysBase(0)) / 2].base = val;
> > >> - break;
> > >> - case MSR_MTRRphysMask(0):
> > >> - case MSR_MTRRphysMask(1):
> > >> - case MSR_MTRRphysMask(2):
> > >> - case MSR_MTRRphysMask(3):
> > >> - case MSR_MTRRphysMask(4):
> > >> - case MSR_MTRRphysMask(5):
> > >> - case MSR_MTRRphysMask(6):
> > >> - case MSR_MTRRphysMask(7):
> > >> - env->mtrr_var[((uint32_t)env->regs[R_ECX] -
> > >> - MSR_MTRRphysMask(0)) / 2].mask = val;
> > >> - break;
> > >> - case MSR_MTRRfix64K_00000:
> > >> - env->mtrr_fixed[(uint32_t)env->regs[R_ECX] -
> > >> - MSR_MTRRfix64K_00000] = val;
> > >> - break;
> > >> - case MSR_MTRRfix16K_80000:
> > >> - case MSR_MTRRfix16K_A0000:
> > >> - env->mtrr_fixed[(uint32_t)env->regs[R_ECX] -
> > >> - MSR_MTRRfix16K_80000 + 1] = val;
> > >> - break;
> > >> - case MSR_MTRRfix4K_C0000:
> > >> - case MSR_MTRRfix4K_C8000:
> > >> - case MSR_MTRRfix4K_D0000:
> > >> - case MSR_MTRRfix4K_D8000:
> > >> - case MSR_MTRRfix4K_E0000:
> > >> - case MSR_MTRRfix4K_E8000:
> > >> - case MSR_MTRRfix4K_F0000:
> > >> - case MSR_MTRRfix4K_F8000:
> > >> - env->mtrr_fixed[(uint32_t)env->regs[R_ECX] -
> > >> - MSR_MTRRfix4K_C0000 + 3] = val;
> > >> - break;
> > >> - case MSR_MTRRdefType:
> > >> - env->mtrr_deftype = val;
> > >> - break;
> > >> - case MSR_MCG_STATUS:
> > >> - env->mcg_status = val;
> > >> - break;
> > >> - case MSR_MCG_CTL:
> > >> - if ((env->mcg_cap & MCG_CTL_P)
> > >> - && (val == 0 || val == ~(uint64_t)0)) {
> > >> - env->mcg_ctl = val;
> > >> - }
> > >> - break;
> > >> - case MSR_TSC_AUX:
> > >> - env->tsc_aux = val;
> > >> - break;
> > >> - case MSR_IA32_MISC_ENABLE:
> > >> - env->msr_ia32_misc_enable = val;
> > >> - break;
> > >> - case MSR_IA32_BNDCFGS:
> > >> - /* FIXME: #GP if reserved bits are set. */
> > >> - /* FIXME: Extend highest implemented bit of linear address. */
> > >> - env->msr_bndcfgs = val;
> > >> - cpu_sync_bndcs_hflags(env);
> > >> - break;
> > >> - default:
> > >> - if ((uint32_t)env->regs[R_ECX] >= MSR_MC0_CTL
> > >> - && (uint32_t)env->regs[R_ECX] < MSR_MC0_CTL +
> > >> - (4 * env->mcg_cap & 0xff)) {
> > >> - uint32_t offset = (uint32_t)env->regs[R_ECX] - MSR_MC0_CTL;
> > >> - if ((offset & 0x3) != 0
> > >> - || (val == 0 || val == ~(uint64_t)0)) {
> > >> - env->mce_banks[offset] = val;
> > >> - }
> > >> - break;
> > >> - }
> > >> - /* XXX: exception? */
> > >> - break;
> > >> - }
> > >> }
> > >>
> > >> void helper_rdmsr(CPUX86State *env)
> > >> {
> > >> uint64_t val;
> > >> + bool res_valid;
> > >>
> > >> cpu_svm_check_intercept_param(env, SVM_EXIT_MSR, 0, GETPC());
> > >>
> > >> - switch ((uint32_t)env->regs[R_ECX]) {
> > >> - case MSR_IA32_SYSENTER_CS:
> > >> - val = env->sysenter_cs;
> > >> - break;
> > >> - case MSR_IA32_SYSENTER_ESP:
> > >> - val = env->sysenter_esp;
> > >> - break;
> > >> - case MSR_IA32_SYSENTER_EIP:
> > >> - val = env->sysenter_eip;
> > >> - break;
> > >> - case MSR_IA32_APICBASE:
> > >> - val = cpu_get_apic_base(x86_env_get_cpu(env)->apic_state);
> > >> - break;
> > >> - case MSR_EFER:
> > >> - val = env->efer;
> > >> - break;
> > >> - case MSR_STAR:
> > >> - val = env->star;
> > >> - break;
> > >> - case MSR_PAT:
> > >> - val = env->pat;
> > >> - break;
> > >> - case MSR_VM_HSAVE_PA:
> > >> - val = env->vm_hsave;
> > >> - break;
> > >> - case MSR_IA32_PERF_STATUS:
> > >> - /* tsc_increment_by_tick */
> > >> - val = 1000ULL;
> > >> - /* CPU multiplier */
> > >> - val |= (((uint64_t)4ULL) << 40);
> > >> - break;
> > >> -#ifdef TARGET_X86_64
> > >> - case MSR_LSTAR:
> > >> - val = env->lstar;
> > >> - break;
> > >> - case MSR_CSTAR:
> > >> - val = env->cstar;
> > >> - break;
> > >> - case MSR_FMASK:
> > >> - val = env->fmask;
> > >> - break;
> > >> - case MSR_FSBASE:
> > >> - val = env->segs[R_FS].base;
> > >> - break;
> > >> - case MSR_GSBASE:
> > >> - val = env->segs[R_GS].base;
> > >> - break;
> > >> - case MSR_KERNELGSBASE:
> > >> - val = env->kernelgsbase;
> > >> - break;
> > >> - case MSR_TSC_AUX:
> > >> - val = env->tsc_aux;
> > >> - break;
> > >> -#endif
> > >> - case MSR_MTRRphysBase(0):
> > >> - case MSR_MTRRphysBase(1):
> > >> - case MSR_MTRRphysBase(2):
> > >> - case MSR_MTRRphysBase(3):
> > >> - case MSR_MTRRphysBase(4):
> > >> - case MSR_MTRRphysBase(5):
> > >> - case MSR_MTRRphysBase(6):
> > >> - case MSR_MTRRphysBase(7):
> > >> - val = env->mtrr_var[((uint32_t)env->regs[R_ECX] -
> > >> - MSR_MTRRphysBase(0)) / 2].base;
> > >> - break;
> > >> - case MSR_MTRRphysMask(0):
> > >> - case MSR_MTRRphysMask(1):
> > >> - case MSR_MTRRphysMask(2):
> > >> - case MSR_MTRRphysMask(3):
> > >> - case MSR_MTRRphysMask(4):
> > >> - case MSR_MTRRphysMask(5):
> > >> - case MSR_MTRRphysMask(6):
> > >> - case MSR_MTRRphysMask(7):
> > >> - val = env->mtrr_var[((uint32_t)env->regs[R_ECX] -
> > >> - MSR_MTRRphysMask(0)) / 2].mask;
> > >> - break;
> > >> - case MSR_MTRRfix64K_00000:
> > >> - val = env->mtrr_fixed[0];
> > >> - break;
> > >> - case MSR_MTRRfix16K_80000:
> > >> - case MSR_MTRRfix16K_A0000:
> > >> - val = env->mtrr_fixed[(uint32_t)env->regs[R_ECX] -
> > >> - MSR_MTRRfix16K_80000 + 1];
> > >> - break;
> > >> - case MSR_MTRRfix4K_C0000:
> > >> - case MSR_MTRRfix4K_C8000:
> > >> - case MSR_MTRRfix4K_D0000:
> > >> - case MSR_MTRRfix4K_D8000:
> > >> - case MSR_MTRRfix4K_E0000:
> > >> - case MSR_MTRRfix4K_E8000:
> > >> - case MSR_MTRRfix4K_F0000:
> > >> - case MSR_MTRRfix4K_F8000:
> > >> - val = env->mtrr_fixed[(uint32_t)env->regs[R_ECX] -
> > >> - MSR_MTRRfix4K_C0000 + 3];
> > >> - break;
> > >> - case MSR_MTRRdefType:
> > >> - val = env->mtrr_deftype;
> > >> - break;
> > >> - case MSR_MTRRcap:
> > >> - if (env->features[FEAT_1_EDX] & CPUID_MTRR) {
> > >> - val = MSR_MTRRcap_VCNT | MSR_MTRRcap_FIXRANGE_SUPPORT |
> > >> - MSR_MTRRcap_WC_SUPPORTED;
> > >> - } else {
> > >> - /* XXX: exception? */
> > >> - val = 0;
> > >> - }
> > >> - break;
> > >> - case MSR_MCG_CAP:
> > >> - val = env->mcg_cap;
> > >> - break;
> > >> - case MSR_MCG_CTL:
> > >> - if (env->mcg_cap & MCG_CTL_P) {
> > >> - val = env->mcg_ctl;
> > >> - } else {
> > >> - val = 0;
> > >> - }
> > >> - break;
> > >> - case MSR_MCG_STATUS:
> > >> - val = env->mcg_status;
> > >> - break;
> > >> - case MSR_IA32_MISC_ENABLE:
> > >> - val = env->msr_ia32_misc_enable;
> > >> - break;
> > >> - case MSR_IA32_BNDCFGS:
> > >> - val = env->msr_bndcfgs;
> > >> - break;
> > >> - default:
> > >> - if ((uint32_t)env->regs[R_ECX] >= MSR_MC0_CTL
> > >> - && (uint32_t)env->regs[R_ECX] < MSR_MC0_CTL +
> > >> - (4 * env->mcg_cap & 0xff)) {
> > >> - uint32_t offset = (uint32_t)env->regs[R_ECX] - MSR_MC0_CTL;
> > >> - val = env->mce_banks[offset];
> > >> - break;
> > >> - }
> > >> - /* XXX: exception? */
> > >> - val = 0;
> > >> - break;
> > >> - }
> > >> + val = x86_cpu_rdmsr(env, (uint32_t)env->regs[R_ECX], &res_valid);
> > >> +
> > >> + /* XXX: exception? */
> > >> + if (!res_valid) { }
> > >> +
> > >> env->regs[R_EAX] = (uint32_t)(val);
> > >> env->regs[R_EDX] = (uint32_t)(val >> 32);
> > >> }
> > >> --
> > >> 2.11.0
> > >>
> > >
>
> --
> Eduardo
>
next prev parent reply other threads:[~2017-04-19 9:44 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20170418073946.4177-1-git@kirschju.re>
[not found] ` <20170418073946.4177-2-git@kirschju.re>
2017-04-18 15:28 ` [Qemu-devel] [PATCH RESEND v4 1/4] X86: Move rdmsr/wrmsr functionality to standalone functions Eduardo Habkost
2017-04-18 15:38 ` Paolo Bonzini
2017-04-18 18:16 ` Eduardo Habkost
2017-04-19 9:44 ` Paolo Bonzini [this message]
[not found] ` <20170418073946.4177-5-git@kirschju.re>
2017-04-18 18:31 ` [Qemu-devel] [PATCH RESEND v4 4/4] HMP: Introduce msr_get and msr_set HMP commands Eduardo Habkost
2017-04-24 16:32 ` Dr. David Alan Gilbert
2017-04-24 20:25 ` Julian Kirsch
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=311531997.14466224.1492595046139.JavaMail.zimbra@redhat.com \
--to=pbonzini@redhat.com \
--cc=crosthwaite.peter@gmail.com \
--cc=dgilbert@redhat.com \
--cc=eblake@redhat.com \
--cc=ehabkost@redhat.com \
--cc=git@kirschju.re \
--cc=qemu-devel@nongnu.org \
--cc=rth@twiddle.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.