Introduce the auto_topoext bit to to control topoext feature.

Also add new field auto_topoext(in X86CPUDefinition). This will
be used to enable topoext on newer CPU models where topoext can
be supported.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 include/hw/i386/pc.h |  4 ++++
 target/i386/cpu.c    | 12 ++++++++++++
 target/i386/cpu.h    |  5 +++++
 3 files changed, 21 insertions(+)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 04d1f8c..cc30ec3 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -303,6 +303,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
         .driver   = TYPE_X86_CPU,\
         .property = "legacy-cache",\
         .value    = "on",\
+    },{\
+        .driver   = TYPE_X86_CPU,\
+        .property = "auto-topoext",\
+        .value    = "off",\
     },
 
 #define PC_COMPAT_2_11 \
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 86fb1a4..d3411ed 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1283,6 +1283,8 @@ struct X86CPUDefinition {
     FeatureWordArray features;
     const char *model_id;
     CPUCaches *cache_info;
+    /* Set it if topoext can be enabled in CPU models */
+    int auto_topoext;
 };
 
 static CPUCaches epyc_cache_info = {
@@ -3517,6 +3519,9 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
     /* legacy-cache defaults to 'off' if CPU model provides cache info */
     cpu->legacy_cache = !def->cache_info;
 
+    /* Set auto_topoext if both machine property and CPU model supports it */
+    cpu->auto_topoext =  cpu->auto_topoext & def->auto_topoext;
+
     /* Special cases not set in the X86CPUDefinition structs: */
     /* TODO: in-kernel irqchip for hvf */
     if (kvm_enabled()) {
@@ -5382,6 +5387,13 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
 
     /*
+     * auto-topoext property will be used to enable topoext feature.
+     * This will be disabled on all the older CPU models. Will be
+     * enabled on newer CPU modeles which can support topology extention.
+     */
+     DEFINE_PROP_BOOL("auto-topoext", X86CPU, auto_topoext, false),
+
+    /*
      * From "Requirements for Implementing the Microsoft
      * Hypervisor Interface":
      * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 89c82be..8783d36 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1409,6 +1409,11 @@ struct X86CPU {
      */
     bool legacy_cache;
 
+    /* Compatibility bits to enable topoext feature on all newer machines
+     * Disabled on older machines. Enabled on newer CPU models
+     */
+    bool auto_topoext;
+
     /* Compatibility bits for old machine types: */
     bool enable_cpuid_0xb;
 
-- 
1.8.3.1

Hi Eduardo,
Planning  to make couple of changes after testing and review.  Let me know if I missed something.
Will wait for your feedback before the next revision.

> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On Behalf Of Babu Moger
> Sent: Friday, June 8, 2018 5:56 PM
> To: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> rth@twiddle.net; ehabkost@redhat.com; mtosatti@redhat.com
> Cc: qemu-devel@nongnu.org; kvm@vger.kernel.org; Moger, Babu
> <Babu.Moger@amd.com>; kash@tripleback.net; geoff@hostfission.com
> Subject: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage
> topoext
> 
> Introduce the auto_topoext bit to to control topoext feature.
> 
> Also add new field auto_topoext(in X86CPUDefinition). This will
> be used to enable topoext on newer CPU models where topoext can
> be supported.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  include/hw/i386/pc.h |  4 ++++
>  target/i386/cpu.c    | 12 ++++++++++++
>  target/i386/cpu.h    |  5 +++++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 04d1f8c..cc30ec3 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -303,6 +303,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *,
> uint64_t *);
>          .driver   = TYPE_X86_CPU,\
>          .property = "legacy-cache",\
>          .value    = "on",\
> +    },{\
> +        .driver   = TYPE_X86_CPU,\
> +        .property = "auto-topoext",\
> +        .value    = "off",\
>      },

We don't need this.  We are already setting this false in x86_cpu_properties.
But we need to set it "on" for EPYC on PC_COMPAT_3_0 whenever we define it. 
That will be a separate patch. Correct?

