Re: [PATCH 0/3] x86: structs for cpuid info in x86

Re: [PATCH 0/3] x86: structs for cpuid info in x86

[Ingo Molnar]

* Nadav Amit <namit@cs.technion.ac.il> wrote:

> The code that deals with x86 cpuid fields is hard to follow since it performs
> many bit operations and does not refer to cpuid field explicitly.  To
> eliminate the need of openning a spec whenever dealing with cpuid fields, this
> patch-set introduces structs that reflect the various cpuid functions.
> 
> Thanks for reviewing the patch-set.
> 
> Nadav Amit (3):
>   x86: Adding structs to reflect cpuid fields
>   x86: Use new cpuid structs in cpuid functions
>   KVM: x86: Using cpuid structs in KVM
> 
>  arch/x86/include/asm/cpuid_def.h | 163 +++++++++++++++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/common.c     |  56 ++++++++------
>  arch/x86/kvm/cpuid.c             |  36 +++++----
>  3 files changed, 219 insertions(+), 36 deletions(-)
>  create mode 100644 arch/x86/include/asm/cpuid_def.h

I personally like bitfields in theory (they provide type clarity 
and abstract robustness, compared to open-coded bitmask numeric 
literals that are often used in cpuid using code, obfuscating 
cpuid usage), with the big caveat that for many years I didn't 
like bitfields in practice: older versions of GCC did a really 
poor job of optimizing them.

So such a series would only be acceptable if it's demonstrated 
that both 'latest' and 'reasonably old' GCC versions do a good 
job in that department, compared to the old open-coded bitmask 
ops ...

Comparing the 'size vmlinux' output of before/after kernels would 
probably be a good start in seeing the impact of such a change.

If those results are positive then this technique could be 
propagated to all cpuid using code in arch/x86/, of which
there's plenty.

Thanks,

	Ingo

Re: [PATCH 0/3] x86: structs for cpuid info in x86

[Nadav Amit]

On 9/16/14 4:22 PM, Ingo Molnar wrote:
> 
> * Nadav Amit <namit@cs.technion.ac.il> wrote:
> 
>> The code that deals with x86 cpuid fields is hard to follow since it performs
>> many bit operations and does not refer to cpuid field explicitly.  To
>> eliminate the need of openning a spec whenever dealing with cpuid fields, this
>> patch-set introduces structs that reflect the various cpuid functions.
>>
>> Thanks for reviewing the patch-set.
>>
>> Nadav Amit (3):
>>   x86: Adding structs to reflect cpuid fields
>>   x86: Use new cpuid structs in cpuid functions
>>   KVM: x86: Using cpuid structs in KVM
>>
>>  arch/x86/include/asm/cpuid_def.h | 163 +++++++++++++++++++++++++++++++++++++++
>>  arch/x86/kernel/cpu/common.c     |  56 ++++++++------
>>  arch/x86/kvm/cpuid.c             |  36 +++++----
>>  3 files changed, 219 insertions(+), 36 deletions(-)
>>  create mode 100644 arch/x86/include/asm/cpuid_def.h
> 
> I personally like bitfields in theory (they provide type clarity 
> and abstract robustness, compared to open-coded bitmask numeric 
> literals that are often used in cpuid using code, obfuscating 
> cpuid usage), with the big caveat that for many years I didn't 
> like bitfields in practice: older versions of GCC did a really 
> poor job of optimizing them.
> 
> So such a series would only be acceptable if it's demonstrated 
> that both 'latest' and 'reasonably old' GCC versions do a good 
> job in that department, compared to the old open-coded bitmask 
> ops ...
> 
> Comparing the 'size vmlinux' output of before/after kernels would 
> probably be a good start in seeing the impact of such a change.
> 
> If those results are positive then this technique could be 
> propagated to all cpuid using code in arch/x86/, of which
> there's plenty.

Thanks for the quick response. I was not aware GCC behaves this way. I
made some small experiments with GCC-4.8 and GCC-4.4 and in brief my
conclusions are:
1. The assembled code of bitmask and bitfields is indeed different.
2. GCC-4.8 and GCC-4.4 behave pretty much the same, yet GCC-4.8 appears
to make better instructions reordering.
3. Loading/storing a single bitfield seems to be pretty much optimized
(marginal advantage from code size point-of-view for bitmask, same
number of instructions).
4. Loading/storing multiple bitfields seems to be somewhat
under-optimized - multiple accesses to the original value result in ~30%
more instructions and code-size.

So you are correct - bitfields are less optimized. Nonetheless, since
cpuid data is mostly used during startup, and otherwise a single
bitfield is usually accessed in each function - I wonder whether it
worth keeping the optimized but "obfuscate" code. Obviously, I can guess
your answer to this question...

Nadav

Re: [PATCH 0/3] x86: structs for cpuid info in x86

[Ingo Molnar]

* Nadav Amit <nadav.amit@gmail.com> wrote:

