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

[Eduardo Habkost]

On Mon, Jul 04, 2016 at 08:16:04PM +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.

target-i386/cpu.h has:

  #ifdef TARGET_X86_64
  #define TARGET_PHYS_ADDR_SPACE_BITS 52
  /* ??? This is really 48 bits, sign-extended, but the only thing
     accessible to userland with bit 48 set is the VSYSCALL, and that
     is handled via other mechanisms.  */
  #define TARGET_VIRT_ADDR_SPACE_BITS 47
  #else
  #define TARGET_PHYS_ADDR_SPACE_BITS 36
  #define TARGET_VIRT_ADDR_SPACE_BITS 32
  #endif

Where did those values come from? What are they used for?

  /* XXX: This value should match the one returned by CPUID
   * and in exec.c */
  # if defined(TARGET_X86_64)
  # define PHYS_ADDR_MASK 0xffffffffffLL
  # else
  # define PHYS_ADDR_MASK 0xfffffffffLL
  # endif

PHYS_ADDR_MASK is only used at:

  #define PG_HI_RSVD_MASK  (PG_ADDRESS_MASK & ~PHYS_ADDR_MASK)

PG_HI_RSVD_MASK is only used at x86_cpu_handle_mmu_fault(), and I
don't know if that function is TCG-specific or not.


> 
> 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.
> 
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> ---
>  target-i386/cpu.c | 8 +++++---
>  target-i386/cpu.h | 3 +++
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3bd3cfc..ab13ef5 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2604,9 +2604,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
> uint32_t count,
>          /* 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;

We need to ensure phys-bits is not out of range, either on the
property setter or on realizefn.

(I suggest realizefn. Custom property setters are evil)

Now, related to the questions above about the cpu.h macros: what
would be the appropriate range to check for?

>          } else {
>              if (env->features[FEAT_1_EDX] & CPUID_PSE36) {
>                  *eax = 0x00000024; /* 36 bits physical */
> @@ -3257,6 +3258,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, 40),
>      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