[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YoSVglItX1PhveEP@kroah.com>
Date: Wed, 18 May 2022 08:43:14 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Kohei Tarumizu <tarumizu.kohei@...itsu.com>
Cc: catalin.marinas@....com, will@...nel.org, tglx@...utronix.de,
mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
x86@...nel.org, hpa@...or.com, rafael@...nel.org,
mchehab+huawei@...nel.org, eugenis@...gle.com, tony.luck@...el.com,
pcc@...gle.com, peterz@...radead.org, marcos@...a.pet,
conor.dooley@...rochip.com, nicolas.ferre@...rochip.com,
marcan@...can.st, linus.walleij@...aro.org, arnd@...db.de,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4 6/8] x86: Add hardware prefetch control support for x86
On Wed, May 18, 2022 at 03:30:30PM +0900, Kohei Tarumizu wrote:
> +/*
> + * Returns the cpu number of the cpu_device(/sys/devices/system/cpu/cpuX)
> + * in the ancestor directory of prefetch_control.
> + *
> + * When initializing this driver, it is verified that the cache directory exists
> + * under cpuX device. Therefore, the third level up from prefetch_control is
> + * cpuX device as shown below.
> + *
> + * /sys/devices/system/cpu/cpuX/cache/indexX/prefetch_control
> + */
> +static inline unsigned int pfctl_dev_get_cpu(struct device *pfctl_dev)
> +{
> + return pfctl_dev->parent->parent->parent->id;
As much fun as it is to see a function like this, that is not ok, and
never guaranteed to keep working, sorry. Please find the device
properly, never assume a specific driver model topology as they are
guaranteed to break over time and never supposed to be static like this.
> +}
> +
> +static ssize_t
> +pfctl_show(struct device *pfctl_dev, struct device_attribute *attr, char *buf)
> +{
> + unsigned int cpu = pfctl_dev_get_cpu(pfctl_dev);
> + struct x86_pfctl_attr *xa;
> + u64 val;
> +
> + xa = container_of(attr, struct x86_pfctl_attr, attr);
> +
> + rdmsrl_on_cpu(cpu, MSR_MISC_FEATURE_CONTROL, &val);
> + return sysfs_emit(buf, "%d\n", val & xa->mask ? 0 : 1);
> +}
> +
> +struct write_info {
> + u64 mask;
> + bool enable;
> +};
> +
> +/*
> + * wrmsrl() in this patch is only done inside of an interrupt-disabled region
> + * to avoid a conflict of write access from other drivers,
> + */
> +static void pfctl_write(void *info)
> +{
> + struct write_info *winfo = info;
> + u64 reg;
> +
> + reg = 0;
> + rdmsrl(MSR_MISC_FEATURE_CONTROL, reg);
> +
> + if (winfo->enable)
> + reg &= ~winfo->mask;
> + else
> + reg |= winfo->mask;
> +
> + wrmsrl(MSR_MISC_FEATURE_CONTROL, reg);
> +}
> +
> +/*
> + * MSR_MISC_FEATURE_CONTROL has "core" scope, so define the lock to avoid a
> + * conflict of write access from different logical processors in the same core.
> + */
> +static DEFINE_MUTEX(pfctl_mutex);
> +
> +static ssize_t
> +pfctl_store(struct device *pfctl_dev, struct device_attribute *attr,
> + const char *buf, size_t size)
> +{
> + unsigned int cpu = pfctl_dev_get_cpu(pfctl_dev);
> + struct x86_pfctl_attr *xa;
> + struct write_info info;
> +
> + xa = container_of(attr, struct x86_pfctl_attr, attr);
> + info.mask = xa->mask;
> +
> + if (strtobool(buf, &info.enable) < 0)
> + return -EINVAL;
> +
> + mutex_lock(&pfctl_mutex);
> + smp_call_function_single(cpu, pfctl_write, &info, true);
> + mutex_unlock(&pfctl_mutex);
> +
> + return size;
> +}
> +
> +#define PFCTL_ATTR(_name, _level, _bit) \
> + struct x86_pfctl_attr attr_l##_level##_##_name = { \
> + .attr = __ATTR(_name, 0600, pfctl_show, pfctl_store), \
> + .mask = BIT_ULL(_bit), }
> +
> +static PFCTL_ATTR(hardware_prefetcher_enable, 1, 2);
> +static PFCTL_ATTR(hardware_prefetcher_enable, 2, 0);
> +static PFCTL_ATTR(ip_prefetcher_enable, 1, 3);
> +static PFCTL_ATTR(adjacent_cache_line_prefetcher_enable, 2, 1);
> +
> +static struct device_attribute *l1_attrs[] __ro_after_init = {
How do you know attributes are to be marked read only after init? Do
other parts of the kernel do that? I don't think the driver core
guarantees that at all.
thanks,
greg k-h
Powered by blists - more mailing lists