Re: [Qemu-devel] [PATCH v3 1/4] x86: Allow physical address bits to be set

["Dr. David Alan Gilbert" <dgilbert@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" <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 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 <ehabkost@redhat.com>
> 
> 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 <dgilbert@redhat.com>
> > ---
> >  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