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

2018-03-02 Thread Moger, Babu


> -Original Message-
> From: Radim Krčmář [mailto:rkrc...@redhat.com]
> Sent: Thursday, March 1, 2018 1:56 PM
> To: Moger, Babu 
> Cc: pbonz...@redhat.com; r...@twiddle.net; ehabk...@redhat.com;
> mtosa...@redhat.com; qemu-devel@nongnu.org; k...@vger.kernel.org;
> p...@polepetko.eu; Hook, Gary 
> Subject: Re: [PATCH v2 2/5] target/i386: Populate AMD Processor Cache
> Information
> 
> 2018-03-01 15:55+, 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:rkrc...@redhat.com]
> > > Sent: Wednesday, February 28, 2018 12:09 PM
> > > To: Moger, Babu 
> > > Cc: pbonz...@redhat.com; r...@twiddle.net; ehabk...@redhat.com;
> > > mtosa...@redhat.com; qemu-devel@nongnu.org; k...@vger.kernel.org;
> > > p...@polepetko.eu; Hook, Gary 
> > > Subject: Re: [PATCH v2 2/5] target/i386: Populate AMD Processor Cache
> > > Information
> > >
> > > 2018-02-23 21:30-0500, Babu Moger:
> > > > From: Stanislav Lanci 
> > > >
> > > > Adds information about cache size and topology from cpuid 0x801D
> > > leaf
> > > > for different cache types on AMD processors.
> > > >
> > > > Signed-off-by: Stanislav Lanci 
> > > > Signed-off-by: Babu Moger 
> > > > ---
> > > > 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_Fn801D_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.


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

2018-03-01 Thread Radim Krčmář
2018-03-01 15:55+, 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:rkrc...@redhat.com]
> > Sent: Wednesday, February 28, 2018 12:09 PM
> > To: Moger, Babu 
> > Cc: pbonz...@redhat.com; r...@twiddle.net; ehabk...@redhat.com;
> > mtosa...@redhat.com; qemu-devel@nongnu.org; k...@vger.kernel.org;
> > p...@polepetko.eu; Hook, Gary 
> > Subject: Re: [PATCH v2 2/5] target/i386: Populate AMD Processor Cache
> > Information
> > 
> > 2018-02-23 21:30-0500, Babu Moger:
> > > From: Stanislav Lanci 
> > >
> > > Adds information about cache size and topology from cpuid 0x801D
> > leaf
> > > for different cache types on AMD processors.
> > >
> > > Signed-off-by: Stanislav Lanci 
> > > Signed-off-by: Babu Moger 
> > > ---
> > > 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_Fn801D_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.



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

