[Qemu-devel] [PATCH v4 0/5] x86: Physical address limit patches

[Qemu-devel] [PATCH v4 0/5] x86: Physical address limit patches

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

QEMU sets the guests physical address bits to 40; this is wrong
on most hardware, and can be detected by the guest.
It also stops you using really huge multi-TB VMs.

Red Hat has had a patch, that Andrea wrote, downstream for a couple
of years that reads the hosts value and uses that in the guest.  That's
correct as far as the guest sees it, and lets you create huge VMs.

The downside, is that if you've got a mix of hosts, say an i7 and a Xeon,
life gets complicated in migration; prior to 2.6 it all apparently
worked (although a guest that looked might spot the change).
In 2.6 Paolo started checking MSR writes and they failed when the
incoming MTRR mask didn't fit.

This series:
   a) Fixes up mtrr masks so that if you're migrating between hosts
      of different physical address size it tries to do something sensible.

   b) Lets you specify the guest physical address size via a CPU property, i.e.
        -cpu SandyBridge,phys-bits=36

      The default is still to use the existing 40 bits value.

   c) Lets you tell qemu to use the same setting as the host, i.e.
        -cpu SandyBridge,host-phys-bits
      (This overrides phys-bits)

   d) Warns if the host physical bits don't match a value specified with phys-bits

Note that mixed size hosts are still not necessarily safe; a guest
started on a host with a large physical address size might start using
those bits and get upset when it's moved to a small host.
However that was already potentially broken in existing qemu that
used a magic value of 40.

There's potential to add some extra guards against people
doing silly stuff; e.g. stop people running VMs using 1TB of
address space on a tiny host.

Dave

v4
  Provide TCG_PHYS_ADDR_BITS constants
  use separate boolean for getting the host bits
  Now use the magic 0 to mean default no. of bits
  Split out cpuid code into x86_host_phys_bits

Dr. David Alan Gilbert (5):
  x86: Provide TCG_PHYS_ADDR_BITS
  x86: Allow physical address bits to be set
  x86: Mask mtrr mask based on CPU physical address limits
  x86: fill high bits of mtrr mask
  x86: Set physical address bits based on host

 include/hw/i386/pc.h |   5 +++
 target-i386/cpu.c    | 107 ++++++++++++++++++++++++++++++++++++++++++++++-----
 target-i386/cpu.h    |  15 +++++++-
 target-i386/kvm.c    |  39 +++++++++++++++++--
 4 files changed, 152 insertions(+), 14 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v4 1/5] x86: Provide TCG_PHYS_ADDR_BITS

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Provide a constant for the number of address bits supported under TCG.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 474b0b9..b3162b7 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1403,11 +1403,13 @@ uint64_t cpu_get_tsc(CPUX86State *env);
 /* XXX: This value should match the one returned by CPUID
  * and in exec.c */
 # if defined(TARGET_X86_64)
-# define PHYS_ADDR_MASK 0xffffffffffLL
+# define TCG_PHYS_ADDR_BITS 40
 # else
-# define PHYS_ADDR_MASK 0xfffffffffLL
+# define TCG_PHYS_ADDR_BITS 36
 # endif
 
+#define PHYS_ADDR_MASK MAKE_64BIT_MASK(0, TCG_PHYS_ADDR_BITS)
+
 #define cpu_init(cpu_model) CPU(cpu_x86_init(cpu_model))
 
 #define cpu_signal_handler cpu_x86_signal_handler
-- 
2.7.4

[Qemu-devel] [PATCH v4 2/5] x86: Allow physical address bits to be set

[Dr. David Alan Gilbert (git)]

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 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 <dgilbert@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).
+         */
+        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");
+            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 +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

[Qemu-devel] [PATCH v4 3/5] x86: Mask mtrr mask based on CPU physical address limits

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The CPU GPs if we try and set a bit in a variable MTRR mask above
the limit of physical address bits on the host.  We hit this
when loading a migration from a host with a larger physical
address limit than our destination (e.g. a Xeon->i7 of same
generation) but previously used to get away with it
until 48e1a45 started checking that msr writes actually worked.

