> -----Original Message----- > From: Radim Krčmář [mailto:rkrcmar@xxxxxxxxxx] > Sent: Thursday, March 1, 2018 1:56 PM > To: Moger, Babu <Babu.Moger@xxxxxxx> > Cc: pbonzini@xxxxxxxxxx; rth@xxxxxxxxxxx; ehabkost@xxxxxxxxxx; > mtosatti@xxxxxxxxxx; qemu-devel@xxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; > pixo@xxxxxxxxxxxx; Hook, Gary <Gary.Hook@xxxxxxx> > Subject: Re: [PATCH v2 2/5] target/i386: Populate AMD Processor Cache > Information > > 2018-03-01 15:55+0000, Moger, Babu: > > Radim, Thanks for your comments. I am working on the changes. > > But, I need few clarifications on your comments. Please see inline. > > > > > -----Original Message----- > > > From: Radim Krčmář [mailto:rkrcmar@xxxxxxxxxx] > > > Sent: Wednesday, February 28, 2018 12:09 PM > > > To: Moger, Babu <Babu.Moger@xxxxxxx> > > > Cc: pbonzini@xxxxxxxxxx; rth@xxxxxxxxxxx; ehabkost@xxxxxxxxxx; > > > mtosatti@xxxxxxxxxx; qemu-devel@xxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; > > > pixo@xxxxxxxxxxxx; Hook, Gary <Gary.Hook@xxxxxxx> > > > Subject: Re: [PATCH v2 2/5] target/i386: Populate AMD Processor Cache > > > Information > > > > > > 2018-02-23 21:30-0500, Babu Moger: > > > > From: Stanislav Lanci <pixo@xxxxxxxxxxxx> > > > > > > > > Adds information about cache size and topology from cpuid 0x8000001D > > > leaf > > > > for different cache types on AMD processors. > > > > > > > > Signed-off-by: Stanislav Lanci <pixo@xxxxxxxxxxxx> > > > > Signed-off-by: Babu Moger <babu.moger@xxxxxxx> > > > > --- > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > > > @@ -3590,6 +3594,78 @@ void cpu_x86_cpuid(CPUX86State *env, > > > > + *ebx = (L1D_LINE_SIZE - 1) | \ > > > > + ((L1D_PARTITIONS - 1) << 12) | \ > > > > + ((L1D_ASSOCIATIVITY - 1) << 22); > > > > + *ecx = L1D_SETS - 1; > > > > > > These numbers seem to have the same meaning as CPUID 4, but have > > > conflicting values. > > > > I am not sure about conflicting values. Looking at the specs(page 78 > CPUID_Fn8000001D_EBX_x00) > > > https://support.amd.com/TechDocs/54945_PPR_Family_17h_Models_00h- > 0Fh.pdf > > It looks correct to me. > > I agree. My comment is misplaced -- it should have been under the place > that uses *_AMD macros. > > I wanted to point out that CPUID in QEMU is very Intel-focused and > always contains CPUID leaf 4, which has fields of the very same meaning, > but with different values. > > > > I think we should not expose CPUID 4 with AMD CPUs or at least when > they > > > have CPUID_EXT3_TOPOEXT (the latter is easier wrt. compatibility). > > > > Can you please elaborate on these comments? > > Did you mean we should remove the check CPUID_EXT3_TOPOEXT and > remove all CPUID 4 references? > > CPUID 4 should have never been present when emulating AMD CPUs, but it's > worse now that the numbers are conflicting. > > I meant to hide CPUID 4 for all AMD CPUs on future machine types, or at > least when CPUID_EXT3_TOPOEXT is enabled. Sorry, I think I created some confusion here by using CPUID 4 definitions which are mostly for intel. Let me rework on this. I will repost the patches. Thanks for the feedback. > > Keeping the current logic not a big problem as CPUID 4 should never be > used by operating systems on AMD nor trusted inside a VM. Bringing the > emulation closer to real state would be nice, but this can definitely be > done later (aka never). > > > > The numbers looks like real hardware, > > > > Do you want me to change anything here? > > No, I was just commending, > > thanks.
