| Message ID | 1467659769-15900-5-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jul 04, 2016 at 08:16:07PM +0100, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > 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 <dgilbert@redhat.com> > --- > 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? > + > + 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. > 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 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" <dgilbert@redhat.com> > > > > 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 <dgilbert@redhat.com> > > --- > > 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
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; + } + + 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), 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),