> 
>  #define PC_COMPAT_2_11 \
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 86fb1a4..d3411ed 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1283,6 +1283,8 @@ struct X86CPUDefinition {
>      FeatureWordArray features;
>      const char *model_id;
>      CPUCaches *cache_info;
> +    /* Set it if topoext can be enabled in CPU models */
> +    int auto_topoext;
>  };
> 
>  static CPUCaches epyc_cache_info = {
> @@ -3517,6 +3519,9 @@ static void x86_cpu_load_def(X86CPU *cpu,
> X86CPUDefinition *def, Error **errp)
>      /* legacy-cache defaults to 'off' if CPU model provides cache info */
>      cpu->legacy_cache = !def->cache_info;
> 
> +    /* Set auto_topoext if both machine property and CPU model supports it
> */
> +    cpu->auto_topoext =  cpu->auto_topoext & def->auto_topoext;

This could be more appropriate like this.

 cpu->auto_topoext =  cpu->auto_topoext && def->auto_topoext;

> +
>      /* Special cases not set in the X86CPUDefinition structs: */
>      /* TODO: in-kernel irqchip for hvf */
>      if (kvm_enabled()) {
> @@ -5382,6 +5387,13 @@ static Property x86_cpu_properties[] = {
>      DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
> 
>      /*
> +     * auto-topoext property will be used to enable topoext feature.
> +     * This will be disabled on all the older CPU models. Will be
> +     * enabled on newer CPU modeles which can support topology extention.
> +     */
> +     DEFINE_PROP_BOOL("auto-topoext", X86CPU, auto_topoext, false),
> +
> +    /*
>       * From "Requirements for Implementing the Microsoft
>       * Hypervisor Interface":
>       * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-
> windows/reference/tlfs
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 89c82be..8783d36 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1409,6 +1409,11 @@ struct X86CPU {
>       */
>      bool legacy_cache;
> 
> +    /* Compatibility bits to enable topoext feature on all newer machines
> +     * Disabled on older machines. Enabled on newer CPU models
> +     */
> +    bool auto_topoext;
> +
>      /* Compatibility bits for old machine types: */
>      bool enable_cpuid_0xb;
> 
> --
> 1.8.3.1

On Mon, Jun 11, 2018 at 08:25:32PM +0000, Moger, Babu wrote:
> Hi Eduardo,
> Planning  to make couple of changes after testing and review.  Let me know if I missed something.
> Will wait for your feedback before the next revision.
> 
> > -----Original Message-----
> > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> > On Behalf Of Babu Moger
> > Sent: Friday, June 8, 2018 5:56 PM
> > To: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> > rth@twiddle.net; ehabkost@redhat.com; mtosatti@redhat.com
> > Cc: qemu-devel@nongnu.org; kvm@vger.kernel.org; Moger, Babu
> > <Babu.Moger@amd.com>; kash@tripleback.net; geoff@hostfission.com
> > Subject: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage
> > topoext
> > 
> > Introduce the auto_topoext bit to to control topoext feature.
> > 
> > Also add new field auto_topoext(in X86CPUDefinition). This will
> > be used to enable topoext on newer CPU models where topoext can
> > be supported.
> > 
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  include/hw/i386/pc.h |  4 ++++
> >  target/i386/cpu.c    | 12 ++++++++++++
> >  target/i386/cpu.h    |  5 +++++
> >  3 files changed, 21 insertions(+)
> > 
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 04d1f8c..cc30ec3 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -303,6 +303,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *,
> > uint64_t *);
> >          .driver   = TYPE_X86_CPU,\
> >          .property = "legacy-cache",\
> >          .value    = "on",\
> > +    },{\
> > +        .driver   = TYPE_X86_CPU,\
> > +        .property = "auto-topoext",\
> > +        .value    = "off",\
> >      },
> 
> We don't need this.  We are already setting this false in x86_cpu_properties.
> But we need to set it "on" for EPYC on PC_COMPAT_3_0 whenever we define it. 
> That will be a separate patch. Correct?

PC_COMPAT_3_0 will exist only on QEMU 3.1.  If you want to define
something as the default behavior in pc-3.0, just define it as
the default in the device code, which is exactly what you are
already doing in x86_cpu_load_def() below.

In other words, just set auto_topoext=true on the CPU model table
for EPYC, and it should be enough.

On PC_COMPAT_2_12, both would work:
  { TYPE_X86_CPU, "auto-topoext", "off" }
or
  { "EPYC" "-" TYPE_X86_CPU, "auto-topoext", "off" }.

I prefer the latter, but both would work.


> 
> > 
> >  #define PC_COMPAT_2_11 \
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 86fb1a4..d3411ed 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -1283,6 +1283,8 @@ struct X86CPUDefinition {
> >      FeatureWordArray features;
> >      const char *model_id;
> >      CPUCaches *cache_info;
> > +    /* Set it if topoext can be enabled in CPU models */
> > +    int auto_topoext;
> >  };
> > 
> >  static CPUCaches epyc_cache_info = {
> > @@ -3517,6 +3519,9 @@ static void x86_cpu_load_def(X86CPU *cpu,
> > X86CPUDefinition *def, Error **errp)
> >      /* legacy-cache defaults to 'off' if CPU model provides cache info */
> >      cpu->legacy_cache = !def->cache_info;
> > 
> > +    /* Set auto_topoext if both machine property and CPU model supports it
> > */
> > +    cpu->auto_topoext =  cpu->auto_topoext & def->auto_topoext;
> 
> This could be more appropriate like this.
> 
>  cpu->auto_topoext =  cpu->auto_topoext && def->auto_topoext;

cpu->auto_topoext is supposed to be already false, here.  Why not
just:
    cpu->auto_topoext = def->auto_topoext;
?

> 
> > +
> >      /* Special cases not set in the X86CPUDefinition structs: */
> >      /* TODO: in-kernel irqchip for hvf */
> >      if (kvm_enabled()) {
> > @@ -5382,6 +5387,13 @@ static Property x86_cpu_properties[] = {
> >      DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
> > 
> >      /*
> > +     * auto-topoext property will be used to enable topoext feature.
> > +     * This will be disabled on all the older CPU models. Will be
> > +     * enabled on newer CPU modeles which can support topology extention.
> > +     */
> > +     DEFINE_PROP_BOOL("auto-topoext", X86CPU, auto_topoext, false),
> > +
> > +    /*
> >       * From "Requirements for Implementing the Microsoft
> >       * Hypervisor Interface":
> >       * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-
> > windows/reference/tlfs
> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > index 89c82be..8783d36 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -1409,6 +1409,11 @@ struct X86CPU {
> >       */
> >      bool legacy_cache;
> > 
> > +    /* Compatibility bits to enable topoext feature on all newer machines
> > +     * Disabled on older machines. Enabled on newer CPU models
> > +     */
> > +    bool auto_topoext;
> > +
> >      /* Compatibility bits for old machine types: */
> >      bool enable_cpuid_0xb;
> > 
> > --
> > 1.8.3.1
> 

-- 
Eduardo

> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Monday, June 11, 2018 3:46 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com
> Subject: Re: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage
> topoext
> 
> On Mon, Jun 11, 2018 at 08:25:32PM +0000, Moger, Babu wrote:
> > Hi Eduardo,
> > Planning  to make couple of changes after testing and review.  Let me know
> if I missed something.
> > Will wait for your feedback before the next revision.
> >
> > > -----Original Message-----
> > > From: kvm-owner@vger.kernel.org [mailto:kvm-
> owner@vger.kernel.org]
> > > On Behalf Of Babu Moger
> > > Sent: Friday, June 8, 2018 5:56 PM
> > > To: mst@redhat.com; marcel.apfelbaum@gmail.com;
> pbonzini@redhat.com;
> > > rth@twiddle.net; ehabkost@redhat.com; mtosatti@redhat.com
> > > Cc: qemu-devel@nongnu.org; kvm@vger.kernel.org; Moger, Babu
> > > <Babu.Moger@amd.com>; kash@tripleback.net; geoff@hostfission.com
> > > Subject: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage
> > > topoext
> > >
> > > Introduce the auto_topoext bit to to control topoext feature.
> > >
> > > Also add new field auto_topoext(in X86CPUDefinition). This will
> > > be used to enable topoext on newer CPU models where topoext can
> > > be supported.
> > >
> > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > ---
> > >  include/hw/i386/pc.h |  4 ++++
> > >  target/i386/cpu.c    | 12 ++++++++++++
> > >  target/i386/cpu.h    |  5 +++++
> > >  3 files changed, 21 insertions(+)
> > >
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index 04d1f8c..cc30ec3 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -303,6 +303,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *,
> > > uint64_t *);
> > >          .driver   = TYPE_X86_CPU,\
> > >          .property = "legacy-cache",\
> > >          .value    = "on",\
> > > +    },{\
> > > +        .driver   = TYPE_X86_CPU,\
> > > +        .property = "auto-topoext",\
> > > +        .value    = "off",\
> > >      },
> >
> > We don't need this.  We are already setting this false in
> x86_cpu_properties.
> > But we need to set it "on" for EPYC on PC_COMPAT_3_0 whenever we
> define it.
> > That will be a separate patch. Correct?
> 
> PC_COMPAT_3_0 will exist only on QEMU 3.1.  If you want to define
> something as the default behavior in pc-3.0, just define it as
> the default in the device code, which is exactly what you are
> already doing in x86_cpu_load_def() below.
> 
> In other words, just set auto_topoext=true on the CPU model table
> for EPYC, and it should be enough.
> 
> On PC_COMPAT_2_12, both would work:
>   { TYPE_X86_CPU, "auto-topoext", "off" }
> or
>   { "EPYC" "-" TYPE_X86_CPU, "auto-topoext", "off" }.
> 
> I prefer the latter, but both would work.

Ok. Sure

> 
> 
> >
> > >
> > >  #define PC_COMPAT_2_11 \
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index 86fb1a4..d3411ed 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -1283,6 +1283,8 @@ struct X86CPUDefinition {
> > >      FeatureWordArray features;
> > >      const char *model_id;
> > >      CPUCaches *cache_info;
> > > +    /* Set it if topoext can be enabled in CPU models */
> > > +    int auto_topoext;
> > >  };
> > >
> > >  static CPUCaches epyc_cache_info = {
> > > @@ -3517,6 +3519,9 @@ static void x86_cpu_load_def(X86CPU *cpu,
> > > X86CPUDefinition *def, Error **errp)
> > >      /* legacy-cache defaults to 'off' if CPU model provides cache info */
> > >      cpu->legacy_cache = !def->cache_info;
> > >
> > > +    /* Set auto_topoext if both machine property and CPU model
> supports it
> > > */
> > > +    cpu->auto_topoext =  cpu->auto_topoext & def->auto_topoext;
> >
> > This could be more appropriate like this.
> >
> >  cpu->auto_topoext =  cpu->auto_topoext && def->auto_topoext;
> 
> cpu->auto_topoext is supposed to be already false, here.  Why not
> just:
>     cpu->auto_topoext = def->auto_topoext;
> ?

Ok. Will do it.

> 
> >
> > > +
> > >      /* Special cases not set in the X86CPUDefinition structs: */
> > >      /* TODO: in-kernel irqchip for hvf */
> > >      if (kvm_enabled()) {
> > > @@ -5382,6 +5387,13 @@ static Property x86_cpu_properties[] = {
> > >      DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
> > >
> > >      /*
> > > +     * auto-topoext property will be used to enable topoext feature.
> > > +     * This will be disabled on all the older CPU models. Will be
> > > +     * enabled on newer CPU modeles which can support topology
> extention.
> > > +     */
> > > +     DEFINE_PROP_BOOL("auto-topoext", X86CPU, auto_topoext, false),
> > > +
> > > +    /*
> > >       * From "Requirements for Implementing the Microsoft
> > >       * Hypervisor Interface":
> > >       * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-
> > > windows/reference/tlfs
> > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > > index 89c82be..8783d36 100644
> > > --- a/target/i386/cpu.h
> > > +++ b/target/i386/cpu.h
> > > @@ -1409,6 +1409,11 @@ struct X86CPU {
> > >       */
> > >      bool legacy_cache;
> > >
> > > +    /* Compatibility bits to enable topoext feature on all newer machines
> > > +     * Disabled on older machines. Enabled on newer CPU models
> > > +     */
> > > +    bool auto_topoext;
> > > +
> > >      /* Compatibility bits for old machine types: */
> > >      bool enable_cpuid_0xb;
> > >
> > > --
> > > 1.8.3.1
> >
> 
> --
> Eduardo

On Mon, Jun 11, 2018 at 05:46:23PM -0300, Eduardo Habkost wrote:
[...]
> On PC_COMPAT_2_12, both would work:
>   { TYPE_X86_CPU, "auto-topoext", "off" }
> or
>   { "EPYC" "-" TYPE_X86_CPU, "auto-topoext", "off" }.
> 
> I prefer the latter, but both would work.

Oh, while we're at it: please name the property "x-auto-topoext",
to indicate it's only for QEMU internal use or debugging, and not
a supported command-line option.

-- 
Eduardo

> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On Behalf Of Eduardo Habkost
> Sent: Monday, June 11, 2018 4:05 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com
> Subject: Re: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage
> topoext
> 
> On Mon, Jun 11, 2018 at 05:46:23PM -0300, Eduardo Habkost wrote:
> [...]
> > On PC_COMPAT_2_12, both would work:
> >   { TYPE_X86_CPU, "auto-topoext", "off" }
> > or
> >   { "EPYC" "-" TYPE_X86_CPU, "auto-topoext", "off" }.
> >
> > I prefer the latter, but both would work.
> 
> Oh, while we're at it: please name the property "x-auto-topoext",
> to indicate it's only for QEMU internal use or debugging, and not
> a supported command-line option.

Ok. Sure.

> 
> --
> Eduardo