It seems in our case the GP probably comes from KVM emulating
that GP.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index f3698f1..6429205 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1677,6 +1677,8 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
             }
         }
         if (has_msr_mtrr) {
+            uint64_t phys_mask = MAKE_64BIT_MASK(0, cpu->phys_bits);
+
             kvm_msr_entry_add(cpu, MSR_MTRRdefType, env->mtrr_deftype);
             kvm_msr_entry_add(cpu, MSR_MTRRfix64K_00000, env->mtrr_fixed[0]);
             kvm_msr_entry_add(cpu, MSR_MTRRfix16K_80000, env->mtrr_fixed[1]);
@@ -1690,10 +1692,15 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
             kvm_msr_entry_add(cpu, MSR_MTRRfix4K_F0000, env->mtrr_fixed[9]);
             kvm_msr_entry_add(cpu, MSR_MTRRfix4K_F8000, env->mtrr_fixed[10]);
             for (i = 0; i < MSR_MTRRcap_VCNT; i++) {
+                /* The CPU GPs if we write to a bit above the physical limit of
+                 * the host CPU (and KVM emulates that)
+                 */
+                uint64_t mask = env->mtrr_var[i].mask;
+                mask &= phys_mask;
+
                 kvm_msr_entry_add(cpu, MSR_MTRRphysBase(i),
                                   env->mtrr_var[i].base);
-                kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i),
-                                  env->mtrr_var[i].mask);
+                kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
             }
         }
 
-- 
2.7.4

[Qemu-devel] [PATCH v4 4/5] x86: fill high bits of mtrr mask

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Fill the bits between 51..number-of-physical-address-bits in the
MTRR_PHYSMASKn variable range mtrr masks so that they're consistent
in the migration stream irrespective of the physical address space
of the source VM in a migration.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/hw/i386/pc.h |  5 +++++
 target-i386/cpu.c    |  1 +
 target-i386/cpu.h    |  3 +++
 target-i386/kvm.c    | 28 +++++++++++++++++++++++++++-
 4 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 2123532..7d9d03c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -373,6 +373,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
         .driver   = "vmxnet3",\
         .property = "romfile",\
         .value    = "",\
+    },\
+    {\
+        .driver = TYPE_X86_CPU,\
+        .property = "fill-mtrr-mask",\
+        .value = "off",\
     },
 
 #define PC_COMPAT_2_5 \
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2cc5609..641c38b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3290,6 +3290,7 @@ static Property x86_cpu_properties[] = {
     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_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
     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 202f9a3..9d79146 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;
 
+    /* if true fill the top bits of the MTRR_PHYSMASKn variable range */
+    bool fill_mtrr_mask;
+
     /* Number of physical address bits supported */
     uint32_t phys_bits;
 
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 6429205..6fe0f48 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1935,6 +1935,7 @@ static int kvm_get_msrs(X86CPU *cpu)
     CPUX86State *env = &cpu->env;
     struct kvm_msr_entry *msrs = cpu->kvm_msr_buf->entries;
     int ret, i;
+    uint64_t mtrr_top_bits;
 
     kvm_msr_buf_reset(cpu);
 
@@ -2084,6 +2085,30 @@ static int kvm_get_msrs(X86CPU *cpu)
     }
 
     assert(ret == cpu->kvm_msr_buf->nmsrs);
