linux-kernel - Re: [PATCH v5 3/7] powerpc/powernv: Nest PMU detection and device tree parser

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, 22 Jul 2015 13:49:41 +1000
From:	Daniel Axtens <dja@...ens.net>
To:	Madhavan Srinivasan <maddy@...ux.vnet.ibm.com>
Cc:	linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
	Stephane Eranian <eranian@...gle.com>,
	Paul Mackerras <paulus@...ba.org>,
	Anton Blanchard <anton@...ba.org>,
	Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>,
	Anshuman Khandual <khandual@...ux.vnet.ibm.com>
Subject: Re: [PATCH v5 3/7] powerpc/powernv: Nest PMU detection and device
 tree parser

Hi,

> +static struct perchip_nest_info p8_nest_perchip_info[P8_NEST_MAX_CHIPS];
> +
> +static int nest_ima_dt_parser(void)
> +{
> +	const __be32 *gcid;
> +	const __be64 *chip_ima_reg;
> +	const __be64 *chip_ima_size;
> +	struct device_node *dev;
> +	struct perchip_nest_info *p8ni;
> +	int idx;
> +
> +	/*
> +	 * "nest-ima" folder contains two things,
> +	 * a) per-chip reserved memory region for Nest PMU Counter data
> +	 * b) Support Nest PMU units and their event files
> +	 */
> +	for_each_node_with_property(dev, "ibm,ima-chip") {
> +		gcid = of_get_property(dev, "ibm,chip-id", NULL);
> +		chip_ima_reg = of_get_property(dev, "reg", NULL);
> +		chip_ima_size = of_get_property(dev, "size", NULL);
> +
> +		if ((!gcid) || (!chip_ima_reg) || (!chip_ima_size)) {
> +			pr_err("Nest_PMU: device %s missing property\n",
> +							dev->full_name);
> +			return -ENODEV;
> +		}
> +
> +		/* chip id to save reserve memory region */
> +		idx = (uint32_t)be32_to_cpup(gcid);
So be32_to_cpup returns a __u32. You're casting to a uint32_t and then
assigning to an int.
 - Do you need the intermediate cast?
 - Should idx be an unsigned type?
> +
> +		/*
> +		 * Using a local variable to make it compact and
> +		 * easier to read
> +		 */
We probably don't need this comment. But a better variable name would be
helpful! 
> +		p8ni = &p8_nest_perchip_info[idx];
> +		p8ni->pbase = be64_to_cpup(chip_ima_reg);
> +		p8ni->size = be64_to_cpup(chip_ima_size);
> +		p8ni->vbase = (uint64_t) phys_to_virt(p8ni->pbase);
> +	}
> +
> +	return 0;
> +}
> +
> +static int __init nest_pmu_init(void)
> +{
> +	int ret = -ENODEV;
> +
> +	/*
> +	 * Lets do this only if we are hypervisor
> +	 */
> +	if (!cur_cpu_spec->oprofile_cpu_type ||
> +	    !(strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power8") == 0) ||
> +	    !cpu_has_feature(CPU_FTR_HVMODE))
> +		return ret;
> +
I'm still really uncomfortable with doing this via oprofile_cpu_type.
If the kernel is compiled without oprofile support, will that get
populated?

I'm also curious about why we need checking for power8 at all. We
already know we're not going to run on hardware without a nest PMU
because of the device tree check below. 

What happens if there's a future generation of chip that supports nest
PMUs?

If it's really important to check versions in this function, maybe you
could put a version property in the ibm,ima-chip node?
> +	/*
> +	 * Nest PMU information is grouped under "nest-ima" node
> +	 * of the top-level device-tree directory. Detect Nest PMU
> +	 * by the "ibm,ima-chip" property.
> +	 */
> +	if (!of_find_node_with_property(NULL, "ibm,ima-chip"))
> +		return ret;
> +
> +	/*
> +	 * Parse device-tree for Nest PMU information
> +	 */
> +	ret = nest_ima_dt_parser();
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +device_initcall(nest_pmu_init);

-- 
Regards,
Daniel

Download attachment "signature.asc" of type "application/pgp-signature" (861 bytes)