[PATCH 01/10] soc: fujitsu: hwb: Add hardware barrier driver init/exit code
Jonathan Cameron
Jonathan.Cameron at Huawei.com
Fri Jan 8 06:58:04 EST 2021
On Fri, 8 Jan 2021 19:32:18 +0900
Misono Tomohiro <misono.tomohiro at jp.fujitsu.com> wrote:
> This adds hardware barrier driver's struct definitions and
> module init/exit code. We use miscdeice for barrier driver ioctl
device
> and /dev/fujitsu_hwb will be created upon module load.
> Following commits will add each ioctl definition.
>
> Signed-off-by: Misono Tomohiro <misono.tomohiro at jp.fujitsu.com>
Hi Misono,
I was taking a look out of curiosity so the following is definitely not
a full review but a few general comments inline.
Thanks,
Jonathan
> ---
> drivers/soc/fujitsu/fujitsu_hwb.c | 313 ++++++++++++++++++++++++++++++
> 1 file changed, 313 insertions(+)
> create mode 100644 drivers/soc/fujitsu/fujitsu_hwb.c
>
> diff --git a/drivers/soc/fujitsu/fujitsu_hwb.c b/drivers/soc/fujitsu/fujitsu_hwb.c
> new file mode 100644
> index 000000000000..44c32c1683df
> --- /dev/null
> +++ b/drivers/soc/fujitsu/fujitsu_hwb.c
> @@ -0,0 +1,313 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2020 FUJITSU LIMITED
> + *
> + * This hardware barrier (HWB) driver provides a set of ioctls to realize synchronization
> + * by PEs in the same Come Memory Group (CMG) by using implementation defined registers.
> + * On A64FX, CMG is the same as L3 cache domain.
> + *
> + * The main purpose of the driver is setting up registers which cannot be accessed
> + * from EL0. However, after initialization, BST_SYNC/LBSY_SYNC registers which is used
> + * in synchronization main logic can be accessed from EL0 (therefore it is fast).
> + *
> + * Simplified barrier operation flow of user application is as follows:
> + * (one PE)
> + * 1. Call IOC_BB_ALLOC to setup INIT_SYNC register which is shared in a CMG.
> + * This specifies which PEs join synchronization
> + * (on each PE joining synchronization)
> + * 2. Call IOC_BW_ASSIGN to setup ASSIGN_SYNC register per PE
> + * 3. Barrier main logic (all logic runs in EL0)
> + * a) Write 1 to BST_SYNC register
> + * b) Read LBSY_SYNC register
> + * c) If LBSY_SYNC value is 1, sync is finished, otherwise go back to b
> + * (If all PEs joining synchronization write 1 to BST_SYNC, LBSY_SYNC becomes 1)
> + * 4. Call IOC_BW_UNASSIGN to reset ASSIGN_SYNC register
> + * (one PE)
> + * 5. Call IOC_BB_FREE to reset INIT_SYNC register
> + */
> +
> +#include <asm/cputype.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/cpu.h>
> +#include <linux/cpumask.h>
> +#include <linux/kernel.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/spinlock.h>
> +#include <linux/slab.h>
> +#include <linux/wait.h>
> +
> +#ifdef pr_fmt
> +#undef pr_fmt
> +#endif
> +#define pr_fmt(fmt) "[%s:%s:%d] " fmt, KBUILD_MODNAME, __func__, __LINE__
> +
> +/* Since miscdevice is used, /dev/fujitsu_hwb will be created when module is loaded */
> +#define FHWB_DEV_NAME "fujitsu_hwb"
> +
> +/* Implementation defined registers for barrier shared in CMG */
> +#define FHWB_INIT_SYNC_BB0_EL1 sys_reg(3, 0, 15, 13, 0)
> +#define FHWB_INIT_SYNC_BB1_EL1 sys_reg(3, 0, 15, 13, 1)
> +#define FHWB_INIT_SYNC_BB2_EL1 sys_reg(3, 0, 15, 13, 2)
> +#define FHWB_INIT_SYNC_BB3_EL1 sys_reg(3, 0, 15, 13, 3)
> +#define FHWB_INIT_SYNC_BB4_EL1 sys_reg(3, 0, 15, 13, 4)
> +#define FHWB_INIT_SYNC_BB5_EL1 sys_reg(3, 0, 15, 13, 5)
> +
> +/* Implementation defined registers for barrier per PE */
> +#define FHWB_CTRL_EL1 sys_reg(3, 0, 11, 12, 0)
> +#define FHWB_BST_BIT_EL1 sys_reg(3, 0, 11, 12, 4)
> +#define FHWB_ASSIGN_SYNC_W0_EL1 sys_reg(3, 0, 15, 15, 0)
> +#define FHWB_ASSIGN_SYNC_W1_EL1 sys_reg(3, 0, 15, 15, 1)
> +#define FHWB_ASSIGN_SYNC_W2_EL1 sys_reg(3, 0, 15, 15, 2)
> +#define FHWB_ASSIGN_SYNC_W3_EL1 sys_reg(3, 0, 15, 15, 3)
> +
> +/* Field definitions for above registers */
> +#define FHWB_INIT_SYNC_BB_EL1_MASK_FIELD GENMASK_ULL(44, 32)
> +#define FHWB_INIT_SYNC_BB_EL1_BST_FIELD GENMASK_ULL(12, 0)
> +#define FHWB_CTRL_EL1_EL1AE BIT_ULL(63)
> +#define FHWB_CTRL_EL1_EL0AE BIT_ULL(62)
> +#define FHWB_BST_BIT_EL1_CMG_FILED GENMASK_ULL(5, 4)
> +#define FHWB_BST_BIT_EL1_PE_FILED GENMASK_ULL(3, 0)
> +#define FHWB_ASSIGN_SYNC_W_EL1_VALID BIT_ULL(63)
> +
> +static enum cpuhp_state _hp_state;
> +
> +/*
> + * Each PE has its own CMG and Physical PE number (determined by BST_BIT_EL1 register).
> + * Barrier operation can be performed by PEs which belong to the same CMG.
> + */
> +struct pe_info {
> + /* CMG number of this PE */
> + u8 cmg;
> + /* Physical PE number of this PE */
> + u8 ppe;
> +};
> +
> +/* Hardware information of running system */
> +struct hwb_hwinfo {
> + /* CPU type (part number) */
> + unsigned int type;
> + /* Number of CMG */
> + u8 num_cmg;
> + /* Number of barrier blade(BB) per CMG */
> + u8 num_bb;
> + /* Number of barrier window(BW) per PE */
> + u8 num_bw;
> + /*
> + * Maximum number of PE per CMG.
> + * Depending on BIOS configuration, each CMG has up to max_pe_per_cmg PEs
> + * and each PE has unique physical PE number between 0 ~ (max_pe_per_cmg-1)
> + */
> + u8 max_pe_per_cmg;
> +
> + /* Bitmap for currently allocated BB per CMG */
> + unsigned long *used_bb_bmap;
> + /* Bitmap for currently allocated BW per PE */
> + unsigned long *used_bw_bmap;
> + /* Mapping table of cpuid -> CMG/PE number */
> + struct pe_info *core_map;
> +};
> +static struct hwb_hwinfo _hwinfo;
> +
> +/* List for barrier blade currently used per FD */
> +struct hwb_private_data {
> + struct list_head bb_list;
> + spinlock_t list_lock;
> +};
> +
> +/* Each barrier blade info */
> +#define BB_FREEING 1
> +struct bb_info {
> + /* cpumask for PEs which participate synchronization */
> + cpumask_var_t pemask;
> + /* cpumask for PEs which currently assigned BW for this BB */
> + cpumask_var_t assigned_pemask;
> + /* Added to hwb_private_data::bb_list */
> + struct list_head node;
> + /* For indicating if this bb is currently being freed or not */
> + unsigned long flag;
> + /* For waiting ongoing assign/unassign operation to finish before freeing BB */
> + wait_queue_head_t wq;
> + /* Track ongoing assign/unassign operation count */
> + atomic_t ongoing_assign_count;
> + /* CMG number of this blade */
nitpick: Double space currently after CMG that looks inconsistent.
> + u8 cmg;
> + /* BB number of this blade */
> + u8 bb;
> + /* Hold assigned window number of each PE corresponding to @assigned_pemask */
> + u8 *bw;
> + /* Track usage count as IOC_BB_FREE and IOC_BW_[UN]ASSIGN might be run in parallel */
> + struct kref kref;
> +};
> +static struct kmem_cache *bb_info_cachep;
> +
> +static const struct file_operations fujitsu_hwb_dev_fops = {
> + .owner = THIS_MODULE,
> +};
> +
> +static struct miscdevice bar_miscdev = {
> + .fops = &fujitsu_hwb_dev_fops,
> + .minor = MISC_DYNAMIC_MINOR,
> + .mode = 0666,
> + .name = FHWB_DEV_NAME,
> +};
> +
> +static void destroy_bb_info_cachep(void)
> +{
> + kmem_cache_destroy(bb_info_cachep);
> +}
> +
> +static int __init init_bb_info_cachep(void)
> +{
> + /*
> + * Since cpumask value will be copied from userspace to the beginning of
> + * struct bb_info, use kmem_cache_create_usercopy to mark that region.
> + * Otherwise CONFIG_HARDENED_USERCOPY gives user_copy_warn.
> + */
> + bb_info_cachep = kmem_cache_create_usercopy("bb_info_cache", sizeof(struct bb_info),
> + 0, SLAB_HWCACHE_ALIGN, 0, sizeof(cpumask_var_t), NULL);
> + if (bb_info_cachep == NULL)
inconsistent on ! vs == NULL checks. I personally don't care which you use, but better to chose
one style and use if everywhere in a given driver.
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static void free_map(void)
> +{
> + kfree(_hwinfo.used_bw_bmap);
> + kfree(_hwinfo.used_bb_bmap);
> + kfree(_hwinfo.core_map);
> +}
> +
> +static int __init alloc_map(void)
For generic sounding function names, it is better to prefix with something driver specific.
perhaps hwb_alloc_map() or similar? Both avoids possible clashes of naming in future and
leads to more readable code as people then know the function is local.
> +{
> + _hwinfo.core_map = kcalloc(num_possible_cpus(), sizeof(struct pe_info), GFP_KERNEL);
preferred to use sizeof(*_hwinfo.core_map) as saves reviewer checking the types. Note
applies in other places as well.
Whilst it's nice to make these flexible in size, the separate allocations do add overheads.
Given num_possible_cpus is probably constrained, perhaps better to just make these fixed
size and big enough for all plausible usecases?
> + _hwinfo.used_bb_bmap = kcalloc(_hwinfo.num_cmg, sizeof(unsigned long), GFP_KERNEL);
> + _hwinfo.used_bw_bmap = kcalloc(num_possible_cpus(), sizeof(unsigned long), GFP_KERNEL);
> + if (!_hwinfo.core_map || !_hwinfo.used_bb_bmap || !_hwinfo.used_bw_bmap)
> + goto fail;
I'd prefer you check these individually and handle the frees explicitly. Makes for easier reviewing
as we can match each fail against the clean up.
> +
> + /* 0 is valid number for both CMG/PE. Set all bits to 1 to represents uninitialized state */
> + memset(_hwinfo.core_map, 0xFF, sizeof(struct pe_info) * num_possible_cpus());
> +
> + return 0;
> +
> +fail:
> + free_map();
> + return -ENOMEM;
> +}
> +
> +/* Get this system's CPU type (part number). If it is not fujitsu CPU, return -1 */
> +static int __init get_cpu_type(void)
> +{
> + if (read_cpuid_implementor() != ARM_CPU_IMP_FUJITSU)
> + return -1;
Better to return a meaningful error code from here, then pass it on at the caller.
> +
> + return read_cpuid_part_number();
> +}
> +
> +static int __init setup_hwinfo(void)
> +{
> + int type;
> +
> + type = get_cpu_type();
> + if (type < 0)
As above, I'd expect to see return type; here.
> + return -ENODEV;
> +
> + _hwinfo.type = type;
> + switch (type) {
> + case FUJITSU_CPU_PART_A64FX:
> + _hwinfo.num_cmg = 4;
> + _hwinfo.num_bb = 6;
> + _hwinfo.num_bw = 4;
> + _hwinfo.max_pe_per_cmg = 13;
> + break;
> + default:
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> +static int hwb_cpu_online(unsigned int cpu)
> +{
> + u64 val;
> + int i;
> +
> + /* Setup core_map by reading BST_BIT_EL1 register of each PE */
> + val = read_sysreg_s(FHWB_BST_BIT_EL1);
> + _hwinfo.core_map[cpu].cmg = FIELD_GET(FHWB_BST_BIT_EL1_CMG_FILED, val);
> + _hwinfo.core_map[cpu].ppe = FIELD_GET(FHWB_BST_BIT_EL1_PE_FILED, val);
> +
> + /* Since these registers' values are UNKNOWN on reset, explicitly clear all */
> + for (i = 0; i < _hwinfo.num_bw; i++)
> + write_bw_reg(i, 0);
> +
> + write_sysreg_s(0, FHWB_CTRL_EL1);
> +
> + return 0;
> +}
> +
> +static int __init hwb_init(void)
> +{
> + int ret;
> +
> + ret = setup_hwinfo();
> + if (ret < 0) {
As it's not obvious from the function name that the only thing it is doing is
checking the cpu type, I'd move this error print into that function call or
rename setup_hwinfo()
> + pr_err("Unsupported CPU type\n");
> + return ret;
> + }
> +
> + ret = alloc_map();
> + if (ret < 0)
> + return ret;
> +
> + ret = init_bb_info_cachep();
> + if (ret < 0)
> + goto out1;
> +
> + /*
> + * Setup cpuhp callback to ensure each PE's resource will be initialized
> + * even if some PEs are offline at this point
> + */
> + ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "soc/fujitsu_hwb:online",
> + hwb_cpu_online, NULL);
> + if (ret < 0) {
> + pr_err("cpuhp setup failed: %d\n", ret);
> + goto out2;
> + }
> + _hp_state = ret;
> +
> + ret = misc_register(&bar_miscdev);
> + if (ret < 0) {
> + pr_err("misc_register failed: %d\n", ret);
> + goto out3;
> + }
> +
> + return 0;
> +
> +out3:
> + cpuhp_remove_state(_hp_state);
> +out2:
> + destroy_bb_info_cachep();
> +out1:
> + free_map();
> +
> + return ret;
> +}
> +
> +static void __exit hwb_exit(void)
> +{
> + misc_deregister(&bar_miscdev);
> + cpuhp_remove_state(_hp_state);
> + destroy_bb_info_cachep();
> + free_map();
> +}
> +
> +module_init(hwb_init);
> +module_exit(hwb_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("FUJITSU LIMITED");
> +MODULE_DESCRIPTION("FUJITSU HPC Hardware Barrier Driver");
More information about the linux-arm-kernel
mailing list