2018-03-01 Thread 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:rkrc...@redhat.com]
> Sent: Wednesday, February 28, 2018 12:09 PM
> To: Moger, Babu 
> Cc: pbonz...@redhat.com; r...@twiddle.net; ehabk...@redhat.com;
> mtosa...@redhat.com; qemu-devel@nongnu.org; k...@vger.kernel.org;
> p...@polepetko.eu; Hook, Gary 
> Subject: Re: [PATCH v2 2/5] target/i386: Populate AMD Processor Cache
> Information
> 
> 2018-02-23 21:30-0500, Babu Moger:
> > From: Stanislav Lanci 
> >
> > Adds information about cache size and topology from cpuid 0x801D
> leaf
> > for different cache types on AMD processors.
> >
> > Signed-off-by: Stanislav Lanci 
> > Signed-off-by: Babu Moger 
> > ---
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > @@ -3590,6 +3594,78 @@ void cpu_x86_cpuid(CPUX86State *env,
> uint32_t index, uint32_t count,
> >  *edx = 0;
> >  }
> >  break;
> > +case 0x801D: /* AMD TOPOEXT cache info */
> > +if (cpu->cache_info_passthrough) {
> > +host_cpuid(index, count, eax, ebx, ecx, edx);
> > +break;
> > +} else if (env->features[FEAT_8000_0001_ECX] &
> CPUID_EXT3_TOPOEXT) {
> > +*eax = 0;
> > +switch (count) {
> > +case 0: /* L1 dcache info */
> > +*eax |= CPUID_4_TYPE_DCACHE | \
> > +CPUID_4_LEVEL(1) | \
> > +CPUID_4_SELF_INIT_LEVEL | \
> > +((cs->nr_threads - 1) << 14);
> 
> CPUID_4 uses the same format even for bits 25-14, so this would look
> better with a macro.

Yes. We can do that. 

> 
> > +*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_Fn801D_EBX_x00)
 https://support.amd.com/TechDocs/54945_PPR_Family_17h_Models_00h-0Fh.pdf
 It looks correct to me.

> 
> 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? 

> 
> > +*edx = 0;
> > +break;
> > +case 1: /* L1 icache info */
> > +*eax |= CPUID_4_TYPE_ICACHE | \
> > +CPUID_4_LEVEL(1) | \
> > +CPUID_4_SELF_INIT_LEVEL | \
> > +((cs->nr_threads - 1) << 14);
> > +*ebx = (L1I_LINE_SIZE - 1) | \
> > +   ((L1I_PARTITIONS - 1) << 12) | \
> > +   ((L1I_ASSOCIATIVITY_AMD - 1) << 22);
> > +*ecx = L1I_SETS_AMD - 1;
> > +*edx = 0;
> > +break;
> > +case 2: /* L2 cache info */
> > +*eax |= CPUID_4_TYPE_UNIFIED | \
> > +CPUID_4_LEVEL(2) | \
> > +CPUID_4_SELF_INIT_LEVEL | \
> > +((cs->nr_threads - 1) << 14);
> > +*ebx = (L2_LINE_SIZE - 1) | \
> > +   ((L2_PARTITIONS - 1) << 12) | \
> > +   ((L2_ASSOCIATIVITY_AMD - 1) << 22);
> > +*ecx = L2_SETS_AMD - 1;
> > +*edx = CPUID_4_INCLUSIVE;
> > +break;
> > +case 3: /* L3 cache info */
> > +if (!cpu->enable_l3_cache) {
> > +*eax = 0;
> > +*ebx = 0;
> > +*ecx = 0;
> > +*edx = 0;
> > +break;
> > +}
> > +*eax |= CPUID_4_TYPE_UNIFIED | \
> > +CPUID_4_LEVEL(3) | \
> > +CPUID_4_SELF_INIT_LEVEL | \
> > +((cs->nr_cores * cs->nr_threads - 1) << 14);
> 
> This number seems to be the only difference that isn't just a difference
> constant.  It tempts me to merge the cases for 4 and 0x801D as it
> seems that vendors try to be compatible.

Yes. We could merge cases for 4 and 0x801D.

> 
> > +*ebx = (L3_N_LINE_SIZE - 1) | \
> > +   ((L3_N_PARTITIONS - 1) << 12) | \
> > +   ((L3_N_ASSOCIATIVITY - 1) << 22);
> > +*ecx = L3_N_SETS_AMD - 1;
> > +*edx = CPUID_4_NO_INVD_SHARING;
> > +break;
> > +default: /* end of info */
> > +*eax = 0;
> > +*ebx = 0;
> > +

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

2018-02-28 Thread Radim Krčmář
2018-02-23 21:30-0500, Babu Moger:
> From: Stanislav Lanci 
> 
> Adds information about cache size and topology from cpuid 0x801D leaf
> for different cache types on AMD processors.
> 
> Signed-off-by: Stanislav Lanci 
> Signed-off-by: Babu Moger 
> ---
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> @@ -3590,6 +3594,78 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
> uint32_t count,
>  *edx = 0;
>  }
>  break;
> +case 0x801D: /* AMD TOPOEXT cache info */
> +if (cpu->cache_info_passthrough) {
> +host_cpuid(index, count, eax, ebx, ecx, edx);
> +break;
> +} else if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) {
> +*eax = 0;
> +switch (count) {
> +case 0: /* L1 dcache info */
> +*eax |= CPUID_4_TYPE_DCACHE | \
> +CPUID_4_LEVEL(1) | \
> +CPUID_4_SELF_INIT_LEVEL | \
> +((cs->nr_threads - 1) << 14);

CPUID_4 uses the same format even for bits 25-14, so this would look
better with a macro.

> +*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 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).

> +*edx = 0;
> +break;
> +case 1: /* L1 icache info */
> +*eax |= CPUID_4_TYPE_ICACHE | \
> +CPUID_4_LEVEL(1) | \
> +CPUID_4_SELF_INIT_LEVEL | \
> +((cs->nr_threads - 1) << 14);
> +*ebx = (L1I_LINE_SIZE - 1) | \
> +   ((L1I_PARTITIONS - 1) << 12) | \
> +   ((L1I_ASSOCIATIVITY_AMD - 1) << 22);
> +*ecx = L1I_SETS_AMD - 1;
> +*edx = 0;
> +break;
> +case 2: /* L2 cache info */
> +*eax |= CPUID_4_TYPE_UNIFIED | \
> +CPUID_4_LEVEL(2) | \
> +CPUID_4_SELF_INIT_LEVEL | \
> +((cs->nr_threads - 1) << 14);
> +*ebx = (L2_LINE_SIZE - 1) | \
> +   ((L2_PARTITIONS - 1) << 12) | \
> +   ((L2_ASSOCIATIVITY_AMD - 1) << 22);
> +*ecx = L2_SETS_AMD - 1;
> +*edx = CPUID_4_INCLUSIVE;
> +break;
> +case 3: /* L3 cache info */
> +if (!cpu->enable_l3_cache) {
> +*eax = 0;
> +*ebx = 0;
> +*ecx = 0;
> +*edx = 0;
> +break;
> +}
> +*eax |= CPUID_4_TYPE_UNIFIED | \
> +CPUID_4_LEVEL(3) | \
> +CPUID_4_SELF_INIT_LEVEL | \
> +((cs->nr_cores * cs->nr_threads - 1) << 14);

This number seems to be the only difference that isn't just a difference
constant.  It tempts me to merge the cases for 4 and 0x801D as it
seems that vendors try to be compatible.

> +*ebx = (L3_N_LINE_SIZE - 1) | \
> +   ((L3_N_PARTITIONS - 1) << 12) | \
> +   ((L3_N_ASSOCIATIVITY - 1) << 22);
> +*ecx = L3_N_SETS_AMD - 1;
> +*edx = CPUID_4_NO_INVD_SHARING;
> +break;
> +default: /* end of info */
> +*eax = 0;
> +*ebx = 0;
> +*ecx = 0;
> +*edx = 0;
> +break;
> +}
> +} else {
> +*eax = 0;
> +*ebx = 0;
> +*ecx = 0;
> +*edx = 0;
> +}
> +break;
>  case 0xC000:
>  *eax = env->cpuid_xlevel2;
>  *ebx = 0;

The numbers looks like real hardware,

thanks.