> 
> 
> On 9/16/14 4:22 PM, Ingo Molnar wrote:
> > 
> > * Nadav Amit <namit@cs.technion.ac.il> wrote:
> > 
> >> The code that deals with x86 cpuid fields is hard to follow since it performs
> >> many bit operations and does not refer to cpuid field explicitly.  To
> >> eliminate the need of openning a spec whenever dealing with cpuid fields, this
> >> patch-set introduces structs that reflect the various cpuid functions.
> >>
> >> Thanks for reviewing the patch-set.
> >>
> >> Nadav Amit (3):
> >>   x86: Adding structs to reflect cpuid fields
> >>   x86: Use new cpuid structs in cpuid functions
> >>   KVM: x86: Using cpuid structs in KVM
> >>
> >>  arch/x86/include/asm/cpuid_def.h | 163 +++++++++++++++++++++++++++++++++++++++
> >>  arch/x86/kernel/cpu/common.c     |  56 ++++++++------
> >>  arch/x86/kvm/cpuid.c             |  36 +++++----
> >>  3 files changed, 219 insertions(+), 36 deletions(-)
> >>  create mode 100644 arch/x86/include/asm/cpuid_def.h
> > 
> > I personally like bitfields in theory (they provide type clarity 
> > and abstract robustness, compared to open-coded bitmask numeric 
> > literals that are often used in cpuid using code, obfuscating 
> > cpuid usage), with the big caveat that for many years I didn't 
> > like bitfields in practice: older versions of GCC did a really 
> > poor job of optimizing them.
> > 
> > So such a series would only be acceptable if it's demonstrated 
> > that both 'latest' and 'reasonably old' GCC versions do a good 
> > job in that department, compared to the old open-coded bitmask 
> > ops ...
> > 
> > Comparing the 'size vmlinux' output of before/after kernels would 
> > probably be a good start in seeing the impact of such a change.
> > 
> > If those results are positive then this technique could be 
> > propagated to all cpuid using code in arch/x86/, of which
> > there's plenty.
> 
> Thanks for the quick response. I was not aware GCC behaves this 
> way. I made some small experiments with GCC-4.8 and GCC-4.4 and 
> in brief my conclusions are:
>
> 1. The assembled code of bitmask and bitfields is indeed different.
> 2. GCC-4.8 and GCC-4.4 behave pretty much the same, yet GCC-4.8 appears
> to make better instructions reordering.
> 3. Loading/storing a single bitfield seems to be pretty much optimized
> (marginal advantage from code size point-of-view for bitmask, same
> number of instructions).
> 4. Loading/storing multiple bitfields seems to be somewhat
> under-optimized - multiple accesses to the original value result in ~30%
> more instructions and code-size.

That's better than what I remembered.

> So you are correct - bitfields are less optimized. Nonetheless, 
> since cpuid data is mostly used during startup, and otherwise a 
> single bitfield is usually accessed in each function - I wonder 
> whether it worth keeping the optimized but "obfuscate" code. 
> Obviously, I can guess your answer to this question...

So with the condition that you are actively watching out for 
performance critical code paths, I think the type clarity (i.e. 
bitfields) is a win.

If hpa, tglx or Linus objects I'll yield to that objection 
though.

Opinions, objections?

Thanks,

	Ingo

Re: [PATCH 0/3] x86: structs for cpuid info in x86

[Borislav Petkov]

On Wed, Sep 17, 2014 at 02:37:10PM +0200, Ingo Molnar wrote:
> Opinions, objections?

Can I see those patches please? I can't find them on lkml or on the net
- I only see this sub-thread...

Thanks.

-- 
Regards/Gruss,
    Boris.
--

[RESEND PATCH 0/3] x86: structs for cpuid info in x86

[Nadav Amit]

The code that deals with x86 cpuid fields is hard to follow since it performs
many bit operations and does not refer to cpuid field explicitly.  To
eliminate the need of openning a spec whenever dealing with cpuid fields, this
patch-set introduces structs that reflect the various cpuid functions.

Thanks for reviewing the patch-set.

Nadav Amit (3):
  x86: Adding structs to reflect cpuid fields
  x86: Use new cpuid structs in cpuid functions
  KVM: x86: Using cpuid structs in KVM

 arch/x86/include/asm/cpuid_def.h | 163 +++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/common.c     |  56 ++++++++------
 arch/x86/kvm/cpuid.c             |  36 +++++----
 3 files changed, 219 insertions(+), 36 deletions(-)
 create mode 100644 arch/x86/include/asm/cpuid_def.h

-- 
1.9.1

[RESEND PATCH 1/3] x86: Adding structs to reflect cpuid fields

