From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57429) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bLCKj-0006Xo-2w for qemu-devel@nongnu.org; Thu, 07 Jul 2016 12:39:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bLCKf-0001cz-Gu for qemu-devel@nongnu.org; Thu, 07 Jul 2016 12:39:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55423) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bLCKf-0001cG-8w for qemu-devel@nongnu.org; Thu, 07 Jul 2016 12:39:21 -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 D348B3F723 for ; Thu, 7 Jul 2016 16:39:19 +0000 (UTC) Date: Thu, 7 Jul 2016 17:39:14 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20160707163913.GB2458@work-vm> References: <1467745398-28183-1-git-send-email-dgilbert@redhat.com> <1467745398-28183-2-git-send-email-dgilbert@redhat.com> <20160706190110.GV4131@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160706190110.GV4131@thinpad.lan.raisama.net> Subject: Re: [Qemu-devel] [PATCH v3 1/4] x86: Allow physical address bits to be set 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:15PM +0100, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" > > > > Currently QEMU sets the x86 number of physical address bits to the > > magic number 40. This is only correct on some small AMD systems; > > Intel systems tend to have 36, 39, 46 bits, and large AMD systems > > tend to have 48. > > > > Having the value different from your actual hardware is detectable > > by the guest and in principal can cause problems; > > The current limit of 40 stops TB VMs being created by those lucky > > enough to have that much. > > > > This patch lets you set the physical bits by a cpu property but > > defaults to the same existing magic 40. > > > > I've removed the ancient warning about the 42 bit limit in exec.c; > > I can't find that limit in there and no one else seems to know where > > it is. > > > > We use a magic value of 9999 as the property default so that we can > > later distinguish between the default and a user set value. > > I'd prefer to use -1 or 0xFFFFFFFF (UINT32_MAX) to indicate it > was not set by the user, and not document it as "use the old > default" but just as "it was not set explicitly". > > This won't allow us to differentiate between "set by user" and > "set by machine-type compat_props" in the future. But I would > prefer to use a MachineClass field or boolean property than magic > numbers for that, anyway. > > If you replace 9999 with UINT32_MAX or 0xFFFFFFFF in this patch: > > Reviewed-by: Eduardo Habkost > > BTW, using 0 to indicate "not set" would be acceptable, too, but > the magic 0 value in patch 4/4 would need to be replaced with a > boolean "host-phys-bits" property. But I prefer boolean properties > instead of magic numbers, anyway. OK, lets do that then. I'll use 0 here for the magic default and then add the host-phys-bits boolean that strictly overrides the phys-bits numeric. I didn't use UINT32_MAX or 0xffffffff because I wasn't convinced how that would interact with the machine-type/compat code which uses strings to represent default values for machine types. (And we have ~40 cases of DEFINE_PROP_UINT*(.....,-1) which I just find very wrong). Dave > > > > > Signed-off-by: Dr. David Alan Gilbert > > --- > > target-i386/cpu.c | 43 ++++++++++++++++++++++++++++++++++--------- > > target-i386/cpu.h | 3 +++ > > 2 files changed, 37 insertions(+), 9 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 3bd3cfc..74d53c5 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -2602,17 +2602,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > > break; > > case 0x80000008: > > /* virtual & phys address size in low 2 bytes. */ > > -/* XXX: This value must match the one used in the MMU code. */ > > if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) { > > - /* 64 bit processor */ > > -/* XXX: The physical address space is limited to 42 bits in exec.c. */ > > - *eax = 0x00003028; /* 48 bits virtual, 40 bits physical */ > > + /* 64 bit processor, 48 bits virtual, configurable > > + * physical bits. > > + */ > > + *eax = 0x00003000 + cpu->phys_bits; > > } else { > > - if (env->features[FEAT_1_EDX] & CPUID_PSE36) { > > - *eax = 0x00000024; /* 36 bits physical */ > > - } else { > > - *eax = 0x00000020; /* 32 bits physical */ > > - } > > + *eax = cpu->phys_bits; > > } > > *ebx = 0; > > *ecx = 0; > > @@ -2956,7 +2952,35 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > > & CPUID_EXT2_AMD_ALIASES); > > } > > > > + if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) { > > + /* 9999 is a special meaning 'use the old default', > > + */ > > + if (cpu->phys_bits == 9999) { > > + /* this must match the PHYS_ADDR_MASK in cpu.h */ > > + cpu->phys_bits = 40; > > + } > > + if (cpu->phys_bits > TARGET_PHYS_ADDR_SPACE_BITS || > > + cpu->phys_bits < 32) { > > + error_setg(errp, "phys_bits should be between 32 and %u or 0 to" > > + " use host size (but is %u)", > > + TARGET_PHYS_ADDR_SPACE_BITS, cpu->phys_bits); > > + return; > > + } > > + } else { > > + /* For 32 bit systems don't use the user set value, but keep > > + * phys_bits consistent with what we tell the guest. > > + */ > > + if (cpu->phys_bits != 9999) { > > + error_setg(errp, "phys_bits is not user-configurable in 32 bit"); > > + return; > > + } > > > > + if (env->features[FEAT_1_EDX] & CPUID_PSE36) { > > + cpu->phys_bits = 36; > > + } else { > > + cpu->phys_bits = 32; > > + } > > + } > > cpu_exec_init(cs, &error_abort); > > > > if (tcg_enabled()) { > > @@ -3257,6 +3281,7 @@ static Property x86_cpu_properties[] = { > > DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true), > > DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false), > > DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true), > > + DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 9999), > > 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), > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > > index 474b0b9..221b1a2 100644 > > --- a/target-i386/cpu.h > > +++ b/target-i386/cpu.h > > @@ -1181,6 +1181,9 @@ struct X86CPU { > > /* Compatibility bits for old machine types: */ > > bool enable_cpuid_0xb; > > > > + /* Number of physical address bits supported */ > > + uint32_t phys_bits; > > + > > /* in order to simplify APIC support, we leave this pointer to the > > user */ > > struct DeviceState *apic_state; > > -- > > 2.7.4 > > > > -- > Eduardo -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK