Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost [mailto:address@hidden
> Sent: Wednesday, June 13, 2018 1:49 PM
> To: Moger, Babu <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden; Jiri
> Denemark <address@hidden>
> Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Wed, Jun 13, 2018 at 06:21:58PM +0000, Moger, Babu wrote:
> >
> >
> > > -----Original Message-----
> > > From: Eduardo Habkost [mailto:address@hidden
> > > Sent: Wednesday, June 13, 2018 1:18 PM
> > > To: Moger, Babu <address@hidden>
> > > Cc: address@hidden; address@hidden;
> address@hidden;
> > > address@hidden; address@hidden; address@hidden;
> > > address@hidden; address@hidden; address@hidden; Jiri
> > > Denemark <address@hidden>
> > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> > > CPU
> > >
> > > On Wed, Jun 13, 2018 at 06:10:30PM +0000, Moger, Babu wrote:
> > > > > -----Original Message-----
> > > > > From: Eduardo Habkost [mailto:address@hidden
> > > > > Sent: Wednesday, June 13, 2018 12:18 PM
> > > > > To: Moger, Babu <address@hidden>
> > > > > Cc: address@hidden; address@hidden;
> > > address@hidden;
> > > > > address@hidden; address@hidden; address@hidden;
> > > > > address@hidden; address@hidden; address@hidden;
> Jiri
> > > > > Denemark <address@hidden>
> > > > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD
> EPYC
> > > > > CPU
> > > > >
> > > > > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote:
> > > > > [...]
> > > > > > > What do you think our options are here?
> > > > > >
> > > > > > Should we drop automatic topoext completely and move forward?
> > > > > > What are your thoughts?
> > > > >
> > > > > Let's drop automatic topoext by now, and see if we find solutions
> > > > > later.  I don't want to hold the rest of the patches because of
> > > > > this.
> > > >
> > > > Ok. I will drop topoext.
> > > >
> > > > >
> > > > > I'm thinking we could simply make kvm_arch_get_supported_cpuid()
> > > > > always return TOPOEXT on AMD CPUs, because the feature flag don't
> > > > > really depend on any KVM code to work (is that correct?).
> > > >
> > > > Yes, that is correct. I don't see any dependent code on TOPOEXT in
> KVM
> > > driver.
> > > >
> > > > Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes.
> > >
> > > Hmm, this could actually solve all of our problems, then:
> > >
> > > We can forget about auto-topoext: just add TOPOEXT in
> > > kvm_arch_get_supported_cpuid(), add TOPOEXT unconditionally to
> > > the CPU models where you are interested into (EPYC only?), and
> > > add topoext=off to pc-2.12 compat_props.
> > >
> >
> > Ok Sure.
> 
> Sorry, I forgot we still need to decide what to do if TOPOEXT is
> enabled in the CPU model (or command-line) but the -smp options
> are not compatible with it.

Yes.  I have kept that check.  But, I had to implement 
topology_supports_topoext bit differently.
Reason for this is we need to disable this feature before the  
x86_cpu_expand_features.
But problem is nr_cores and nr_threads are not populated at this time. It is 
populated in qemu_init_vcpus.
Please take a look at topology_supports_topoext again.
> 
> In other words, what should guest see on CPUID if using:
> 
> "-machine pc-q35-3.0 -cpu EPYC -smp 64,cores=64"
> or:
> "-machine pc-q35-3.0 -cpu Opteron_G5,+topoext -smp 64,cores=64"
> 
Tested both these cases. It works fine with some warning messages.

> I wonder what would happen if we just return zeroes on
> CPUID[0x800001E] if !topology_supports_topoext(), instead of
> trying to clear/set TOPOEXT depending on the -smp option?  It
> would make things much simpler for QEMU and libvirt.

I did not see that difference in behavior if we clear the bit versus return 0s.
Sending new patches now. Please review.
One note: I will going on vacation from June 20th for couple of weeks.
 If possible I would like to close this feature. If we cannot that is fine. 
Just an FYI.

> 
> --
> Eduardo