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

[Dr. David Alan Gilbert]

* Eduardo Habkost (address@hidden) wrote:
> 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.

Ah, so that explains what that other comment was about;
to me that looks TCG only, so I should actually be careful in my phys-bits=0
case to go back to using 40 for TCG.

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

Yep I've moved the sanity check up to this patch as commented in the
later one.

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

Probably TARGET_PHYS_ADDR_SPACE_BITS which is the magic 52 in the x86-64
case.

Dave
> >          } 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
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK