On Mon, Nov 08, 2021 at 09:47:54AM +0100, Peter Zijlstra wrote:
> On Fri, Nov 05, 2021 at 06:33:08PM -0700, Ricardo Neri wrote:
> > +static __init int hfi_parse_features(void)
> > +{
> > + unsigned int nr_capabilities, reg;
> > +
>
> > + /*
> > + * If we are here we know that CPUID_HFI_LEAF exists. Parse the
> > + * supported capabilities and the size of the HFI table.
> > + */
> > + reg = cpuid_edx(CPUID_HFI_LEAF);
> > +
> > + hfi_features.capabilities = reg & HFI_CAPABILITIES_MASK;
> > + if (!(hfi_features.capabilities & HFI_CAPABILITIES_PERFORMANCE)) {
> > + pr_err("Performance reporting not supported! Not using HFI\n");
> > + return -ENODEV;
> > + }
> > +
> > + /* The number of 4KB pages required by the table */
> > + hfi_features.nr_table_pages = ((reg & CPUID_HFI_TABLE_SIZE_MASK) >>
> > + CPUID_HFI_TABLE_SIZE_SHIFT) + 1;
> > +
>
> > +/* Hardware Feedback Interface Enumeration */
> > +#define CPUID_HFI_LEAF 6
> > +#define CPUID_HFI_CAP_MASK 0xff
> > +#define CPUID_HFI_TABLE_SIZE_MASK 0x0f00
> > +#define CPUID_HFI_TABLE_SIZE_SHIFT 8
> > +#define CPUID_HFI_CPU_INDEX_MASK 0xffff0000
>
> Also, *if* you're going to do something like this, then at least write
> out the masks in full so you can easily see how they relate. The above
> is crap.
>
> > +#define CPUID_HFI_CPU_INDEX_SHIFT 16
> > +
> > +/* Hardware Feedback Interface Pointer */
> > +#define HFI_PTR_VALID_BIT BIT(0)
> > +#define HFI_PTR_ADDR_SHIFT 12
> > +
> > +/* Hardware Feedback Interface Configuration */
> > +#define HFI_CONFIG_ENABLE_BIT BIT(0)
> > +
> > +/* Hardware Feedback Interface Capabilities */
> > +#define HFI_CAPABILITIES_MASK 0xff
> > +#define HFI_CAPABILITIES_NR 8
> > +#define HFI_CAPABILITIES_PERFORMANCE BIT(0)
> > +#define HFI_CAPABILITIES_ENERGY_EFF BIT(1)
>
>
> So personally I prefer a bitfield union a-la cpuid10_eax, cpuid10_ebx
> cpuid10_edx etc.. Barring that, the above can also be written more
> concise using FIELD_GET() from bitfields.
>
> union cpuid6_edx {
> struct {
> unsigned int capabilities : 8;
> unsigned int table_size : 4;
> unsigned int __reserved : 4;
> unsigned int cpu_index : 16;
> };
> unsigned int full;
> };
Sure Peter. This looks more readable. I'll implement it like this.
Thanks and BR,
Ricardo
