[Qemu-devel] [PATCH 0/2] Support over 64 vcpus for windows guests

[Qemu-devel] [PATCH 0/2] Support over 64 vcpus for windows guests

[Gonglei]

Pls see the detail of patches.

Gonglei (2):
  pc: add 2.11 machine type
  i386/cpu/hyperv: support over 64 vcpus for windows guests

 hw/i386/pc_piix.c    | 15 ++++++++++++---
 hw/i386/pc_q35.c     | 13 +++++++++++--
 include/hw/i386/pc.h |  8 ++++++++
 target/i386/cpu.c    |  1 +
 target/i386/cpu.h    |  2 ++
 target/i386/kvm.c    | 18 +++++++++++++++++-
 6 files changed, 51 insertions(+), 6 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] pc: add 2.11 machine type

[Gonglei]

CC: "Michael S. Tsirkin" <mst@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Richard Henderson <rth@twiddle.net>
CC: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/i386/pc_piix.c    | 15 ++++++++++++---
 hw/i386/pc_q35.c     | 13 +++++++++++--
 include/hw/i386/pc.h |  3 +++
 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 46dfd2c..b03cc04 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -436,21 +436,30 @@ static void pc_i440fx_machine_options(MachineClass *m)
     m->default_display = "std";
 }
 
-static void pc_i440fx_2_10_machine_options(MachineClass *m)
+static void pc_i440fx_2_11_machine_options(MachineClass *m)
 {
     pc_i440fx_machine_options(m);
     m->alias = "pc";
     m->is_default = 1;
 }
 
+DEFINE_I440FX_MACHINE(v2_11, "pc-i440fx-2.11", NULL,
+                      pc_i440fx_2_11_machine_options);
+
+static void pc_i440fx_2_10_machine_options(MachineClass *m)
+{
+    pc_i440fx_2_11_machine_options(m);
+    m->is_default = 0;
+    m->alias = NULL;
+    SET_MACHINE_COMPAT(m, PC_COMPAT_2_10);
+}
+
 DEFINE_I440FX_MACHINE(v2_10, "pc-i440fx-2.10", NULL,
                       pc_i440fx_2_10_machine_options);
 
 static void pc_i440fx_2_9_machine_options(MachineClass *m)
 {
     pc_i440fx_2_10_machine_options(m);
-    m->is_default = 0;
-    m->alias = NULL;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);
     m->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 169a214..d6fef8d 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -302,20 +302,29 @@ static void pc_q35_machine_options(MachineClass *m)
     m->max_cpus = 288;
 }
 
-static void pc_q35_2_10_machine_options(MachineClass *m)
+static void pc_q35_2_11_machine_options(MachineClass *m)
 {
     pc_q35_machine_options(m);
     m->alias = "q35";
     m->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
 }
 
+DEFINE_Q35_MACHINE(v2_11, "pc-q35-2.11", NULL,
+                   pc_q35_2_11_machine_options);
+
+static void pc_q35_2_10_machine_options(MachineClass *m)
+{
+    pc_q35_2_11_machine_options(m);
+    m->alias = NULL;
+    SET_MACHINE_COMPAT(m, PC_COMPAT_2_10);
+}
+
 DEFINE_Q35_MACHINE(v2_10, "pc-q35-2.10", NULL,
                    pc_q35_2_10_machine_options);
 
 static void pc_q35_2_9_machine_options(MachineClass *m)
 {
     pc_q35_2_10_machine_options(m);
-    m->alias = NULL;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);
 }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index d80859b..8226904 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -369,6 +369,9 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
 int e820_get_num_entries(void);
 bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
+#define PC_COMPAT_2_10 \
+    HW_COMPAT_2_10 \
+
 #define PC_COMPAT_2_9 \
     HW_COMPAT_2_9 \
     {\
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] i386/cpu/hyperv: support over 64 vcpus for windows guests

[Gonglei]

Starting with Windows Server 2012 and Windows 8, if
CPUID.40000005.EAX contains a value of -1, Windows assumes specific
limit to the number of VPs. In this case, Windows Server 2012
guest VMs may use more than 64 VPs, up to the maximum supported
number of processors applicable to the specific Windows
version being used.

https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs

For compatibility, Let's introduce a new property for X86CPU,
named "hv-cpuid-limits-eax" as Paolo's suggestion, and set it
to "on" before machine 2.10.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 include/hw/i386/pc.h |  5 +++++
 target/i386/cpu.c    |  1 +
 target/i386/cpu.h    |  2 ++
 target/i386/kvm.c    | 18 +++++++++++++++++-
 4 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 8226904..db32e58 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -371,6 +371,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
 #define PC_COMPAT_2_10 \
     HW_COMPAT_2_10 \
+    {\
+        .driver   = TYPE_X86_CPU,\
+        .property = "hv_cpuid_limits_eax",\
+        .value    = "on",\
+    },\
 
 #define PC_COMPAT_2_9 \
     HW_COMPAT_2_9 \
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 69676e1..0d47bdd 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4145,6 +4145,7 @@ static Property x86_cpu_properties[] = {
                      false),
     DEFINE_PROP_BOOL("vmware-cpuid-freq", X86CPU, vmware_cpuid_freq, true),
     DEFINE_PROP_BOOL("tcg-cpuid", X86CPU, expose_tcg, true),
+    DEFINE_PROP_BOOL("hv-cpuid-limits-eax", X86CPU, hv_cpuid_limits_eax, false),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 525d35d..f8b455a 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1282,6 +1282,8 @@ struct X86CPU {
     int32_t socket_id;
     int32_t core_id;
     int32_t thread_id;
+
+    bool hv_cpuid_limits_eax;
 };
 
 static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 6db7783..cf6ef96 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -751,7 +751,23 @@ int kvm_arch_init_vcpu(CPUState *cs)
 
         c = &cpuid_data.entries[cpuid_i++];
         c->function = HYPERV_CPUID_IMPLEMENT_LIMITS;
-        c->eax = 0x40;
+
+        if (!cpu->hv_cpuid_limits_eax) {
+            /*
+             * Starting with Windows Server 2012 and Windows 8, if
+             * CPUID.40000005.EAX contains a value of -1, Windows
+             * assumes specific limit to the number of VPs. In this case,
+             * Windows Server 2012 guest VMs may use more than 64 VPs,
+             * up to the maximum supported number of processors
+             * applicable to the specific Windows version being used.
+             *
+             * https://docs.microsoft.com/en-us/virtualization/
+             *    hyper-v-on-windows/reference/tlfs
+             */
+            c->eax = -1;
+        } else {
+            c->eax = 0x40;
+        }
         c->ebx = 0x40;
 
         kvm_base = KVM_CPUID_SIGNATURE_NEXT;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] i386/cpu/hyperv: support over 64 vcpus for windows guests

[Eduardo Habkost]

On Tue, Sep 05, 2017 at 05:30:05PM +0800, Gonglei wrote:
> Starting with Windows Server 2012 and Windows 8, if
> CPUID.40000005.EAX contains a value of -1, Windows assumes specific
> limit to the number of VPs. In this case, Windows Server 2012
> guest VMs may use more than 64 VPs, up to the maximum supported
> number of processors applicable to the specific Windows
> version being used.
> 
> https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
> 
> For compatibility, Let's introduce a new property for X86CPU,
> named "hv-cpuid-limits-eax" as Paolo's suggestion, and set it
> to "on" before machine 2.10.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  include/hw/i386/pc.h |  5 +++++
>  target/i386/cpu.c    |  1 +
>  target/i386/cpu.h    |  2 ++
>  target/i386/kvm.c    | 18 +++++++++++++++++-
>  4 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 8226904..db32e58 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -371,6 +371,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>  
>  #define PC_COMPAT_2_10 \
>      HW_COMPAT_2_10 \
> +    {\
> +        .driver   = TYPE_X86_CPU,\
> +        .property = "hv_cpuid_limits_eax",\

The property name is hv-cpuid-limits-eax.

> +        .value    = "on",\
> +    },\
>  
>  #define PC_COMPAT_2_9 \
>      HW_COMPAT_2_9 \
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 69676e1..0d47bdd 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4145,6 +4145,7 @@ static Property x86_cpu_properties[] = {
>                       false),
>      DEFINE_PROP_BOOL("vmware-cpuid-freq", X86CPU, vmware_cpuid_freq, true),
>      DEFINE_PROP_BOOL("tcg-cpuid", X86CPU, expose_tcg, true),
> +    DEFINE_PROP_BOOL("hv-cpuid-limits-eax", X86CPU, hv_cpuid_limits_eax, false),

The property name "hv-cpuid-limits-eax" doesn't say anything
about what it does exactly when set to true.

What about just making it int32?  e.g.:

    DEFINE_PROP_INT32("x-hv-max-vps", X86CPU, hv_max_vps, -1)
[...]
    {\
        .driver   = TYPE_X86_CPU,\
        .property = "x-hv-max-vps",\
        .value    = "0x40",\
    },\
[...]
    c->function = HYPERV_CPUID_IMPLEMENT_LIMITS;
    c->eax = cpu->hv_max_vps;


(The "x-" prefix indicates that the property is not supposed to
be a stable user interface.)


>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 525d35d..f8b455a 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1282,6 +1282,8 @@ struct X86CPU {
>      int32_t socket_id;
>      int32_t core_id;
>      int32_t thread_id;
> +
> +    bool hv_cpuid_limits_eax;
>  };
>  
>  static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 6db7783..cf6ef96 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -751,7 +751,23 @@ int kvm_arch_init_vcpu(CPUState *cs)
>  
>          c = &cpuid_data.entries[cpuid_i++];
>          c->function = HYPERV_CPUID_IMPLEMENT_LIMITS;
> -        c->eax = 0x40;
> +
> +        if (!cpu->hv_cpuid_limits_eax) {
> +            /*
> +             * Starting with Windows Server 2012 and Windows 8, if
> +             * CPUID.40000005.EAX contains a value of -1, Windows
> +             * assumes specific limit to the number of VPs. In this case,
> +             * Windows Server 2012 guest VMs may use more than 64 VPs,
> +             * up to the maximum supported number of processors
> +             * applicable to the specific Windows version being used.

That was a direct quote from a document, so I recommend citing
the specific document you quoted.  e.g.:

    /*
     * From "Requirements for Implementing the Microsoft
     * Hypervisor Interface":
     * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
     *
     * "Starting with Windows Server 2012 and Windows 8, if
     * CPUID.40000005.EAX contains a value of -1, Windows assumes
     * specific limit to the number of VPs. In this case, Windows
     * Server 2012 guest VMs may use more than 64 VPs, up to the
     * maximum supported number of processors applicable to the
     * specific Windows version being used."
     */


> +             *
> +             * https://docs.microsoft.com/en-us/virtualization/
> +             *    hyper-v-on-windows/reference/tlfs

IMO a long line is preferable to a broken URL.

> +             */
> +            c->eax = -1;
> +        } else {
> +            c->eax = 0x40;
> +        }
>          c->ebx = 0x40;
>  
>          kvm_base = KVM_CPUID_SIGNATURE_NEXT;
> -- 
> 1.8.3.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/2] pc: add 2.11 machine type

[Eduardo Habkost]

On Tue, Sep 05, 2017 at 05:30:04PM +0800, Gonglei wrote:
> CC: "Michael S. Tsirkin" <mst@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Richard Henderson <rth@twiddle.net>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/i386/pc_piix.c    | 15 ++++++++++++---
>  hw/i386/pc_q35.c     | 13 +++++++++++--
>  include/hw/i386/pc.h |  3 +++
>  3 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 46dfd2c..b03cc04 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -436,21 +436,30 @@ static void pc_i440fx_machine_options(MachineClass *m)
>      m->default_display = "std";
>  }
>  
> -static void pc_i440fx_2_10_machine_options(MachineClass *m)
> +static void pc_i440fx_2_11_machine_options(MachineClass *m)
>  {
>      pc_i440fx_machine_options(m);
>      m->alias = "pc";
>      m->is_default = 1;
>  }
>  
> +DEFINE_I440FX_MACHINE(v2_11, "pc-i440fx-2.11", NULL,
> +                      pc_i440fx_2_11_machine_options);
> +
> +static void pc_i440fx_2_10_machine_options(MachineClass *m)
> +{
> +    pc_i440fx_2_11_machine_options(m);
> +    m->is_default = 0;
> +    m->alias = NULL;
> +    SET_MACHINE_COMPAT(m, PC_COMPAT_2_10);
> +}
> +
>  DEFINE_I440FX_MACHINE(v2_10, "pc-i440fx-2.10", NULL,
>                        pc_i440fx_2_10_machine_options);
>  
>  static void pc_i440fx_2_9_machine_options(MachineClass *m)
>  {
>      pc_i440fx_2_10_machine_options(m);
> -    m->is_default = 0;
> -    m->alias = NULL;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);
>      m->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
>  }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 169a214..d6fef8d 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -302,20 +302,29 @@ static void pc_q35_machine_options(MachineClass *m)
>      m->max_cpus = 288;
>  }
>  
> -static void pc_q35_2_10_machine_options(MachineClass *m)
> +static void pc_q35_2_11_machine_options(MachineClass *m)
>  {
>      pc_q35_machine_options(m);
>      m->alias = "q35";
>      m->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
>  }
>  
> +DEFINE_Q35_MACHINE(v2_11, "pc-q35-2.11", NULL,
> +                   pc_q35_2_11_machine_options);
> +
> +static void pc_q35_2_10_machine_options(MachineClass *m)
> +{
> +    pc_q35_2_11_machine_options(m);
> +    m->alias = NULL;
> +    SET_MACHINE_COMPAT(m, PC_COMPAT_2_10);
> +}
> +
>  DEFINE_Q35_MACHINE(v2_10, "pc-q35-2.10", NULL,
>                     pc_q35_2_10_machine_options);
>  
>  static void pc_q35_2_9_machine_options(MachineClass *m)
>  {
>      pc_q35_2_10_machine_options(m);
> -    m->alias = NULL;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);
>  }
>  
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index d80859b..8226904 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -369,6 +369,9 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
>  int e820_get_num_entries(void);
>  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>  
> +#define PC_COMPAT_2_10 \
> +    HW_COMPAT_2_10 \
> +

Patch looks good, but I would remove the trailing backslash here.

With that changed:

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


>  #define PC_COMPAT_2_9 \
>      HW_COMPAT_2_9 \
>      {\
> -- 
> 1.8.3.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/2] pc: add 2.11 machine type

[Gonglei (Arei)]

> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Tuesday, September 05, 2017 9:21 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org; mst@redhat.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; vrozenfe@redhat.com;
> Huangweidong (C)
> Subject: Re: [PATCH 1/2] pc: add 2.11 machine type
> 
> On Tue, Sep 05, 2017 at 05:30:04PM +0800, Gonglei wrote:
> > CC: "Michael S. Tsirkin" <mst@redhat.com>
> > CC: Paolo Bonzini <pbonzini@redhat.com>
> > CC: Richard Henderson <rth@twiddle.net>
> > CC: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  hw/i386/pc_piix.c    | 15 ++++++++++++---
> >  hw/i386/pc_q35.c     | 13 +++++++++++--
> >  include/hw/i386/pc.h |  3 +++
> >  3 files changed, 26 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 46dfd2c..b03cc04 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -436,21 +436,30 @@ static void
> pc_i440fx_machine_options(MachineClass *m)
> >      m->default_display = "std";
> >  }
> >
> > -static void pc_i440fx_2_10_machine_options(MachineClass *m)
> > +static void pc_i440fx_2_11_machine_options(MachineClass *m)
> >  {
> >      pc_i440fx_machine_options(m);
> >      m->alias = "pc";
> >      m->is_default = 1;
> >  }
> >
> > +DEFINE_I440FX_MACHINE(v2_11, "pc-i440fx-2.11", NULL,
> > +                      pc_i440fx_2_11_machine_options);
> > +
> > +static void pc_i440fx_2_10_machine_options(MachineClass *m)
> > +{
> > +    pc_i440fx_2_11_machine_options(m);
> > +    m->is_default = 0;
> > +    m->alias = NULL;
> > +    SET_MACHINE_COMPAT(m, PC_COMPAT_2_10);
> > +}
> > +
> >  DEFINE_I440FX_MACHINE(v2_10, "pc-i440fx-2.10", NULL,
> >                        pc_i440fx_2_10_machine_options);
> >
> >  static void pc_i440fx_2_9_machine_options(MachineClass *m)
> >  {
> >      pc_i440fx_2_10_machine_options(m);
> > -    m->is_default = 0;
> > -    m->alias = NULL;
> >      SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);
> >      m->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
> >  }
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 169a214..d6fef8d 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -302,20 +302,29 @@ static void
> pc_q35_machine_options(MachineClass *m)
> >      m->max_cpus = 288;
> >  }
> >
> > -static void pc_q35_2_10_machine_options(MachineClass *m)
> > +static void pc_q35_2_11_machine_options(MachineClass *m)
> >  {
> >      pc_q35_machine_options(m);
> >      m->alias = "q35";
> >      m->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
> >  }
> >
> > +DEFINE_Q35_MACHINE(v2_11, "pc-q35-2.11", NULL,
> > +                   pc_q35_2_11_machine_options);
> > +
> > +static void pc_q35_2_10_machine_options(MachineClass *m)
> > +{
> > +    pc_q35_2_11_machine_options(m);
> > +    m->alias = NULL;
> > +    SET_MACHINE_COMPAT(m, PC_COMPAT_2_10);
> > +}
> > +
> >  DEFINE_Q35_MACHINE(v2_10, "pc-q35-2.10", NULL,
> >                     pc_q35_2_10_machine_options);
> >
> >  static void pc_q35_2_9_machine_options(MachineClass *m)
> >  {
> >      pc_q35_2_10_machine_options(m);
> > -    m->alias = NULL;
> >      SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);
> >  }
> >
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index d80859b..8226904 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -369,6 +369,9 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
> >  int e820_get_num_entries(void);
> >  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
> >
> > +#define PC_COMPAT_2_10 \
> > +    HW_COMPAT_2_10 \
> > +
> 
> Patch looks good, but I would remove the trailing backslash here.
> 
OK.

> With that changed:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
Thanks.

Re: [Qemu-devel] [PATCH 2/2] i386/cpu/hyperv: support over 64 vcpus for windows guests

[Gonglei (Arei)]

> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Tuesday, September 05, 2017 9:17 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org; mst@redhat.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; vrozenfe@redhat.com;
> Huangweidong (C)
> Subject: Re: [PATCH 2/2] i386/cpu/hyperv: support over 64 vcpus for windows
> guests
> 
> On Tue, Sep 05, 2017 at 05:30:05PM +0800, Gonglei wrote:
> > Starting with Windows Server 2012 and Windows 8, if
> > CPUID.40000005.EAX contains a value of -1, Windows assumes specific
> > limit to the number of VPs. In this case, Windows Server 2012
> > guest VMs may use more than 64 VPs, up to the maximum supported
> > number of processors applicable to the specific Windows
> > version being used.
> >
> >
> https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/referenc
> e/tlfs
> >
> > For compatibility, Let's introduce a new property for X86CPU,
> > named "hv-cpuid-limits-eax" as Paolo's suggestion, and set it
> > to "on" before machine 2.10.
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  include/hw/i386/pc.h |  5 +++++
> >  target/i386/cpu.c    |  1 +
> >  target/i386/cpu.h    |  2 ++
> >  target/i386/kvm.c    | 18 +++++++++++++++++-
> >  4 files changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 8226904..db32e58 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -371,6 +371,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *,
> uint64_t *);
> >
> >  #define PC_COMPAT_2_10 \
> >      HW_COMPAT_2_10 \
> > +    {\
> > +        .driver   = TYPE_X86_CPU,\
> > +        .property = "hv_cpuid_limits_eax",\
> 
> The property name is hv-cpuid-limits-eax.
> 
Make sense to me.

> > +        .value    = "on",\
> > +    },\
> >
> >  #define PC_COMPAT_2_9 \
> >      HW_COMPAT_2_9 \
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 69676e1..0d47bdd 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -4145,6 +4145,7 @@ static Property x86_cpu_properties[] = {
> >                       false),
> >      DEFINE_PROP_BOOL("vmware-cpuid-freq", X86CPU,
> vmware_cpuid_freq, true),
> >      DEFINE_PROP_BOOL("tcg-cpuid", X86CPU, expose_tcg, true),
> > +    DEFINE_PROP_BOOL("hv-cpuid-limits-eax", X86CPU,
> hv_cpuid_limits_eax, false),
> 
> The property name "hv-cpuid-limits-eax" doesn't say anything
> about what it does exactly when set to true.
> 
> What about just making it int32?  e.g.:
> 
>     DEFINE_PROP_INT32("x-hv-max-vps", X86CPU, hv_max_vps, -1)
> [...]
>     {\
>         .driver   = TYPE_X86_CPU,\
>         .property = "x-hv-max-vps",\
>         .value    = "0x40",\
>     },\
> [...]
>     c->function = HYPERV_CPUID_IMPLEMENT_LIMITS;
>     c->eax = cpu->hv_max_vps;
> 
> 
> (The "x-" prefix indicates that the property is not supposed to
> be a stable user interface.)
> 
I thought about this as well.
but actually we can't assure that users set the x-hv-max-vps an invalid value if
we use this method. Do we really need to expose eax directly?

> 
> >      DEFINE_PROP_END_OF_LIST()
> >  };
> >
> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > index 525d35d..f8b455a 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -1282,6 +1282,8 @@ struct X86CPU {
> >      int32_t socket_id;
> >      int32_t core_id;
> >      int32_t thread_id;
> > +
> > +    bool hv_cpuid_limits_eax;
> >  };
> >
> >  static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
> > diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> > index 6db7783..cf6ef96 100644
> > --- a/target/i386/kvm.c
> > +++ b/target/i386/kvm.c
> > @@ -751,7 +751,23 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >
> >          c = &cpuid_data.entries[cpuid_i++];
> >          c->function = HYPERV_CPUID_IMPLEMENT_LIMITS;
> > -        c->eax = 0x40;
> > +
> > +        if (!cpu->hv_cpuid_limits_eax) {
> > +            /*
> > +             * Starting with Windows Server 2012 and Windows 8, if
> > +             * CPUID.40000005.EAX contains a value of -1, Windows
> > +             * assumes specific limit to the number of VPs. In this case,
> > +             * Windows Server 2012 guest VMs may use more than 64
> VPs,
> > +             * up to the maximum supported number of processors
> > +             * applicable to the specific Windows version being used.
> 
> That was a direct quote from a document, so I recommend citing
> the specific document you quoted.  e.g.:
> 
>     /*
>      * From "Requirements for Implementing the Microsoft
>      * Hypervisor Interface":
>      *
> https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/referenc
> e/tlfs
>      *
>      * "Starting with Windows Server 2012 and Windows 8, if
>      * CPUID.40000005.EAX contains a value of -1, Windows assumes
>      * specific limit to the number of VPs. In this case, Windows
>      * Server 2012 guest VMs may use more than 64 VPs, up to the
>      * maximum supported number of processors applicable to the
>      * specific Windows version being used."
>      */
> 
> 
> > +             *
> > +             * https://docs.microsoft.com/en-us/virtualization/
> > +             *    hyper-v-on-windows/reference/tlfs
> 
> IMO a long line is preferable to a broken URL.
> 
Make sense to me.

Thanks,
-Gonglei

Re: [Qemu-devel] [PATCH 2/2] i386/cpu/hyperv: support over 64 vcpus for windows guests

[Eduardo Habkost]

On Thu, Sep 07, 2017 at 01:05:33AM +0000, Gonglei (Arei) wrote:
> 
> 
> > -----Original Message-----
> > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > Sent: Tuesday, September 05, 2017 9:17 PM
> > To: Gonglei (Arei)
> > Cc: qemu-devel@nongnu.org; mst@redhat.com; pbonzini@redhat.com;
> > rth@twiddle.net; mtosatti@redhat.com; vrozenfe@redhat.com;
> > Huangweidong (C)
> > Subject: Re: [PATCH 2/2] i386/cpu/hyperv: support over 64 vcpus for windows
> > guests
> > 
> > On Tue, Sep 05, 2017 at 05:30:05PM +0800, Gonglei wrote:
> > > Starting with Windows Server 2012 and Windows 8, if
> > > CPUID.40000005.EAX contains a value of -1, Windows assumes specific
> > > limit to the number of VPs. In this case, Windows Server 2012
> > > guest VMs may use more than 64 VPs, up to the maximum supported
> > > number of processors applicable to the specific Windows
> > > version being used.
> > >
> > >
> > https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/referenc
> > e/tlfs
> > >
> > > For compatibility, Let's introduce a new property for X86CPU,
> > > named "hv-cpuid-limits-eax" as Paolo's suggestion, and set it
> > > to "on" before machine 2.10.
> > >
> > > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > > ---
> > >  include/hw/i386/pc.h |  5 +++++
> > >  target/i386/cpu.c    |  1 +
> > >  target/i386/cpu.h    |  2 ++
> > >  target/i386/kvm.c    | 18 +++++++++++++++++-
> > >  4 files changed, 25 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index 8226904..db32e58 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -371,6 +371,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *,
> > uint64_t *);
> > >
> > >  #define PC_COMPAT_2_10 \
> > >      HW_COMPAT_2_10 \
> > > +    {\
> > > +        .driver   = TYPE_X86_CPU,\
> > > +        .property = "hv_cpuid_limits_eax",\
> > 
> > The property name is hv-cpuid-limits-eax.
> > 
> Make sense to me.
> 
> > > +        .value    = "on",\
> > > +    },\
> > >
> > >  #define PC_COMPAT_2_9 \
> > >      HW_COMPAT_2_9 \
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index 69676e1..0d47bdd 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -4145,6 +4145,7 @@ static Property x86_cpu_properties[] = {
> > >                       false),
> > >      DEFINE_PROP_BOOL("vmware-cpuid-freq", X86CPU,
> > vmware_cpuid_freq, true),
> > >      DEFINE_PROP_BOOL("tcg-cpuid", X86CPU, expose_tcg, true),
> > > +    DEFINE_PROP_BOOL("hv-cpuid-limits-eax", X86CPU,
> > hv_cpuid_limits_eax, false),
> > 
> > The property name "hv-cpuid-limits-eax" doesn't say anything
> > about what it does exactly when set to true.
> > 
> > What about just making it int32?  e.g.:
> > 
> >     DEFINE_PROP_INT32("x-hv-max-vps", X86CPU, hv_max_vps, -1)
> > [...]
> >     {\
> >         .driver   = TYPE_X86_CPU,\
> >         .property = "x-hv-max-vps",\
> >         .value    = "0x40",\
> >     },\
> > [...]
> >     c->function = HYPERV_CPUID_IMPLEMENT_LIMITS;
> >     c->eax = cpu->hv_max_vps;
> > 
> > 
> > (The "x-" prefix indicates that the property is not supposed to
> > be a stable user interface.)
> > 
> I thought about this as well.
> but actually we can't assure that users set the x-hv-max-vps an invalid value if
> we use this method. Do we really need to expose eax directly?

We don't really need to expose eax directly and I'm not against a
boolean property, but I think an integer property with the actual
CPUID value makes the compat code simpler and the purpose of the
entry at PC_COMPAT_* more obvious.

Properties prefixed with "x-" are for internal QEMU usage or
debugging, if users want to fiddle with it, they do it at their
own risk.  I don't see a problem with that.

> 
> > 
> > >      DEFINE_PROP_END_OF_LIST()
> > >  };
> > >
> > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > > index 525d35d..f8b455a 100644
> > > --- a/target/i386/cpu.h
> > > +++ b/target/i386/cpu.h
> > > @@ -1282,6 +1282,8 @@ struct X86CPU {
> > >      int32_t socket_id;
> > >      int32_t core_id;
> > >      int32_t thread_id;
> > > +
> > > +    bool hv_cpuid_limits_eax;
> > >  };
> > >
> > >  static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
> > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> > > index 6db7783..cf6ef96 100644
> > > --- a/target/i386/kvm.c
> > > +++ b/target/i386/kvm.c
> > > @@ -751,7 +751,23 @@ int kvm_arch_init_vcpu(CPUState *cs)
> > >
> > >          c = &cpuid_data.entries[cpuid_i++];
> > >          c->function = HYPERV_CPUID_IMPLEMENT_LIMITS;
> > > -        c->eax = 0x40;
> > > +
> > > +        if (!cpu->hv_cpuid_limits_eax) {
> > > +            /*
> > > +             * Starting with Windows Server 2012 and Windows 8, if
> > > +             * CPUID.40000005.EAX contains a value of -1, Windows
> > > +             * assumes specific limit to the number of VPs. In this case,
> > > +             * Windows Server 2012 guest VMs may use more than 64
> > VPs,
> > > +             * up to the maximum supported number of processors
> > > +             * applicable to the specific Windows version being used.
> > 
> > That was a direct quote from a document, so I recommend citing
> > the specific document you quoted.  e.g.:
> > 
> >     /*
> >      * From "Requirements for Implementing the Microsoft
> >      * Hypervisor Interface":
> >      *
> > https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/referenc
> > e/tlfs
> >      *
> >      * "Starting with Windows Server 2012 and Windows 8, if
> >      * CPUID.40000005.EAX contains a value of -1, Windows assumes
> >      * specific limit to the number of VPs. In this case, Windows
> >      * Server 2012 guest VMs may use more than 64 VPs, up to the
> >      * maximum supported number of processors applicable to the
> >      * specific Windows version being used."
> >      */
> > 
> > 
> > > +             *
> > > +             * https://docs.microsoft.com/en-us/virtualization/
> > > +             *    hyper-v-on-windows/reference/tlfs
> > 
> > IMO a long line is preferable to a broken URL.
> > 
> Make sense to me.
> 
> Thanks,
> -Gonglei
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] i386/cpu/hyperv: support over 64 vcpus for windows guests

[Gonglei (Arei)]

> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Sunday, September 10, 2017 4:46 AM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org; mst@redhat.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; vrozenfe@redhat.com;
> Huangweidong (C)
> Subject: Re: [PATCH 2/2] i386/cpu/hyperv: support over 64 vcpus for windows
> guests
> 
> On Thu, Sep 07, 2017 at 01:05:33AM +0000, Gonglei (Arei) wrote:
> >
> >
> > > -----Original Message-----
> > > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > > Sent: Tuesday, September 05, 2017 9:17 PM
> > > To: Gonglei (Arei)
> > > Cc: qemu-devel@nongnu.org; mst@redhat.com; pbonzini@redhat.com;
> > > rth@twiddle.net; mtosatti@redhat.com; vrozenfe@redhat.com;
> > > Huangweidong (C)
> > > Subject: Re: [PATCH 2/2] i386/cpu/hyperv: support over 64 vcpus for
> windows
> > > guests
> > >
> > > On Tue, Sep 05, 2017 at 05:30:05PM +0800, Gonglei wrote:
> > > > Starting with Windows Server 2012 and Windows 8, if
> > > > CPUID.40000005.EAX contains a value of -1, Windows assumes specific
> > > > limit to the number of VPs. In this case, Windows Server 2012
> > > > guest VMs may use more than 64 VPs, up to the maximum supported
> > > > number of processors applicable to the specific Windows
> > > > version being used.
> > > >
> > > >
> > >
> https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/referenc
> > > e/tlfs
> > > >
> > > > For compatibility, Let's introduce a new property for X86CPU,
> > > > named "hv-cpuid-limits-eax" as Paolo's suggestion, and set it
> > > > to "on" before machine 2.10.
> > > >
> > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > > > ---
> > > >  include/hw/i386/pc.h |  5 +++++
> > > >  target/i386/cpu.c    |  1 +
> > > >  target/i386/cpu.h    |  2 ++
> > > >  target/i386/kvm.c    | 18 +++++++++++++++++-
> > > >  4 files changed, 25 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > > index 8226904..db32e58 100644
> > > > --- a/include/hw/i386/pc.h
> > > > +++ b/include/hw/i386/pc.h
> > > > @@ -371,6 +371,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *,
> > > uint64_t *);
> > > >
> > > >  #define PC_COMPAT_2_10 \
> > > >      HW_COMPAT_2_10 \
> > > > +    {\
> > > > +        .driver   = TYPE_X86_CPU,\
> > > > +        .property = "hv_cpuid_limits_eax",\
> > >
> > > The property name is hv-cpuid-limits-eax.
> > >
> > Make sense to me.
> >
> > > > +        .value    = "on",\
> > > > +    },\
> > > >
> > > >  #define PC_COMPAT_2_9 \
> > > >      HW_COMPAT_2_9 \
> > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > > index 69676e1..0d47bdd 100644
> > > > --- a/target/i386/cpu.c
> > > > +++ b/target/i386/cpu.c
> > > > @@ -4145,6 +4145,7 @@ static Property x86_cpu_properties[] = {
> > > >                       false),
> > > >      DEFINE_PROP_BOOL("vmware-cpuid-freq", X86CPU,
> > > vmware_cpuid_freq, true),
> > > >      DEFINE_PROP_BOOL("tcg-cpuid", X86CPU, expose_tcg, true),
> > > > +    DEFINE_PROP_BOOL("hv-cpuid-limits-eax", X86CPU,
> > > hv_cpuid_limits_eax, false),
> > >
> > > The property name "hv-cpuid-limits-eax" doesn't say anything
> > > about what it does exactly when set to true.
> > >
> > > What about just making it int32?  e.g.:
> > >
> > >     DEFINE_PROP_INT32("x-hv-max-vps", X86CPU, hv_max_vps, -1)
> > > [...]
> > >     {\
> > >         .driver   = TYPE_X86_CPU,\
> > >         .property = "x-hv-max-vps",\
> > >         .value    = "0x40",\
> > >     },\
> > > [...]
> > >     c->function = HYPERV_CPUID_IMPLEMENT_LIMITS;
> > >     c->eax = cpu->hv_max_vps;
> > >
> > >
> > > (The "x-" prefix indicates that the property is not supposed to
> > > be a stable user interface.)
> > >
> > I thought about this as well.
> > but actually we can't assure that users set the x-hv-max-vps an invalid value if
> > we use this method. Do we really need to expose eax directly?
> 
> We don't really need to expose eax directly and I'm not against a
> boolean property, but I think an integer property with the actual
> CPUID value makes the compat code simpler and the purpose of the
> entry at PC_COMPAT_* more obvious.
> 
> Properties prefixed with "x-" are for internal QEMU usage or
> debugging, if users want to fiddle with it, they do it at their
> own risk.  I don't see a problem with that.
> 
Ok, it's fair. Will do it as your suggestion.

Thanks,
-Gonglei