| Message ID | 1467745398-28183-5-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jul 05, 2016 at 08:03:18PM +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. > > 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 <dgilbert@redhat.com> > --- > 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? 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. > + 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. > + > + 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". > + } > + } 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 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" <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. > > > > 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 <dgilbert@redhat.com> > > --- > > 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
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 + */ + 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; + } + + 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; + } + } 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) {