diff mbox

[v3,1/4] x86: Allow physical address bits to be set

Message ID 1467745398-28183-2-git-send-email-dgilbert@redhat.com
State New
Headers show

Commit Message

Dr. David Alan Gilbert July 5, 2016, 7:03 p.m. UTC
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.

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(-)

Comments

Eduardo Habkost July 6, 2016, 7:01 p.m. UTC | #1
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.

> 
> 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 Habkost July 6, 2016, 7:57 p.m. UTC | #2
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.
> 
> 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) {

Can it be set to anything except 40/36 in TCG? It looks like the
TCG reserved-bits code added by Paolo in
e8f6d00c30ed88910d0d985f4b2bf41654172ceb assumes the reserved
bits are defined at compile time. It even has a comment saying:

/* XXX: This value should match the one returned by CPUID and in exec.c */

(Paolo, what's the value "in exec.c" mentioned in this comment?)

To make it better documented, I would rework the cpu.h macros
like this:

cpu.h:

  #if defined(TARGET_X86_64)
  #define TCG_PHYS_ADDR_LIMIT 40
  #else
  #define TCG_PHYS_ADDR_LIMIT 36
  #endif

  #define TCG_PHYS_ADDR_MASK MAKE_64BIT_MASK(0, TCG_PHYS_ADDR_LIMIT)

  #define KVM_PHYS_ADDR_LIMIT 52

  QEMU_BUILD_BUG_ON(TARGET_PHYS_ADDR_SPACE_BITS > KVM_PHYS_ADDR_LIMIT)

cpu.c:

  if (kvm_enabled()) {
      if (cpu->phys_bits > KVM_PHYS_ADDR_LIMIT() ||
          cpu->phys_bits < 32) {
          error_setg(errp, "phys-bits should be between 32 and %u (but is %u)",
                           phys_addr_limit(), cpu->phys_bits);
          return;
      }
  } else {
     if (cpu->phys_bits != TCG_PHYS_ADDR_LIMIT) {
         error_setg(errp, "TCG only supports phys-bits=%u", TCG_PHYS_ADDR_LIMIT);
         return;
     }
  }

> +            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);

The "0 to use host size" part is not valid until we apply patch
4/4.

> +            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
>
Dr. David Alan Gilbert July 7, 2016, 4:39 p.m. UTC | #3
* 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
Eduardo Habkost July 7, 2016, 6:02 p.m. UTC | #4
On Thu, Jul 07, 2016 at 05:39:14PM +0100, Dr. David Alan Gilbert wrote:
[...]
> * 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.

Good point. Integer parsing code in QEMU is ... interesting.

> 
> (And we have ~40 cases of DEFINE_PROP_UINT*(.....,-1) which I just
> find very wrong).

Just wait until you see how string-input-visitor.c parses
uint64_t values.
Dr. David Alan Gilbert July 7, 2016, 6:36 p.m. UTC | #5
* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Thu, Jul 07, 2016 at 05:39:14PM +0100, Dr. David Alan Gilbert wrote:
> [...]
> > * 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.
> 
> Good point. Integer parsing code in QEMU is ... interesting.
> 
> > 
> > (And we have ~40 cases of DEFINE_PROP_UINT*(.....,-1) which I just
> > find very wrong).
> 
> Just wait until you see how string-input-visitor.c parses
> uint64_t values.

Blech!

Dave

> 
> -- 
> Eduardo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox

Patch

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;