Re: [Qemu-devel] [PATCH V2] Resend - Enabling IA32_TSC_ADJUST for QEMU KVM guest VMs

[Auld, Will]

Andreas, 

Thanks. I just sent the update patch (V3) to address your comments. 

Will

> -----Original Message-----
> From: address@hidden [mailto:address@hidden On
> Behalf Of Andreas Färber
> Sent: Monday, November 26, 2012 11:05 AM
> To: Auld, Will
> Cc: Will Auld; qemu-devel; Gleb; Marcelo Tosatti; address@hidden;
> Dugger, Donald D; Liu, Jinsong; Zhang, Xiantao; address@hidden
> Subject: Re: [Qemu-devel] [PATCH V2] Resend - Enabling IA32_TSC_ADJUST
> for Qemu KVM guest VMs
> 
> Hello,
> 
> Am 26.11.2012 19:42, schrieb Will Auld:
> > CPUID.7.0.EBX[1]=1 indicates IA32_TSC_ADJUST MSR 0x3b is supported
> >
> > Basic design is to emulate the MSR by allowing reads and writes to
> the
> > hypervisor vcpu specific locations to store the value of the emulated
> MSRs.
> > In this way the IA32_TSC_ADJUST value will be included in all reads
> to
> > the TSC MSR whether through rdmsr or rdtsc.
> >
> > As this is a new MSR that the guest may access and modify its value
> > needs to be migrated along with the other MRSs. The changes here are
> > specifically for recognizing when IA32_TSC_ADJUST is enabled in CPUID
> > and code added for migrating its value.
> >
> > Signed-off-by: Will Auld <address@hidden>
> 
> $subject should get a prefix of "target-i386: " and "resend" is better
> used inside a tag so that it doesn't end up in the commit.
> And it's "QEMU". ;)
> 
> Some more stylistic issues inline:
> 
> > ---
> >  target-i386/cpu.h     |  2 ++
> >  target-i386/kvm.c     | 15 +++++++++++++++
> >  target-i386/machine.c | 21 +++++++++++++++++++++
> >  3 files changed, 38 insertions(+)
> >
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h index
> > aabf993..13d4152 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -284,6 +284,7 @@
> >  #define MSR_IA32_APICBASE_BSP           (1<<8)
> >  #define MSR_IA32_APICBASE_ENABLE        (1<<11)
> >  #define MSR_IA32_APICBASE_BASE          (0xfffff<<12)
> > +#define MSR_TSC_ADJUST                     0x0000003b
> 
> Tabs. You can use scripts/checkpatch.pl to verify.
> 
> >  #define MSR_IA32_TSCDEADLINE            0x6e0
> >
> >  #define MSR_MTRRcap                        0xfe
> > @@ -701,6 +702,7 @@ typedef struct CPUX86State {
> >      uint64_t async_pf_en_msr;
> >
> >      uint64_t tsc;
> > +    uint64_t tsc_adjust;
> >      uint64_t tsc_deadline;
> >
> >      uint64_t mcg_status;
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c index
> > 696b14a..e974c42 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -61,6 +61,7 @@ const KVMCapabilityInfo
> > kvm_arch_required_capabilities[] = {
> >
> >  static bool has_msr_star;
> >  static bool has_msr_hsave_pa;
> > +static bool has_msr_tsc_adjust;
> >  static bool has_msr_tsc_deadline;
> >  static bool has_msr_async_pf_en;
> >  static bool has_msr_misc_enable;
> > @@ -641,6 +642,10 @@ static int kvm_get_supported_msrs(KVMState *s)
> >                      has_msr_hsave_pa = true;
> >                      continue;
> >                  }
> > +                if (kvm_msr_list->indices[i] == MSR_TSC_ADJUST) {
> > +                    has_msr_tsc_adjust = true;
> > +                    continue;
> > +                }
> >                  if (kvm_msr_list->indices[i] ==
> MSR_IA32_TSCDEADLINE) {
> >                      has_msr_tsc_deadline = true;
> >                      continue;
> > @@ -978,6 +983,10 @@ static int kvm_put_msrs(CPUX86State *env, int
> level)
> >      if (has_msr_hsave_pa) {
> >          kvm_msr_entry_set(&msrs[n++], MSR_VM_HSAVE_PA, env-
> >vm_hsave);
> >      }
> > +    if (has_msr_tsc_adjust) {
> > +        kvm_msr_entry_set(&msrs[n++],
> > +                   MSR_TSC_ADJUST, env->tsc_adjust);
> 
> Tabs.
> 
> > +    }
> >      if (has_msr_tsc_deadline) {
> >          kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSCDEADLINE, env-
> >tsc_deadline);
> >      }
> > @@ -1234,6 +1243,9 @@ static int kvm_get_msrs(CPUX86State *env)
> >      if (has_msr_hsave_pa) {
> >          msrs[n++].index = MSR_VM_HSAVE_PA;
> >      }
> > +    if (has_msr_tsc_adjust) {
> > +        msrs[n++].index = MSR_TSC_ADJUST;
> > +    }
> >      if (has_msr_tsc_deadline) {
> >          msrs[n++].index = MSR_IA32_TSCDEADLINE;
> >      }
> > @@ -1308,6 +1320,9 @@ static int kvm_get_msrs(CPUX86State *env)
> >          case MSR_IA32_TSC:
> >              env->tsc = msrs[i].data;
> >              break;
> > +        case MSR_TSC_ADJUST:
> > +            env->tsc_adjust = msrs[i].data;
> > +            break;
> >          case MSR_IA32_TSCDEADLINE:
> >              env->tsc_deadline = msrs[i].data;
> >              break;
> > diff --git a/target-i386/machine.c b/target-i386/machine.c index
> > a8be058..95bda9b 100644
> > --- a/target-i386/machine.c
> > +++ b/target-i386/machine.c
> > @@ -310,6 +310,24 @@ static const VMStateDescription
> vmstate_fpop_ip_dp = {
> >      }
> >  };
> >
> > +static bool tsc_adjust_needed(void *opaque) {
> > +    CPUX86State *cpu = opaque;
> 
> Please name this "env" to differentiate from CPUState / X86CPU.
> Since there are other tsc_* fields already I won't request that you
> move your new field to the containing X86CPU struct but at some point
> we will need to convert the VMSDs to X86CPU.
> 
> > +
> > +    return cpu->tsc_adjust != 0;
> > +}
> > +
> > +static const VMStateDescription vmstate_msr_tsc_adjust = {
> > +    .name = "cpu/msr_tsc_adjust",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .minimum_version_id_old = 1,
> > +    .fields      = (VMStateField []) {
> > +        VMSTATE_UINT64(tsc_adjust, CPUX86State),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  static bool tscdeadline_needed(void *opaque)  {
> >      CPUX86State *env = opaque;
> > @@ -457,6 +475,9 @@ static const VMStateDescription vmstate_cpu = {
> >              .vmsd = &vmstate_fpop_ip_dp,
> >              .needed = fpop_ip_dp_needed,
> >          }, {
> > +            .vmsd = &vmstate_msr_tsc_adjust,
> > +            .needed = tsc_adjust_needed,
> > +        }, {
> >              .vmsd = &vmstate_msr_tscdeadline,
> >              .needed = tscdeadline_needed,
> >          }, {
> 
> Otherwise looks okay to me. I'm expecting this to go through Marcello's
> queue unless I'm told otherwise.
> 
> Regards,
> Andreas
> 
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in the
> body of a message to address@hidden More majordomo info at
> http://vger.kernel.org/majordomo-info.html