+    /*
+     * MTRR masks: Each mask consists of 5 parts
+     * a  10..0: must be zero
+     * b  11   : valid bit
+     * c n-1.12: actual mask bits
+     * d  51..n: reserved must be zero
+     * e  63.52: reserved must be zero
+     *
+     * 'n' is the number of physical bits supported by the CPU and is
+     * apparently always <= 52.   We know our 'n' but don't know what
+     * the destinations 'n' is; it might be smaller, in which case
+     * it masks (c) on loading. It might be larger, in which case
+     * we fill 'd' so that d..c is consistent irrespetive of the 'n'
+     * we're migrating to.
+     */
+
+    if (cpu->fill_mtrr_mask) {
+        QEMU_BUILD_BUG_ON(TARGET_PHYS_ADDR_SPACE_BITS > 52);
+        assert(cpu->phys_bits <= TARGET_PHYS_ADDR_SPACE_BITS);
+        mtrr_top_bits = MAKE_64BIT_MASK(cpu->phys_bits, 52 - cpu->phys_bits);
+    } else {
+        mtrr_top_bits = 0;
+    }
+
     for (i = 0; i < ret; i++) {
         uint32_t index = msrs[i].index;
         switch (index) {
@@ -2279,7 +2304,8 @@ static int kvm_get_msrs(X86CPU *cpu)
             break;
         case MSR_MTRRphysBase(0) ... MSR_MTRRphysMask(MSR_MTRRcap_VCNT - 1):
             if (index & 1) {
-                env->mtrr_var[MSR_MTRRphysIndex(index)].mask = msrs[i].data;
+                env->mtrr_var[MSR_MTRRphysIndex(index)].mask = msrs[i].data |
+                                                               mtrr_top_bits;
             } else {
                 env->mtrr_var[MSR_MTRRphysIndex(index)].base = msrs[i].data;
             }
-- 
2.7.4

[Qemu-devel] [PATCH v4 5/5] x86: Set physical address bits based on host

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Add the host-phys-bits boolean property, if true, take phys-bits
from the hosts physical bits value, overriding either the default
or the user specified value.

We can also use the value we read from the host to check the users
explicitly set value and warn them if it doesn't match.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 target-i386/cpu.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++-------
 target-i386/cpu.h |  3 +++
 2 files changed, 67 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 641c38b..72ca7fb 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2885,6 +2885,31 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 }
 #endif
 
+/* Note: Only safe for use on x86(-64) hosts */
+static uint32_t x86_host_phys_bits(void)
+{
+    uint32_t eax;
+    uint32_t host_phys_bits;
+
+    host_cpuid(0x80000000, 0, &eax, NULL, NULL, NULL);
+    if (eax >= 0x80000008) {
+        host_cpuid(0x80000008, 0, &eax, NULL, NULL, NULL);
+        /* Note: According to AMD doc 25481 rev 2.34 they have a field
+         * at 23:16 that can specify a maximum physical address bits for
+         * the guest that can override this value; but I've not seen
+         * anything with that set.
+         */
+        host_phys_bits = eax & 0xff;
+    } else {
+        /* It's an odd 64 bit machine that doesn't have the leaf for
+         * physical address bits; fall back to 36 that's most older
+         * Intel.
+         */
+        host_phys_bits = 36;
+    }
+
+    return host_phys_bits;
+}
 
 #define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \
                            (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \
@@ -2952,28 +2977,57 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
            & CPUID_EXT2_AMD_ALIASES);
     }
 
+    /* For 64bit systems think about the number of physical bits to present.
+     * ideally this should be the same as the host; anything other than matching
+     * the host can cause incorrect guest behaviour.
+     * QEMU used to pick the magic value of 40 bits that corresponds to
+     * consumer AMD devices but nothing else.
+     */
+    if (cpu->host_phys_bits && !kvm_enabled()) {
+        error_setg(errp, "phys-bits can not be read from the host in"
+                         " TCG mode");
+        return;
+    }
+
     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).
-         */
-        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) {
+            uint32_t host_phys_bits = x86_host_phys_bits();
+            static bool warned;
+
+            if (cpu->host_phys_bits) {
+                /* The user asked for us to use the host physical bits */
+                cpu->phys_bits = host_phys_bits;
+            }
+
+            /* Print a warning if the user set it to a value that's not the
+             * host value.
+             */
+            if (cpu->phys_bits != host_phys_bits && cpu->phys_bits != 0 &&
+                !warned) {
+                error_report("Warning: Host physical bits (%u)"
+                                 " does not match phys-bits property (%u)",
+                                 host_phys_bits, cpu->phys_bits);
+                warned = true;
+            }
+
+            if (cpu->phys_bits &&
+                (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) {
+            if (cpu->phys_bits && cpu->phys_bits != TCG_PHYS_ADDR_BITS) {
                 error_setg(errp, "TCG only supports phys-bits=%u",
                                   TCG_PHYS_ADDR_BITS);
                 return;
             }
         }
+        if (cpu->phys_bits == 0 && !cpu->host_phys_bits) {
+            cpu->phys_bits = TCG_PHYS_ADDR_BITS;
+        }
     } else {
         /* For 32 bit systems don't use the user set value, but keep
          * phys_bits consistent with what we tell the guest.
@@ -3290,6 +3344,7 @@ static Property x86_cpu_properties[] = {
     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_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
     DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
     DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
     DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 9d79146..3c4e64a 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1184,6 +1184,9 @@ struct X86CPU {
     /* if true fill the top bits of the MTRR_PHYSMASKn variable range */
     bool fill_mtrr_mask;
 
+    /* if true override the phys_bits value with a value read from the host */
+    bool host_phys_bits;
+
     /* Number of physical address bits supported */
     uint32_t phys_bits;
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 5/5] x86: Set physical address bits based on host

[Paolo Bonzini]

On 08/07/2016 17:01, Dr. David Alan Gilbert (git) wrote:
> +            uint32_t host_phys_bits = x86_host_phys_bits();
> +            static bool warned;
> +
> +            if (cpu->host_phys_bits) {
> +                /* The user asked for us to use the host physical bits */
> +                cpu->phys_bits = host_phys_bits;
> +            }
> +
> +            /* Print a warning if the user set it to a value that's not the
> +             * host value.
> +             */
> +            if (cpu->phys_bits != host_phys_bits && cpu->phys_bits != 0 &&
> +                !warned) {
> +                error_report("Warning: Host physical bits (%u)"
> +                                 " does not match phys-bits property (%u)",
> +                                 host_phys_bits, cpu->phys_bits);
> +                warned = true;
> +            }
> +
> +            if (cpu->phys_bits &&
> +                (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);

Michael Tsirkin suggested a way to support guest-phys-bits <
host-phys-bits in KVM.  I plan to implement it soonish.  In the
meanwhile I guess this patch is fine, we can refine it later.

Paolo

Re: [Qemu-devel] [PATCH v4 1/5] x86: Provide TCG_PHYS_ADDR_BITS

[Eduardo Habkost]

On Fri, Jul 08, 2016 at 04:01:35PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Provide a constant for the number of address bits supported under TCG.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> ---
>  target-i386/cpu.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 474b0b9..b3162b7 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1403,11 +1403,13 @@ uint64_t cpu_get_tsc(CPUX86State *env);
>  /* XXX: This value should match the one returned by CPUID
>   * and in exec.c */
>  # if defined(TARGET_X86_64)
> -# define PHYS_ADDR_MASK 0xffffffffffLL
> +# define TCG_PHYS_ADDR_BITS 40
>  # else
> -# define PHYS_ADDR_MASK 0xfffffffffLL
> +# define TCG_PHYS_ADDR_BITS 36
>  # endif
>  
> +#define PHYS_ADDR_MASK MAKE_64BIT_MASK(0, TCG_PHYS_ADDR_BITS)
> +
>  #define cpu_init(cpu_model) CPU(cpu_x86_init(cpu_model))
>  
>  #define cpu_signal_handler cpu_x86_signal_handler
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 2/5] x86: Allow physical address bits to be set

[Eduardo Habkost]

On Fri, Jul 08, 2016 at 04:01:36PM +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 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 <dgilbert@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).
  */

In either case:

Reviewed-by: Eduardo Habkost <ehabkost@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.

> +            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?

> +        }
> +    }
>      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

Re: [Qemu-devel] [PATCH v4 2/5] x86: Allow physical address bits to be set

[Paolo Bonzini]

On 08/07/2016 20:59, Eduardo Habkost wrote:
> > +        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?

This makes sense as a default, apparently if you don't have PSE36 but 
you have phys_bits > 32, Windows complains:

    commit 45fd08effd461f85d0480d3b8f85a07751fc55b3
    Author: aurel32 <aurel32@c046a42c-6fe2-441c-8c8c-71466251a162>
    Date:   Tue Oct 14 19:20:52 2008 +0000

    target-i386: Add Core Duo Definition
    
    This patch adds a CPU definition for the Core Duo CPU. I tried to
    resemble the original as closely as possible and document what features
    are missing still. This patch enables the use of a recent CPU definition
    on 32 bit platforms.
    
    It also fixes two issues that went along the line:
    
    - invalid xlevel in core2duo spec
      While looking though the CPUIDs again, I found that xlevel is actually 8.
    
    - non-PSE36 support
      The CoreDuo CPUID does not expose the PSE36 capability, but CPUID
    0x80000008 is tied to 36 bits. This broke Windows XP installation for
    me, so I just set it to 32 bits width when PSE36 is not available. The
    original CPU also exposes 32 bit width in CPUID 0x80000008.
    
    Signed-off-by: Alexander Graf <agraf@suse.de>
    Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
    
    git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@5488 c046a42c-6fe2-441c-8c8c-71466251a162

As long as it's overridable I guess it's fine...

Paolo

Re: [Qemu-devel] [PATCH v4 2/5] x86: Allow physical address bits to be set

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Fri, Jul 08, 2016 at 04:01:36PM +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 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 <dgilbert@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 <ehabkost@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 / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 2/5] x86: Allow physical address bits to be set

