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'. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html