[Nadav Amit]

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@cs.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;
+		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;
+	} split;
+	unsigned int full;
+};
+
+union cpuid5_ebx {
+	struct {
+		unsigned int max_monitor_line_size:16;
+		unsigned int reserved:16;
+	} split;
+	unsigned int full;
+};
+
+union cpuid5_ecx {
+	struct {
+		unsigned int monitor_mwait_ext:1;
+		unsigned int interrupt_as_break:1;
+	} split;
+	unsigned int full;
+};
+
+union cpuid5_edx {
+	struct {
+		unsigned int c0_sub_states:4;
+		unsigned int c1_sub_states:4;
+		unsigned int c2_sub_states:4;
+		unsigned int c3_sub_states:4;
+		unsigned int c4_sub_states:4;
+		unsigned int c5_sub_states:4;
+		unsigned int c6_sub_states:4;
+		unsigned int c7_sub_states:4;
+	} split;
+	unsigned int full;
+};
+
+union cpuid11_eax {
+	struct {
+		unsigned int x2apic_shift:5;
+		unsigned int reserved:27;
+	} split;
+	unsigned int full;
+};
+
+union cpuid11_ebx {
+	struct {
+		unsigned int logical_proc:16;
+		unsigned int reserved:16;
+	} split;
+	unsigned int full;
+};
+
+union cpuid11_ecx {
+	struct {
+		unsigned int level_number:8;
+		unsigned int level_type:8;
+		unsigned int reserved:16;
+	} split;
+	unsigned int full;
+};
+
+union cpuid8_5_eax_ebx {
+	struct {
+		unsigned int itlb_entries:8;
+		unsigned int itlb_assoc:8;
+		unsigned int dtlb_entries:8;
+		unsigned int dtlb_assoc:8;
+	} split;
+	unsigned int full;
+};
+
+union cpuid8_5_ecx_edx {
+	struct {
+		unsigned int line_size:8;
+		unsigned int lines_per_tag:8;
+		unsigned int associativity:8;
+		unsigned int cache_size:8;
+	} split;
+	unsigned int full;
+};
+
+union cpuid8_6_ebx {
+	struct {
+		unsigned int itlb_entries:12;
+		unsigned int itlb_assoc:4;
+		unsigned int dtlb_entries:12;
+		unsigned int dtlb_assoc:4;
+	} split;
+	unsigned int full;
+};
+
+union cpuid8_6_ecx {
+	struct {
+		unsigned int cache_line_size:8;
+		unsigned int lines_per_tag:4;
+		unsigned int l2_associativity:4;
+		unsigned int cache_size:16;
+	} split;
+	unsigned int full;
+};
+
+union cpuid_8_8_eax {
+	struct {
+		unsigned int phys_as:8;
+		unsigned int virt_as:8;
+		unsigned int guest_phys_as:8;
+		unsigned int reserved:8;
+	} split;
+	unsigned int full;
+};
+
+#endif
-- 
1.9.1

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

[Borislav Petkov]

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@cs.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...

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

And not all vendors define all bits the same so you're probably going to
have to differentiate there too, at some point.

Oh, and I don't see what is wrong with opening the CPUID manual in
parallel and looking at the bits.

So, IMO, doing this is a bad idea.

-- 
Regards/Gruss,
    Boris.
--

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

[Nadav Amit]

Boris,

Thanks for you comments - please see inline.

On Wed, Sep 17, 2014 at 4:21 PM, Borislav Petkov <bp@alien8.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@cs.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

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

[Borislav Petkov]

On Wed, Sep 17, 2014 at 04:53:39PM +0300, Nadav Amit wrote:
> 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.

You still don't get my point: what are you going to do when
min_monitor_line_size needs to be 17 bits all of a sudden?

Currently, you simply do an if-else check before using the respective
mask and with your defined structs, you need to keep two versions:

union cpuid5_ebx_before_family_X {
       struct {
               unsigned int max_monitor_line_size:16;
               unsigned int reserved:16;
       } split;
       unsigned int full;
};

union cpuid5_ebx_after_family_X {
       struct {
               unsigned int max_monitor_line_size:17;
               unsigned int reserved:15;
       } split;
       unsigned int full;
};

> I don't understand what all the fuss is about.

And I don't understand why you're "fixing" code which doesn't need
fixing in the first place.

-- 
Regards/Gruss,
    Boris.
--

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

[Radim Krčmář]

2014-09-17 16:06+0200, Borislav Petkov:
> On Wed, Sep 17, 2014 at 04:53:39PM +0300, Nadav Amit wrote:
> > 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.
> 
> You still don't get my point: what are you going to do when
> min_monitor_line_size needs to be 17 bits all of a sudden?
> 
> Currently, you simply do an if-else check before using the respective
> mask and with your defined structs, you need to keep two versions:
> 
> union cpuid5_ebx_before_family_X {
>        struct {
>                unsigned int max_monitor_line_size:16;
>                unsigned int reserved:16;
>        } split;
>        unsigned int full;
> };
> 
> union cpuid5_ebx_after_family_X {
>        struct {
>                unsigned int max_monitor_line_size:17;
>                unsigned int reserved:15;
>        } split;
>        unsigned int full;
> };

New union wouldn't be very convenient if the change touched just a small
part of the register ... probably the best choice is using anonymous
elements like this,

  union cpuid5_ebx {
  	union {
  		struct {
  			unsigned int max_monitor_line_size:16;
  			unsigned int reserved:16;
  		};
  		struct {
  			unsigned int max_monitor_line_size_after_family_X:17;
  			unsigned int reserved_after_family_X:15;
  		};
  	} split;
  	unsigned int full;
  };

which would result in a similar if-else hack

  if (family > X)
  	ebx.split.max_monitor_line_size_after_family_X = 0
  else
  	ebx.split.max_monitor_line_size = 0

other options are
  ebx.split.after_family_X.max_monitor_line_size
or even
  ebx.split.max_monitor_line_size.after_family_X

Flat namespace is more flexible wrt. code.

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

[Borislav Petkov]