[Eduardo Habkost]

On Fri, Jul 08, 2016 at 08:25:32PM +0100, Dr. David Alan Gilbert wrote:
> > > +            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).

Yes, I was wondering about the existing behavior. You don't
change any behavior here, as far as I can see.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 4/5] x86: fill high bits of mtrr mask

[Eduardo Habkost]

On Fri, Jul 08, 2016 at 04:01:38PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Fill the bits between 51..number-of-physical-address-bits in the
> MTRR_PHYSMASKn variable range mtrr masks so that they're consistent
> in the migration stream irrespective of the physical address space
> of the source VM in a migration.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 5/5] x86: Set physical address bits based on host

[Eduardo Habkost]

On Fri, Jul 08, 2016 at 04:01:39PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Add the host-phys-bits boolean property, if true, take phys-bits
> from the hosts physical bits value, overriding either the default
> or the user specified value.
> 
> We can also use the value we read from the host to check the users
> explicitly set value and warn them if it doesn't match.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
[...]
> @@ -2952,28 +2977,57 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>             & CPUID_EXT2_AMD_ALIASES);
>      }
>  
> +    /* For 64bit systems think about the number of physical bits to present.
> +     * ideally this should be the same as the host; anything other than matching
> +     * the host can cause incorrect guest behaviour.
> +     * QEMU used to pick the magic value of 40 bits that corresponds to
> +     * consumer AMD devices but nothing else.
> +     */
> +    if (cpu->host_phys_bits && !kvm_enabled()) {
> +        error_setg(errp, "phys-bits can not be read from the host in"
> +                         " TCG mode");
> +        return;
> +    }
> +
>      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).
> -         */
> -        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) {
> +            uint32_t host_phys_bits = x86_host_phys_bits();
> +            static bool warned;
> +
> +            if (cpu->host_phys_bits) {
> +                /* The user asked for us to use the host physical bits */
> +                cpu->phys_bits = host_phys_bits;
> +            }
> +
> +            /* Print a warning if the user set it to a value that's not the
> +             * host value.
> +             */
> +            if (cpu->phys_bits != host_phys_bits && cpu->phys_bits != 0 &&
> +                !warned) {
> +                error_report("Warning: Host physical bits (%u)"
> +                                 " does not match phys-bits property (%u)",
> +                                 host_phys_bits, cpu->phys_bits);
> +                warned = true;
> +            }
> +
> +            if (cpu->phys_bits &&
> +                (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) {
> +            if (cpu->phys_bits && cpu->phys_bits != TCG_PHYS_ADDR_BITS) {
>                  error_setg(errp, "TCG only supports phys-bits=%u",
>                                    TCG_PHYS_ADDR_BITS);
>                  return;
>              }
>          }
> +        if (cpu->phys_bits == 0 && !cpu->host_phys_bits) {

Why the !cpu->host_phys_bits check? It seems to be impossible to
have (cpu->host_phys_bits == true && cpu->phys_bits == 0) here.

> +            cpu->phys_bits = TCG_PHYS_ADDR_BITS;
> +        }
>      } else {
>          /* For 32 bit systems don't use the user set value, but keep
>           * phys_bits consistent with what we tell the guest.

Shouldn't we return error if host-phys-bits is set in 32-bit
mode?

> @@ -3290,6 +3344,7 @@ static Property x86_cpu_properties[] = {
>      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_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
>      DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
>      DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
>      DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 9d79146..3c4e64a 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1184,6 +1184,9 @@ struct X86CPU {
>      /* if true fill the top bits of the MTRR_PHYSMASKn variable range */
>      bool fill_mtrr_mask;
>  
> +    /* if true override the phys_bits value with a value read from the host */
> +    bool host_phys_bits;
> +
>      /* Number of physical address bits supported */
>      uint32_t phys_bits;
>  
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 2/5] x86: Allow physical address bits to be set

[Eduardo Habkost]

On Fri, Jul 08, 2016 at 04:01:36PM +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 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.

I think I've found it, buried in ancient commits:

  commit 5270589032f450ae7c3448730855aa18ff68ccff
  Author: Richard Henderson <rth@twiddle.net>
  Date:   Wed Mar 10 14:33:23 2010 -0800
  
      Move TARGET_PHYS_ADDR_SPACE_BITS to target-*/cpu.h.
      
      Removes a set of ifdefs from exec.c.
      
      Introduce TARGET_VIRT_ADDR_SPACE_BITS for all targets other
      than Alpha.  This will be used for page_find_alloc, which is
      supposed to be using virtual addresses in the first place.
      
      Signed-off-by: Richard Henderson <rth@twiddle.net>
[...]
  --- a/exec.c
  +++ b/exec.c
  [...]
  -#elif defined(TARGET_X86_64)
  -#define TARGET_PHYS_ADDR_SPACE_BITS 42
  -#elif defined(TARGET_I386)
  -#define TARGET_PHYS_ADDR_SPACE_BITS 36
[...]
  --- a/target-i386/cpu.h
  +++ b/target-i386/cpu.h
[...]
  +#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
  +

But I really don't understand why it was changed to 52 when the
macros were moved. A (fortunate) typo?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 0/5] x86: Physical address limit patches

