| Message ID | 1467990099-27853-3-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jul 08, 2016 at 04:01:36PM +0100, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > 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 40bits which matches TCGs setup. > > 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 0 as the property default so that we can > later distinguish between the default and a user set value. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > target-i386/cpu.c | 51 ++++++++++++++++++++++++++++++++++++++++++--------- > target-i386/cpu.h | 3 +++ > 2 files changed, 45 insertions(+), 9 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 3bd3cfc..2cc5609 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,43 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > & CPUID_EXT2_AMD_ALIASES); > } > > + if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) { > + /* 0 is a special meaning 'use the old default', which matches > + * the value used by TCG (40). > + */ I am not sure 0 should be described as the "old" default. I just understand it as meaning it not being set explicitly by the user. Evidence of that is the 32-bit branch below, where 0 is the only acceptable value, not just the "old" default. Do you mind if I reword it when committing? I would describe it as: /* 0 means it was not explicitly set by the user (or by machine * compat_props). In this case, the default is the value used by * TCG (40). */ In either case: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > + if (cpu->phys_bits == 0) { > + cpu->phys_bits = TCG_PHYS_ADDR_BITS; > + } > + if (kvm_enabled()) { > + if (cpu->phys_bits > TARGET_PHYS_ADDR_SPACE_BITS || > + cpu->phys_bits < 32) { > + error_setg(errp, "phys-bits should be between 32 and %u " > + " (but is %u)", > + TARGET_PHYS_ADDR_SPACE_BITS, cpu->phys_bits); > + return; > + } > + } else { > + if (cpu->phys_bits != TCG_PHYS_ADDR_BITS) { > + error_setg(errp, "TCG only supports phys-bits=%u", > + TCG_PHYS_ADDR_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 != 0) { > + error_setg(errp, "phys_bits is not user-configurable in 32 bit"); I will change it to "phys-bits" when committing. > + return; > + } > > + if (env->features[FEAT_1_EDX] & CPUID_PSE36) { > + cpu->phys_bits = 36; > + } else { > + cpu->phys_bits = 32; But TCG_PHYS_ADDR_BITS is still 36. Does this mean TCG reserved-bit handling is broken if pse36 is disabled? > + } > + } > cpu_exec_init(cs, &error_abort); > > if (tcg_enabled()) { > @@ -3257,6 +3289,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, 0), > 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 b3162b7..202f9a3 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 >
On 08/07/2016 20:59, Eduardo Habkost wrote: > > + if (env->features[FEAT_1_EDX] & CPUID_PSE36) { > > + cpu->phys_bits = 36; > > + } else { > > + cpu->phys_bits = 32; > > But TCG_PHYS_ADDR_BITS is still 36. Does this mean TCG > reserved-bit handling is broken if pse36 is disabled? This makes sense as a default, apparently if you don't have PSE36 but you have phys_bits > 32, Windows complains: commit 45fd08effd461f85d0480d3b8f85a07751fc55b3 Author: aurel32 <aurel32@c046a42c-6fe2-441c-8c8c-71466251a162> Date: Tue Oct 14 19:20:52 2008 +0000 target-i386: Add Core Duo Definition This patch adds a CPU definition for the Core Duo CPU. I tried to resemble the original as closely as possible and document what features are missing still. This patch enables the use of a recent CPU definition on 32 bit platforms. It also fixes two issues that went along the line: - invalid xlevel in core2duo spec While looking though the CPUIDs again, I found that xlevel is actually 8. - non-PSE36 support The CoreDuo CPUID does not expose the PSE36 capability, but CPUID 0x80000008 is tied to 36 bits. This broke Windows XP installation for me, so I just set it to 32 bits width when PSE36 is not available. The original CPU also exposes 32 bit width in CPUID 0x80000008. Signed-off-by: Alexander Graf <agraf@suse.de> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@5488 c046a42c-6fe2-441c-8c8c-71466251a162 As long as it's overridable I guess it's fine... Paolo
* Eduardo Habkost (ehabkost@redhat.com) wrote: > On Fri, Jul 08, 2016 at 04:01:36PM +0100, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > 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 40bits which matches TCGs setup. > > > > 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 0 as the property default so that we can > > later distinguish between the default and a user set value. > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > target-i386/cpu.c | 51 ++++++++++++++++++++++++++++++++++++++++++--------- > > target-i386/cpu.h | 3 +++ > > 2 files changed, 45 insertions(+), 9 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 3bd3cfc..2cc5609 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,43 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > > & CPUID_EXT2_AMD_ALIASES); > > } > > > > + if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) { > > + /* 0 is a special meaning 'use the old default', which matches > > + * the value used by TCG (40). > > + */ > > I am not sure 0 should be described as the "old" default. I just > understand it as meaning it not being set explicitly by the user. > > Evidence of that is the 32-bit branch below, where 0 is the only > acceptable value, not just the "old" default. > > Do you mind if I reword it when committing? I would describe it > as: > > /* 0 means it was not explicitly set by the user (or by machine > * compat_props). In this case, the default is the value used by > * TCG (40). > */ Yes, that's fine. > > In either case: > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > + if (cpu->phys_bits == 0) { > > + cpu->phys_bits = TCG_PHYS_ADDR_BITS; > > + } > > + if (kvm_enabled()) { > > + if (cpu->phys_bits > TARGET_PHYS_ADDR_SPACE_BITS || > > + cpu->phys_bits < 32) { > > + error_setg(errp, "phys-bits should be between 32 and %u " > > + " (but is %u)", > > + TARGET_PHYS_ADDR_SPACE_BITS, cpu->phys_bits); > > + return; > > + } > > + } else { > > + if (cpu->phys_bits != TCG_PHYS_ADDR_BITS) { > > + error_setg(errp, "TCG only supports phys-bits=%u", > > + TCG_PHYS_ADDR_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 != 0) { > > + error_setg(errp, "phys_bits is not user-configurable in 32 bit"); > > I will change it to "phys-bits" when committing. Sure. > > + return; > > + } > > > > + if (env->features[FEAT_1_EDX] & CPUID_PSE36) { > > + cpu->phys_bits = 36; > > + } else { > > + cpu->phys_bits = 32; > > But TCG_PHYS_ADDR_BITS is still 36. Does this mean TCG > reserved-bit handling is broken if pse36 is disabled? Good question; I don't understand the TCG code enough to comment; but I'm reasonably sure that this patchset doesn't change the behaviour. I've booted a 32bit fedora, on a qemu that claims to be running with phys_bits = 32 - I don't believe that works on real hardware. (Mind you booting it on qemu-system-i386 with qemu32 CPU just doesn't work, but it doesn't work before my patch either). Dave > > + } > > + } > > cpu_exec_init(cs, &error_abort); > > > > if (tcg_enabled()) { > > @@ -3257,6 +3289,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, 0), > > 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 b3162b7..202f9a3 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
On Fri, Jul 08, 2016 at 08:25:32PM +0100, Dr. David Alan Gilbert wrote: > > > + return; > > > + } > > > > > > + if (env->features[FEAT_1_EDX] & CPUID_PSE36) { > > > + cpu->phys_bits = 36; > > > + } else { > > > + cpu->phys_bits = 32; > > > > But TCG_PHYS_ADDR_BITS is still 36. Does this mean TCG > > reserved-bit handling is broken if pse36 is disabled? > > Good question; I don't understand the TCG code enough to comment; > but I'm reasonably sure that this patchset doesn't change the behaviour. > I've booted a 32bit fedora, on a qemu that claims to be running > with phys_bits = 32 - I don't believe that works on real hardware. > (Mind you booting it on qemu-system-i386 with qemu32 CPU just doesn't > work, but it doesn't work before my patch either). Yes, I was wondering about the existing behavior. You don't change any behavior here, as far as I can see.
On Fri, Jul 08, 2016 at 04:01:36PM +0100, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > 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 40bits which matches TCGs setup. > > 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. I think I've found it, buried in ancient commits: commit 5270589032f450ae7c3448730855aa18ff68ccff Author: Richard Henderson <rth@twiddle.net> Date: Wed Mar 10 14:33:23 2010 -0800 Move TARGET_PHYS_ADDR_SPACE_BITS to target-*/cpu.h. Removes a set of ifdefs from exec.c. Introduce TARGET_VIRT_ADDR_SPACE_BITS for all targets other than Alpha. This will be used for page_find_alloc, which is supposed to be using virtual addresses in the first place. Signed-off-by: Richard Henderson <rth@twiddle.net> [...] --- a/exec.c +++ b/exec.c [...] -#elif defined(TARGET_X86_64) -#define TARGET_PHYS_ADDR_SPACE_BITS 42 -#elif defined(TARGET_I386) -#define TARGET_PHYS_ADDR_SPACE_BITS 36 [...] --- a/target-i386/cpu.h +++ b/target-i386/cpu.h [...] +#ifdef TARGET_X86_64 +#define TARGET_PHYS_ADDR_SPACE_BITS 52 +/* ??? This is really 48 bits, sign-extended, but the only thing + accessible to userland with bit 48 set is the VSYSCALL, and that + is handled via other mechanisms. */ +#define TARGET_VIRT_ADDR_SPACE_BITS 47 +#else +#define TARGET_PHYS_ADDR_SPACE_BITS 36 +#define TARGET_VIRT_ADDR_SPACE_BITS 32 +#endif + But I really don't understand why it was changed to 52 when the macros were moved. A (fortunate) typo?
On 07/08/2016 04:36 PM, Eduardo Habkost wrote: > -#elif defined(TARGET_X86_64) > -#define TARGET_PHYS_ADDR_SPACE_BITS 42 > -#elif defined(TARGET_I386) > -#define TARGET_PHYS_ADDR_SPACE_BITS 36 > [...] > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > [...] > +#ifdef TARGET_X86_64 > +#define TARGET_PHYS_ADDR_SPACE_BITS 52 > +/* ??? This is really 48 bits, sign-extended, but the only thing > + accessible to userland with bit 48 set is the VSYSCALL, and that > + is handled via other mechanisms. */ > +#define TARGET_VIRT_ADDR_SPACE_BITS 47 > +#else > +#define TARGET_PHYS_ADDR_SPACE_BITS 36 > +#define TARGET_VIRT_ADDR_SPACE_BITS 32 > +#endif > + > > But I really don't understand why it was changed to 52 when the > macros were moved. A (fortunate) typo? It was 6 years ago, but I assume that I looked up 52 as a theoretical maximum. I'm not going to look it up again to verify though. r~
On Fri, Jul 08, 2016 at 05:59:16PM -0700, Richard Henderson wrote: > On 07/08/2016 04:36 PM, Eduardo Habkost wrote: > > -#elif defined(TARGET_X86_64) > > -#define TARGET_PHYS_ADDR_SPACE_BITS 42 > > -#elif defined(TARGET_I386) > > -#define TARGET_PHYS_ADDR_SPACE_BITS 36 > > [...] > > --- a/target-i386/cpu.h > > +++ b/target-i386/cpu.h > > [...] > > +#ifdef TARGET_X86_64 > > +#define TARGET_PHYS_ADDR_SPACE_BITS 52 > > +/* ??? This is really 48 bits, sign-extended, but the only thing > > + accessible to userland with bit 48 set is the VSYSCALL, and that > > + is handled via other mechanisms. */ > > +#define TARGET_VIRT_ADDR_SPACE_BITS 47 > > +#else > > +#define TARGET_PHYS_ADDR_SPACE_BITS 36 > > +#define TARGET_VIRT_ADDR_SPACE_BITS 32 > > +#endif > > + > > > > But I really don't understand why it was changed to 52 when the > > macros were moved. A (fortunate) typo? > > It was 6 years ago, but I assume that I looked up 52 as a theoretical > maximum. I'm not going to look it up again to verify though. Probably that's the case. David Gilbert also found 52 to be the maximum.
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 3bd3cfc..2cc5609 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,43 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) & CPUID_EXT2_AMD_ALIASES); } + if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) { + /* 0 is a special meaning 'use the old default', which matches + * the value used by TCG (40). + */ + if (cpu->phys_bits == 0) { + cpu->phys_bits = TCG_PHYS_ADDR_BITS; + } + if (kvm_enabled()) { + if (cpu->phys_bits > TARGET_PHYS_ADDR_SPACE_BITS || + cpu->phys_bits < 32) { + error_setg(errp, "phys-bits should be between 32 and %u " + " (but is %u)", + TARGET_PHYS_ADDR_SPACE_BITS, cpu->phys_bits); + return; + } + } else { + if (cpu->phys_bits != TCG_PHYS_ADDR_BITS) { + error_setg(errp, "TCG only supports phys-bits=%u", + TCG_PHYS_ADDR_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 != 0) { + 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 +3289,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, 0), 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 b3162b7..202f9a3 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;