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

[Eduardo Habkost]

On Tue, Jul 05, 2016 at 08:03:15PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
> 
> 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 <address@hidden>
> ---
>  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
> 

-- 
Eduardo