diff mbox

[2/5] x86: Mask mtrr mask based on CPU physical address limits

Message ID 1466097133-5489-3-git-send-email-dgilbert@redhat.com
State New
Headers show

Commit Message

Dr. David Alan Gilbert June 16, 2016, 5:12 p.m. UTC 
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The CPU GPs if we try and set a bit in a variable MTRR mask above
the limit of physical address bits on the host.  We hit this
when loading a migration from a host with a larger physical
address limit than our destination (e.g. a Xeon->i7 of same
generation) but previously used to get away with it
until 48e1a45 started checking that msr writes actually worked.

It seems in our case the GP probably comes from KVM emulating
that GP.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 target-i386/cpu.c | 13 +++++++++++++
 target-i386/cpu.h |  1 +
 target-i386/kvm.c | 13 +++++++++++--
 3 files changed, 25 insertions(+), 2 deletions(-)

Comments

Eduardo Habkost June 16, 2016, 7:59 p.m. UTC | #1 
On Thu, Jun 16, 2016 at 06:12:10PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> The CPU GPs if we try and set a bit in a variable MTRR mask above
> the limit of physical address bits on the host.  We hit this
> when loading a migration from a host with a larger physical
> address limit than our destination (e.g. a Xeon->i7 of same
> generation) but previously used to get away with it
> until 48e1a45 started checking that msr writes actually worked.
> 
> It seems in our case the GP probably comes from KVM emulating
> that GP.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  target-i386/cpu.c | 13 +++++++++++++
>  target-i386/cpu.h |  1 +
>  target-i386/kvm.c | 13 +++++++++++--
>  3 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3665fec..f9302f9 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2668,6 +2668,19 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>      }
>  }
>  
> +/* Returns the number of physical bits supported by the guest CPU */
> +unsigned int cpu_x86_guest_phys_address_bits(CPUX86State *env)
> +{
> +    /* the cpuid emulation code already calculates a value to give to the
> +     * guest and this should match.
> +     */
> +    uint32_t eax, unused;
> +    cpu_x86_cpuid(env, 0x80000008, 0, &eax, &unused, &unused, &unused);
> +
> +    /* Bottom 8 bits of leaf 0x80000008 see Table 3-17 in CPUID definition */
> +    return eax & 0xff;
> +}

If you are adding X86CPU::phys_bits in patch 4/5, can't
kvm_put_msrs() just query it directly?

(It would require changing patch 5/5 to set phys_bits on
realizefn, like Paolo suggested.)

[...]
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index abf50e6..d95d06b 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -1674,6 +1674,10 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
>              }
>          }
>          if (has_msr_mtrr) {
> +            uint64_t phys_mask = BIT_RANGE(
> +                                   cpu_x86_guest_phys_address_bits(env) - 1,
> +                                   0);
> +
>              kvm_msr_entry_add(cpu, MSR_MTRRdefType, env->mtrr_deftype);
>              kvm_msr_entry_add(cpu, MSR_MTRRfix64K_00000, env->mtrr_fixed[0]);
>              kvm_msr_entry_add(cpu, MSR_MTRRfix16K_80000, env->mtrr_fixed[1]);
> @@ -1687,10 +1691,15 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
>              kvm_msr_entry_add(cpu, MSR_MTRRfix4K_F0000, env->mtrr_fixed[9]);
>              kvm_msr_entry_add(cpu, MSR_MTRRfix4K_F8000, env->mtrr_fixed[10]);
>              for (i = 0; i < MSR_MTRRcap_VCNT; i++) {
> +                /* The CPU GPs if we write to a bit above the physical limit of
> +                 * the host CPU (and KVM emulates that)
> +                 */
> +                uint64_t mask = env->mtrr_var[i].mask;
> +                mask &= phys_mask;
> +

We are silently changing the MSR value seen by the guest, should
we print a warning in case mask != env->mtrr_var[i].mask?

>                  kvm_msr_entry_add(cpu, MSR_MTRRphysBase(i),
>                                    env->mtrr_var[i].base);
> -                kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i),
> -                                  env->mtrr_var[i].mask);
> +                kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
>              }
>          }
>  
> -- 
> 2.7.4
>
Dr. David Alan Gilbert June 17, 2016, 8:23 a.m. UTC | #2 
* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Thu, Jun 16, 2016 at 06:12:10PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > The CPU GPs if we try and set a bit in a variable MTRR mask above
> > the limit of physical address bits on the host.  We hit this
> > when loading a migration from a host with a larger physical
> > address limit than our destination (e.g. a Xeon->i7 of same
> > generation) but previously used to get away with it
> > until 48e1a45 started checking that msr writes actually worked.
> > 
> > It seems in our case the GP probably comes from KVM emulating
> > that GP.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  target-i386/cpu.c | 13 +++++++++++++
> >  target-i386/cpu.h |  1 +
> >  target-i386/kvm.c | 13 +++++++++++--
> >  3 files changed, 25 insertions(+), 2 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 3665fec..f9302f9 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2668,6 +2668,19 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> >      }
> >  }
> >  
> > +/* Returns the number of physical bits supported by the guest CPU */
> > +unsigned int cpu_x86_guest_phys_address_bits(CPUX86State *env)
> > +{
> > +    /* the cpuid emulation code already calculates a value to give to the
> > +     * guest and this should match.
> > +     */
> > +    uint32_t eax, unused;
> > +    cpu_x86_cpuid(env, 0x80000008, 0, &eax, &unused, &unused, &unused);
> > +
> > +    /* Bottom 8 bits of leaf 0x80000008 see Table 3-17 in CPUID definition */
> > +    return eax & 0xff;
> > +}
> 
> If you are adding X86CPU::phys_bits in patch 4/5, can't
> kvm_put_msrs() just query it directly?
> 
> (It would require changing patch 5/5 to set phys_bits on
> realizefn, like Paolo suggested.)