On Wed, Sep 17, 2014 at 05:04:33PM +0200, Radim Krčmář wrote:
> which would result in a similar if-else hack
> 
>   if (family > X)
>   	ebx.split.max_monitor_line_size_after_family_X = 0
>   else
>   	ebx.split.max_monitor_line_size = 0
> 
> other options are
>   ebx.split.after_family_X.max_monitor_line_size
> or even
>   ebx.split.max_monitor_line_size.after_family_X

And how is that better than simply doing

	cpuid = cpuid_ebx(5);

	if (family > X)
		max_monitor_line_size = cpuid & MASK_FAM_X;
	else
		max_monitor_line_size = cpuid & MASK_BEFORE_FAM_X;

?

With proper variable naming all is perfectly clear, readable
and simple. You don't need to open even the CPUID manual - the
variable tells you you're getting the max monitor line size -
"ebx.split.max_monitor_line_size_after_family_X" needs me to parse it
with my eyes first.

-- 
Regards/Gruss,
    Boris.
--

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

[Radim Krčmář]

2014-09-17 17:22+0200, Borislav Petkov:
> On Wed, Sep 17, 2014 at 05:04:33PM +0200, Radim Krčmář wrote:
> > which would result in a similar if-else hack
> > 
> >   if (family > X)
> >   	ebx.split.max_monitor_line_size_after_family_X = 0
> >   else
> >   	ebx.split.max_monitor_line_size = 0
> > 
> > other options are
> >   ebx.split.after_family_X.max_monitor_line_size
> > or even
> >   ebx.split.max_monitor_line_size.after_family_X
> 
> And how is that better than simply doing
> 
> 	cpuid = cpuid_ebx(5);
> 
> 	if (family > X)
> 		max_monitor_line_size = cpuid & MASK_FAM_X;
> 	else
> 		max_monitor_line_size = cpuid & MASK_BEFORE_FAM_X;
> 
> ?
> 
> With proper variable naming all is perfectly clear, readable
> and simple. You don't need to open even the CPUID manual - the
> variable tells you you're getting the max monitor line size -
> "ebx.split.max_monitor_line_size_after_family_X" needs me to parse it
> with my eyes first.

I think you proposed to use magic constant in place of of MASK_FAM_X, so
the code above is

  	if (family > X)
  		max_monitor_line_size = cpuid & 0x1ffff;
  	else
  		max_monitor_line_size = cpuid & 0xffff;

We can nicely oneline it, but that's about all the benefits I can see.
It is prone to typos, hard to search for and limiting our operations to
a simple assignment to a properly named variable.

(I prefer descriptive, horribly long, names to raw constant everywhere,
 MASK_MAX_MONITOR_LINE_SIZE_FAM_X.)


Second problem:  Most elements don't begin at offset 0, so the usual
retrieval would add a shift, (repurposing max_monitor_line_size)

 max_monitor_line_size = (cpuid & MASK_FAM_X) >> OFFSET_FAM_X;

and it's not better when we write it back after doing stuff.

 cpuid = (cpuid & ~MASK_FAM_X) | (max_monitor_line_size << OFFSET_FAM_X
                                  & MASK_FAM_X);

All would be fine if we abstracted this with more macros ... wait,
bitfield already does that!

 max_monitor_line_size = cpuid.split.max_monitor_line_size_fam_x;

 cpuid.split.max_monitor_line_size_fam_x = max_monitor_line_size;


---
OT: I'd remove '.split', but we probably wouldn't agree about '.full'.

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

[Borislav Petkov]

On Thu, Sep 18, 2014 at 02:29:54AM +0200, Radim Krčmář wrote:
> I think you proposed to use magic constant in place of of MASK_FAM_X, so

Huh, what?

> Second problem:  Most elements don't begin at offset 0, so the usual
> retrieval would add a shift, (repurposing max_monitor_line_size)

So what?

>  max_monitor_line_size = (cpuid & MASK_FAM_X) >> OFFSET_FAM_X;
> 
> and it's not better when we write it back after doing stuff.

Writing back CPUID on baremetal? You can't be serious.

Ok, this is getting ridiculous so I'll stop wasting time. I stand by my
initial statement - this is a bad idea.

-- 
Regards/Gruss,
    Boris.
--

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

[Radim Krčmář]

2014-09-18 09:19+0200, Borislav Petkov:
> On Thu, Sep 18, 2014 at 02:29:54AM +0200, Radim Krčmář wrote:
> > I think you proposed to use magic constant in place of of MASK_FAM_X, so
> 
> Huh, what?

Your example.  It cannot be verbatim MASK_FAM_X in real code.

I interpreted it to be a placeholder for 0x1ffff (first 17 bits) and
said why I think it is not a good thing.
The other interpretation (well named macro) was against your goal.

> > Second problem:  Most elements don't begin at offset 0, so the usual
> > retrieval would add a shift, (repurposing max_monitor_line_size)
> 
> So what?

Your goal was easy parsing (last sentence of the mail).

My argument is that bitfields are easier to read.  (With examples.)

> >  max_monitor_line_size = (cpuid & MASK_FAM_X) >> OFFSET_FAM_X;
> > 
> > and it's not better when we write it back after doing stuff.
> 
> Writing back CPUID on baremetal? You can't be serious.

We are not talking just about baremetal, see patch [3/3].

There are, and will be new, use cases for modifying and storing cpuid.

> Ok, this is getting ridiculous so I'll stop wasting time. I stand by my
> initial statement - this is a bad idea.

Acknowledged.

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

[Paolo Bonzini]

Il 17/09/2014 16:06, Borislav Petkov ha scritto:
>> > 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.
> You still don't get my point: what are you going to do when
> min_monitor_line_size needs to be 17 bits all of a sudden?

The extra bit used to be reserved and thus will be zero on older
families.  So, nothing?

Paolo

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

[Borislav Petkov]

On Thu, Sep 18, 2014 at 03:06:59PM +0200, Paolo Bonzini wrote:
> The extra bit used to be reserved and thus will be zero on older
> families.  So, nothing?

"thus will be zero" is unfortunately simply not true.

>From the SDM:

"1.3.2 Reserved Bits and Software Compatibility

In many register and memory layout descriptions, certain bits are marked
as reserved. When bits are marked as reserved, it is essential for
compatibility with future processors that software treat these bits as
having a future, though unknown, effect. The behavior of reserved bits
should be regarded as not only undefined, but unpredict- able. Software
should follow these guidelines in dealing with reserved bits:

* Do not depend on the states of any reserved bits when testing the values of registers that contain such bits.
Mask out the reserved bits before testing.

*  Do not depend on the states of any reserved bits when storing to memory or to a register.

* Do not depend on the ability to retain information written into any reserved bits.

* When loading a register, always load the reserved bits with the values indicated in the documentation, if any,
or reload them with values previously read from the same register."

And so on and so on.

Aaanyway, I see kvm folks are strongly determined to do this so I'll
propose you do it for kvm only and not touch the rest of arch/x86/.

I really really disagree with casting CPUID words in stone and with
touching code which is perfectly fine.

But this is just me and the final word lies with x86 people so it'll
happen whatever they say.

-- 
Regards/Gruss,
    Boris.
--

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

[Paolo Bonzini]

Il 18/09/2014 15:26, Borislav Petkov ha scritto:
> On Thu, Sep 18, 2014 at 03:06:59PM +0200, Paolo Bonzini wrote:
>> The extra bit used to be reserved and thus will be zero on older
>> families.  So, nothing?
> 
> "thus will be zero" is unfortunately simply not true.
> 
> From the SDM:
> 
> "1.3.2 Reserved Bits and Software Compatibility
> 
> In many register and memory layout descriptions, certain bits are marked
> as reserved. When bits are marked as reserved, it is essential for
> compatibility with future processors that software treat these bits as
> having a future, though unknown, effect. The behavior of reserved bits
> should be regarded as not only undefined, but unpredict- able. Software
> should follow these guidelines in dealing with reserved bits:

We're talking about the case where the field is not reserved anymore and
we _know_ that the vendor has just decided to grow the bitfield that
precedes it.

As soon as we know that the field is not reserved anymore, we obviously
rely on reserved bits being zero in older processors, and in future
processors from other vendors.  The trivial example is feature bits like
XSAVE.  We query them all the time without checking the family when they
were first introduced, don't we?

> * Do not depend on the states of any reserved bits when testing the values of registers that contain such bits.
> Mask out the reserved bits before testing.

Done by the bitfields.

> *  Do not depend on the states of any reserved bits when storing to memory or to a register.

Doesn't apply to CPUID.

> * Do not depend on the ability to retain information written into any reserved bits.

Doesn't apply to CPUID.

> * When loading a register, always load the reserved bits with the values indicated in the documentation, if any,
> or reload them with values previously read from the same register."

Only applies to KVM, where it can be done with the bitfields too.

> And so on and so on.
> 
> Aaanyway, I see kvm folks are strongly determined to do this so I'll
> propose you do it for kvm only and not touch the rest of arch/x86/.
> 
> I really really disagree with casting CPUID words in stone and with
> touching code which is perfectly fine.

It's not really the KVM folks.  I actually kinda agree with you about
"touching code that works", which is why I hadn't yet commented on the
series.  But Ingo and Peter like it, so I can accept the patch as well.

> But this is just me and the final word lies with x86 people so it'll
> happen whatever they say.

Yep, same with me---KVM will just follow suit.

Paolo

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

[Borislav Petkov]

On Thu, Sep 18, 2014 at 03:36:43PM +0200, Paolo Bonzini wrote:
> We're talking about the case where the field is not reserved anymore and
> we _know_ that the vendor has just decided to grow the bitfield that
> precedes it.

We're talking about the case where you assumed that a reserved bit is 0
which is an unsafe assumption, the least.

> As soon as we know that the field is not reserved anymore, we
> obviously rely on reserved bits being zero in older processors, and in
> future processors from other vendors.

Again, this is an unsafe assumption.

> The trivial example is feature bits like XSAVE. We query them all the
> time without checking the family when they were first introduced,
> don't we?

The feature bits would obviously be 0 if features are not supported.

However, even there

"16 - Reserved - Do not count on the value."

I'm quoting Intel's CPUID doc 241618-037 from 2011 (there might be a
newer one though), the CPUID(1).ECX description.

-- 
Regards/Gruss,
    Boris.
--

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

[Nadav Amit]

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]


