From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45526) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDp3j-0004an-4B for qemu-devel@nongnu.org; Fri, 17 Jun 2016 04:23:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bDp3e-0007YH-Ta for qemu-devel@nongnu.org; Fri, 17 Jun 2016 04:23:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34344) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDp3e-0007YA-LU for qemu-devel@nongnu.org; Fri, 17 Jun 2016 04:23:18 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1AB743710C4 for ; Fri, 17 Jun 2016 08:23:18 +0000 (UTC) Date: Fri, 17 Jun 2016 09:23:14 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20160617082314.GC2273@work-vm> References: <1466097133-5489-1-git-send-email-dgilbert@redhat.com> <1466097133-5489-3-git-send-email-dgilbert@redhat.com> <20160616195953.GW18662@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160616195953.GW18662@thinpad.lan.raisama.net> Subject: Re: [Qemu-devel] [PATCH 2/5] x86: Mask mtrr mask based on CPU physical address limits List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu-devel@nongnu.org, pbonzini@redhat.com, aarcange@redhat.com * 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" > > > > 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 > > --- > > 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