linux-kernel - Re: [PATCH v4 04/11] VAS: Define vas_init() and vas

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:   Mon, 24 Apr 2017 16:25:57 +1000
From:   Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:     Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>,
        Michael Ellerman <mpe@...erman.id.au>
Cc:     michael.neuling@....ibm.com, stewart@...ux.vnet.ibm.com,
        apopple@....ibm.com, hbabu@...ibm.com, oohall@...il.com,
        bsingharora@...il.com, linuxppc-dev@...abs.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 04/11] VAS: Define vas_init() and vas_exit()

On Thu, 2017-03-30 at 22:13 -0700, Sukadev Bhattiprolu wrote:
> 
> +	p = of_get_property(dn, "vas-id", NULL);
> +	if (!p) {
> +		pr_err("VAS: NULL vas-id? %p\n", p);
> +		return -ENODEV;
> +	}
> +
> +	vinst->vas_id = of_read_number(p, 1);

of_property_read_u32()

> +	/*
> +	 * Hardcoded 6 is tied to corresponding code in
> +	 *      skiboot.git/core/vas.c
> +	 */
> +	rc = of_property_read_variable_u64_array(dn, "reg", values,
> 6, 6);
> +	if (rc != 6) {
> +		pr_err("VAS %d: Unable to read reg properties, rc
> %d\n",
> +				vinst->vas_id, rc);
> +		return rc;
> +	}
> +
> +	vinst->hvwc_bar_start = values[0];
> +	vinst->hvwc_bar_len = values[1];
> +	vinst->uwc_bar_start = values[2];
> +	vinst->uwc_bar_len = values[3];
> +	vinst->win_base_addr = values[4];
> +	vinst->win_id_shift = values[5];

If you make the VAS a proper MMIO device on the powerbus (as explained
in my comment to your OPAL patch), and you create this as a platform
device based on the DT, the core will parse the reg property for you
and will populate the device struct resource array for you.

> +	return 0;
> +}
> +
> +/*
> + * Although this is read/used multiple times, it is written to only
> + * during initialization.
> + */
> +struct vas_instance *find_vas_instance(int vasid)
> +{
> +	int i;
> +	struct vas_instance *vinst;
> +
> +	for (i = 0; i < vas_num_instances; i++) {
> +		vinst = &vas_instances[i];
> +		if (vinst->vas_id == vasid)
> +			return vinst;
> +	}
> +	pr_err("VAS instance for vas-id %d not found\n", vasid);
> +	WARN_ON_ONCE(1);
> +	return NULL;
> +}

This is gross. What is it needed for  ?

> +
> +bool vas_initialized(void)
> +{
> +	return init_done;
> +}
> +
> +int vas_init(void)
> +{
> +	int rc;
> +	struct device_node *dn;
> +	struct vas_instance *vinst;
> +
> +	if (!pvr_version_is(PVR_POWER9))
> +		return -ENODEV;

No. Create a platform device that matches the corresponding device-tree
node. One per instance.

The resulting VAS "driver" thus becomes a loadable module which you can
put elsewhere in the tree, matching on the DT node "compatible"
property.

You can then have the copros be chilren of it. The VAS driver can then
create additional platform devices for its children, who can have their
own module (which *can* depend on symbols exportd by the VAS one)
etc...

> +	vas_num_instances = 0;
> +	for_each_node_by_name(dn, "vas")
> +		vas_num_instances++;
> +
> +	if (!vas_num_instances)
> +		return -ENODEV;
> +
> +	vas_instances = kcalloc(vas_num_instances, sizeof(*vinst),
> GFP_KERNEL);
> +	if (!vas_instances)
> +		return -ENOMEM;
> +
> +	vinst = &vas_instances[0];
> +	for_each_node_by_name(dn, "vas") {
> +		rc = init_vas_instance(dn, vinst);
> +		if (rc) {
> +			pr_err("Error %d initializing VAS instance
> %ld\n", rc,
> +					(vinst-&vas_instances[0]));
> +			goto cleanup;
> +		}
> +		vinst++;
> +	}
> +
> +	rc = -ENODEV;
> +	if (vinst == &vas_instances[0]) {
> +		/* Should not happen as we saw some above. */
> +		pr_err("VAS: Did not find any VAS DT nodes now!\n");
> +		goto cleanup;
> +	}
> +
> +	pr_devel("VAS: Initialized %d instances\n",
> vas_num_instances);
> +	init_done = true;
> +
> +	return 0;
> +
> +cleanup:
> +	kfree(vas_instances);
> +	return rc;
> +}
> +
> +void vas_exit(void)
> +{
> +	init_done = false;
> +	kfree(vas_instances);
> +}
> +
> +module_init(vas_init);
> +module_exit(vas_exit);
> +MODULE_DESCRIPTION("IBM Virtual Accelerator Switchboard");
> +MODULE_AUTHOR("Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/arch/powerpc/platforms/powernv/vas.h
> b/arch/powerpc/platforms/powernv/vas.h
> index c63395d..46aaa17 100644
> --- a/arch/powerpc/platforms/powernv/vas.h
> +++ b/arch/powerpc/platforms/powernv/vas.h
> @@ -384,4 +384,7 @@ struct vas_winctx {
>  	enum vas_notify_after_count notify_after_count;
>  };
>  
> +extern bool vas_initialized(void);
> +extern struct vas_instance *find_vas_instance(int vasid);
> +
>  #endif /* _VAS_H */