[Eduardo Habkost]

On Fri, Jul 08, 2016 at 04:01:34PM +0100, Dr. David Alan Gilbert (git) wrote:
[...]
> Dr. David Alan Gilbert (5):
>   x86: Provide TCG_PHYS_ADDR_BITS
>   x86: Allow physical address bits to be set
>   x86: Mask mtrr mask based on CPU physical address limits
>   x86: fill high bits of mtrr mask

Patches 1-3 applied to x86-next:

  git://github.com/ehabkost/qemu.git x86-next

>   x86: Set physical address bits based on host

This one is not in yet. I sent a few comments.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 2/5] x86: Allow physical address bits to be set

[Richard Henderson]

On 07/08/2016 04:36 PM, Eduardo Habkost wrote:
>   -#elif defined(TARGET_X86_64)
>   -#define TARGET_PHYS_ADDR_SPACE_BITS 42
>   -#elif defined(TARGET_I386)
>   -#define TARGET_PHYS_ADDR_SPACE_BITS 36
> [...]
>   --- a/target-i386/cpu.h
>   +++ b/target-i386/cpu.h
> [...]
>   +#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
>   +
>
> But I really don't understand why it was changed to 52 when the
> macros were moved. A (fortunate) typo?

It was 6 years ago, but I assume that I looked up 52 as a theoretical maximum. 
I'm not going to look it up again to verify though.


r~

Re: [Qemu-devel] [PATCH v4 2/5] x86: Allow physical address bits to be set

[Eduardo Habkost]

On Fri, Jul 08, 2016 at 05:59:16PM -0700, Richard Henderson wrote:
> On 07/08/2016 04:36 PM, Eduardo Habkost wrote:
> >   -#elif defined(TARGET_X86_64)
> >   -#define TARGET_PHYS_ADDR_SPACE_BITS 42
> >   -#elif defined(TARGET_I386)
> >   -#define TARGET_PHYS_ADDR_SPACE_BITS 36
> > [...]
> >   --- a/target-i386/cpu.h
> >   +++ b/target-i386/cpu.h
> > [...]
> >   +#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
> >   +
> > 
> > But I really don't understand why it was changed to 52 when the
> > macros were moved. A (fortunate) typo?
> 
> It was 6 years ago, but I assume that I looked up 52 as a theoretical
> maximum. I'm not going to look it up again to verify though.

