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

[Eduardo Habkost <ehabkost@redhat.com>]

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 <ehabkost@redhat.com>
> > Sent: Friday, March 16, 2018 1:00 PM
> > To: Moger, Babu <Babu.Moger@amd.com>
> > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > devel@nongnu.org
> > 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 <pixo@polepetko.eu>
> > >
> > > 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 <pixo@polepetko.eu>
> > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > 
> > 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).

-- 
Eduardo