* Eduardo Habkost (ehabk...@redhat.com) wrote:
> On Fri, Jul 08, 2016 at 04:01:36PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilb...@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 <dgilb...@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 <ehabk...@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 / dgilb...@redhat.com / Manchester, UK