Probably that's the case. David Gilbert also found 52 to be the
maximum.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 5/5] x86: Set physical address bits based on host

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Fri, Jul 08, 2016 at 04:01:39PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Add the host-phys-bits boolean property, if true, take phys-bits
> > from the hosts physical bits value, overriding either the default
> > or the user specified value.
> > 
> > We can also use the value we read from the host to check the users
> > explicitly set value and warn them if it doesn't match.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> [...]
> > @@ -2952,28 +2977,57 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> >             & CPUID_EXT2_AMD_ALIASES);
> >      }
> >  
> > +    /* For 64bit systems think about the number of physical bits to present.
> > +     * ideally this should be the same as the host; anything other than matching
> > +     * the host can cause incorrect guest behaviour.
> > +     * QEMU used to pick the magic value of 40 bits that corresponds to
> > +     * consumer AMD devices but nothing else.
> > +     */
> > +    if (cpu->host_phys_bits && !kvm_enabled()) {
> > +        error_setg(errp, "phys-bits can not be read from the host in"
> > +                         " TCG mode");
> > +        return;
> > +    }
> > +
> >      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).
> > -         */
> > -        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) {
> > +            uint32_t host_phys_bits = x86_host_phys_bits();
> > +            static bool warned;
> > +
> > +            if (cpu->host_phys_bits) {
> > +                /* The user asked for us to use the host physical bits */
> > +                cpu->phys_bits = host_phys_bits;
> > +            }
> > +
> > +            /* Print a warning if the user set it to a value that's not the
> > +             * host value.
> > +             */
> > +            if (cpu->phys_bits != host_phys_bits && cpu->phys_bits != 0 &&
> > +                !warned) {
> > +                error_report("Warning: Host physical bits (%u)"
> > +                                 " does not match phys-bits property (%u)",
> > +                                 host_phys_bits, cpu->phys_bits);
> > +                warned = true;
> > +            }
> > +
> > +            if (cpu->phys_bits &&
> > +                (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) {
> > +            if (cpu->phys_bits && cpu->phys_bits != TCG_PHYS_ADDR_BITS) {
> >                  error_setg(errp, "TCG only supports phys-bits=%u",
> >                                    TCG_PHYS_ADDR_BITS);
> >                  return;
> >              }
> >          }
> > +        if (cpu->phys_bits == 0 && !cpu->host_phys_bits) {
> 
> Why the !cpu->host_phys_bits check? It seems to be impossible to
> have (cpu->host_phys_bits == true && cpu->phys_bits == 0) here.

Right; I think I added that before I added the TCG check at the top.

> > +            cpu->phys_bits = TCG_PHYS_ADDR_BITS;
> > +        }
> >      } else {
> >          /* For 32 bit systems don't use the user set value, but keep
> >           * phys_bits consistent with what we tell the guest.
> 
> Shouldn't we return error if host-phys-bits is set in 32-bit
> mode?

Yes; easy to add.

Do you want me to repost just this patch or the whole set?

Dave

> > @@ -3290,6 +3344,7 @@ static Property x86_cpu_properties[] = {
> >      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_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
> >      DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
> >      DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
> >      DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 9d79146..3c4e64a 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -1184,6 +1184,9 @@ struct X86CPU {
> >      /* if true fill the top bits of the MTRR_PHYSMASKn variable range */
> >      bool fill_mtrr_mask;
> >  
> > +    /* if true override the phys_bits value with a value read from the host */
> > +    bool host_phys_bits;
> > +
> >      /* Number of physical address bits supported */
> >      uint32_t phys_bits;
> >  
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Eduardo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 5/5] x86: Set physical address bits based on host

[Eduardo Habkost]

On Mon, Jul 11, 2016 at 01:29:56PM +0100, Dr. David Alan Gilbert wrote:
[...]
> 
> Do you want me to repost just this patch or the whole set?

You can use my x86-next branch as base (it already includes the
other patches), so you can repost just this patch.

 git://github.com/ehabkost/qemu.git x86-next

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 5/5] x86: Set physical address bits based on host

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Fri, Jul 08, 2016 at 04:01:39PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Add the host-phys-bits boolean property, if true, take phys-bits
> > from the hosts physical bits value, overriding either the default
> > or the user specified value.
> > 
> > We can also use the value we read from the host to check the users
> > explicitly set value and warn them if it doesn't match.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> [...]
> > @@ -2952,28 +2977,57 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> >             & CPUID_EXT2_AMD_ALIASES);
> >      }
> >  
> > +    /* For 64bit systems think about the number of physical bits to present.
> > +     * ideally this should be the same as the host; anything other than matching
> > +     * the host can cause incorrect guest behaviour.
> > +     * QEMU used to pick the magic value of 40 bits that corresponds to
> > +     * consumer AMD devices but nothing else.
> > +     */
> > +    if (cpu->host_phys_bits && !kvm_enabled()) {
> > +        error_setg(errp, "phys-bits can not be read from the host in"
> > +                         " TCG mode");
> > +        return;
> > +    }
> > +
> >      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).
> > -         */
> > -        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) {
> > +            uint32_t host_phys_bits = x86_host_phys_bits();
> > +            static bool warned;
> > +
> > +            if (cpu->host_phys_bits) {
> > +                /* The user asked for us to use the host physical bits */
> > +                cpu->phys_bits = host_phys_bits;
> > +            }
> > +
> > +            /* Print a warning if the user set it to a value that's not the
> > +             * host value.
> > +             */
> > +            if (cpu->phys_bits != host_phys_bits && cpu->phys_bits != 0 &&
> > +                !warned) {
> > +                error_report("Warning: Host physical bits (%u)"
> > +                                 " does not match phys-bits property (%u)",
> > +                                 host_phys_bits, cpu->phys_bits);
> > +                warned = true;
> > +            }
> > +
> > +            if (cpu->phys_bits &&
> > +                (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) {
> > +            if (cpu->phys_bits && cpu->phys_bits != TCG_PHYS_ADDR_BITS) {
> >                  error_setg(errp, "TCG only supports phys-bits=%u",
> >                                    TCG_PHYS_ADDR_BITS);
> >                  return;
> >              }
> >          }
> > +        if (cpu->phys_bits == 0 && !cpu->host_phys_bits) {
> 
> Why the !cpu->host_phys_bits check? It seems to be impossible to
> have (cpu->host_phys_bits == true && cpu->phys_bits == 0) here.
> 
> > +            cpu->phys_bits = TCG_PHYS_ADDR_BITS;
> > +        }
> >      } else {
> >          /* For 32 bit systems don't use the user set value, but keep
> >           * phys_bits consistent with what we tell the guest.
> 
> Shouldn't we return error if host-phys-bits is set in 32-bit
> mode?

I've just realised there's a reason that erroring in this case is a problem.
Imagine a future (or downstream) machine type that made host-phys-bits the default;
how would it run with a 32bit CPU?

Dave

> > @@ -3290,6 +3344,7 @@ static Property x86_cpu_properties[] = {
> >      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_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
> >      DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
> >      DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
> >      DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 9d79146..3c4e64a 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -1184,6 +1184,9 @@ struct X86CPU {
> >      /* if true fill the top bits of the MTRR_PHYSMASKn variable range */
> >      bool fill_mtrr_mask;
> >  
> > +    /* if true override the phys_bits value with a value read from the host */
> > +    bool host_phys_bits;
> > +
> >      /* Number of physical address bits supported */
> >      uint32_t phys_bits;
> >  
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Eduardo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 5/5] x86: Set physical address bits based on host

[Eduardo Habkost]

On Mon, Jul 11, 2016 at 04:39:22PM +0100, Dr. David Alan Gilbert wrote:
> * Eduardo Habkost (ehabkost@redhat.com) wrote:
[...]
> > > +            cpu->phys_bits = TCG_PHYS_ADDR_BITS;
> > > +        }
> > >      } else {
> > >          /* For 32 bit systems don't use the user set value, but keep
> > >           * phys_bits consistent with what we tell the guest.
> > 
> > Shouldn't we return error if host-phys-bits is set in 32-bit
> > mode?
> 
> I've just realised there's a reason that erroring in this case is a problem.
> Imagine a future (or downstream) machine type that made host-phys-bits the default;
> how would it run with a 32bit CPU?

Oh, that's right. Ignoring it when explicitly set isn't
intuitive, but we need to be able to ignore it when set by a
machine compat_props (or if it's enabled by default). And
creating two separate properties sounds like overkill...

I'm reluctant, but I think it's OK to ignore it if LM is not set.
But we need to make sure it's documented somewhere.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 5/5] x86: Set physical address bits based on host

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Mon, Jul 11, 2016 at 04:39:22PM +0100, Dr. David Alan Gilbert wrote:
> > * Eduardo Habkost (ehabkost@redhat.com) wrote:
> [...]
> > > > +            cpu->phys_bits = TCG_PHYS_ADDR_BITS;
> > > > +        }
> > > >      } else {
> > > >          /* For 32 bit systems don't use the user set value, but keep
> > > >           * phys_bits consistent with what we tell the guest.
> > > 
> > > Shouldn't we return error if host-phys-bits is set in 32-bit
> > > mode?
> > 
> > I've just realised there's a reason that erroring in this case is a problem.
> > Imagine a future (or downstream) machine type that made host-phys-bits the default;
> > how would it run with a 32bit CPU?
> 
> Oh, that's right. Ignoring it when explicitly set isn't
> intuitive, but we need to be able to ignore it when set by a
> machine compat_props (or if it's enabled by default). And
> creating two separate properties sounds like overkill...
> I'm reluctant, but I think it's OK to ignore it if LM is not set.
> But we need to make sure it's documented somewhere.

OK, I'll add a comment and a note in the commit message.

Dave

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