[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/5] target/i386: Populate AMD Processor Cach
From: |
Radim Krčmář |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/5] target/i386: Populate AMD Processor Cache Information |
Date: |
Thu, 1 Mar 2018 20:56:16 +0100 |
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:address@hidden
> > Sent: Wednesday, February 28, 2018 12:09 PM
> > To: Moger, Babu <address@hidden>
> > Cc: address@hidden; address@hidden; address@hidden;
> > address@hidden; address@hidden; address@hidden;
> > address@hidden; Hook, Gary <address@hidden>
> > Subject: Re: [PATCH v2 2/5] target/i386: Populate AMD Processor Cache
> > Information
> >
> > 2018-02-23 21:30-0500, Babu Moger:
> > > From: Stanislav Lanci <address@hidden>
> > >
> > > Adds information about cache size and topology from cpuid 0x8000001D
> > leaf
> > > for different cache types on AMD processors.
> > >
> > > Signed-off-by: Stanislav Lanci <address@hidden>
> > > Signed-off-by: Babu Moger <address@hidden>
> > > ---
> > > 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.
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.