From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60366) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bLEJS-00082r-UK for qemu-devel@nongnu.org; Thu, 07 Jul 2016 14:46:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bLEJM-0004OY-KK for qemu-devel@nongnu.org; Thu, 07 Jul 2016 14:46:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36215) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bLEJM-0004OE-CB for qemu-devel@nongnu.org; Thu, 07 Jul 2016 14:46:08 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (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 50C0685540 for ; Thu, 7 Jul 2016 18:46:07 +0000 (UTC) Date: Thu, 7 Jul 2016 19:46:02 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20160707184601.GE2458@work-vm> References: <1467745398-28183-1-git-send-email-dgilbert@redhat.com> <1467745398-28183-5-git-send-email-dgilbert@redhat.com> <20160706193246.GX4131@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160706193246.GX4131@thinpad.lan.raisama.net> Subject: Re: [Qemu-devel] [PATCH v3 4/4] x86: Set physical address bits based on host List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu-devel@nongnu.org, pbonzini@redhat.com, marcel@redhat.com, mst@redhat.com, kraxel@redhat.com * Eduardo Habkost (ehabkost@redhat.com) wrote: > On Tue, Jul 05, 2016 at 08:03:18PM +0100, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" > > > > A special case based on the previous phys-bits property; if it's > > the magic value 0 then use the hosts capabilities. > > > > We can also use the value we read from the host to check the users > > explicitly set value and warn them if it doesn't match. > > > > Signed-off-by: Dr. David Alan Gilbert > > --- > > target-i386/cpu.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 53 insertions(+) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index f33cf58..6ebd26b 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -2952,7 +2952,60 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > > & CPUID_EXT2_AMD_ALIASES); > > } > > > > + /* For 64bit systems think about the number of physical bits to present. > > + * ideally this should be the same as the host; anything other than matching > > + * the host can cause incorrect guest behaviour. > > + * QEMU used to pick the magic value of 40 bits that corresponds to > > + * consumer AMD devices but nothing esle. > > + */ > > if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) { > > + if (kvm_enabled()) { > > + /* Read the hosts physical address size, and compare it to what we > > + * were asked for; note old machine types default to 40 bits > > + */ > > Isn't the "old machine types default to 40 bits" part obsolete, > now that you use 9999 to indicate it was not set explicitly? Yes it is; gone. > Also, the observation makes me confused: I know the old machine > types default to 40 bits, but I don't know why I need to know > that to understand the host_cpuid() logic below. Yes that comment also made more sense the way it used to be arranged. > > > + uint32_t eax; > > + uint32_t host_phys_bits = 0; > > + static bool warned; > > + > > + host_cpuid(0x80000000, 0, &eax, NULL, NULL, NULL); > > + if (eax >= 0x80000008) { > > + host_cpuid(0x80000008, 0, &eax, NULL, NULL, NULL); > > + /* Note: According to AMD doc 25481 rev 2.34 they have a field > > + * at 23:16 that can specify a maximum physical address bits for > > + * the guest that can override this value; but I've not seen > > + * anything with that set. > > + */ > > + host_phys_bits = eax & 0xff; > > + } else { > > + /* It's an odd 64 bit machine that doesn't have the leaf for > > + * physical address bits; fall back to 36 that's most older > > + * Intel. > > + */ > > + host_phys_bits = 36; > > + } > > I would love to see this logic moved inside a > x86_host_phys_bits() function. Done. > > + > > + if (cpu->phys_bits == 0) { > > + /* The user asked for us to use the host physical bits */ > > + cpu->phys_bits = host_phys_bits; > > + } > > + > > + /* Print a warning if the user set it to a value that's not the > > + * host value. > > + */ > > + if (cpu->phys_bits != host_phys_bits && cpu->phys_bits != 9999 && > > + !warned) { > > + error_report("Warning: Host physical bits (%u)" > > + " does not match phys_bits (%u)", > > + host_phys_bits, cpu->phys_bits); > > + warned = true; > > The name of the user-visible property is "phys-bits", not > phys_bits. Maybe we could say "does not match phys-bits > property". Done. Dave > > > + } > > + } else { > > + if (cpu->phys_bits == 0) { > > + error_setg(errp, "phys_bits can not be read from the host in" > > + " TCG mode"); > > + return; > > + } > > + } > > /* 9999 is a special meaning 'use the old default', > > */ > > if (cpu->phys_bits == 9999) { > > -- > > 2.7.4 > > > > -- > Eduardo -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK