Re: [Qemu-devel] [PATCH 2/5] x86: Mask mtrr mask based on CPU physical address limits

[Eduardo Habkost <ehabkost@redhat.com>]

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
> 

-- 
Eduardo