Yes, I think that's doable.

> [...]
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index abf50e6..d95d06b 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -1674,6 +1674,10 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> >              }
> >          }
> >          if (has_msr_mtrr) {
> > +            uint64_t phys_mask = BIT_RANGE(
> > +                                   cpu_x86_guest_phys_address_bits(env) - 1,
> > +                                   0);
> > +
> >              kvm_msr_entry_add(cpu, MSR_MTRRdefType, env->mtrr_deftype);
> >              kvm_msr_entry_add(cpu, MSR_MTRRfix64K_00000, env->mtrr_fixed[0]);
> >              kvm_msr_entry_add(cpu, MSR_MTRRfix16K_80000, env->mtrr_fixed[1]);
> > @@ -1687,10 +1691,15 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> >              kvm_msr_entry_add(cpu, MSR_MTRRfix4K_F0000, env->mtrr_fixed[9]);
> >              kvm_msr_entry_add(cpu, MSR_MTRRfix4K_F8000, env->mtrr_fixed[10]);
> >              for (i = 0; i < MSR_MTRRcap_VCNT; i++) {
> > +                /* The CPU GPs if we write to a bit above the physical limit of
> > +                 * the host CPU (and KVM emulates that)
> > +                 */
> > +                uint64_t mask = env->mtrr_var[i].mask;
> > +                mask &= phys_mask;
> > +
> 
> We are silently changing the MSR value seen by the guest, should
> we print a warning in case mask != env->mtrr_var[i].mask?

That would work, except that now we fill in the extra bits we'll always
get that warning.

Dave

> 
> >                  kvm_msr_entry_add(cpu, MSR_MTRRphysBase(i),
> >                                    env->mtrr_var[i].base);
> > -                kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i),
> > -                                  env->mtrr_var[i].mask);
> > +                kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> >              }
> >          }
> >  
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Eduardo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Paolo Bonzini June 17, 2016, 12:13 p.m. UTC | #3 
On 16/06/2016 21:59, Eduardo Habkost wrote:
> > +                /* The CPU GPs if we write to a bit above the physical limit of
> > +                 * the host CPU (and KVM emulates that)
> > +                 */
> > +                uint64_t mask = env->mtrr_var[i].mask;
> > +                mask &= phys_mask;
> > +
> 
> We are silently changing the MSR value seen by the guest, should
> we print a warning in case mask != env->mtrr_var[i].mask?

You're right.  post_load is probably a better place to remove these bits
(and to fail if they are not all ones).

Paolo
diff mbox

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3665fec..f9302f9 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2668,6 +2668,19 @@  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     }
 }
 
+/* Returns the number of physical bits supported by the guest CPU */
+unsigned int cpu_x86_guest_phys_address_bits(CPUX86State *env)
+{
+    /* the cpuid emulation code already calculates a value to give to the
+     * guest and this should match.
+     */
+    uint32_t eax, unused;
+    cpu_x86_cpuid(env, 0x80000008, 0, &eax, &unused, &unused, &unused);
+
+    /* Bottom 8 bits of leaf 0x80000008 see Table 3-17 in CPUID definition */
+    return eax & 0xff;
+}
+
 /* CPUClass::reset() */
 static void x86_cpu_reset(CPUState *s)
 {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index d9ab884..61c06b7 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1238,6 +1238,7 @@  X86CPU *cpu_x86_create(const char *cpu_model, Error **errp);
 int cpu_x86_exec(CPUState *cpu);
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 int cpu_x86_support_mca_broadcast(CPUX86State *env);
+unsigned int cpu_x86_guest_phys_address_bits(CPUX86State *env);
 
 int cpu_get_pic_interrupt(CPUX86State *s);
 /* MSDOS compatibility mode FPU exception support */
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index abf50e6..d95d06b 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1674,6 +1674,10 @@  static int kvm_put_msrs(X86CPU *cpu, int level)
             }
         }
         if (has_msr_mtrr) {
+            uint64_t phys_mask = BIT_RANGE(
+                                   cpu_x86_guest_phys_address_bits(env) - 1,
+                                   0);
+
             kvm_msr_entry_add(cpu, MSR_MTRRdefType, env->mtrr_deftype);
             kvm_msr_entry_add(cpu, MSR_MTRRfix64K_00000, env->mtrr_fixed[0]);
             kvm_msr_entry_add(cpu, MSR_MTRRfix16K_80000, env->mtrr_fixed[1]);
@@ -1687,10 +1691,15 @@  static int kvm_put_msrs(X86CPU *cpu, int level)
             kvm_msr_entry_add(cpu, MSR_MTRRfix4K_F0000, env->mtrr_fixed[9]);
             kvm_msr_entry_add(cpu, MSR_MTRRfix4K_F8000, env->mtrr_fixed[10]);
             for (i = 0; i < MSR_MTRRcap_VCNT; i++) {
+                /* The CPU GPs if we write to a bit above the physical limit of
+                 * the host CPU (and KVM emulates that)
+                 */
+                uint64_t mask = env->mtrr_var[i].mask;
+                mask &= phys_mask;
+
                 kvm_msr_entry_add(cpu, MSR_MTRRphysBase(i),
                                   env->mtrr_var[i].base);
-                kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i),
-                                  env->mtrr_var[i].mask);
+                kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
             }
         }