On Sep 19, 2014, at 10:58 AM, Borislav Petkov <bp@alien8.de> wrote:

> On Thu, Sep 18, 2014 at 03:36:43PM +0200, Paolo Bonzini wrote:
>> We're talking about the case where the field is not reserved anymore and
>> we _know_ that the vendor has just decided to grow the bitfield that
>> precedes it.
> 
> We're talking about the case where you assumed that a reserved bit is 0
> which is an unsafe assumption, the least.
> 
>> As soon as we know that the field is not reserved anymore, we
>> obviously rely on reserved bits being zero in older processors, and in
>> future processors from other vendors.
> 
> Again, this is an unsafe assumption.
> 
>> The trivial example is feature bits like XSAVE. We query them all the
>> time without checking the family when they were first introduced,
>> don't we?
> 
> The feature bits would obviously be 0 if features are not supported.
> 
> However, even there
> 
> "16 - Reserved - Do not count on the value."
> 
> I'm quoting Intel's CPUID doc 241618-037 from 2011 (there might be a
> newer one though), the CPUID(1).ECX description.

New fields which which replace reserved bits would be handled the same way with bitfields and bit masks.

As for the concern that CPUID fields would be extended into non-zero reserved bits - can someone point me to a single case that occurred in the last 20 years during which CPUID existed? 

The closest thing I found was “extended family id”, but this field is separate than “family id” and treated this way. 

Nadav

[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Borislav Petkov]

On Fri, Sep 19, 2014 at 11:59:32AM +0300, Nadav Amit wrote:
> As for the concern that CPUID fields would be extended into non-zero
> reserved bits - can someone point me to a single case that occurred in
> the last 20 years during which CPUID existed?

Do you have a guarantee that this won't happen in the future and break
all your fancy bitfields assumptions?

-- 
Regards/Gruss,
    Boris.
--

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

[Paolo Bonzini]

Il 19/09/2014 09:58, Borislav Petkov ha scritto:
>> > The trivial example is feature bits like XSAVE. We query them all the
>> > time without checking the family when they were first introduced,
>> > don't we?
> The feature bits would obviously be 0 if features are not supported.

And similarly, Intel would not extend a bit from 16 to 17 bits if it
weren't zero on all older processors.

> However, even there
> 
> "16 - Reserved - Do not count on the value."
> 
> I'm quoting Intel's CPUID doc 241618-037 from 2011 (there might be a
> newer one though), the CPUID(1).ECX description.

Once that bit gets a meaning in newer processors, the same meaning will
work retroactively for existing processors.  That's just how CPUID is
used.  Nobody checks families before testing bits, Intel/AMD do not even
suggest that.

> Do you have a guarantee that this won't happen in the future and break
> all your fancy bitfields assumptions?

No guarantee, but were that to happen, I'd expect tar and feathers
spectacles around Intel's engineering headquarters.

Paolo

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

[Borislav Petkov]

On Fri, Sep 19, 2014 at 03:40:29PM +0200, Paolo Bonzini wrote:
> And similarly, Intel would not extend a bit from 16 to 17 bits if it
> weren't zero on all older processors.

Nothing is stopping a hw designer to say we're using 17 bits now. And
software needs to deal with that. It is that simple!

> > However, even there
> > 
> > "16 - Reserved - Do not count on the value."
> > 
> > I'm quoting Intel's CPUID doc 241618-037 from 2011 (there might be a
> > newer one though), the CPUID(1).ECX description.
> 
> Once that bit gets a meaning in newer processors, the same meaning will
> work retroactively for existing processors.  That's just how CPUID is
> used.  Nobody checks families before testing bits, Intel/AMD do not even
> suggest that.

I know that, and I bet those bits won't even get reused but we don't
know, do we?! All I'm pointing at is that even the feature bits which
are reserved cannot be relied to be of any value.

In any case, the moment hw designers decide to change any field width
for which you have an union defined, this fragile scheme breaks with you
needing to introduce ifdeffery.

And frankly, this whole waste-time discussion is only about you guys
wanting to use those bitfields so that constructing a CPUID value works
more conveniently. (Reportedly, I'm not convinced of that at all).

While the gazillion other places in the kernel simply use logical
operations to construct a value and write it into the corresponding
register. Does that mean that they have to go and define unions for
those registers too? Of course not! That would be insane.

I remember a time where we did remove exactly that type of bitfields
from the ide code because it wasn't helping, it was counter-intuitive
and needless.

Geez, can we drop this already! It is a dumb idea, it doesn't bring us
anything besides some superficial readability which you don't really
need. Nowadays you can use even goddam wikipedia to understand what the
CPUID bits mean:

https://en.wikipedia.org/wiki/CPUID

Everytime we have to access registers, we need to open the corresponding
manual (or wikipedia, alternatively :-)) and look at the bit
definitions. This will not change.

> > Do you have a guarantee that this won't happen in the future and break
> > all your fancy bitfields assumptions?
> 
> No guarantee, but were that to happen, I'd expect tar and feathers
> spectacles around Intel's engineering headquarters.

You're going over and doing some fireworks there?

:-P

-- 
Regards/Gruss,
    Boris.
--

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

[Peter Zijlstra]

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@cs.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

Stale name? Its not exclusively used for KVM and the header name itself
doesn't include KVM either.

