From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44735) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKLyO-0002MY-S8 for qemu-devel@nongnu.org; Tue, 05 Jul 2016 04:44:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bKLyN-000677-3Q for qemu-devel@nongnu.org; Tue, 05 Jul 2016 04:44:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54936) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKLyM-000672-Ra for qemu-devel@nongnu.org; Tue, 05 Jul 2016 04:44:51 -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 D8317C05B1ED for ; Tue, 5 Jul 2016 08:44:49 +0000 (UTC) Date: Tue, 5 Jul 2016 09:44:45 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20160705084445.GB2118@work-vm> References: <1467659769-15900-1-git-send-email-dgilbert@redhat.com> <1467659769-15900-5-git-send-email-dgilbert@redhat.com> <20160704202717.GJ4131@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160704202717.GJ4131@thinpad.lan.raisama.net> Subject: Re: [Qemu-devel] [PATCH v2 4/6] 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 Mon, Jul 04, 2016 at 08:16:07PM +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. > > > > This becomes the default on new machine types. > > > > Signed-off-by: Dr. David Alan Gilbert > > --- > > include/hw/i386/pc.h | 5 +++++ > > target-i386/cpu.c | 36 +++++++++++++++++++++++++++++++++++- > > 2 files changed, 40 insertions(+), 1 deletion(-) > > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > > index d85e924..bf31609 100644 > > --- a/include/hw/i386/pc.h > > +++ b/include/hw/i386/pc.h > > @@ -379,6 +379,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *); > > .driver = TYPE_X86_CPU,\ > > .property = "fill-mtrr-mask",\ > > .value = "off",\ > > + },\ > > + {\ > > + .driver = TYPE_X86_CPU,\ > > + .property = "phys-bits",\ > > + .value = "40",\ > > }, > > > > #define PC_COMPAT_2_5 \ > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 5737aba..d45d2a6 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -2957,6 +2957,40 @@ 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) { > > + uint32_t eax; > > + /* Read the hosts physical address size, and compare it to what we > > + * were asked for; note old machine types default to 40 bits > > + */ > > + uint32_t host_phys_bits = 0; > > + 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; > > + } > > Why do we need to calculate host_phys_bits when phys_bits is > already set? Shouldn't we put all the code above after the "if > (cpu->phys_bits)" check? Because I reuse host_phys_bits to generate the warning if you've explicitly set phys-bits and it doesn't match the host. > > + > > + if (cpu->phys_bits == 0) { > > + /* The user asked for us to use the host physical bits */ > > + cpu->phys_bits = host_phys_bits; > > + > > + } > > + } > > > > cpu_exec_init(cs, &error_abort); > > > > @@ -3259,7 +3293,7 @@ static Property x86_cpu_properties[] = { > > DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false), > > DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true), > > DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true), > > - DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 40), > > + DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 0), > > I would put this part (that sets the default to 0) and the > PC_COMPAT_2_6 part in a separate patch. This way we can include > the mechanism for setting phys-bits=0 even if we didn't reach a > conclusion about the proper pc-2.7 default yet. Will do. Dave > > > DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0), > > DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0), > > DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0), > > -- > > 2.7.4 > > > > -- > Eduardo -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK