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

[Gonglei (Arei)]

> -----Original Message-----
> From: Eduardo Habkost [mailto:address@hidden
> Sent: Sunday, September 10, 2017 4:46 AM
> To: Gonglei (Arei)
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> 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:address@hidden
> > > Sent: Tuesday, September 05, 2017 9:17 PM
> > > To: Gonglei (Arei)
> > > Cc: address@hidden; address@hidden; address@hidden;
> > > address@hidden; address@hidden; address@hidden;
> > > 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 <address@hidden>
> > > > ---
> > > >  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