Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD Processor Cache Information

[Eduardo Habkost]

On Wed, Mar 21, 2018 at 08:07:54PM +0000, Moger, Babu wrote:
> 
> > -----Original Message-----
> > From: Eduardo Habkost <address@hidden>
> > Sent: Wednesday, March 21, 2018 1:15 PM
> > To: Moger, Babu <address@hidden>
> > Cc: address@hidden; address@hidden; address@hidden;
> > Lendacky, Thomas <address@hidden>; Singh, Brijesh
> > <address@hidden>; address@hidden; address@hidden;
> > address@hidden; Hook, Gary <address@hidden>; qemu-
> > address@hidden
> > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > Processor Cache Information
> > 
> > On Wed, Mar 21, 2018 at 05:47:28PM +0000, Moger, Babu wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Eduardo Habkost <address@hidden>
> > > > Sent: Wednesday, March 21, 2018 12:10 PM
> > > > To: Moger, Babu <address@hidden>
> > > > Cc: address@hidden; address@hidden; address@hidden;
> > > > Lendacky, Thomas <address@hidden>; Singh, Brijesh
> > > > <address@hidden>; address@hidden; address@hidden;
> > > > address@hidden; Hook, Gary <address@hidden>; qemu-
> > > > address@hidden
> > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > > > Processor Cache Information
> > > >
> > > > On Wed, Mar 21, 2018 at 03:58:41PM +0000, Moger, Babu wrote:
> > > > > Hi Eduardo,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Eduardo Habkost <address@hidden>
> > > > > > Sent: Tuesday, March 20, 2018 12:54 PM
> > > > > > To: Moger, Babu <address@hidden>
> > > > > > Cc: address@hidden; address@hidden; address@hidden;
> > > > > > Lendacky, Thomas <address@hidden>; Singh, Brijesh
> > > > > > <address@hidden>; address@hidden;
> > address@hidden;
> > > > > > address@hidden; Hook, Gary <address@hidden>; qemu-
> > > > > > address@hidden
> > > > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > > > > > Processor Cache Information
> > > > > >
> > > > > > On Tue, Mar 20, 2018 at 05:25:52PM +0000, Moger, Babu wrote:
> > > > > > > Hi Eduardo, Thanks for the comments. Please see the response
> > inline.
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Eduardo Habkost <address@hidden>
> > > > > > > > Sent: Friday, March 16, 2018 1:00 PM
> > > > > > > > To: Moger, Babu <address@hidden>
> > > > > > > > Cc: address@hidden; address@hidden;
> > address@hidden;
> > > > > > > > Lendacky, Thomas <address@hidden>; Singh, Brijesh
> > > > > > > > <address@hidden>; address@hidden;
> > > > address@hidden;
> > > > > > > > address@hidden; Hook, Gary <address@hidden>;
> > qemu-
> > > > > > > > address@hidden
> > > > > > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate
> > AMD
> > > > > > > > Processor Cache Information
> > > > > > > >
> > > > > > > > On Mon, Mar 12, 2018 at 05:00:46PM -0400, Babu Moger wrote:
> > > > > > > > > From: Stanislav Lanci <address@hidden>
> > > > > > > > >
> > > > > > > > > Add information for cpuid 0x8000001D leaf. Populate cache
> > topology
> > > > > > > > information
> > > > > > > > > for different cache types(Data Cache, Instruction Cache, L2 
> > > > > > > > > and
> > L3)
> > > > > > > > supported
> > > > > > > > > by 0x8000001D leaf. Please refer Processor Programming
> > Reference
> > > > > > (PPR)
> > > > > > > > for AMD
> > > > > > > > > Family 17h Model for more details.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Stanislav Lanci <address@hidden>
> > > > > > > > > Signed-off-by: Babu Moger <address@hidden>
> > > > > > > >
> > > > > > > > The new CPUID leaves don't seem to match the existing AMD
> > cache
> > > > > > > > information
> > > > > > > > leaves.  Is this intentional?  Why?
> > > > > > >
> > > > > > > It is not intentional. These values are from older family of
> > processors.
> > > > > > These values have changed from Family 14  or later.
> > > > > > > The latest one is Family 17. You can see the differences here.
> > > > > > >  https://support.amd.com/TechDocs/41131.pdf
> > > > > > >
> > > > > >
> > > >
> > https://support.amd.com/TechDocs/55072_AMD_Family_15h_Models_70h-
> > > > > > 7Fh_BKDG.pdf
> > > > > > >
> > > > > >
> > > >
> > https://support.amd.com/TechDocs/54945_PPR_Family_17h_Models_00h-
> > > > > > 0Fh.pdf
> > > > > > >
> > > > > > > Some of these are bugs in our code. For some we need to add
> > checks
> > > > for
> > > > > > the family and correct these values.
> > > > > > > You understand the code much better than me. Correct me if I
> > missed
> > > > > > something.
> > > > > > >
> > > > > > > Note that older family of processors don't support topology
> > extensions.
> > > > > >
> > > > > > If you want to make the cache size/topology look different
> > > > > > depending on the CPU model/options, this would require more work,
> > > > > > but it would be an interesting feature.
> > > > > >
> > > > > > The "i386: Helpers to encode cache information consistently"
> > > > > > patch I sent last week might be a useful starting point for that.
> > > > > >
> > > > > > If you plan to implement that, please keep in mind that existing
> > > > > > CPUID cache info needs to be kept on previous machine-types (this
> > > > > > is implemented by adding QOM properties that can be used to
> > > > > > enable the old behavior, and by setting them at
> > > > > > MachineClass::compat_props).
> > > > >
> > > > > Wanted to get some confirmation what you meant by setting
> > > > MachineClass::compat_props.
> > > > > Here is the patch I created to add new property for cpu. Now, I can 
> > > > > use
> > > > enable_topoext to display
> > > > > new  change properly based on family.  Is that what you meant ?
> > > >
> > > > If the only change you introduce in the defaults is enabling
> > > > topoext but keeping the same default cache sizes, the example
> > > > following would make sense (but see comments below about
> > > > implementation details).
> > > >
> > > > But if you also want to change the default cache size, you will
> > > > also need properties that control the cache size.
> > >
> > > Yes. We will need cpu family information.  Let me see if that information 
> > > is
> > >  already available or we need to add that as well.
> > 
> > CPU family doesn't seem to be enough, if you want to keep
> > compatibility on older machine-type versions.  e.g.:
> > 
> > If you want to make EPYC expose a different cache size, you'd
> > just need a new field on X86CPUDefinition.  This is the easy
> > part.
> 
> Actually, I already have this information.  I can just decode cpuid_version 
> information.
> I may not need to add new field in new field on X86CPUDefinition.

I'd prefer CPUCacheInfo* fields on X86CPUDefinition instead of
hardcoding a cpuid_version check.  This way all
family/model-specific information about CPU models will
centralized in the CPU model table.

> 
> > 
> > But you still need "-machine pc-2.11 -cpu EPYC" to expose the old
> > cache size, for guest ABI compatibility.
> > 
> > This means you need an additional knob that pc-2.10 can use in
> > compat_props, to override the new family/model-dependent cache
> > size in EPYC, and force the old cache sizes to be used even with
> > "-cpu EPYC".
> 
> Ok.  Will add a new property legacy_cache(or something). I can use this to  
> override
> all the new information. Will plan to send first version tomorrow.
>  
> > 
> > --
> > Eduardo

-- 
Eduardo