linux-kernel - Re: [RESEND PATCH 1/3] x86: Adding structs to reflect cpuid fields

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 17 Sep 2014 16:53:39 +0300
From:	Nadav Amit <namit@...technion.ac.il>
To:	Borislav Petkov <bp@...en8.de>
Cc:	Ingo Molnar <mingo@...nel.org>,
	Paolo Bonzini <pbonzini@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"the arch/x86 maintainers" <x86@...nel.org>,
	kvm <kvm@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Subject: Re: [RESEND PATCH 1/3] x86: Adding structs to reflect cpuid fields

Boris,

Thanks for you comments - please see inline.

On Wed, Sep 17, 2014 at 4:21 PM, Borislav Petkov <bp@...en8.de> wrote:
> On Wed, Sep 17, 2014 at 03:54:12PM +0300, Nadav Amit wrote:
>> Adding structs that reflect various cpuid fields in x86 architecture. Structs
>> were added only for functions that are not pure bitmaps.
>>
>> Signed-off-by: Nadav Amit <namit@...technion.ac.il>
>> ---
>>  arch/x86/include/asm/cpuid_def.h | 163 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 163 insertions(+)
>>  create mode 100644 arch/x86/include/asm/cpuid_def.h
>>
>> diff --git a/arch/x86/include/asm/cpuid_def.h b/arch/x86/include/asm/cpuid_def.h
>> new file mode 100644
>> index 0000000..0cf43ba
>> --- /dev/null
>> +++ b/arch/x86/include/asm/cpuid_def.h
>> @@ -0,0 +1,163 @@
>> +#ifndef ARCH_X86_KVM_CPUID_DEF_H
>> +#define ARCH_X86_KVM_CPUID_DEF_H
>> +
>> +union cpuid1_eax {
>> +     struct {
>> +             unsigned int stepping_id:4;
>> +             unsigned int model:4;
>> +             unsigned int family_id:4;
>> +             unsigned int processor_type:2;
>
> This is not present on AMD so now you need to start differentiate
> between vendors. Not a big deal, it simply doesn't get touched as it is
> in the reserved range there...

It does not seem to be coincidence; AMD and Intel appear to be
synchronised when it comes to CPUID, MSRs and opcodes.

>
>> +             unsigned int reserved:2;
>> +             unsigned int extended_model_id:4;
>> +             unsigned int extended_family_id:8;
>> +             unsigned int reserved2:4;
>> +     } split;
>> +     unsigned int full;
>> +};
>> +
>> +union cpuid1_ebx {
>> +     struct {
>> +             unsigned int brand_index:8;
>> +             unsigned int clflush_size:8;
>> +             unsigned int max_logical_proc:8;
>> +             unsigned int initial_apicid:8;
>> +     } split;
>> +     unsigned int full;
>> > +
>> +
>> +union cpuid4_eax {
>> +     struct {
>> +             unsigned int cache_type:5;
>> +             unsigned int cache_level:3;
>> +             unsigned int self_init_cache_level:1;
>> +             unsigned int fully_associative:1;
>> +             unsigned int reserved:4;
>> +             unsigned int max_logical_proc:12;
>> +             unsigned int max_package_proc:6;
>> +     } split;
>> +     unsigned int full;
>> +};
>> +
>> +union cpuid4_ebx {
>> +     struct {
>> +             unsigned int coherency_line_size:12;
>> +             unsigned int physical_line_partitions:10;
>> +             unsigned int ways:10;
>> +     } split;
>> +     unsigned int full;
>> +};
>> +
>> +union cpuid5_eax {
>> +     struct {
>> +             unsigned int min_monitor_line_size:16;
>> +             unsigned int reserved:16;
>
> ... the problem with hardcoding those bitfields I see are those reserved
> fields. The moment hw guys decide to widen, say, that smallest monitor
> line size, you need the ifdeffery around it. Which automatically becomes
> ugly and now all of a sudden you need to pay attention to it.

AFAIK backward compatibility is usually maintained in x86. I did not
see in Intel SDM anything that says "this CPUID field means something
for CPU X and something else for CPU Y". Anyhow, it is not different
than bitmasks in this respect.

>
> And not all vendors define all bits the same so you're probably going to
> have to differentiate there too, at some point.
I am not sure, unless an additional x86 competitor comes to the market...

>
> Oh, and I don't see what is wrong with opening the CPUID manual in
> parallel and looking at the bits.
Well, cscope still does not handle bitmasks. You could have made the
same argument for segment descriptors, yet desc_struct exists and I
think its wide use shows its easier to use.

>
> So, IMO, doing this is a bad idea.
I don't want to be a snitch, but cpuid10_eax and other PMU related
cpuid structures already use bitfields, so I don't understand what all
the fuss is about.

Regards,
Nadav
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/