[RESEND PATCH 2/3] x86: Use new cpuid structs in cpuid functions

[Nadav Amit]

The current code that decodes cpuid fields is somewhat cryptic, since it uses
many bit operations.  Using cpuid structs instead for clarifying the code.
Introducign no functional change.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kernel/cpu/common.c | 56 +++++++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index e4ab2b4..b57c160 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -41,6 +41,7 @@
 #include <asm/pat.h>
 #include <asm/microcode.h>
 #include <asm/microcode_intel.h>
+#include <asm/cpuid_def.h>
 
 #ifdef CONFIG_X86_LOCAL_APIC
 #include <asm/uv/uv.h>
@@ -444,13 +445,17 @@ static void get_model_name(struct cpuinfo_x86 *c)
 
 void cpu_detect_cache_sizes(struct cpuinfo_x86 *c)
 {
-	unsigned int n, dummy, ebx, ecx, edx, l2size;
+	unsigned int n, dummy, dummy2, l2size;
+	union cpuid8_5_ecx_edx ecx5, edx5;
+	union cpuid8_6_ebx ebx6;
+	union cpuid8_6_ecx ecx6;
 
 	n = c->extended_cpuid_level;
 
 	if (n >= 0x80000005) {
-		cpuid(0x80000005, &dummy, &ebx, &ecx, &edx);
-		c->x86_cache_size = (ecx>>24) + (edx>>24);
+		cpuid(0x80000005, &dummy, &dummy2, &ecx5.full, &edx5.full);
+		c->x86_cache_size = ecx5.split.cache_size +
+				    edx5.split.cache_size;
 #ifdef CONFIG_X86_64
 		/* On K8 L1 TLB is inclusive, so don't count it */
 		c->x86_tlbsize = 0;
@@ -460,11 +465,11 @@ void cpu_detect_cache_sizes(struct cpuinfo_x86 *c)
 	if (n < 0x80000006)	/* Some chips just has a large L1. */
 		return;
 
-	cpuid(0x80000006, &dummy, &ebx, &ecx, &edx);
-	l2size = ecx >> 16;
+	cpuid(0x80000006, &dummy, &ebx6.full, &ecx6.full, &dummy2);
+	l2size = ecx6.split.cache_size;
 
 #ifdef CONFIG_X86_64
-	c->x86_tlbsize += ((ebx >> 16) & 0xfff) + (ebx & 0xfff);
+	c->x86_tlbsize += ebx6.split.dtlb_entries + ebx6.split.itlb_entries;
 #else
 	/* do processor-specific cache resizing */
 	if (this_cpu->legacy_cache_size)
@@ -505,9 +510,10 @@ void cpu_detect_tlb(struct cpuinfo_x86 *c)
 void detect_ht(struct cpuinfo_x86 *c)
 {
 #ifdef CONFIG_X86_HT
-	u32 eax, ebx, ecx, edx;
+	u32 eax, ecx, edx;
 	int index_msb, core_bits;
 	static bool printed;
+	union cpuid1_ebx ebx;
 
 	if (!cpu_has(c, X86_FEATURE_HT))
 		return;
@@ -518,9 +524,9 @@ void detect_ht(struct cpuinfo_x86 *c)
 	if (cpu_has(c, X86_FEATURE_XTOPOLOGY))
 		return;
 
-	cpuid(1, &eax, &ebx, &ecx, &edx);
+	cpuid(1, &eax, &ebx.full, &ecx, &edx);
 
-	smp_num_siblings = (ebx & 0xff0000) >> 16;
+	smp_num_siblings = ebx.split.max_logical_proc;
 
 	if (smp_num_siblings == 1) {
 		printk_once(KERN_INFO "CPU0: Hyper-Threading is disabled\n");
@@ -591,20 +597,22 @@ void cpu_detect(struct cpuinfo_x86 *c)
 	c->x86 = 4;
 	/* Intel-defined flags: level 0x00000001 */
 	if (c->cpuid_level >= 0x00000001) {
-		u32 junk, tfms, cap0, misc;
+		u32 junk, cap0;
+		union cpuid1_eax tfms;
+		union cpuid1_ebx misc;
 
-		cpuid(0x00000001, &tfms, &misc, &junk, &cap0);
-		c->x86 = (tfms >> 8) & 0xf;
-		c->x86_model = (tfms >> 4) & 0xf;
-		c->x86_mask = tfms & 0xf;
+		cpuid(0x00000001, &tfms.full, &misc.full, &junk, &cap0);
+		c->x86 = tfms.split.family_id;
+		c->x86_model = tfms.split.model;
+		c->x86_mask = tfms.split.stepping_id;
 
 		if (c->x86 == 0xf)
-			c->x86 += (tfms >> 20) & 0xff;
+			c->x86 += tfms.split.extended_family_id;
 		if (c->x86 >= 0x6)
-			c->x86_model += ((tfms >> 16) & 0xf) << 4;
+			c->x86_model += tfms.split.extended_model_id << 4;
 
-		if (cap0 & (1<<19)) {
-			c->x86_clflush_size = ((misc >> 8) & 0xff) * 8;
+		if (cap0 & (1 << (X86_FEATURE_CLFLUSH & 31))) {
+			c->x86_clflush_size = misc.split.clflush_size * 8;
 			c->x86_cache_alignment = c->x86_clflush_size;
 		}
 	}
@@ -654,10 +662,11 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 	}
 
 	if (c->extended_cpuid_level >= 0x80000008) {
-		u32 eax = cpuid_eax(0x80000008);
+		union cpuid_8_8_eax eax;
 
-		c->x86_virt_bits = (eax >> 8) & 0xff;
-		c->x86_phys_bits = eax & 0xff;
+		eax.full = cpuid_eax(0x80000008);
+		c->x86_virt_bits = eax.split.virt_as;
+		c->x86_phys_bits = eax.split.phys_as;
 	}
 #ifdef CONFIG_X86_32
 	else if (cpu_has(c, X86_FEATURE_PAE) || cpu_has(c, X86_FEATURE_PSE36))
@@ -814,7 +823,10 @@ static void generic_identify(struct cpuinfo_x86 *c)
 	get_cpu_cap(c);
 
 	if (c->cpuid_level >= 0x00000001) {
-		c->initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF;
+		union cpuid1_ebx ebx;
+
+		ebx.full = cpuid_ebx(1);
+		c->initial_apicid = ebx.split.initial_apicid;
 #ifdef CONFIG_X86_32
 # ifdef CONFIG_X86_HT
 		c->apicid = apic->phys_pkg_id(c->initial_apicid, 0);
-- 
1.9.1

[RESEND PATCH 3/3] KVM: x86: Using cpuid structs in KVM

[Nadav Amit]

Using cpuid structs in KVM to eliminate cryptic code with many bit operations.
The code does not introduce functional changes.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/cpuid.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 38a0afe..a7479ab 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -18,6 +18,7 @@
 #include <linux/uaccess.h>
 #include <asm/user.h>
 #include <asm/xsave.h>
+#include <asm/cpuid_def.h>
 #include "cpuid.h"
 #include "lapic.h"
 #include "mmu.h"
@@ -357,16 +358,18 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	}
 	/* function 4 has additional index. */
 	case 4: {
-		int i, cache_type;
+		int i;
 
 		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
 		/* read more entries until cache_type is zero */
 		for (i = 1; ; ++i) {
+			union cpuid4_eax eax;
+
 			if (*nent >= maxnent)
 				goto out;
 
-			cache_type = entry[i - 1].eax & 0x1f;
-			if (!cache_type)
+			eax.full = entry[i - 1].eax;
+			if (!eax.split.cache_type)
 				break;
 			do_cpuid_1_ent(&entry[i], function, i);
 			entry[i].flags |=
@@ -423,16 +426,18 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	}
 	/* function 0xb has additional index. */
 	case 0xb: {
-		int i, level_type;
+		int i;
 
 		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
 		/* read more entries until level_type is zero */
 		for (i = 1; ; ++i) {
+			union cpuid11_ecx ecx;
+
 			if (*nent >= maxnent)
 				goto out;
 
-			level_type = entry[i - 1].ecx & 0xff00;
-			if (!level_type)
+			ecx.full = entry[i - 1].ecx;
+			if (!ecx.split.level_type)
 				break;
 			do_cpuid_1_ent(&entry[i], function, i);
 			entry[i].flags |=
@@ -505,13 +510,13 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		entry->eax = entry->ebx = entry->ecx = 0;
 		break;
 	case 0x80000008: {
-		unsigned g_phys_as = (entry->eax >> 16) & 0xff;
-		unsigned virt_as = max((entry->eax >> 8) & 0xff, 48U);
-		unsigned phys_as = entry->eax & 0xff;
+		union cpuid_8_8_eax eax;
 
-		if (!g_phys_as)
-			g_phys_as = phys_as;
-		entry->eax = g_phys_as | (virt_as << 8);
+		eax.full = entry->eax;
+		eax.split.virt_as = max_t(int, eax.split.virt_as, 48);
+		if (!eax.split.guest_phys_as)
+			eax.split.guest_phys_as = eax.split.phys_as;
+		entry->eax = eax.full;
 		entry->ebx = entry->edx = 0;
 		break;
 	}
@@ -724,13 +729,16 @@ EXPORT_SYMBOL_GPL(kvm_find_cpuid_entry);
 int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
+	union cpuid_8_8_eax eax;
 
 	best = kvm_find_cpuid_entry(vcpu, 0x80000000, 0);
 	if (!best || best->eax < 0x80000008)
 		goto not_found;
 	best = kvm_find_cpuid_entry(vcpu, 0x80000008, 0);
-	if (best)
-		return best->eax & 0xff;
+	if (best) {
+		eax.full = best->eax;
+		return eax.split.phys_as;
+	}
 not_found:
 	return 36;
 }
-- 
1.9.1

Re: [PATCH 0/3] x86: structs for cpuid info in x86

[Peter Zijlstra]

On Wed, Sep 17, 2014 at 02:37:10PM +0200, Ingo Molnar wrote:
> If hpa, tglx or Linus objects I'll yield to that objection 
> though.
> 
> Opinions, objections?

They generally look fine to me. I appreciate the bitfields for
readability. I often use the same when having to deal with hardware
bitfields. See for example the cpuid10_a?x unions in asm/perf_event.h